All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-11  7:47 ` Jingchang Lu
  0 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-11  7:47 UTC (permalink / raw)
  To: wim; +Cc: shawn.guo, arnd, linux-watchdog, linux-arm-kernel, Jingchang Lu

  Register the watchdog as the system restart function
to the new introducing kernel restart call chain in the
driver instead of providing the restart in machine desc.
  This restart handler function is from the mxc_restart()
in arch/arm/mach-imx/system.c

Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 68c3d37..fa723e8 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -22,14 +22,17 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/timer.h>
 #include <linux/watchdog.h>
@@ -59,6 +62,7 @@ struct imx2_wdt_device {
 	struct regmap *regmap;
 	struct timer_list timer;	/* Pings the watchdog when closed */
 	struct watchdog_device wdog;
+	struct notifier_block restart_handler;
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
 };
 
+static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
+				void *cmd)
+{
+	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
+	struct imx2_wdt_device *wdev = container_of(this,
+							struct imx2_wdt_device,
+							restart_handler);
+	/* Assert SRS signal */
+	regmap_write(wdev->regmap, 0, wcr_enable);
+	/*
+	 * Due to imx6q errata ERR004346 (WDOG: WDOG SRS bit requires to be
+	 * written twice), we add another two writes to ensure there must be at
+	 * least two writes happen in the same one 32kHz clock period.  We save
+	 * the target check here, since the writes shouldn't be a huge burden
+	 * for other platforms.
+	 */
+	regmap_write(wdev->regmap, 0, wcr_enable);
+	regmap_write(wdev->regmap, 0, wcr_enable);
+
+	/* wait for reset to assert... */
+	mdelay(500);
+
+	return NOTIFY_DONE;
+}
+
 static inline void imx2_wdt_setup(struct watchdog_device *wdog)
 {
 	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
@@ -257,6 +286,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	wdev->restart_handler.notifier_call = imx2_restart_handler;
+	wdev->restart_handler.priority = 128;
+	ret = register_restart_handler(&wdev->restart_handler);
+	if (ret)
+		dev_err(&pdev->dev, "cannot register restart handler\n");
+
 	dev_info(&pdev->dev, "timeout %d sec (nowayout=%d)\n",
 		 wdog->timeout, nowayout);
 
@@ -268,6 +303,8 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
 	struct watchdog_device *wdog = platform_get_drvdata(pdev);
 	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
 
+	unregister_restart_handler(&wdev->restart_handler);
+
 	watchdog_unregister_device(wdog);
 
 	if (imx2_wdt_is_running(wdev)) {
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-11  7:47 ` Jingchang Lu
  0 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-11  7:47 UTC (permalink / raw)
  To: linux-arm-kernel

  Register the watchdog as the system restart function
to the new introducing kernel restart call chain in the
driver instead of providing the restart in machine desc.
  This restart handler function is from the mxc_restart()
in arch/arm/mach-imx/system.c

Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 68c3d37..fa723e8 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -22,14 +22,17 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/timer.h>
 #include <linux/watchdog.h>
@@ -59,6 +62,7 @@ struct imx2_wdt_device {
 	struct regmap *regmap;
 	struct timer_list timer;	/* Pings the watchdog when closed */
 	struct watchdog_device wdog;
+	struct notifier_block restart_handler;
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
 };
 
+static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
+				void *cmd)
+{
+	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
+	struct imx2_wdt_device *wdev = container_of(this,
+							struct imx2_wdt_device,
+							restart_handler);
+	/* Assert SRS signal */
+	regmap_write(wdev->regmap, 0, wcr_enable);
+	/*
+	 * Due to imx6q errata ERR004346 (WDOG: WDOG SRS bit requires to be
+	 * written twice), we add another two writes to ensure there must be at
+	 * least two writes happen in the same one 32kHz clock period.  We save
+	 * the target check here, since the writes shouldn't be a huge burden
+	 * for other platforms.
+	 */
+	regmap_write(wdev->regmap, 0, wcr_enable);
+	regmap_write(wdev->regmap, 0, wcr_enable);
+
+	/* wait for reset to assert... */
+	mdelay(500);
+
+	return NOTIFY_DONE;
+}
+
 static inline void imx2_wdt_setup(struct watchdog_device *wdog)
 {
 	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
@@ -257,6 +286,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	wdev->restart_handler.notifier_call = imx2_restart_handler;
+	wdev->restart_handler.priority = 128;
+	ret = register_restart_handler(&wdev->restart_handler);
+	if (ret)
+		dev_err(&pdev->dev, "cannot register restart handler\n");
+
 	dev_info(&pdev->dev, "timeout %d sec (nowayout=%d)\n",
 		 wdog->timeout, nowayout);
 
@@ -268,6 +303,8 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
 	struct watchdog_device *wdog = platform_get_drvdata(pdev);
 	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
 
+	unregister_restart_handler(&wdev->restart_handler);
+
 	watchdog_unregister_device(wdog);
 
 	if (imx2_wdt_is_running(wdev)) {
-- 
1.8.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH] watchdog: imx2_wdt: add restart handler support
  2014-09-11  7:47 ` Jingchang Lu
@ 2014-09-11 15:55   ` Guenter Roeck
  -1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2014-09-11 15:55 UTC (permalink / raw)
  To: Jingchang Lu; +Cc: wim, shawn.guo, arnd, linux-watchdog, linux-arm-kernel

On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>   Register the watchdog as the system restart function
> to the new introducing kernel restart call chain in the
> driver instead of providing the restart in machine desc.
>   This restart handler function is from the mxc_restart()
> in arch/arm/mach-imx/system.c
> 
> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
> ---
>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 68c3d37..fa723e8 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -22,14 +22,17 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/timer.h>
>  #include <linux/watchdog.h>
> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>  	struct regmap *regmap;
>  	struct timer_list timer;	/* Pings the watchdog when closed */
>  	struct watchdog_device wdog;
> +	struct notifier_block restart_handler;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>  };
>  
> +static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
> +				void *cmd)
> +{
> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;

In the original code this value is conditional depending on cpu_is_mx1().
Are there any implications for this reset mechanism ? Does mx1 have a
different watchdog driver ?

> +	struct imx2_wdt_device *wdev = container_of(this,
> +							struct imx2_wdt_device,
> +							restart_handler);

Please align second lines with '('.

> +	/* Assert SRS signal */
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +	/*
> +	 * Due to imx6q errata ERR004346 (WDOG: WDOG SRS bit requires to be
> +	 * written twice), we add another two writes to ensure there must be at
> +	 * least two writes happen in the same one 32kHz clock period.  We save
> +	 * the target check here, since the writes shouldn't be a huge burden
> +	 * for other platforms.
> +	 */
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +
> +	/* wait for reset to assert... */
> +	mdelay(500);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  {
>  	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
> @@ -257,6 +286,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	wdev->restart_handler.notifier_call = imx2_restart_handler;
> +	wdev->restart_handler.priority = 128;
> +	ret = register_restart_handler(&wdev->restart_handler);
> +	if (ret)
> +		dev_err(&pdev->dev, "cannot register restart handler\n");
> +
>  	dev_info(&pdev->dev, "timeout %d sec (nowayout=%d)\n",
>  		 wdog->timeout, nowayout);
>  
> @@ -268,6 +303,8 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
>  	struct watchdog_device *wdog = platform_get_drvdata(pdev);
>  	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
>  
> +	unregister_restart_handler(&wdev->restart_handler);
> +
>  	watchdog_unregister_device(wdog);
>  
>  	if (imx2_wdt_is_running(wdev)) {
> -- 
> 1.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-11 15:55   ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2014-09-11 15:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>   Register the watchdog as the system restart function
> to the new introducing kernel restart call chain in the
> driver instead of providing the restart in machine desc.
>   This restart handler function is from the mxc_restart()
> in arch/arm/mach-imx/system.c
> 
> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
> ---
>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 68c3d37..fa723e8 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -22,14 +22,17 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/timer.h>
>  #include <linux/watchdog.h>
> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>  	struct regmap *regmap;
>  	struct timer_list timer;	/* Pings the watchdog when closed */
>  	struct watchdog_device wdog;
> +	struct notifier_block restart_handler;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>  };
>  
> +static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
> +				void *cmd)
> +{
> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;

In the original code this value is conditional depending on cpu_is_mx1().
Are there any implications for this reset mechanism ? Does mx1 have a
different watchdog driver ?

> +	struct imx2_wdt_device *wdev = container_of(this,
> +							struct imx2_wdt_device,
> +							restart_handler);

Please align second lines with '('.

> +	/* Assert SRS signal */
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +	/*
> +	 * Due to imx6q errata ERR004346 (WDOG: WDOG SRS bit requires to be
> +	 * written twice), we add another two writes to ensure there must be at
> +	 * least two writes happen in the same one 32kHz clock period.  We save
> +	 * the target check here, since the writes shouldn't be a huge burden
> +	 * for other platforms.
> +	 */
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +	regmap_write(wdev->regmap, 0, wcr_enable);
> +
> +	/* wait for reset to assert... */
> +	mdelay(500);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  {
>  	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
> @@ -257,6 +286,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	wdev->restart_handler.notifier_call = imx2_restart_handler;
> +	wdev->restart_handler.priority = 128;
> +	ret = register_restart_handler(&wdev->restart_handler);
> +	if (ret)
> +		dev_err(&pdev->dev, "cannot register restart handler\n");
> +
>  	dev_info(&pdev->dev, "timeout %d sec (nowayout=%d)\n",
>  		 wdog->timeout, nowayout);
>  
> @@ -268,6 +303,8 @@ static int __exit imx2_wdt_remove(struct platform_device *pdev)
>  	struct watchdog_device *wdog = platform_get_drvdata(pdev);
>  	struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
>  
> +	unregister_restart_handler(&wdev->restart_handler);
> +
>  	watchdog_unregister_device(wdog);
>  
>  	if (imx2_wdt_is_running(wdev)) {
> -- 
> 1.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [PATCH] watchdog: imx2_wdt: add restart handler support
  2014-09-11 15:55   ` Guenter Roeck
@ 2014-09-12  2:42     ` Jingchang Lu
  -1 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-12  2:42 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, Shawn Guo, arnd, linux-watchdog, linux-arm-kernel

>-----Original Message-----
>From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck
>Sent: Thursday, September 11, 2014 11:55 PM
>To: Lu Jingchang-B35083
>Cc: wim@iguana.be; Guo Shawn-R65073; arnd@arndb.de; linux-
>watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>
>On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>   Register the watchdog as the system restart function
>> to the new introducing kernel restart call chain in the
>> driver instead of providing the restart in machine desc.
>>   This restart handler function is from the mxc_restart()
>> in arch/arm/mach-imx/system.c
>>
>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>> ---
>>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 68c3d37..fa723e8 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -22,14 +22,17 @@
>>   */
>>
>>  #include <linux/clk.h>
>> +#include <linux/delay.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/moduleparam.h>
>> +#include <linux/notifier.h>
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>>  #include <linux/regmap.h>
>>  #include <linux/timer.h>
>>  #include <linux/watchdog.h>
>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>  	struct regmap *regmap;
>>  	struct timer_list timer;	/* Pings the watchdog when closed */
>>  	struct watchdog_device wdog;
>> +	struct notifier_block restart_handler;
>>  };
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>>  };
>>
>> +static int imx2_restart_handler(struct notifier_block *this, unsigned
>long mode,
>> +				void *cmd)
>> +{
>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>
>In the original code this value is conditional depending on cpu_is_mx1().
>Are there any implications for this reset mechanism ? Does mx1 have a
>different watchdog driver ?
>
Hi, Shawn, is this driver also used by the mx1 SoC? 
The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
restart function, I removed the cpu_is_mx1() condition determination, Thanks.


>> +	struct imx2_wdt_device *wdev = container_of(this,
>> +							struct imx2_wdt_device,
>> +							restart_handler);
>
>Please align second lines with '('.
Ok, thanks.

Best Regards,
Jingchang


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-12  2:42     ` Jingchang Lu
  0 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-12  2:42 UTC (permalink / raw)
  To: linux-arm-kernel

>-----Original Message-----
>From: Guenter Roeck [mailto:groeck7 at gmail.com] On Behalf Of Guenter Roeck
>Sent: Thursday, September 11, 2014 11:55 PM
>To: Lu Jingchang-B35083
>Cc: wim at iguana.be; Guo Shawn-R65073; arnd at arndb.de; linux-
>watchdog at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>
>On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>   Register the watchdog as the system restart function
>> to the new introducing kernel restart call chain in the
>> driver instead of providing the restart in machine desc.
>>   This restart handler function is from the mxc_restart()
>> in arch/arm/mach-imx/system.c
>>
>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>> ---
>>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 68c3d37..fa723e8 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -22,14 +22,17 @@
>>   */
>>
>>  #include <linux/clk.h>
>> +#include <linux/delay.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/moduleparam.h>
>> +#include <linux/notifier.h>
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>>  #include <linux/regmap.h>
>>  #include <linux/timer.h>
>>  #include <linux/watchdog.h>
>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>  	struct regmap *regmap;
>>  	struct timer_list timer;	/* Pings the watchdog when closed */
>>  	struct watchdog_device wdog;
>> +	struct notifier_block restart_handler;
>>  };
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>>  };
>>
>> +static int imx2_restart_handler(struct notifier_block *this, unsigned
>long mode,
>> +				void *cmd)
>> +{
>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>
>In the original code this value is conditional depending on cpu_is_mx1().
>Are there any implications for this reset mechanism ? Does mx1 have a
>different watchdog driver ?
>
Hi, Shawn, is this driver also used by the mx1 SoC? 
The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
restart function, I removed the cpu_is_mx1() condition determination, Thanks.


>> +	struct imx2_wdt_device *wdev = container_of(this,
>> +							struct imx2_wdt_device,
>> +							restart_handler);
>
>Please align second lines with '('.
Ok, thanks.

Best Regards,
Jingchang

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] watchdog: imx2_wdt: add restart handler support
  2014-09-12  2:42     ` Jingchang Lu
@ 2014-09-12  2:49       ` Guenter Roeck
  -1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2014-09-12  2:49 UTC (permalink / raw)
  To: Jingchang Lu; +Cc: wim, Shawn Guo, arnd, linux-watchdog, linux-arm-kernel

On 09/11/2014 07:42 PM, Jingchang Lu wrote:
>> -----Original Message-----
>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter Roeck
>> Sent: Thursday, September 11, 2014 11:55 PM
>> To: Lu Jingchang-B35083
>> Cc: wim@iguana.be; Guo Shawn-R65073; arnd@arndb.de; linux-
>> watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>>
>> On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>>    Register the watchdog as the system restart function
>>> to the new introducing kernel restart call chain in the
>>> driver instead of providing the restart in machine desc.
>>>    This restart handler function is from the mxc_restart()
>>> in arch/arm/mach-imx/system.c
>>>
>>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>>> ---
>>>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>>> index 68c3d37..fa723e8 100644
>>> --- a/drivers/watchdog/imx2_wdt.c
>>> +++ b/drivers/watchdog/imx2_wdt.c
>>> @@ -22,14 +22,17 @@
>>>    */
>>>
>>>   #include <linux/clk.h>
>>> +#include <linux/delay.h>
>>>   #include <linux/init.h>
>>>   #include <linux/io.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   #include <linux/moduleparam.h>
>>> +#include <linux/notifier.h>
>>>   #include <linux/of_address.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/reboot.h>
>>>   #include <linux/regmap.h>
>>>   #include <linux/timer.h>
>>>   #include <linux/watchdog.h>
>>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>>   	struct regmap *regmap;
>>>   	struct timer_list timer;	/* Pings the watchdog when closed */
>>>   	struct watchdog_device wdog;
>>> +	struct notifier_block restart_handler;
>>>   };
>>>
>>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>>> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>>>   	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>>>   };
>>>
>>> +static int imx2_restart_handler(struct notifier_block *this, unsigned
>> long mode,
>>> +				void *cmd)
>>> +{
>>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>>
>> In the original code this value is conditional depending on cpu_is_mx1().
>> Are there any implications for this reset mechanism ? Does mx1 have a
>> different watchdog driver ?
>>
> Hi, Shawn, is this driver also used by the mx1 SoC?
> The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
> restart function, I removed the cpu_is_mx1() condition determination, Thanks.
>
I assumed that the original code would be removed. If you have to keep it
around for mx1, the conversion doesn't seem to make much sense as you
just end up having the code twice. Am I missing something ?

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-12  2:49       ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2014-09-12  2:49 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/11/2014 07:42 PM, Jingchang Lu wrote:
>> -----Original Message-----
>> From: Guenter Roeck [mailto:groeck7 at gmail.com] On Behalf Of Guenter Roeck
>> Sent: Thursday, September 11, 2014 11:55 PM
>> To: Lu Jingchang-B35083
>> Cc: wim at iguana.be; Guo Shawn-R65073; arnd at arndb.de; linux-
>> watchdog at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>>
>> On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>>    Register the watchdog as the system restart function
>>> to the new introducing kernel restart call chain in the
>>> driver instead of providing the restart in machine desc.
>>>    This restart handler function is from the mxc_restart()
>>> in arch/arm/mach-imx/system.c
>>>
>>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>>> ---
>>>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>>> index 68c3d37..fa723e8 100644
>>> --- a/drivers/watchdog/imx2_wdt.c
>>> +++ b/drivers/watchdog/imx2_wdt.c
>>> @@ -22,14 +22,17 @@
>>>    */
>>>
>>>   #include <linux/clk.h>
>>> +#include <linux/delay.h>
>>>   #include <linux/init.h>
>>>   #include <linux/io.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   #include <linux/moduleparam.h>
>>> +#include <linux/notifier.h>
>>>   #include <linux/of_address.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/reboot.h>
>>>   #include <linux/regmap.h>
>>>   #include <linux/timer.h>
>>>   #include <linux/watchdog.h>
>>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>>   	struct regmap *regmap;
>>>   	struct timer_list timer;	/* Pings the watchdog when closed */
>>>   	struct watchdog_device wdog;
>>> +	struct notifier_block restart_handler;
>>>   };
>>>
>>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>>> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
>>>   	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
>>>   };
>>>
>>> +static int imx2_restart_handler(struct notifier_block *this, unsigned
>> long mode,
>>> +				void *cmd)
>>> +{
>>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>>
>> In the original code this value is conditional depending on cpu_is_mx1().
>> Are there any implications for this reset mechanism ? Does mx1 have a
>> different watchdog driver ?
>>
> Hi, Shawn, is this driver also used by the mx1 SoC?
> The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
> restart function, I removed the cpu_is_mx1() condition determination, Thanks.
>
I assumed that the original code would be removed. If you have to keep it
around for mx1, the conversion doesn't seem to make much sense as you
just end up having the code twice. Am I missing something ?

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [PATCH] watchdog: imx2_wdt: add restart handler support
  2014-09-12  2:49       ` Guenter Roeck
@ 2014-09-12  3:12         ` Jingchang Lu
  -1 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-12  3:12 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, Shawn Guo, arnd, linux-watchdog, linux-arm-kernel



>-----Original Message-----
>From: Guenter Roeck [mailto:linux@roeck-us.net]
>Sent: Friday, September 12, 2014 10:49 AM
>To: Lu Jingchang-B35083
>Cc: wim@iguana.be; Guo Shawn-R65073; arnd@arndb.de; linux-
>watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>
>On 09/11/2014 07:42 PM, Jingchang Lu wrote:
>>> -----Original Message-----
>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>>> Roeck
>>> Sent: Thursday, September 11, 2014 11:55 PM
>>> To: Lu Jingchang-B35083
>>> Cc: wim@iguana.be; Guo Shawn-R65073; arnd@arndb.de; linux-
>>> watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>>>
>>> On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>>>    Register the watchdog as the system restart function to the new
>>>> introducing kernel restart call chain in the driver instead of
>>>> providing the restart in machine desc.
>>>>    This restart handler function is from the mxc_restart() in
>>>> arch/arm/mach-imx/system.c
>>>>
>>>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>>>> ---
>>>>   drivers/watchdog/imx2_wdt.c | 37
>+++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/imx2_wdt.c
>>>> b/drivers/watchdog/imx2_wdt.c index 68c3d37..fa723e8 100644
>>>> --- a/drivers/watchdog/imx2_wdt.c
>>>> +++ b/drivers/watchdog/imx2_wdt.c
>>>> @@ -22,14 +22,17 @@
>>>>    */
>>>>
>>>>   #include <linux/clk.h>
>>>> +#include <linux/delay.h>
>>>>   #include <linux/init.h>
>>>>   #include <linux/io.h>
>>>>   #include <linux/jiffies.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/module.h>
>>>>   #include <linux/moduleparam.h>
>>>> +#include <linux/notifier.h>
>>>>   #include <linux/of_address.h>
>>>>   #include <linux/platform_device.h>
>>>> +#include <linux/reboot.h>
>>>>   #include <linux/regmap.h>
>>>>   #include <linux/timer.h>
>>>>   #include <linux/watchdog.h>
>>>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>>>   	struct regmap *regmap;
>>>>   	struct timer_list timer;	/* Pings the watchdog when
>closed */
>>>>   	struct watchdog_device wdog;
>>>> +	struct notifier_block restart_handler;
>>>>   };
>>>>
>>>>   static bool nowayout = WATCHDOG_NOWAYOUT; @@ -77,6 +81,31 @@
>>>> static const struct watchdog_info imx2_wdt_info = {
>>>>   	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
>WDIOF_MAGICCLOSE,
>>>>   };
>>>>
>>>> +static int imx2_restart_handler(struct notifier_block *this,
>>>> +unsigned
>>> long mode,
>>>> +				void *cmd)
>>>> +{
>>>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>>>
>>> In the original code this value is conditional depending on
>cpu_is_mx1().
>>> Are there any implications for this reset mechanism ? Does mx1 have a
>>> different watchdog driver ?
>>>
>> Hi, Shawn, is this driver also used by the mx1 SoC?
>> The imx2_wdt.c says it is for IMX2 and later processors, so when
>> moving the restart function, I removed the cpu_is_mx1() condition
>determination, Thanks.
>>
>I assumed that the original code would be removed. If you have to keep it
>around for mx1, the conversion doesn't seem to make much sense as you just
>end up having the code twice. Am I missing something ?
>
>Thanks,
>Guenter
 
Do you mean removing the code of mxc_restart() in arch/arm/mach-imx/system.c ?
Yes, when this patch is accepted, all platform compatible with the imx2_wdt can
migrate to the restart handler. I only add the restart handler for imx2_wdt and
reserve the original code for that still uses it currently. Thanks.

Best Regards,
Jingchang


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-12  3:12         ` Jingchang Lu
  0 siblings, 0 replies; 12+ messages in thread
From: Jingchang Lu @ 2014-09-12  3:12 UTC (permalink / raw)
  To: linux-arm-kernel



>-----Original Message-----
>From: Guenter Roeck [mailto:linux at roeck-us.net]
>Sent: Friday, September 12, 2014 10:49 AM
>To: Lu Jingchang-B35083
>Cc: wim at iguana.be; Guo Shawn-R65073; arnd at arndb.de; linux-
>watchdog at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>
>On 09/11/2014 07:42 PM, Jingchang Lu wrote:
>>> -----Original Message-----
>>> From: Guenter Roeck [mailto:groeck7 at gmail.com] On Behalf Of Guenter
>>> Roeck
>>> Sent: Thursday, September 11, 2014 11:55 PM
>>> To: Lu Jingchang-B35083
>>> Cc: wim at iguana.be; Guo Shawn-R65073; arnd at arndb.de; linux-
>>> watchdog at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>>> Subject: Re: [PATCH] watchdog: imx2_wdt: add restart handler support
>>>
>>> On Thu, Sep 11, 2014 at 03:47:41PM +0800, Jingchang Lu wrote:
>>>>    Register the watchdog as the system restart function to the new
>>>> introducing kernel restart call chain in the driver instead of
>>>> providing the restart in machine desc.
>>>>    This restart handler function is from the mxc_restart() in
>>>> arch/arm/mach-imx/system.c
>>>>
>>>> Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
>>>> ---
>>>>   drivers/watchdog/imx2_wdt.c | 37
>+++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/imx2_wdt.c
>>>> b/drivers/watchdog/imx2_wdt.c index 68c3d37..fa723e8 100644
>>>> --- a/drivers/watchdog/imx2_wdt.c
>>>> +++ b/drivers/watchdog/imx2_wdt.c
>>>> @@ -22,14 +22,17 @@
>>>>    */
>>>>
>>>>   #include <linux/clk.h>
>>>> +#include <linux/delay.h>
>>>>   #include <linux/init.h>
>>>>   #include <linux/io.h>
>>>>   #include <linux/jiffies.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/module.h>
>>>>   #include <linux/moduleparam.h>
>>>> +#include <linux/notifier.h>
>>>>   #include <linux/of_address.h>
>>>>   #include <linux/platform_device.h>
>>>> +#include <linux/reboot.h>
>>>>   #include <linux/regmap.h>
>>>>   #include <linux/timer.h>
>>>>   #include <linux/watchdog.h>
>>>> @@ -59,6 +62,7 @@ struct imx2_wdt_device {
>>>>   	struct regmap *regmap;
>>>>   	struct timer_list timer;	/* Pings the watchdog when
>closed */
>>>>   	struct watchdog_device wdog;
>>>> +	struct notifier_block restart_handler;
>>>>   };
>>>>
>>>>   static bool nowayout = WATCHDOG_NOWAYOUT; @@ -77,6 +81,31 @@
>>>> static const struct watchdog_info imx2_wdt_info = {
>>>>   	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
>WDIOF_MAGICCLOSE,
>>>>   };
>>>>
>>>> +static int imx2_restart_handler(struct notifier_block *this,
>>>> +unsigned
>>> long mode,
>>>> +				void *cmd)
>>>> +{
>>>> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
>>>
>>> In the original code this value is conditional depending on
>cpu_is_mx1().
>>> Are there any implications for this reset mechanism ? Does mx1 have a
>>> different watchdog driver ?
>>>
>> Hi, Shawn, is this driver also used by the mx1 SoC?
>> The imx2_wdt.c says it is for IMX2 and later processors, so when
>> moving the restart function, I removed the cpu_is_mx1() condition
>determination, Thanks.
>>
>I assumed that the original code would be removed. If you have to keep it
>around for mx1, the conversion doesn't seem to make much sense as you just
>end up having the code twice. Am I missing something ?
>
>Thanks,
>Guenter
 
Do you mean removing the code of mxc_restart() in arch/arm/mach-imx/system.c ?
Yes, when this patch is accepted, all platform compatible with the imx2_wdt can
migrate to the restart handler. I only add the restart handler for imx2_wdt and
reserve the original code for that still uses it currently. Thanks.

Best Regards,
Jingchang

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] watchdog: imx2_wdt: add restart handler support
  2014-09-12  2:42     ` Jingchang Lu
@ 2014-09-12  7:45       ` Shawn Guo
  -1 siblings, 0 replies; 12+ messages in thread
From: Shawn Guo @ 2014-09-12  7:45 UTC (permalink / raw)
  To: Lu Jingchang-B35083
  Cc: Guenter Roeck, wim, arnd, linux-watchdog, linux-arm-kernel

On Fri, Sep 12, 2014 at 10:42:10AM +0800, Lu Jingchang-B35083 wrote:
> >> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
> >>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> >>  };
> >>
> >> +static int imx2_restart_handler(struct notifier_block *this, unsigned
> >long mode,
> >> +				void *cmd)
> >> +{
> >> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
> >
> >In the original code this value is conditional depending on cpu_is_mx1().
> >Are there any implications for this reset mechanism ? Does mx1 have a
> >different watchdog driver ?
> >
> Hi, Shawn, is this driver also used by the mx1 SoC? 
> The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
> restart function, I removed the cpu_is_mx1() condition determination, Thanks.

I think the driver only works for i.MX2 and later ones.

Shawn

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] watchdog: imx2_wdt: add restart handler support
@ 2014-09-12  7:45       ` Shawn Guo
  0 siblings, 0 replies; 12+ messages in thread
From: Shawn Guo @ 2014-09-12  7:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 12, 2014 at 10:42:10AM +0800, Lu Jingchang-B35083 wrote:
> >> @@ -77,6 +81,31 @@ static const struct watchdog_info imx2_wdt_info = {
> >>  	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> >>  };
> >>
> >> +static int imx2_restart_handler(struct notifier_block *this, unsigned
> >long mode,
> >> +				void *cmd)
> >> +{
> >> +	unsigned int wcr_enable = IMX2_WDT_WCR_WDE;
> >
> >In the original code this value is conditional depending on cpu_is_mx1().
> >Are there any implications for this reset mechanism ? Does mx1 have a
> >different watchdog driver ?
> >
> Hi, Shawn, is this driver also used by the mx1 SoC? 
> The imx2_wdt.c says it is for IMX2 and later processors, so when moving the
> restart function, I removed the cpu_is_mx1() condition determination, Thanks.

I think the driver only works for i.MX2 and later ones.

Shawn

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-09-12  8:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-11  7:47 [PATCH] watchdog: imx2_wdt: add restart handler support Jingchang Lu
2014-09-11  7:47 ` Jingchang Lu
2014-09-11 15:55 ` Guenter Roeck
2014-09-11 15:55   ` Guenter Roeck
2014-09-12  2:42   ` Jingchang Lu
2014-09-12  2:42     ` Jingchang Lu
2014-09-12  2:49     ` Guenter Roeck
2014-09-12  2:49       ` Guenter Roeck
2014-09-12  3:12       ` Jingchang Lu
2014-09-12  3:12         ` Jingchang Lu
2014-09-12  7:45     ` Shawn Guo
2014-09-12  7:45       ` Shawn Guo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.