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

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

[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

[PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT

[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

[PATCH 2/2] watchdog: dw_wdt: add restart handler support

[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

Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT

[Guenter Roeck]

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
> 

Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support

[Guenter Roeck]

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

Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT

[Guenter Roeck]

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

Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support

[Guenter Roeck]

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

Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support

[Guenter Roeck]

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

Re: [PATCH 2/2] watchdog: dw_wdt: add restart handler support

[Jisheng Zhang]

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

Re: [PATCH 1/2] watchdog: dw_wdt: restart the counter immediately after enabling WDT

[Jisheng Zhang]

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