[PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready

[PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready

[Anson Huang]

The imx_scu_irq_group_enable() is normally called during module driver
probe phase to enable SCU group irq, e.g., i.MX system controller
watchdog driver will call it during its driver probe phase, as i.MX
system controller watchdog driver does NOT need SCU IPC handle for
operations, so it could be probed before i.MX SCU IPC is ready, and
below dump will show out:

[    0.933001] Hardware name: Freescale i.MX8QXP MEK (DT)
[    0.938129] pstate: 60000005 (nZCv daif -PAN -UAO)
[    0.942907] pc : imx_scu_call_rpc+0x114/0x158
[    0.947251] lr : imx_scu_irq_group_enable+0x74/0xc4
[    0.952113] sp : ffff00001005bae0
[    0.955415] x29: ffff00001005bae0 x28: ffff0000111bb0a0
[    0.960712] x27: ffff00001140b000 x26: ffff00001111068c
[    0.966011] x25: ffff0000111bb100 x24: 0000000000000000
[    0.971311] x23: ffff0000113d9cd8 x22: 0000000000000001
[    0.976610] x21: 0000000000000001 x20: ffff80083b51a410
[    0.981909] x19: ffff000011259000 x18: 0000000000000480
[    0.987209] x17: 000000000023ffb8 x16: 0000000000000010
[    0.992508] x15: 000000000000023f x14: ffffffffffffffff
[    0.997807] x13: 0000000000000018 x12: 0000000000000030
[    1.003107] x11: 0000000000000003 x10: 0101010101010101
[    1.008406] x9 : ffffffffffffffff x8 : 7f7f7f7f7f7f7f7f
[    1.013706] x7 : fefefeff646c606d x6 : 0000000000000000
[    1.019005] x5 : ffff0000112596c8 x4 : 0000000000000008
[    1.024304] x3 : 0000000000000003 x2 : 0000000000000001
[    1.029604] x1 : ffff00001005bb58 x0 : 0000000000000000
[    1.034905] Call trace:
[    1.037341]  imx_scu_call_rpc+0x114/0x158
[    1.041334]  imx_scu_irq_group_enable+0x74/0xc4
[    1.045856]  imx_sc_wdt_probe+0x24/0x150
[    1.049766]  platform_drv_probe+0x4c/0xb0
[    1.053762]  really_probe+0x1f8/0x2c8
[    1.057407]  driver_probe_device+0x58/0xfc
[    1.061490]  device_driver_attach+0x68/0x70
[    1.065660]  __driver_attach+0x94/0xdc
[    1.069397]  bus_for_each_dev+0x64/0xc0
[    1.073220]  driver_attach+0x20/0x28
[    1.076782]  bus_add_driver+0x148/0x1fc
[    1.080601]  driver_register+0x68/0x120
[    1.084424]  __platform_driver_register+0x4c/0x54
[    1.089120]  imx_sc_wdt_driver_init+0x18/0x20
[    1.093463]  do_one_initcall+0x58/0x1b8
[    1.097287]  kernel_init_freeable+0x1cc/0x288
[    1.101630]  kernel_init+0x10/0x100
[    1.105101]  ret_from_fork+0x10/0x18
[    1.108669] ---[ end trace 9e03302114457de9 ]---
[    1.113296] enable irq failed, group 1, mask 1, ret -22

To avoid such scenario, return -EPROBE_DEFER in imx_scu_irq_group_enable()
API if SCU IPC is NOT ready, then module driver which calls this API
in probe phase will defer probe after SCU IPC ready.

Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq function")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index 043833a..687121f 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -100,6 +100,9 @@ int imx_scu_irq_group_enable(u8 group, u32 mask, u8 enable)
 	struct imx_sc_rpc_msg *hdr = &msg.hdr;
 	int ret;
 
+	if (!imx_sc_irq_ipc_handle)
+		return -EPROBE_DEFER;
+
 	hdr->ver = IMX_SC_RPC_VERSION;
 	hdr->svc = IMX_SC_RPC_SVC_IRQ;
 	hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
-- 
2.7.4

[PATCH 2/2] watchdog: imx_sc: Add pretimeout support

[Anson Huang]

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.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 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;
+}
+
+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)))
+		return 0;
+
+	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;
+
 	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;
 
 	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");
+
 	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);
 
+	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,
-- 
2.7.4

Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support

[Guenter Roeck]

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@nxp.com>
> ---
>   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,
> 

RE: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support

[Anson Huang]

Hi, Guenter

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, April 25, 2019 12:04 PM
> To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> 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
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) ?  

> 
> Additional comment below.
> 
> Guenter
> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >   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

> 
> > +}
> > +
> > +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?

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?

> 
> > +
> >   	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,
> >

Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support

[Guenter Roeck]

On Thu, Apr 25, 2019 at 05:44:45AM +0000, Anson Huang wrote:
> Hi, Guenter
> 
> > -----Original Message-----
> > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: Thursday, April 25, 2019 12:04 PM
> > To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > watchdog@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > 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 <Anson.Huang@nxp.com>
> > > ---
> > >   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,
> > >
> 

RE: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support

[Anson Huang]

Hi, Shawn
	As patch 1/2 is an independent patch to improve the imx_scu_irq_group_enable() API, so I sent out V2 independently with some commit message improved, please help review, thanks.
Hi, Guenter
	I addressed your comments in V2 patch series, please help review, thanks.

Anson. 

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Saturday, April 27, 2019 4:17 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; wim@linux-watchdog.org;
> Aisheng Dong <aisheng.dong@nxp.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support
> 
> On Thu, Apr 25, 2019 at 05:44:45AM +0000, Anson Huang wrote:
> > Hi, Guenter
> >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > > Roeck
> > > Sent: Thursday, April 25, 2019 12:04 PM
> > > To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > > wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>;
> linux-
> > > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > > watchdog@vger.kernel.org
> > > Cc: dl-linux-imx <linux-imx@nxp.com>
> > > 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 <Anson.Huang@nxp.com>
> > > > ---
> > > >   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,
> > > >
> >

Re: [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready

[Guenter Roeck]

On 4/24/19 6:14 PM, Anson Huang wrote:
> The imx_scu_irq_group_enable() is normally called during module driver
> probe phase to enable SCU group irq, e.g., i.MX system controller
> watchdog driver will call it during its driver probe phase, as i.MX
> system controller watchdog driver does NOT need SCU IPC handle for
> operations, so it could be probed before i.MX SCU IPC is ready, and
> below dump will show out:
> 
> [    0.933001] Hardware name: Freescale i.MX8QXP MEK (DT)
> [    0.938129] pstate: 60000005 (nZCv daif -PAN -UAO)
> [    0.942907] pc : imx_scu_call_rpc+0x114/0x158
> [    0.947251] lr : imx_scu_irq_group_enable+0x74/0xc4
> [    0.952113] sp : ffff00001005bae0
> [    0.955415] x29: ffff00001005bae0 x28: ffff0000111bb0a0
> [    0.960712] x27: ffff00001140b000 x26: ffff00001111068c
> [    0.966011] x25: ffff0000111bb100 x24: 0000000000000000
> [    0.971311] x23: ffff0000113d9cd8 x22: 0000000000000001
> [    0.976610] x21: 0000000000000001 x20: ffff80083b51a410
> [    0.981909] x19: ffff000011259000 x18: 0000000000000480
> [    0.987209] x17: 000000000023ffb8 x16: 0000000000000010
> [    0.992508] x15: 000000000000023f x14: ffffffffffffffff
> [    0.997807] x13: 0000000000000018 x12: 0000000000000030
> [    1.003107] x11: 0000000000000003 x10: 0101010101010101
> [    1.008406] x9 : ffffffffffffffff x8 : 7f7f7f7f7f7f7f7f
> [    1.013706] x7 : fefefeff646c606d x6 : 0000000000000000
> [    1.019005] x5 : ffff0000112596c8 x4 : 0000000000000008
> [    1.024304] x3 : 0000000000000003 x2 : 0000000000000001
> [    1.029604] x1 : ffff00001005bb58 x0 : 0000000000000000
> [    1.034905] Call trace:
> [    1.037341]  imx_scu_call_rpc+0x114/0x158
> [    1.041334]  imx_scu_irq_group_enable+0x74/0xc4
> [    1.045856]  imx_sc_wdt_probe+0x24/0x150
> [    1.049766]  platform_drv_probe+0x4c/0xb0
> [    1.053762]  really_probe+0x1f8/0x2c8
> [    1.057407]  driver_probe_device+0x58/0xfc
> [    1.061490]  device_driver_attach+0x68/0x70
> [    1.065660]  __driver_attach+0x94/0xdc
> [    1.069397]  bus_for_each_dev+0x64/0xc0
> [    1.073220]  driver_attach+0x20/0x28
> [    1.076782]  bus_add_driver+0x148/0x1fc
> [    1.080601]  driver_register+0x68/0x120
> [    1.084424]  __platform_driver_register+0x4c/0x54
> [    1.089120]  imx_sc_wdt_driver_init+0x18/0x20
> [    1.093463]  do_one_initcall+0x58/0x1b8
> [    1.097287]  kernel_init_freeable+0x1cc/0x288
> [    1.101630]  kernel_init+0x10/0x100
> [    1.105101]  ret_from_fork+0x10/0x18
> [    1.108669] ---[ end trace 9e03302114457de9 ]---
> [    1.113296] enable irq failed, group 1, mask 1, ret -22
> 
> To avoid such scenario, return -EPROBE_DEFER in imx_scu_irq_group_enable()
> API if SCU IPC is NOT ready, then module driver which calls this API
> in probe phase will defer probe after SCU IPC ready.
> 
Difficult to comment - I seem to have missed the patch introducing
the call to imx_scu_irq_group_enable() from the watchdog driver, and
I don't see it in -next either.

However, as far as I can see, imx_sc_irq_ipc_handle is initialized from
imx_scu_probe(). If the watchdog driver depends on it, it should be a
sub-node of fsl,imx-scu, and be instantiated from the call to
devm_of_platform_populate() in imx_scu_probe(). This should make the
EPROBE_DEFER unnecessary.

In other words, the above statement "i.MX system controller watchdog
driver does NOT need SCU IPC handle for operations" is no longer accurate.
If it needs the interrupt, and the interrupt needs the IPC handle, the
watchdog driver does require the IPC handle. It would otherwise not need
to defer its probe function until the IPC handle is available.

I would like to add that it is highly unusual for a watchdog driver to depend
on a firmware driver. However, again, that is difficult to argue since I seem
to have missed the patch series introducing that dependency.

Guenter

> Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq function")
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>   drivers/firmware/imx/imx-scu-irq.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index 043833a..687121f 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -100,6 +100,9 @@ int imx_scu_irq_group_enable(u8 group, u32 mask, u8 enable)
>   	struct imx_sc_rpc_msg *hdr = &msg.hdr;
>   	int ret;
>   
> +	if (!imx_sc_irq_ipc_handle)
> +		return -EPROBE_DEFER;
> +
>   	hdr->ver = IMX_SC_RPC_VERSION;
>   	hdr->svc = IMX_SC_RPC_SVC_IRQ;
>   	hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
> 

RE: [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready

[Anson Huang]

Hi, Guenter

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, April 25, 2019 10:15 AM
> To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled
> after SCU IPC is ready
> 
> On 4/24/19 6:14 PM, Anson Huang wrote:
> > The imx_scu_irq_group_enable() is normally called during module driver
> > probe phase to enable SCU group irq, e.g., i.MX system controller
> > watchdog driver will call it during its driver probe phase, as i.MX
> > system controller watchdog driver does NOT need SCU IPC handle for
> > operations, so it could be probed before i.MX SCU IPC is ready, and
> > below dump will show out:
> >
> > [    0.933001] Hardware name: Freescale i.MX8QXP MEK (DT)
> > [    0.938129] pstate: 60000005 (nZCv daif -PAN -UAO)
> > [    0.942907] pc : imx_scu_call_rpc+0x114/0x158
> > [    0.947251] lr : imx_scu_irq_group_enable+0x74/0xc4
> > [    0.952113] sp : ffff00001005bae0
> > [    0.955415] x29: ffff00001005bae0 x28: ffff0000111bb0a0
> > [    0.960712] x27: ffff00001140b000 x26: ffff00001111068c
> > [    0.966011] x25: ffff0000111bb100 x24: 0000000000000000
> > [    0.971311] x23: ffff0000113d9cd8 x22: 0000000000000001
> > [    0.976610] x21: 0000000000000001 x20: ffff80083b51a410
> > [    0.981909] x19: ffff000011259000 x18: 0000000000000480
> > [    0.987209] x17: 000000000023ffb8 x16: 0000000000000010
> > [    0.992508] x15: 000000000000023f x14: ffffffffffffffff
> > [    0.997807] x13: 0000000000000018 x12: 0000000000000030
> > [    1.003107] x11: 0000000000000003 x10: 0101010101010101
> > [    1.008406] x9 : ffffffffffffffff x8 : 7f7f7f7f7f7f7f7f
> > [    1.013706] x7 : fefefeff646c606d x6 : 0000000000000000
> > [    1.019005] x5 : ffff0000112596c8 x4 : 0000000000000008
> > [    1.024304] x3 : 0000000000000003 x2 : 0000000000000001
> > [    1.029604] x1 : ffff00001005bb58 x0 : 0000000000000000
> > [    1.034905] Call trace:
> > [    1.037341]  imx_scu_call_rpc+0x114/0x158
> > [    1.041334]  imx_scu_irq_group_enable+0x74/0xc4
> > [    1.045856]  imx_sc_wdt_probe+0x24/0x150
> > [    1.049766]  platform_drv_probe+0x4c/0xb0
> > [    1.053762]  really_probe+0x1f8/0x2c8
> > [    1.057407]  driver_probe_device+0x58/0xfc
> > [    1.061490]  device_driver_attach+0x68/0x70
> > [    1.065660]  __driver_attach+0x94/0xdc
> > [    1.069397]  bus_for_each_dev+0x64/0xc0
> > [    1.073220]  driver_attach+0x20/0x28
> > [    1.076782]  bus_add_driver+0x148/0x1fc
> > [    1.080601]  driver_register+0x68/0x120
> > [    1.084424]  __platform_driver_register+0x4c/0x54
> > [    1.089120]  imx_sc_wdt_driver_init+0x18/0x20
> > [    1.093463]  do_one_initcall+0x58/0x1b8
> > [    1.097287]  kernel_init_freeable+0x1cc/0x288
> > [    1.101630]  kernel_init+0x10/0x100
> > [    1.105101]  ret_from_fork+0x10/0x18
> > [    1.108669] ---[ end trace 9e03302114457de9 ]---
> > [    1.113296] enable irq failed, group 1, mask 1, ret -22
> >
> > To avoid such scenario, return -EPROBE_DEFER in
> > imx_scu_irq_group_enable() API if SCU IPC is NOT ready, then module
> > driver which calls this API in probe phase will defer probe after SCU IPC
> ready.
> >
> Difficult to comment - I seem to have missed the patch introducing the call to
> imx_scu_irq_group_enable() from the watchdog driver, and I don't see it in -
> next either.
> 
> However, as far as I can see, imx_sc_irq_ipc_handle is initialized from
> imx_scu_probe(). If the watchdog driver depends on it, it should be a sub-
> node of fsl,imx-scu, and be instantiated from the call to
> devm_of_platform_populate() in imx_scu_probe(). This should make the
> EPROBE_DEFER unnecessary.
> 
> In other words, the above statement "i.MX system controller watchdog
> driver does NOT need SCU IPC handle for operations" is no longer accurate.
> If it needs the interrupt, and the interrupt needs the IPC handle, the
> watchdog driver does require the IPC handle. It would otherwise not need to
> defer its probe function until the IPC handle is available.
> 
> I would like to add that it is highly unusual for a watchdog driver to depend
> on a firmware driver. However, again, that is difficult to argue since I seem to
> have missed the patch series introducing that dependency.

I agreed watchdog should be a sub-node of SCU if it depends ON it, but as this API is
exposed, I think adding such check is necessary, we do NOT know when this API could
be called, so this protection should be necessary, it is just because I met this issue
during adding pretimeout function of i.MX SC wdog, so I put the patches together.

Thanks,
Anson

> 
> Guenter
> 
> > Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq
> > function")
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >   drivers/firmware/imx/imx-scu-irq.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/imx/imx-scu-irq.c
> > b/drivers/firmware/imx/imx-scu-irq.c
> > index 043833a..687121f 100644
> > --- a/drivers/firmware/imx/imx-scu-irq.c
> > +++ b/drivers/firmware/imx/imx-scu-irq.c
> > @@ -100,6 +100,9 @@ int imx_scu_irq_group_enable(u8 group, u32 mask,
> u8 enable)
> >   	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> >   	int ret;
> >
> > +	if (!imx_sc_irq_ipc_handle)
> > +		return -EPROBE_DEFER;
> > +
> >   	hdr->ver = IMX_SC_RPC_VERSION;
> >   	hdr->svc = IMX_SC_RPC_SVC_IRQ;
> >   	hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
> >

Re: [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready

[Guenter Roeck]

On Thu, Apr 25, 2019 at 06:51:11AM +0000, Anson Huang wrote:
> Hi, Guenter
> 
> > -----Original Message-----
> > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: Thursday, April 25, 2019 10:15 AM
> > To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > watchdog@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled
> > after SCU IPC is ready
> > 
> > On 4/24/19 6:14 PM, Anson Huang wrote:
> > > The imx_scu_irq_group_enable() is normally called during module driver
> > > probe phase to enable SCU group irq, e.g., i.MX system controller
> > > watchdog driver will call it during its driver probe phase, as i.MX
> > > system controller watchdog driver does NOT need SCU IPC handle for
> > > operations, so it could be probed before i.MX SCU IPC is ready, and
> > > below dump will show out:
> > >
> > > [    0.933001] Hardware name: Freescale i.MX8QXP MEK (DT)
> > > [    0.938129] pstate: 60000005 (nZCv daif -PAN -UAO)
> > > [    0.942907] pc : imx_scu_call_rpc+0x114/0x158
> > > [    0.947251] lr : imx_scu_irq_group_enable+0x74/0xc4
> > > [    0.952113] sp : ffff00001005bae0
> > > [    0.955415] x29: ffff00001005bae0 x28: ffff0000111bb0a0
> > > [    0.960712] x27: ffff00001140b000 x26: ffff00001111068c
> > > [    0.966011] x25: ffff0000111bb100 x24: 0000000000000000
> > > [    0.971311] x23: ffff0000113d9cd8 x22: 0000000000000001
> > > [    0.976610] x21: 0000000000000001 x20: ffff80083b51a410
> > > [    0.981909] x19: ffff000011259000 x18: 0000000000000480
> > > [    0.987209] x17: 000000000023ffb8 x16: 0000000000000010
> > > [    0.992508] x15: 000000000000023f x14: ffffffffffffffff
> > > [    0.997807] x13: 0000000000000018 x12: 0000000000000030
> > > [    1.003107] x11: 0000000000000003 x10: 0101010101010101
> > > [    1.008406] x9 : ffffffffffffffff x8 : 7f7f7f7f7f7f7f7f
> > > [    1.013706] x7 : fefefeff646c606d x6 : 0000000000000000
> > > [    1.019005] x5 : ffff0000112596c8 x4 : 0000000000000008
> > > [    1.024304] x3 : 0000000000000003 x2 : 0000000000000001
> > > [    1.029604] x1 : ffff00001005bb58 x0 : 0000000000000000
> > > [    1.034905] Call trace:
> > > [    1.037341]  imx_scu_call_rpc+0x114/0x158
> > > [    1.041334]  imx_scu_irq_group_enable+0x74/0xc4
> > > [    1.045856]  imx_sc_wdt_probe+0x24/0x150
> > > [    1.049766]  platform_drv_probe+0x4c/0xb0
> > > [    1.053762]  really_probe+0x1f8/0x2c8
> > > [    1.057407]  driver_probe_device+0x58/0xfc
> > > [    1.061490]  device_driver_attach+0x68/0x70
> > > [    1.065660]  __driver_attach+0x94/0xdc
> > > [    1.069397]  bus_for_each_dev+0x64/0xc0
> > > [    1.073220]  driver_attach+0x20/0x28
> > > [    1.076782]  bus_add_driver+0x148/0x1fc
> > > [    1.080601]  driver_register+0x68/0x120
> > > [    1.084424]  __platform_driver_register+0x4c/0x54
> > > [    1.089120]  imx_sc_wdt_driver_init+0x18/0x20
> > > [    1.093463]  do_one_initcall+0x58/0x1b8
> > > [    1.097287]  kernel_init_freeable+0x1cc/0x288
> > > [    1.101630]  kernel_init+0x10/0x100
> > > [    1.105101]  ret_from_fork+0x10/0x18
> > > [    1.108669] ---[ end trace 9e03302114457de9 ]---
> > > [    1.113296] enable irq failed, group 1, mask 1, ret -22
> > >
> > > To avoid such scenario, return -EPROBE_DEFER in
> > > imx_scu_irq_group_enable() API if SCU IPC is NOT ready, then module
> > > driver which calls this API in probe phase will defer probe after SCU IPC
> > ready.
> > >
> > Difficult to comment - I seem to have missed the patch introducing the call to
> > imx_scu_irq_group_enable() from the watchdog driver, and I don't see it in -
> > next either.
> > 
> > However, as far as I can see, imx_sc_irq_ipc_handle is initialized from
> > imx_scu_probe(). If the watchdog driver depends on it, it should be a sub-
> > node of fsl,imx-scu, and be instantiated from the call to
> > devm_of_platform_populate() in imx_scu_probe(). This should make the
> > EPROBE_DEFER unnecessary.
> > 
> > In other words, the above statement "i.MX system controller watchdog
> > driver does NOT need SCU IPC handle for operations" is no longer accurate.
> > If it needs the interrupt, and the interrupt needs the IPC handle, the
> > watchdog driver does require the IPC handle. It would otherwise not need to
> > defer its probe function until the IPC handle is available.
> > 
> > I would like to add that it is highly unusual for a watchdog driver to depend
> > on a firmware driver. However, again, that is difficult to argue since I seem to
> > have missed the patch series introducing that dependency.
> 
> I agreed watchdog should be a sub-node of SCU if it depends ON it, but as this API is
> exposed, I think adding such check is necessary, we do NOT know when this API could
> be called, so this protection should be necessary, it is just because I met this issue
> during adding pretimeout function of i.MX SC wdog, so I put the patches together.
> 
I agree that the check is necessary for imx_scu_irq_group_enable().
But the watchdog driver should still only be instantiated after
(or, rather, from) its parent driver since that is now the well
defined initialization order.

Guenter

> Thanks,
> Anson
> 
> > 
> > Guenter
> > 
> > > Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq
> > > function")
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >   drivers/firmware/imx/imx-scu-irq.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/imx/imx-scu-irq.c
> > > b/drivers/firmware/imx/imx-scu-irq.c
> > > index 043833a..687121f 100644
> > > --- a/drivers/firmware/imx/imx-scu-irq.c
> > > +++ b/drivers/firmware/imx/imx-scu-irq.c
> > > @@ -100,6 +100,9 @@ int imx_scu_irq_group_enable(u8 group, u32 mask,
> > u8 enable)
> > >   	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > >   	int ret;
> > >
> > > +	if (!imx_sc_irq_ipc_handle)
> > > +		return -EPROBE_DEFER;
> > > +
> > >   	hdr->ver = IMX_SC_RPC_VERSION;
> > >   	hdr->svc = IMX_SC_RPC_SVC_IRQ;
> > >   	hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
> > >
>