[PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[Doug Anderson]

On some dw_wdt implementations the "top" register may be initted to 0
at bootup.  In such a case, each "pat" of the watchdog will reset the
timer to 0xffff.  That's pretty short.

The input clock of the wdt can be any of a wide range of values.  On
an rk3288 system, I've seen the wdt clock be 24.75 MHz.  That means
each tick is ~40ns and we'll count to 0xffff in ~2.6ms.

Because of the above two facts, it's a really good idea to pat the
watchdog after initting the "top" register properly and before
enabling the watchdog.  If you don't then there's no way we'll get the
next heartbeat in time.

Jisheng Zhang fixed this problem on some dw_mmc versions by using the
TOP_INIT feature.  However, the dw_wdt on rk3288 doesn't have TOP_INIT
so it's a good idea to also pat the watchdog manually.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Add comment about why TOP_INIT doesn't work on rk3288; move pat
  to right next to the attempt to use TOP_INIT.

 drivers/watchdog/dw_wdt.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index b34a2e4..2c24882 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -96,6 +96,12 @@ static inline void dw_wdt_set_next_heartbeat(void)
 	dw_wdt.next_heartbeat = jiffies + dw_wdt_get_top() * HZ;
 }
 
+static void dw_wdt_keepalive(void)
+{
+	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+	       WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
 static int dw_wdt_set_top(unsigned top_s)
 {
 	int i, top_val = DW_WDT_MAX_TOP;
@@ -114,17 +120,18 @@ static int dw_wdt_set_top(unsigned top_s)
 	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
 		dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
+	/*
+	 * On some versions of dw_wdt writing to TOPINIT counts as a pat
+	 * (kick) of the watchdog; for other version of dw_wdt bits 4-7
+	 * are reserved and we need to pat the watchdog dog manually.
+	 */
+	dw_wdt_keepalive();
+
 	dw_wdt_set_next_heartbeat();
 
 	return dw_wdt_top_in_seconds(top_val);
 }
 
-static void dw_wdt_keepalive(void)
-{
-	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
-	       WDOG_COUNTER_RESTART_REG_OFFSET);
-}
-
 static int dw_wdt_restart_handle(struct notifier_block *this,
 				unsigned long mode, void *cmd)
 {
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v2 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default

[Doug Anderson]

The dw_wdt_set_top() function takes in a value in seconds.  In
dw_wdt_open() we were calling it with a value that's supposed to
represent the maximum value programmed into the "top" register with a
comment saying that we were trying to set the watchdog to its maximum
value.  Instead we ended up setting the watchdog to ~15 seconds.

Let's fix this.  However, setting things to the "max" gives me an 86
second watchdog in the system I'm looking at.  86 seconds feels a
little too long.  We'll explicitly choose 30 seconds as a more
reasonable value.

NOTE: Ideally this driver should be transitioned to be a real watchdog
driver.  Then we could use "watchdog_init_timeout" and let the timeout
be specified in a number of ways (device tree, module parameter, etc).
This patch should be considered a bit of a stopgap solution.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
Changes in v2: None

 drivers/watchdog/dw_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 2c24882..3a5155d 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -51,6 +51,8 @@
 /* The maximum TOP (timeout period) value that can be set in the watchdog. */
 #define DW_WDT_MAX_TOP		15
 
+#define DW_WDT_DEFAULT_SECONDS	30
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
@@ -174,9 +176,9 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
 	if (!dw_wdt_is_enabled()) {
 		/*
 		 * The watchdog is not currently enabled. Set the timeout to
-		 * the maximum and then start it.
+		 * something reasonable and then start it.
 		 */
-		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		dw_wdt_set_top(DW_WDT_DEFAULT_SECONDS);
 		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
 		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
 	}
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[Guenter Roeck]

On 01/26/2015 03:27 PM, Doug Anderson wrote:
> On some dw_wdt implementations the "top" register may be initted to 0
> at bootup.  In such a case, each "pat" of the watchdog will reset the
> timer to 0xffff.  That's pretty short.
>
> The input clock of the wdt can be any of a wide range of values.  On
> an rk3288 system, I've seen the wdt clock be 24.75 MHz.  That means
> each tick is ~40ns and we'll count to 0xffff in ~2.6ms.
>
> Because of the above two facts, it's a really good idea to pat the
> watchdog after initting the "top" register properly and before
> enabling the watchdog.  If you don't then there's no way we'll get the
> next heartbeat in time.
>
> Jisheng Zhang fixed this problem on some dw_mmc versions by using the
> TOP_INIT feature.  However, the dw_wdt on rk3288 doesn't have TOP_INIT
> so it's a good idea to also pat the watchdog manually.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[Jisheng Zhang]

Dear Doug,

On Mon, 26 Jan 2015 15:27:15 -0800
Doug Anderson <dianders@chromium.org> wrote:

> On some dw_wdt implementations the "top" register may be initted to 0
> at bootup.  In such a case, each "pat" of the watchdog will reset the
> timer to 0xffff.  That's pretty short.
> 
> The input clock of the wdt can be any of a wide range of values.  On
> an rk3288 system, I've seen the wdt clock be 24.75 MHz.  That means
> each tick is ~40ns and we'll count to 0xffff in ~2.6ms.
> 
> Because of the above two facts, it's a really good idea to pat the
> watchdog after initting the "top" register properly and before
> enabling the watchdog.  If you don't then there's no way we'll get the
> next heartbeat in time.
> 
> Jisheng Zhang fixed this problem on some dw_mmc versions by using the

s/dw_mmc/dw_wdt

> TOP_INIT feature.  However, the dw_wdt on rk3288 doesn't have TOP_INIT
> so it's a good idea to also pat the watchdog manually.

Based on your register dumping, I see the following configurations on rk3288:

WDT_DUAL_TOP is configured as false, so there's no TOP_INIT

WDT_DFLT_TOP is configured as 0, so it will timeout soon.


So an extra pat is a must on such platforms with similar configurations. And it
doesn't hurt anything if we have an extra pat before enabling the WDT

All in all, except the "dw_mmc" typo above, the patch looks good to me.

Thanks,
Jisheng

> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Add comment about why TOP_INIT doesn't work on rk3288; move pat
>   to right next to the attempt to use TOP_INIT.
> 
>  drivers/watchdog/dw_wdt.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index b34a2e4..2c24882 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -96,6 +96,12 @@ static inline void dw_wdt_set_next_heartbeat(void)
>  	dw_wdt.next_heartbeat = jiffies + dw_wdt_get_top() * HZ;
>  }
>  
> +static void dw_wdt_keepalive(void)
> +{
> +	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> +	       WDOG_COUNTER_RESTART_REG_OFFSET);
> +}
> +
>  static int dw_wdt_set_top(unsigned top_s)
>  {
>  	int i, top_val = DW_WDT_MAX_TOP;
> @@ -114,17 +120,18 @@ static int dw_wdt_set_top(unsigned top_s)
>  	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>  		dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
> +	/*
> +	 * On some versions of dw_wdt writing to TOPINIT counts as a pat
> +	 * (kick) of the watchdog; for other version of dw_wdt bits 4-7
> +	 * are reserved and we need to pat the watchdog dog manually.
> +	 */
> +	dw_wdt_keepalive();
> +
>  	dw_wdt_set_next_heartbeat();
>  
>  	return dw_wdt_top_in_seconds(top_val);
>  }
>  
> -static void dw_wdt_keepalive(void)
> -{
> -	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> -	       WDOG_COUNTER_RESTART_REG_OFFSET);
> -}
> -
>  static int dw_wdt_restart_handle(struct notifier_block *this,
>  				unsigned long mode, void *cmd)
>  {

Re: [PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[Guenter Roeck]

On 01/26/2015 07:49 PM, Jisheng Zhang wrote:
> Dear Doug,
>
> On Mon, 26 Jan 2015 15:27:15 -0800
> Doug Anderson <dianders@chromium.org> wrote:
>
>> On some dw_wdt implementations the "top" register may be initted to 0
>> at bootup.  In such a case, each "pat" of the watchdog will reset the
>> timer to 0xffff.  That's pretty short.
>>
>> The input clock of the wdt can be any of a wide range of values.  On
>> an rk3288 system, I've seen the wdt clock be 24.75 MHz.  That means
>> each tick is ~40ns and we'll count to 0xffff in ~2.6ms.
>>
>> Because of the above two facts, it's a really good idea to pat the
>> watchdog after initting the "top" register properly and before
>> enabling the watchdog.  If you don't then there's no way we'll get the
>> next heartbeat in time.
>>
>> Jisheng Zhang fixed this problem on some dw_mmc versions by using the
>
> s/dw_mmc/dw_wdt
>
>> TOP_INIT feature.  However, the dw_wdt on rk3288 doesn't have TOP_INIT
>> so it's a good idea to also pat the watchdog manually.
>
> Based on your register dumping, I see the following configurations on rk3288:
>
> WDT_DUAL_TOP is configured as false, so there's no TOP_INIT
>
> WDT_DFLT_TOP is configured as 0, so it will timeout soon.
>
>
> So an extra pat is a must on such platforms with similar configurations. And it
> doesn't hurt anything if we have an extra pat before enabling the WDT
>
> All in all, except the "dw_mmc" typo above, the patch looks good to me.
>

Jisheng,

it would be great if you can provide configuration information shown in
the (undocumented) registers.

Doug,

can you send another version with this information added as comment
to the code ? This will help others to understand what is going on
(and why) later on.

Thanks,
Guenter

Re: [PATCH v2 1/2] watchdog: dw_wdt: pat the watchdog before enabling it

[Jisheng Zhang]

Dear Guenter,

On Mon, 26 Jan 2015 20:08:04 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 01/26/2015 07:49 PM, Jisheng Zhang wrote:
> > Dear Doug,
> >
> > On Mon, 26 Jan 2015 15:27:15 -0800
> > Doug Anderson <dianders@chromium.org> wrote:
> >
> >> On some dw_wdt implementations the "top" register may be initted to 0
> >> at bootup.  In such a case, each "pat" of the watchdog will reset the
> >> timer to 0xffff.  That's pretty short.
> >>
> >> The input clock of the wdt can be any of a wide range of values.  On
> >> an rk3288 system, I've seen the wdt clock be 24.75 MHz.  That means
> >> each tick is ~40ns and we'll count to 0xffff in ~2.6ms.
> >>
> >> Because of the above two facts, it's a really good idea to pat the
> >> watchdog after initting the "top" register properly and before
> >> enabling the watchdog.  If you don't then there's no way we'll get the
> >> next heartbeat in time.
> >>
> >> Jisheng Zhang fixed this problem on some dw_mmc versions by using the
> >
> > s/dw_mmc/dw_wdt
> >
> >> TOP_INIT feature.  However, the dw_wdt on rk3288 doesn't have TOP_INIT
> >> so it's a good idea to also pat the watchdog manually.
> >
> > Based on your register dumping, I see the following configurations on
> > rk3288:
> >
> > WDT_DUAL_TOP is configured as false, so there's no TOP_INIT
> >
> > WDT_DFLT_TOP is configured as 0, so it will timeout soon.
> >
> >
> > So an extra pat is a must on such platforms with similar configurations.
> > And it doesn't hurt anything if we have an extra pat before enabling the
> > WDT
> >
> > All in all, except the "dw_mmc" typo above, the patch looks good to me.
> >
> 
> Jisheng,
> 
> it would be great if you can provide configuration information shown in
> the (undocumented) registers.

The wdt in rk3288 is a bit old, so I'm not sure whether the meaning is the same
or not. The key related configuration here is the so called CP_WDT_DUAL_TOP, bit[2]
of WDT_COMP_PARAMS_1 (0xf4), which indicates whether the TOP_INIT bits exist or
fixed as zero.

Thanks,
Jisheng

> 
> Doug,
> 
> can you send another version with this information added as comment
> to the code ? This will help others to understand what is going on
> (and why) later on.
> 
> Thanks,
> Guenter
>