On Thu, Apr 25, 2019 at 05:44:45AM +0000, Anson Huang wrote: > Hi, Guenter > > > -----Original Message----- > > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter > > Roeck > > Sent: Thursday, April 25, 2019 12:04 PM > > To: Anson Huang <[email protected]>; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; Aisheng Dong <[email protected]>; linux- > > [email protected]; [email protected]; linux- > > [email protected] > > Cc: dl-linux-imx <[email protected]> > > Subject: Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support > > > > 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. > > So, I have to move the i.MX system controller watchdog node into the i.MX SCU > node in DT file now? As it depends on i.MX SCU firmware. If so, should I > remove the
Yes, that is what I would suggest. > previous i.MX system controller binding doc (fsl-imx-sc-wdt.txt) and add > binding doc > to (Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt) ? > I can't comment on that; I am neutral on how the documentation is handled. > > > > Additional comment below. > > > > Guenter > > > > > Signed-off-by: Anson Huang <[email protected]> > > > --- > > > 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. > > You are right, I will fix it in V2, but looks like some other watchdog > drivers also has such issue, such > as below: > > drivers/watchdog/pm8916_wdt.c > drivers/watchdog/sprd_wdt.c > You are correct, but that doesn't make this one better. > > > > > +} > > > + > > > +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. > > Will improve it and fix the line alignment in V2. > > > > > > + 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. > > OK, I will add a local data structure to do it in V2. > > > > > > + > > > + 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; > > OK. > > > > > > +} > > > + > > > +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 ? > > Other error ONLY means the IPC failed, the IRQ WDOG group will NOT be enabled, > it does NOT impact other wdog functions, so I did NOT handle it, maybe I can > add > some warning message to tell user that pretimeout function is failed if other > error > occurs and also disable the pretimeout function in wdog info? > Something like that, yes. > If interrupt triggers before imx_sc_wdd is set, since the notifier is NOT > registered yet, > so the wdog interrupt will be ignored. > > > > > > 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. > > OK. > > > > > > > > > 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. > > So how to disable pretimeout function in this case, just overwrite the > watchdog_info > to remove the WDIOF_PRETIMEOUT? > Yes, though I don't recall seeing WDIOF_PRETIMEOUT to start with. Guenter > > > > > + > > > 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. > > Ah, agreed, will do it in V2. > > Thanks, > Anson. > > > > > > + 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, > > > >
