Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support
From: Guenter Roeck
Date: Thu Apr 25 2019 - 00:04:29 EST
On 4/24/19 6:14 PM, Anson Huang wrote:
i.MX system controller watchdog can support pretimeout IRQ
via general SCU MU IRQ, it depends on IMX_SCU and driver MUST
be probed after SCU IPC ready, then enable corresponding SCU
IRQ group and register SCU IRQ notifier, when watchdog pretimeout
IRQ fires, SCU MU IRQ will be handled and watchdog pretimeout
notifier will be called to handle the event.
Ah, here is the missing patch.
As mentioned in my other reply, the watchdog driver does now depend
on the SCU IPC handle and should be instantiated accordingly.
Using -EPROBE_DEFER to work around bad dependencies is not a solution.
Additional comment below.
Guenter
Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/imx_sc_wdt.c | 65 +++++++++++++++++++++++++++++++++++++++----
2 files changed, 61 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 44a3158..f2c2c1a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -644,6 +644,7 @@ config IMX2_WDT
config IMX_SC_WDT
tristate "IMX SC Watchdog"
depends on HAVE_ARM_SMCCC
+ depends on IMX_SCU
select WATCHDOG_CORE
help
This is the driver for the system controller watchdog
diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
index 49848b6..f45ed10 100644
--- a/drivers/watchdog/imx_sc_wdt.c
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -4,6 +4,7 @@
*/
#include <linux/arm-smccc.h>
+#include <linux/firmware/imx/sci.h>
#include <linux/io.h>
#include <linux/init.h>
#include <linux/kernel.h>
@@ -33,11 +34,16 @@
#define SC_TIMER_WDOG_ACTION_PARTITION 0
+#define SC_IRQ_WDOG 1
+#define SC_IRQ_GROUP_WDOG 1
+
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0000);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+struct watchdog_device *imx_sc_wdd;
+
static int imx_sc_wdt_ping(struct watchdog_device *wdog)
{
struct arm_smccc_res res;
@@ -85,12 +91,42 @@ static int imx_sc_wdt_set_timeout(struct watchdog_device *wdog,
return res.a0 ? -EACCES : 0;
}
+static int imx_sc_wdt_set_pretimeout(struct watchdog_device *wdog,
+ unsigned int pretimeout)
+{
+ struct arm_smccc_res res;
+
+ wdog->pretimeout = pretimeout;
+ arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_PRETIME_WDOG,
+ pretimeout * 1000, 0, 0, 0, 0, 0, &res);
+
+ return res.a0 ? -EACCES : 0;
If this function returns an error, why does it set wdog->pretimeout
unconditionally ? That seems wrong.
+}
+
+static int imx_sc_wdt_notify(struct notifier_block *nb,
+ unsigned long event, void *group)
+{
+ /* ignore other irqs */
+ if (!(event & SC_IRQ_WDOG &&
+ (*(u8 *)group == SC_IRQ_GROUP_WDOG)))
if (!(event & SC_IRQ_WDO) || *(u8 *)group != SC_IRQ_GROUP_WDOG)
would be easier to understand. Either case, the second part of the expression
has an unnecessary (), and multi-line alignment seems to be off.
+ return 0;
+
+ watchdog_notify_pretimeout(imx_sc_wdd);
The notifier block should be embedded in a local data structure,
which would include strict watchdog_device and struct notifier_block.
This would avoid the need for a static variable.
+
+ return 0;
If the function always returns 0, why not the following ?
Above code seems unnecessary complex for no good reason.
if (event & SC_IRQ_WDOG &&
*(u8 *)group == SC_IRQ_GROUP_WDOG)
watchdog_notify_pretimeout(imx_sc_wdd);
return 0;
+}
+
+static struct notifier_block imx_sc_wdt_notifier = {
+ .notifier_call = imx_sc_wdt_notify,
+};
+
static const struct watchdog_ops imx_sc_wdt_ops = {
.owner = THIS_MODULE,
.start = imx_sc_wdt_start,
.stop = imx_sc_wdt_stop,
.ping = imx_sc_wdt_ping,
.set_timeout = imx_sc_wdt_set_timeout,
+ .set_pretimeout = imx_sc_wdt_set_pretimeout,
};
static const struct watchdog_info imx_sc_wdt_info = {
@@ -102,9 +138,15 @@ static const struct watchdog_info imx_sc_wdt_info = {
static int imx_sc_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct watchdog_device *imx_sc_wdd;
int ret;
+ /* wait until i.MX SCU IPC ready */
+ ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
+ SC_IRQ_WDOG,
+ true);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
And if the error is anything else it is ignored ?
Also, what happens if the interrupt triggers before imx_sc_wdd is set ?
imx_sc_wdd = devm_kzalloc(dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
if (!imx_sc_wdd)
return -ENOMEM;
@@ -117,6 +159,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
imx_sc_wdd->max_timeout = MAX_TIMEOUT;
imx_sc_wdd->parent = dev;
imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
+ imx_sc_wdd->pretimeout = 0;
Unnecessary.
watchdog_init_timeout(imx_sc_wdd, 0, dev);
watchdog_stop_on_reboot(imx_sc_wdd);
@@ -128,13 +171,26 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
return ret;
}
+ ret = imx_scu_irq_register_notifier(&imx_sc_wdt_notifier);
+ if (ret)
+ dev_warn(&pdev->dev,
+ "Failed to register watchdog irq notifier\n");
pretimeout support doesn't work in this case, and any claim to support it
seems inappropriate.
+
return 0;
}
-static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
+static int imx_sc_wdt_remove(struct platform_device *pdev)
{
- struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
+ imx_scu_irq_unregister_notifier(&imx_sc_wdt_notifier);
+ imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
+ SC_IRQ_WDOG,
+ false);
I would prefer to see devm_add_action() calls.
+ return 0;
+}
+
+static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
+{
if (watchdog_active(imx_sc_wdd))
imx_sc_wdt_stop(imx_sc_wdd);
@@ -143,8 +199,6 @@ static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
{
- struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
-
if (watchdog_active(imx_sc_wdd))
imx_sc_wdt_start(imx_sc_wdd);
@@ -162,6 +216,7 @@ MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
static struct platform_driver imx_sc_wdt_driver = {
.probe = imx_sc_wdt_probe,
+ .remove = imx_sc_wdt_remove,
.driver = {
.name = "imx-sc-wdt",
.of_match_table = imx_sc_wdt_dt_ids,