[PATCH V3 1/3] dt-bindings: watchdog: move i.MX system controller watchdog binding to SCU

[PATCH V3 1/3] dt-bindings: watchdog: move i.MX system controller watchdog binding to SCU

[Anson Huang]

i.MX system controller watchdog depends on SCU driver to support
interrupt function, so it needs to be subnode of SCU node in DT,
binding doc should be moved to fsl,scu.txt as well.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V2:
	- no content change, just merge two dt-binding patches and improve the commit message.
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 15 ++++++++++++++
 .../bindings/watchdog/fsl-imx-sc-wdt.txt           | 24 ----------------------
 2 files changed, 15 insertions(+), 24 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx-sc-wdt.txt

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 72d481c..9967212 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -122,6 +122,16 @@ RTC bindings based on SCU Message Protocol
 Required properties:
 - compatible: should be "fsl,imx8qxp-sc-rtc";
 
+Watchdog bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be:
+              "fsl,imx8qxp-sc-wdt"
+              followed by "fsl,imx-sc-wdt";
+Optional properties:
+- timeout-sec: contains the watchdog timeout in seconds.
+
 Example (imx8qxp):
 -------------
 lsio_mu1: mailbox@5d1c0000 {
@@ -168,6 +178,11 @@ firmware {
 		rtc: rtc {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
+
+		watchdog {
+			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
+			timeout-sec = <60>;
+		};
 	};
 };
 
diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-sc-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-sc-wdt.txt
deleted file mode 100644
index 02b87e9..0000000
--- a/Documentation/devicetree/bindings/watchdog/fsl-imx-sc-wdt.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-* Freescale i.MX System Controller Watchdog
-
-i.MX system controller watchdog is for i.MX SoCs with system controller inside,
-the watchdog is managed by system controller, users can ONLY communicate with
-system controller from secure mode for watchdog operations, so Linux i.MX system
-controller watchdog driver will call ARM SMC API and trap into ARM-Trusted-Firmware
-for watchdog operations, ARM-Trusted-Firmware is running at secure EL3 mode and
-it will request system controller to execute the watchdog operation passed from
-Linux kernel.
-
-Required properties:
-- compatible:	Should be :
-		"fsl,imx8qxp-sc-wdt"
-		followed by "fsl,imx-sc-wdt";
-
-Optional properties:
-- timeout-sec : Contains the watchdog timeout in seconds.
-
-Examples:
-
-watchdog {
-	compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
-	timeout-sec = <60>;
-};
-- 
2.7.4

[PATCH V3 2/3] arm64: dts: imx8qxp: Move watchdog node into scu node

[Anson Huang]

i.MX system controller watchdog has pretimeout function which
depends on i.MX SCU driver, so it should be a subnode of SCU.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 0683ee2..b17c22e 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -149,6 +149,11 @@
 		rtc: rtc {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
+
+		watchdog {
+			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
+			timeout-sec = <60>;
+		};
 	};
 
 	timer {
@@ -517,9 +522,4 @@
 			power-domains = <&pd IMX_SC_R_GPIO_7>;
 		};
 	};
-
-	watchdog {
-		compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
-		timeout-sec = <60>;
-	};
 };
-- 
2.7.4

[PATCH V3 3/3] 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>
---
No change.
---
 drivers/watchdog/Kconfig      |   1 +
 drivers/watchdog/imx_sc_wdt.c | 114 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e19960a..dbc74e1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -710,6 +710,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..d8493a3 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,19 @@
 
 #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 imx_sc_wdt_device {
+	struct watchdog_device wdd;
+	struct notifier_block wdt_notifier;
+};
+
 static int imx_sc_wdt_ping(struct watchdog_device *wdog)
 {
 	struct arm_smccc_res res;
@@ -85,24 +94,66 @@ 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;
+
+	arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_PRETIME_WDOG,
+		      pretimeout * 1000, 0, 0, 0, 0, 0, &res);
+	if (res.a0)
+		return -EACCES;
+
+	wdog->pretimeout = pretimeout;
+
+	return 0;
+}
+
+static int imx_sc_wdt_notify(struct notifier_block *nb,
+			     unsigned long event, void *group)
+{
+	struct imx_sc_wdt_device *imx_sc_wdd =
+				 container_of(nb,
+					      struct imx_sc_wdt_device,
+					      wdt_notifier);
+
+	if (event & SC_IRQ_WDOG &&
+	    *(u8 *)group == SC_IRQ_GROUP_WDOG)
+		watchdog_notify_pretimeout(&imx_sc_wdd->wdd);
+
+	return 0;
+}
+
+static void imx_sc_wdt_action(void *data)
+{
+	struct notifier_block *wdt_notifier = data;
+
+	imx_scu_irq_unregister_notifier(wdt_notifier);
+	imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
+				 SC_IRQ_WDOG,
+				 false);
+}
+
 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 = {
+static struct watchdog_info imx_sc_wdt_info = {
 	.identity	= "i.MX SC watchdog timer",
 	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
-			  WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
+			  WDIOF_MAGICCLOSE,
 };
 
 static int imx_sc_wdt_probe(struct platform_device *pdev)
 {
+	struct imx_sc_wdt_device *imx_sc_wdd;
+	struct watchdog_device *wdog;
 	struct device *dev = &pdev->dev;
-	struct watchdog_device *imx_sc_wdd;
 	int ret;
 
 	imx_sc_wdd = devm_kzalloc(dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
@@ -111,42 +162,65 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, imx_sc_wdd);
 
-	imx_sc_wdd->info = &imx_sc_wdt_info;
-	imx_sc_wdd->ops = &imx_sc_wdt_ops;
-	imx_sc_wdd->min_timeout = 1;
-	imx_sc_wdd->max_timeout = MAX_TIMEOUT;
-	imx_sc_wdd->parent = dev;
-	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
+	wdog = &imx_sc_wdd->wdd;
+	wdog->info = &imx_sc_wdt_info;
+	wdog->ops = &imx_sc_wdt_ops;
+	wdog->min_timeout = 1;
+	wdog->max_timeout = MAX_TIMEOUT;
+	wdog->parent = dev;
+	wdog->timeout = DEFAULT_TIMEOUT;
 
-	watchdog_init_timeout(imx_sc_wdd, 0, dev);
-	watchdog_stop_on_reboot(imx_sc_wdd);
-	watchdog_stop_on_unregister(imx_sc_wdd);
+	watchdog_init_timeout(wdog, 0, dev);
+	watchdog_stop_on_reboot(wdog);
+	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, imx_sc_wdd);
+	ret = devm_watchdog_register_device(dev, wdog);
 	if (ret) {
 		dev_err(dev, "Failed to register watchdog device\n");
 		return ret;
 	}
 
-	return 0;
+	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
+				       SC_IRQ_WDOG,
+				       true);
+	if (ret) {
+		dev_warn(dev, "Enable irq failed, pretimeout NOT supported\n");
+		return 0;
+	}
+
+	imx_sc_wdd->wdt_notifier.notifier_call = imx_sc_wdt_notify;
+	ret = imx_scu_irq_register_notifier(&imx_sc_wdd->wdt_notifier);
+	if (ret) {
+		imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
+					 SC_IRQ_WDOG,
+					 false);
+		dev_warn(dev,
+			 "Register irq notifier failed, pretimeout NOT supported\n");
+		return 0;
+	}
+
+	imx_sc_wdt_info.options |= WDIOF_PRETIMEOUT;
+	ret = devm_add_action(dev, imx_sc_wdt_action,
+			      &imx_sc_wdd->wdt_notifier);
+	return ret;
 }
 
 static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
 {
-	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
+	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
 
-	if (watchdog_active(imx_sc_wdd))
-		imx_sc_wdt_stop(imx_sc_wdd);
+	if (watchdog_active(&imx_sc_wdd->wdd))
+		imx_sc_wdt_stop(&imx_sc_wdd->wdd);
 
 	return 0;
 }
 
 static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
 {
-	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
+	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
 
-	if (watchdog_active(imx_sc_wdd))
-		imx_sc_wdt_start(imx_sc_wdd);
+	if (watchdog_active(&imx_sc_wdd->wdd))
+		imx_sc_wdt_start(&imx_sc_wdd->wdd);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH V3 3/3] watchdog: imx_sc: Add pretimeout support

[Guenter Roeck]

On Sun, May 05, 2019 at 02:15:00AM +0000, 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.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> No change.
> ---
>  drivers/watchdog/Kconfig      |   1 +
>  drivers/watchdog/imx_sc_wdt.c | 114 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 95 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e19960a..dbc74e1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -710,6 +710,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..d8493a3 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,19 @@
>  
>  #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 imx_sc_wdt_device {
> +	struct watchdog_device wdd;
> +	struct notifier_block wdt_notifier;
> +};
> +
>  static int imx_sc_wdt_ping(struct watchdog_device *wdog)
>  {
>  	struct arm_smccc_res res;
> @@ -85,24 +94,66 @@ 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;
> +
> +	arm_smccc_smc(IMX_SIP_TIMER, IMX_SIP_TIMER_SET_PRETIME_WDOG,
> +		      pretimeout * 1000, 0, 0, 0, 0, 0, &res);
> +	if (res.a0)
> +		return -EACCES;
> +
> +	wdog->pretimeout = pretimeout;
> +
> +	return 0;
> +}
> +
> +static int imx_sc_wdt_notify(struct notifier_block *nb,
> +			     unsigned long event, void *group)
> +{
> +	struct imx_sc_wdt_device *imx_sc_wdd =
> +				 container_of(nb,
> +					      struct imx_sc_wdt_device,
> +					      wdt_notifier);
> +
> +	if (event & SC_IRQ_WDOG &&
> +	    *(u8 *)group == SC_IRQ_GROUP_WDOG)
> +		watchdog_notify_pretimeout(&imx_sc_wdd->wdd);
> +
> +	return 0;
> +}
> +
> +static void imx_sc_wdt_action(void *data)
> +{
> +	struct notifier_block *wdt_notifier = data;
> +
> +	imx_scu_irq_unregister_notifier(wdt_notifier);
> +	imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> +				 SC_IRQ_WDOG,
> +				 false);
> +}
> +
>  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 = {
> +static struct watchdog_info imx_sc_wdt_info = {
>  	.identity	= "i.MX SC watchdog timer",
>  	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> -			  WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
> +			  WDIOF_MAGICCLOSE,
>  };
>  
>  static int imx_sc_wdt_probe(struct platform_device *pdev)
>  {
> +	struct imx_sc_wdt_device *imx_sc_wdd;
> +	struct watchdog_device *wdog;
>  	struct device *dev = &pdev->dev;
> -	struct watchdog_device *imx_sc_wdd;
>  	int ret;
>  
>  	imx_sc_wdd = devm_kzalloc(dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
> @@ -111,42 +162,65 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, imx_sc_wdd);
>  
> -	imx_sc_wdd->info = &imx_sc_wdt_info;
> -	imx_sc_wdd->ops = &imx_sc_wdt_ops;
> -	imx_sc_wdd->min_timeout = 1;
> -	imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> -	imx_sc_wdd->parent = dev;
> -	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> +	wdog = &imx_sc_wdd->wdd;
> +	wdog->info = &imx_sc_wdt_info;
> +	wdog->ops = &imx_sc_wdt_ops;
> +	wdog->min_timeout = 1;
> +	wdog->max_timeout = MAX_TIMEOUT;
> +	wdog->parent = dev;
> +	wdog->timeout = DEFAULT_TIMEOUT;
>  
> -	watchdog_init_timeout(imx_sc_wdd, 0, dev);
> -	watchdog_stop_on_reboot(imx_sc_wdd);
> -	watchdog_stop_on_unregister(imx_sc_wdd);
> +	watchdog_init_timeout(wdog, 0, dev);
> +	watchdog_stop_on_reboot(wdog);
> +	watchdog_stop_on_unregister(wdog);
>  
> -	ret = devm_watchdog_register_device(dev, imx_sc_wdd);
> +	ret = devm_watchdog_register_device(dev, wdog);
>  	if (ret) {
>  		dev_err(dev, "Failed to register watchdog device\n");
>  		return ret;
>  	}
>  
> -	return 0;
> +	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> +				       SC_IRQ_WDOG,
> +				       true);
> +	if (ret) {
> +		dev_warn(dev, "Enable irq failed, pretimeout NOT supported\n");
> +		return 0;
> +	}
> +
> +	imx_sc_wdd->wdt_notifier.notifier_call = imx_sc_wdt_notify;
> +	ret = imx_scu_irq_register_notifier(&imx_sc_wdd->wdt_notifier);
> +	if (ret) {
> +		imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> +					 SC_IRQ_WDOG,
> +					 false);
> +		dev_warn(dev,
> +			 "Register irq notifier failed, pretimeout NOT supported\n");
> +		return 0;
> +	}
> +
> +	imx_sc_wdt_info.options |= WDIOF_PRETIMEOUT;
> +	ret = devm_add_action(dev, imx_sc_wdt_action,
> +			      &imx_sc_wdd->wdt_notifier);

This needs devm_add_action_or_reset() because otherwise the notifier would
not be removed, and the interrupt group would not be disabled.

> +	return ret;

This should not return an error. Accepting that the notifier can not be
registered and that the irq group can not be enabled but bailing out if
devm_add_action() fails is inconsistent.

Guenter

>  }
>  
>  static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
>  {
> -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
>  
> -	if (watchdog_active(imx_sc_wdd))
> -		imx_sc_wdt_stop(imx_sc_wdd);
> +	if (watchdog_active(&imx_sc_wdd->wdd))
> +		imx_sc_wdt_stop(&imx_sc_wdd->wdd);
>  
>  	return 0;
>  }
>  
>  static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
>  {
> -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> +	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
>  
> -	if (watchdog_active(imx_sc_wdd))
> -		imx_sc_wdt_start(imx_sc_wdd);
> +	if (watchdog_active(&imx_sc_wdd->wdd))
> +		imx_sc_wdt_start(&imx_sc_wdd->wdd);
>  
>  	return 0;
>  }
> -- 
> 2.7.4
> 

RE: [PATCH V3 3/3] 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: Tuesday, May 7, 2019 2:28 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; Aisheng Dong <aisheng.dong@nxp.com>;
> ulf.hansson@linaro.org; sboyd@kernel.org; Daniel Baluta
> <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org; dl-linux-
> imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V3 3/3] watchdog: imx_sc: Add pretimeout support
> 
> On Sun, May 05, 2019 at 02:15:00AM +0000, 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.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > No change.
> > ---
> >  drivers/watchdog/Kconfig      |   1 +
> >  drivers/watchdog/imx_sc_wdt.c | 114
> > ++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 95 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > e19960a..dbc74e1 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -710,6 +710,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..d8493a3 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,19 @@
> >
> >  #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 imx_sc_wdt_device {
> > +	struct watchdog_device wdd;
> > +	struct notifier_block wdt_notifier;
> > +};
> > +
> >  static int imx_sc_wdt_ping(struct watchdog_device *wdog)  {
> >  	struct arm_smccc_res res;
> > @@ -85,24 +94,66 @@ 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;
> > +
> > +	arm_smccc_smc(IMX_SIP_TIMER,
> IMX_SIP_TIMER_SET_PRETIME_WDOG,
> > +		      pretimeout * 1000, 0, 0, 0, 0, 0, &res);
> > +	if (res.a0)
> > +		return -EACCES;
> > +
> > +	wdog->pretimeout = pretimeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_sc_wdt_notify(struct notifier_block *nb,
> > +			     unsigned long event, void *group) {
> > +	struct imx_sc_wdt_device *imx_sc_wdd =
> > +				 container_of(nb,
> > +					      struct imx_sc_wdt_device,
> > +					      wdt_notifier);
> > +
> > +	if (event & SC_IRQ_WDOG &&
> > +	    *(u8 *)group == SC_IRQ_GROUP_WDOG)
> > +		watchdog_notify_pretimeout(&imx_sc_wdd->wdd);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_sc_wdt_action(void *data) {
> > +	struct notifier_block *wdt_notifier = data;
> > +
> > +	imx_scu_irq_unregister_notifier(wdt_notifier);
> > +	imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > +				 SC_IRQ_WDOG,
> > +				 false);
> > +}
> > +
> >  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 = {
> > +static struct watchdog_info imx_sc_wdt_info = {
> >  	.identity	= "i.MX SC watchdog timer",
> >  	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > -			  WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT,
> > +			  WDIOF_MAGICCLOSE,
> >  };
> >
> >  static int imx_sc_wdt_probe(struct platform_device *pdev)  {
> > +	struct imx_sc_wdt_device *imx_sc_wdd;
> > +	struct watchdog_device *wdog;
> >  	struct device *dev = &pdev->dev;
> > -	struct watchdog_device *imx_sc_wdd;
> >  	int ret;
> >
> >  	imx_sc_wdd = devm_kzalloc(dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
> @@
> > -111,42 +162,65 @@ static int imx_sc_wdt_probe(struct platform_device
> > *pdev)
> >
> >  	platform_set_drvdata(pdev, imx_sc_wdd);
> >
> > -	imx_sc_wdd->info = &imx_sc_wdt_info;
> > -	imx_sc_wdd->ops = &imx_sc_wdt_ops;
> > -	imx_sc_wdd->min_timeout = 1;
> > -	imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> > -	imx_sc_wdd->parent = dev;
> > -	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > +	wdog = &imx_sc_wdd->wdd;
> > +	wdog->info = &imx_sc_wdt_info;
> > +	wdog->ops = &imx_sc_wdt_ops;
> > +	wdog->min_timeout = 1;
> > +	wdog->max_timeout = MAX_TIMEOUT;
> > +	wdog->parent = dev;
> > +	wdog->timeout = DEFAULT_TIMEOUT;
> >
> > -	watchdog_init_timeout(imx_sc_wdd, 0, dev);
> > -	watchdog_stop_on_reboot(imx_sc_wdd);
> > -	watchdog_stop_on_unregister(imx_sc_wdd);
> > +	watchdog_init_timeout(wdog, 0, dev);
> > +	watchdog_stop_on_reboot(wdog);
> > +	watchdog_stop_on_unregister(wdog);
> >
> > -	ret = devm_watchdog_register_device(dev, imx_sc_wdd);
> > +	ret = devm_watchdog_register_device(dev, wdog);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to register watchdog device\n");
> >  		return ret;
> >  	}
> >
> > -	return 0;
> > +	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > +				       SC_IRQ_WDOG,
> > +				       true);
> > +	if (ret) {
> > +		dev_warn(dev, "Enable irq failed, pretimeout NOT
> supported\n");
> > +		return 0;
> > +	}
> > +
> > +	imx_sc_wdd->wdt_notifier.notifier_call = imx_sc_wdt_notify;
> > +	ret = imx_scu_irq_register_notifier(&imx_sc_wdd->wdt_notifier);
> > +	if (ret) {
> > +		imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > +					 SC_IRQ_WDOG,
> > +					 false);
> > +		dev_warn(dev,
> > +			 "Register irq notifier failed, pretimeout NOT
> supported\n");
> > +		return 0;
> > +	}
> > +
> > +	imx_sc_wdt_info.options |= WDIOF_PRETIMEOUT;
> > +	ret = devm_add_action(dev, imx_sc_wdt_action,
> > +			      &imx_sc_wdd->wdt_notifier);
> 
> This needs devm_add_action_or_reset() because otherwise the notifier
> would not be removed, and the interrupt group would not be disabled.

Right, fix it in V4.

> 
> > +	return ret;
> 
> This should not return an error. Accepting that the notifier can not be
> registered and that the irq group can not be enabled but bailing out if
> devm_add_action() fails is inconsistent.
> 

Agreed, the pretimeout function should ONLT be enabled after the notifier
register, irq group enable and add action all success, so I use below code
in V4, please help review, thanks,

+
+       ret = devm_add_action_or_reset(dev, imx_sc_wdt_action,
+                                      &imx_sc_wdd->wdt_notifier);
+       if (!ret)
+               imx_sc_wdt_info.options |= WDIOF_PRETIMEOUT;
+       else
+               dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
+
        return 0;

Anson.

> Guenter
> 
> >  }
> >
> >  static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)  {
> > -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > +	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
> >
> > -	if (watchdog_active(imx_sc_wdd))
> > -		imx_sc_wdt_stop(imx_sc_wdd);
> > +	if (watchdog_active(&imx_sc_wdd->wdd))
> > +		imx_sc_wdt_stop(&imx_sc_wdd->wdd);
> >
> >  	return 0;
> >  }
> >
> >  static int __maybe_unused imx_sc_wdt_resume(struct device *dev)  {
> > -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > +	struct imx_sc_wdt_device *imx_sc_wdd = dev_get_drvdata(dev);
> >
> > -	if (watchdog_active(imx_sc_wdd))
> > -		imx_sc_wdt_start(imx_sc_wdd);
> > +	if (watchdog_active(&imx_sc_wdd->wdd))
> > +		imx_sc_wdt_start(&imx_sc_wdd->wdd);
> >
> >  	return 0;
> >  }
> > --
> > 2.7.4
> >