[PATCH 0/2] watchdog: dw_wdt: one bug fix and restart handler support

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/2] watchdog: dw_wdt: one bug fix and restart handler support
@ 2014-09-19  6:29 ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel, Jisheng Zhang

These patches are intend to improve dw_wdt in the following two aspects:

Firstly, the TOP_INIT may be zero at reset on some HW, so the timeout period
may be very short, thus we will see immediate system reset after openning 
the watchdog. Fix this problem by restarting the counter immediately after
WDT enabling.

Secondly, the WDT can also be used to reboot the system with the help of
recently introduced restart handler.

Tested on Marvell BG2Q DMP board.

Jisheng Zhang (2):
  watchdog: dw_wdt: restart the counter immediately after enabling WDT
  watchdog: dw_wdt: add restart handler support

 drivers/watchdog/dw_wdt.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 0/2] watchdog: dw_wdt: one bug fix and restart handler support
@ 2014-09-19  6:29 ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

These patches are intend to improve dw_wdt in the following two aspects:

Firstly, the TOP_INIT may be zero at reset on some HW, so the timeout period
may be very short, thus we will see immediate system reset after openning 
the watchdog. Fix this problem by restarting the counter immediately after
WDT enabling.

Secondly, the WDT can also be used to reboot the system with the help of
recently introduced restart handler.

Tested on Marvell BG2Q DMP board.

Jisheng Zhang (2):
  watchdog: dw_wdt: restart the counter immediately after enabling WDT
  watchdog: dw_wdt: add restart handler support

 drivers/watchdog/dw_wdt.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
  2014-09-19  6:29 ` Jisheng Zhang
@ 2014-09-19  6:29   ` Jisheng Zhang
  -1 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel, Jisheng Zhang

The TOP_INIT may be zero, so the timeout period may be very short after
initialization is done, thus the system may be reset soon after enabling.
We fix this problem by restarting the counter immediately after enabling
WDT.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/watchdog/dw_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 9f21029..ad0619d 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
 		dw_wdt_set_top(DW_WDT_MAX_TOP);
 		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
 		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+		dw_wdt_keepalive();
 	}
 
 	dw_wdt_set_next_heartbeat();
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
@ 2014-09-19  6:29   ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

The TOP_INIT may be zero, so the timeout period may be very short after
initialization is done, thus the system may be reset soon after enabling.
We fix this problem by restarting the counter immediately after enabling
WDT.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/watchdog/dw_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 9f21029..ad0619d 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
 		dw_wdt_set_top(DW_WDT_MAX_TOP);
 		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
 		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+		dw_wdt_keepalive();
 	}
 
 	dw_wdt_set_next_heartbeat();
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
  2014-09-19  6:29   ` Jisheng Zhang
@ 2014-09-19 17:27     ` Guenter Roeck
  -1 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-19 17:27 UTC (permalink / raw)
  To: Jisheng Zhang; +Cc: wim, linux-watchdog, linux-kernel, linux-arm-kernel

On Fri, Sep 19, 2014 at 02:29:58PM +0800, Jisheng Zhang wrote:
> The TOP_INIT may be zero, so the timeout period may be very short after

What is TOP_INIT ?

> initialization is done, thus the system may be reset soon after enabling.
> We fix this problem by restarting the counter immediately after enabling
> WDT.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/watchdog/dw_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 9f21029..ad0619d 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>  		dw_wdt_set_top(DW_WDT_MAX_TOP);

This sets the timeout to the maximum, so I guess there must be some other
event/register (TOP_INIT ?) which can be zero.

>  		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>  		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

What guarantees that the reset didn't already happen by the time
the keepalive call is executed ? Does this change fix the problem,
or does it just make it more unlikely to be seen ?

Thanks,
Guenter

> +		dw_wdt_keepalive();
>  	}
>  
>  	dw_wdt_set_next_heartbeat();
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
@ 2014-09-19 17:27     ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-19 17:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 19, 2014 at 02:29:58PM +0800, Jisheng Zhang wrote:
> The TOP_INIT may be zero, so the timeout period may be very short after

What is TOP_INIT ?

> initialization is done, thus the system may be reset soon after enabling.
> We fix this problem by restarting the counter immediately after enabling
> WDT.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/watchdog/dw_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 9f21029..ad0619d 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>  		dw_wdt_set_top(DW_WDT_MAX_TOP);

This sets the timeout to the maximum, so I guess there must be some other
event/register (TOP_INIT ?) which can be zero.

>  		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>  		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

What guarantees that the reset didn't already happen by the time
the keepalive call is executed ? Does this change fix the problem,
or does it just make it more unlikely to be seen ?

Thanks,
Guenter

> +		dw_wdt_keepalive();
>  	}
>  
>  	dw_wdt_set_next_heartbeat();
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
  2014-09-19  6:29   ` Jisheng Zhang
@ 2014-09-20 13:51     ` Guenter Roeck
  -1 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 13:51 UTC (permalink / raw)
  To: Jisheng Zhang, wim; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The TOP_INIT may be zero, so the timeout period may be very short after
> initialization is done, thus the system may be reset soon after enabling.
> We fix this problem by restarting the counter immediately after enabling
> WDT.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 9f21029..ad0619d 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>   		dw_wdt_set_top(DW_WDT_MAX_TOP);
>   		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>   		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +		dw_wdt_keepalive();
>   	}
>
>   	dw_wdt_set_next_heartbeat();
>
After getting access to the datasheet, I concluded that this fix is wrong
or at least more risky than necessary. The datasheet states that top_init,
ie bit 4-7 of the wdt_torr register, needs to be initialized with the desired
timeout period prior to enabling the watchdog. dw_wdt_set_top() sets it to
0 instead, ie to the lowest possible timeout period.

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
@ 2014-09-20 13:51     ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 13:51 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The TOP_INIT may be zero, so the timeout period may be very short after
> initialization is done, thus the system may be reset soon after enabling.
> We fix this problem by restarting the counter immediately after enabling
> WDT.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 9f21029..ad0619d 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
>   		dw_wdt_set_top(DW_WDT_MAX_TOP);
>   		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>   		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +		dw_wdt_keepalive();
>   	}
>
>   	dw_wdt_set_next_heartbeat();
>
After getting access to the datasheet, I concluded that this fix is wrong
or at least more risky than necessary. The datasheet states that top_init,
ie bit 4-7 of the wdt_torr register, needs to be initialized with the desired
timeout period prior to enabling the watchdog. dw_wdt_set_top() sets it to
0 instead, ie to the lowest possible timeout period.

Guenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
  2014-09-20 13:51     ` Guenter Roeck
@ 2014-09-23  7:45       ` Jisheng Zhang
  -1 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-23  7:45 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim@iguana.be, linux-watchdog@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org

Dear Guenter,

On Sat, 20 Sep 2014 06:51:56 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> > The TOP_INIT may be zero, so the timeout period may be very short after
> > initialization is done, thus the system may be reset soon after enabling.
> > We fix this problem by restarting the counter immediately after enabling
> > WDT.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/watchdog/dw_wdt.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 9f21029..ad0619d 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct
> > file *filp) dw_wdt_set_top(DW_WDT_MAX_TOP);
> >   		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> >   		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> > +		dw_wdt_keepalive();
> >   	}
> >
> >   	dw_wdt_set_next_heartbeat();
> >
> After getting access to the datasheet, I concluded that this fix is wrong
> or at least more risky than necessary. The datasheet states that top_init,
> ie bit 4-7 of the wdt_torr register, needs to be initialized with the
> desired timeout period prior to enabling the watchdog. dw_wdt_set_top()
> sets it to 0 instead, ie to the lowest possible timeout period.
> 

In patch V2, 
I initialize the TOP_INIT when setting TOP in function dw_wdt_set_top(), this
also can fix the "reboot soon" problem. Could you please have a review again?

Thanks,
Jisheng

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT
@ 2014-09-23  7:45       ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-23  7:45 UTC (permalink / raw)
  To: linux-arm-kernel

Dear Guenter,

On Sat, 20 Sep 2014 06:51:56 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> > The TOP_INIT may be zero, so the timeout period may be very short after
> > initialization is done, thus the system may be reset soon after enabling.
> > We fix this problem by restarting the counter immediately after enabling
> > WDT.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/watchdog/dw_wdt.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 9f21029..ad0619d 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct
> > file *filp) dw_wdt_set_top(DW_WDT_MAX_TOP);
> >   		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> >   		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> > +		dw_wdt_keepalive();
> >   	}
> >
> >   	dw_wdt_set_next_heartbeat();
> >
> After getting access to the datasheet, I concluded that this fix is wrong
> or at least more risky than necessary. The datasheet states that top_init,
> ie bit 4-7 of the wdt_torr register, needs to be initialized with the
> desired timeout period prior to enabling the watchdog. dw_wdt_set_top()
> sets it to 0 instead, ie to the lowest possible timeout period.
> 

In patch V2, 
I initialize the TOP_INIT when setting TOP in function dw_wdt_set_top(), this
also can fix the "reboot soon" problem. Could you please have a review again?

Thanks,
Jisheng

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
  2014-09-19  6:29 ` Jisheng Zhang
@ 2014-09-19  6:29   ` Jisheng Zhang
  -1 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel, Jisheng Zhang

The kernel core now provides an API to trigger a system restart.
Register with it to support restarting the system via. watchdog.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index ad0619d..4ca41e9 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -29,9 +29,11 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/uaccess.h>
@@ -62,6 +64,7 @@ static struct {
 	unsigned long		next_heartbeat;
 	struct timer_list	timer;
 	int			expect_close;
+	struct notifier_block	restart_handler;
 } dw_wdt;
 
 static inline int dw_wdt_is_enabled(void)
@@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
 	       WDOG_COUNTER_RESTART_REG_OFFSET);
 }
 
+static int dw_wdt_restart_handle(struct notifier_block *this,
+				unsigned long mode, void *cmd)
+{
+	u32 val;
+
+	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
+		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+			WDOG_COUNTER_RESTART_REG_OFFSET);
+	else
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	return NOTIFY_DONE;
+}
+
 static void dw_wdt_ping(unsigned long data)
 {
 	if (time_before(jiffies, dw_wdt.next_heartbeat) ||
@@ -315,6 +334,12 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_disable_clk;
 
+	dw_wdt.restart_handler.notifier_call = dw_wdt_restart_handle;
+	dw_wdt.restart_handler.priority = 128;
+	ret = register_restart_handler(&dw_wdt.restart_handler);
+	if (ret)
+		pr_warn("cannot register restart handler\n");
+
 	dw_wdt_set_next_heartbeat();
 	setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
 	mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
@@ -329,6 +354,8 @@ out_disable_clk:
 
 static int dw_wdt_drv_remove(struct platform_device *pdev)
 {
+	unregister_restart_handler(&dw_wdt.restart_handler);
+
 	misc_deregister(&dw_wdt_miscdev);
 
 	clk_disable_unprepare(dw_wdt.clk);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
@ 2014-09-19  6:29   ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-19  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

The kernel core now provides an API to trigger a system restart.
Register with it to support restarting the system via. watchdog.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index ad0619d..4ca41e9 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -29,9 +29,11 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/uaccess.h>
@@ -62,6 +64,7 @@ static struct {
 	unsigned long		next_heartbeat;
 	struct timer_list	timer;
 	int			expect_close;
+	struct notifier_block	restart_handler;
 } dw_wdt;
 
 static inline int dw_wdt_is_enabled(void)
@@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
 	       WDOG_COUNTER_RESTART_REG_OFFSET);
 }
 
+static int dw_wdt_restart_handle(struct notifier_block *this,
+				unsigned long mode, void *cmd)
+{
+	u32 val;
+
+	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
+		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+			WDOG_COUNTER_RESTART_REG_OFFSET);
+	else
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	return NOTIFY_DONE;
+}
+
 static void dw_wdt_ping(unsigned long data)
 {
 	if (time_before(jiffies, dw_wdt.next_heartbeat) ||
@@ -315,6 +334,12 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_disable_clk;
 
+	dw_wdt.restart_handler.notifier_call = dw_wdt_restart_handle;
+	dw_wdt.restart_handler.priority = 128;
+	ret = register_restart_handler(&dw_wdt.restart_handler);
+	if (ret)
+		pr_warn("cannot register restart handler\n");
+
 	dw_wdt_set_next_heartbeat();
 	setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
 	mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
@@ -329,6 +354,8 @@ out_disable_clk:
 
 static int dw_wdt_drv_remove(struct platform_device *pdev)
 {
+	unregister_restart_handler(&dw_wdt.restart_handler);
+
 	misc_deregister(&dw_wdt_miscdev);
 
 	clk_disable_unprepare(dw_wdt.clk);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support
  2014-09-19  6:29   ` Jisheng Zhang
@ 2014-09-20  3:07     ` Guenter Roeck
  -1 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20  3:07 UTC (permalink / raw)
  To: Jisheng Zhang, wim; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The kernel core now provides an API to trigger a system restart.
> Register with it to support restarting the system via. watchdog.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index ad0619d..4ca41e9 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -29,9 +29,11 @@
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>   #include <linux/of.h>
>   #include <linux/pm.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include <linux/uaccess.h>
> @@ -62,6 +64,7 @@ static struct {
>   	unsigned long		next_heartbeat;
>   	struct timer_list	timer;
>   	int			expect_close;
> +	struct notifier_block	restart_handler;
>   } dw_wdt;
>
>   static inline int dw_wdt_is_enabled(void)
> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>   	       WDOG_COUNTER_RESTART_REG_OFFSET);
>   }
>
> +static int dw_wdt_restart_handle(struct notifier_block *this,
> +				unsigned long mode, void *cmd)
> +{
> +	u32 val;
> +
> +	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> +		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> +			WDOG_COUNTER_RESTART_REG_OFFSET);
> +	else
> +		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> +		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
@ 2014-09-20  3:07     ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20  3:07 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The kernel core now provides an API to trigger a system restart.
> Register with it to support restarting the system via. watchdog.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index ad0619d..4ca41e9 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -29,9 +29,11 @@
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>   #include <linux/of.h>
>   #include <linux/pm.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include <linux/uaccess.h>
> @@ -62,6 +64,7 @@ static struct {
>   	unsigned long		next_heartbeat;
>   	struct timer_list	timer;
>   	int			expect_close;
> +	struct notifier_block	restart_handler;
>   } dw_wdt;
>
>   static inline int dw_wdt_is_enabled(void)
> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>   	       WDOG_COUNTER_RESTART_REG_OFFSET);
>   }
>
> +static int dw_wdt_restart_handle(struct notifier_block *this,
> +				unsigned long mode, void *cmd)
> +{
> +	u32 val;
> +
> +	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> +		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> +			WDOG_COUNTER_RESTART_REG_OFFSET);
> +	else
> +		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> +		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?

Guenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support
  2014-09-20  3:07     ` Guenter Roeck
@ 2014-09-20 14:08       ` Guenter Roeck
  -1 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 14:08 UTC (permalink / raw)
  To: Jisheng Zhang, wim; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel

On 09/19/2014 08:07 PM, Guenter Roeck wrote:
> On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
>> The kernel core now provides an API to trigger a system restart.
>> Register with it to support restarting the system via. watchdog.
>>
>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> ---
>>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>> index ad0619d..4ca41e9 100644
>> --- a/drivers/watchdog/dw_wdt.c
>> +++ b/drivers/watchdog/dw_wdt.c
>> @@ -29,9 +29,11 @@
>>   #include <linux/miscdevice.h>
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>> +#include <linux/notifier.h>
>>   #include <linux/of.h>
>>   #include <linux/pm.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/timer.h>
>>   #include <linux/uaccess.h>
>> @@ -62,6 +64,7 @@ static struct {
>>       unsigned long        next_heartbeat;
>>       struct timer_list    timer;
>>       int            expect_close;
>> +    struct notifier_block    restart_handler;
>>   } dw_wdt;
>>
>>   static inline int dw_wdt_is_enabled(void)
>> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>>              WDOG_COUNTER_RESTART_REG_OFFSET);
>>   }
>>
>> +static int dw_wdt_restart_handle(struct notifier_block *this,
>> +                unsigned long mode, void *cmd)
>> +{
>> +    u32 val;
>> +
>> +    writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>> +    val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
>> +    if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
>> +        writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
>> +            WDOG_COUNTER_RESTART_REG_OFFSET);
>> +    else
>> +        writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>> +               dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
>
> Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
> WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?
>

According to the datasheet, it should be sufficient to
- Write 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET to select the minimum timeout period
- Write 0x1 into WDOG_CONTROL_REG_OFFSET to enable the watchdog and select reset
   as desired action. This can be unconditional.

Writing into the restart register should not be necessary.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
@ 2014-09-20 14:08       ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/19/2014 08:07 PM, Guenter Roeck wrote:
> On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
>> The kernel core now provides an API to trigger a system restart.
>> Register with it to support restarting the system via. watchdog.
>>
>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> ---
>>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>> index ad0619d..4ca41e9 100644
>> --- a/drivers/watchdog/dw_wdt.c
>> +++ b/drivers/watchdog/dw_wdt.c
>> @@ -29,9 +29,11 @@
>>   #include <linux/miscdevice.h>
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>> +#include <linux/notifier.h>
>>   #include <linux/of.h>
>>   #include <linux/pm.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/timer.h>
>>   #include <linux/uaccess.h>
>> @@ -62,6 +64,7 @@ static struct {
>>       unsigned long        next_heartbeat;
>>       struct timer_list    timer;
>>       int            expect_close;
>> +    struct notifier_block    restart_handler;
>>   } dw_wdt;
>>
>>   static inline int dw_wdt_is_enabled(void)
>> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>>              WDOG_COUNTER_RESTART_REG_OFFSET);
>>   }
>>
>> +static int dw_wdt_restart_handle(struct notifier_block *this,
>> +                unsigned long mode, void *cmd)
>> +{
>> +    u32 val;
>> +
>> +    writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>> +    val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
>> +    if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
>> +        writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
>> +            WDOG_COUNTER_RESTART_REG_OFFSET);
>> +    else
>> +        writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>> +               dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
>
> Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
> WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?
>

According to the datasheet, it should be sufficient to
- Write 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET to select the minimum timeout period
- Write 0x1 into WDOG_CONTROL_REG_OFFSET to enable the watchdog and select reset
   as desired action. This can be unconditional.

Writing into the restart register should not be necessary.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support
  2014-09-20 14:08       ` Guenter Roeck
@ 2014-09-23  6:57         ` Jisheng Zhang
  -1 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-23  6:57 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim@iguana.be, linux-watchdog@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org

Dear Guenter,

On Sat, 20 Sep 2014 07:08:22 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 09/19/2014 08:07 PM, Guenter Roeck wrote:
> > On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> >> The kernel core now provides an API to trigger a system restart.
> >> Register with it to support restarting the system via. watchdog.
> >>
> >> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >> ---
> >>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
> >>   1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> >> index ad0619d..4ca41e9 100644
> >> --- a/drivers/watchdog/dw_wdt.c
> >> +++ b/drivers/watchdog/dw_wdt.c
> >> @@ -29,9 +29,11 @@
> >>   #include <linux/miscdevice.h>
> >>   #include <linux/module.h>
> >>   #include <linux/moduleparam.h>
> >> +#include <linux/notifier.h>
> >>   #include <linux/of.h>
> >>   #include <linux/pm.h>
> >>   #include <linux/platform_device.h>
> >> +#include <linux/reboot.h>
> >>   #include <linux/spinlock.h>
> >>   #include <linux/timer.h>
> >>   #include <linux/uaccess.h>
> >> @@ -62,6 +64,7 @@ static struct {
> >>       unsigned long        next_heartbeat;
> >>       struct timer_list    timer;
> >>       int            expect_close;
> >> +    struct notifier_block    restart_handler;
> >>   } dw_wdt;
> >>
> >>   static inline int dw_wdt_is_enabled(void)
> >> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
> >>              WDOG_COUNTER_RESTART_REG_OFFSET);
> >>   }
> >>
> >> +static int dw_wdt_restart_handle(struct notifier_block *this,
> >> +                unsigned long mode, void *cmd)
> >> +{
> >> +    u32 val;
> >> +
> >> +    writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >> +    val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> >> +    if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> >> +        writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> >> +            WDOG_COUNTER_RESTART_REG_OFFSET);
> >> +    else
> >> +        writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> >> +               dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

> Another comment: You should probably wait here for a bit to let the reset
> catch.

will do in next version. Thanks for pointing out

> >
> > Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
> > WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?
> >
> 
> According to the datasheet, it should be sufficient to
> - Write 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET to select the minimum timeout
> period
> - Write 0x1 into WDOG_CONTROL_REG_OFFSET to enable the watchdog and select
> reset as desired action. This can be unconditional.
> 
> Writing into the restart register should not be necessary.
> 

we want to handle the reboot case if wdt is in already use. In this case,
we dunno what's the timeout period, but obviously it's not as short as 2^16/clk
So we change the timeout period by writing 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET
to select the minimum timeout period. But per the datasheet, a change of the
timeout period takes effect only after the next counter restart. So we programming
the restart register to kick off next counter restart.

Thanks for reviewing,
Jisheng


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
@ 2014-09-23  6:57         ` Jisheng Zhang
  0 siblings, 0 replies; 20+ messages in thread
From: Jisheng Zhang @ 2014-09-23  6:57 UTC (permalink / raw)
  To: linux-arm-kernel

Dear Guenter,

On Sat, 20 Sep 2014 07:08:22 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 09/19/2014 08:07 PM, Guenter Roeck wrote:
> > On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> >> The kernel core now provides an API to trigger a system restart.
> >> Register with it to support restarting the system via. watchdog.
> >>
> >> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >> ---
> >>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
> >>   1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> >> index ad0619d..4ca41e9 100644
> >> --- a/drivers/watchdog/dw_wdt.c
> >> +++ b/drivers/watchdog/dw_wdt.c
> >> @@ -29,9 +29,11 @@
> >>   #include <linux/miscdevice.h>
> >>   #include <linux/module.h>
> >>   #include <linux/moduleparam.h>
> >> +#include <linux/notifier.h>
> >>   #include <linux/of.h>
> >>   #include <linux/pm.h>
> >>   #include <linux/platform_device.h>
> >> +#include <linux/reboot.h>
> >>   #include <linux/spinlock.h>
> >>   #include <linux/timer.h>
> >>   #include <linux/uaccess.h>
> >> @@ -62,6 +64,7 @@ static struct {
> >>       unsigned long        next_heartbeat;
> >>       struct timer_list    timer;
> >>       int            expect_close;
> >> +    struct notifier_block    restart_handler;
> >>   } dw_wdt;
> >>
> >>   static inline int dw_wdt_is_enabled(void)
> >> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
> >>              WDOG_COUNTER_RESTART_REG_OFFSET);
> >>   }
> >>
> >> +static int dw_wdt_restart_handle(struct notifier_block *this,
> >> +                unsigned long mode, void *cmd)
> >> +{
> >> +    u32 val;
> >> +
> >> +    writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >> +    val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> >> +    if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> >> +        writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> >> +            WDOG_COUNTER_RESTART_REG_OFFSET);
> >> +    else
> >> +        writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> >> +               dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

> Another comment: You should probably wait here for a bit to let the reset
> catch.

will do in next version. Thanks for pointing out

> >
> > Don't you have to write WDOG_COUNTER_RESTART_KICK_VALUE into
> > WDOG_COUNTER_RESTART_REG_OFFSET in the else case as well ?
> >
> 
> According to the datasheet, it should be sufficient to
> - Write 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET to select the minimum timeout
> period
> - Write 0x1 into WDOG_CONTROL_REG_OFFSET to enable the watchdog and select
> reset as desired action. This can be unconditional.
> 
> Writing into the restart register should not be necessary.
> 

we want to handle the reboot case if wdt is in already use. In this case,
we dunno what's the timeout period, but obviously it's not as short as 2^16/clk
So we change the timeout period by writing 0 into WDOG_TIMEOUT_RANGE_REG_OFFSET
to select the minimum timeout period. But per the datasheet, a change of the
timeout period takes effect only after the next counter restart. So we programming
the restart register to kick off next counter restart.

Thanks for reviewing,
Jisheng

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support
  2014-09-19  6:29   ` Jisheng Zhang
@ 2014-09-20 21:09     ` Guenter Roeck
  -1 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 21:09 UTC (permalink / raw)
  To: Jisheng Zhang, wim; +Cc: linux-watchdog, linux-kernel, linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The kernel core now provides an API to trigger a system restart.
> Register with it to support restarting the system via. watchdog.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index ad0619d..4ca41e9 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -29,9 +29,11 @@
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>   #include <linux/of.h>
>   #include <linux/pm.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include <linux/uaccess.h>
> @@ -62,6 +64,7 @@ static struct {
>   	unsigned long		next_heartbeat;
>   	struct timer_list	timer;
>   	int			expect_close;
> +	struct notifier_block	restart_handler;
>   } dw_wdt;
>
>   static inline int dw_wdt_is_enabled(void)
> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>   	       WDOG_COUNTER_RESTART_REG_OFFSET);
>   }
>
> +static int dw_wdt_restart_handle(struct notifier_block *this,
> +				unsigned long mode, void *cmd)
> +{
> +	u32 val;
> +
> +	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> +		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> +			WDOG_COUNTER_RESTART_REG_OFFSET);
> +	else
> +		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> +		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

Another comment: You should probably wait here for a bit to let the reset catch.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] watchdog: dw_wdt: add restart handler support
@ 2014-09-20 21:09     ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2014-09-20 21:09 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/18/2014 11:29 PM, Jisheng Zhang wrote:
> The kernel core now provides an API to trigger a system restart.
> Register with it to support restarting the system via. watchdog.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/watchdog/dw_wdt.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index ad0619d..4ca41e9 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -29,9 +29,11 @@
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
>   #include <linux/of.h>
>   #include <linux/pm.h>
>   #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include <linux/uaccess.h>
> @@ -62,6 +64,7 @@ static struct {
>   	unsigned long		next_heartbeat;
>   	struct timer_list	timer;
>   	int			expect_close;
> +	struct notifier_block	restart_handler;
>   } dw_wdt;
>
>   static inline int dw_wdt_is_enabled(void)
> @@ -119,6 +122,22 @@ static void dw_wdt_keepalive(void)
>   	       WDOG_COUNTER_RESTART_REG_OFFSET);
>   }
>
> +static int dw_wdt_restart_handle(struct notifier_block *this,
> +				unsigned long mode, void *cmd)
> +{
> +	u32 val;
> +
> +	writel(0, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	val = readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> +	if (val & WDOG_CONTROL_REG_WDT_EN_MASK)
> +		writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> +			WDOG_COUNTER_RESTART_REG_OFFSET);
> +	else
> +		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> +		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);

Another comment: You should probably wait here for a bit to let the reset catch.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2014-09-23  7:47 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-19  6:29 [PATCH 0/2] watchdog: dw_wdt: one bug fix and restart handler support Jisheng Zhang
2014-09-19  6:29 ` Jisheng Zhang
2014-09-19  6:29 ` [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT Jisheng Zhang
2014-09-19  6:29   ` Jisheng Zhang
2014-09-19 17:27   ` Guenter Roeck
2014-09-19 17:27     ` Guenter Roeck
2014-09-20 13:51   ` Guenter Roeck
2014-09-20 13:51     ` Guenter Roeck
2014-09-23  7:45     ` Jisheng Zhang
2014-09-23  7:45       ` Jisheng Zhang
2014-09-19  6:29 ` [PATCH 2/2] watchdog: dw_wdt: add restart handler support Jisheng Zhang
2014-09-19  6:29   ` Jisheng Zhang
2014-09-20  3:07   ` Guenter Roeck
2014-09-20  3:07     ` Guenter Roeck
2014-09-20 14:08     ` Guenter Roeck
2014-09-20 14:08       ` Guenter Roeck
2014-09-23  6:57       ` Jisheng Zhang
2014-09-23  6:57         ` Jisheng Zhang
2014-09-20 21:09   ` Guenter Roeck
2014-09-20 21:09     ` Guenter Roeck

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.