LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
@ 2015-01-21 23:17 Doug Anderson
  2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Doug Anderson @ 2015-01-21 23:17 UTC (permalink / raw)
  To: Wim Van Sebroeck
  Cc: Heiko Stuebner, Lunxue Dai, Jisheng Zhang, Dinh Nguyen,
	Doug Anderson, linux-watchdog, linux-kernel

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.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 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 b34a2e4..fc92bea 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -170,6 +170,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp)
 		 * the maximum and then start it.
 		 */
 		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		dw_wdt_keepalive();
 		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
 		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
 	}
-- 
2.2.0.rc0.207.ga3a616c


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

* [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default
  2015-01-21 23:17 [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Doug Anderson
@ 2015-01-21 23:17 ` Doug Anderson
  2015-01-21 23:36   ` Guenter Roeck
  2015-01-21 23:35 ` [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Guenter Roeck
  2015-01-22  5:22 ` Jisheng Zhang
  2 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2015-01-21 23:17 UTC (permalink / raw)
  To: Wim Van Sebroeck
  Cc: Heiko Stuebner, Lunxue Dai, Jisheng Zhang, Dinh Nguyen,
	Doug Anderson, linux-watchdog, linux-kernel

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>
---
 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 fc92bea..cc2805d 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 "
@@ -167,9 +169,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);
 		dw_wdt_keepalive();
 		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
 		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
-- 
2.2.0.rc0.207.ga3a616c


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-21 23:17 [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Doug Anderson
  2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
@ 2015-01-21 23:35 ` Guenter Roeck
  2015-01-22  5:22 ` Jisheng Zhang
  2 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2015-01-21 23:35 UTC (permalink / raw)
  To: Doug Anderson, Wim Van Sebroeck
  Cc: Heiko Stuebner, Lunxue Dai, Jisheng Zhang, Dinh Nguyen,
	linux-watchdog, linux-kernel

On 01/21/2015 03:17 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.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---

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


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

* Re: [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default
  2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
@ 2015-01-21 23:36   ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2015-01-21 23:36 UTC (permalink / raw)
  To: Doug Anderson, Wim Van Sebroeck
  Cc: Heiko Stuebner, Lunxue Dai, Jisheng Zhang, Dinh Nguyen,
	linux-watchdog, linux-kernel

On 01/21/2015 03:17 PM, Doug Anderson wrote:
> 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>



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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-21 23:17 [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Doug Anderson
  2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
  2015-01-21 23:35 ` [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Guenter Roeck
@ 2015-01-22  5:22 ` Jisheng Zhang
  2015-01-22  5:31   ` Guenter Roeck
  2015-01-22 17:09   ` Doug Anderson
  2 siblings, 2 replies; 15+ messages in thread
From: Jisheng Zhang @ 2015-01-22  5:22 UTC (permalink / raw)
  To: Doug Anderson, linux
  Cc: Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai, Dinh Nguyen,
	linux-watchdog, linux-kernel

Dear Doug,

On Wed, 21 Jan 2015 15:17:22 -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.

+ Guenter Roeck

This should have been fixed by dfa07141e7a792("watchdog: dw_wdt: initialise
TOP_INIT in dw_wdt_set_top()")

In fact, my original fix is as similar as your patch

http://www.spinics.net/lists/arm-kernel/msg363658.html

Then Guenter Roeck suggest one elegant solution which is the base of
commit dfa07141e7a792.

http://www.spinics.net/lists/arm-kernel/msg363908.html

> 
> 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.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  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 b34a2e4..fc92bea 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -170,6 +170,7 @@ static int dw_wdt_open(struct inode *inode, struct file
> *filp)
>  		 * the maximum and then start it.
>  		 */
>  		dw_wdt_set_top(DW_WDT_MAX_TOP);
> +		dw_wdt_keepalive();
>  		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
>  		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
>  	}


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-22  5:22 ` Jisheng Zhang
@ 2015-01-22  5:31   ` Guenter Roeck
  2015-01-22 17:09   ` Doug Anderson
  1 sibling, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2015-01-22  5:31 UTC (permalink / raw)
  To: Jisheng Zhang, Doug Anderson
  Cc: Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai, Dinh Nguyen,
	linux-watchdog, linux-kernel

On 01/21/2015 09:22 PM, Jisheng Zhang wrote:
> Dear Doug,
>
> On Wed, 21 Jan 2015 15:17:22 -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.
>
> + Guenter Roeck
>
> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt: initialise
> TOP_INIT in dw_wdt_set_top()")
>
Ah, yes. Doug, can you check if your patch is still needed ?

Thanks,
Guenter


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-22  5:22 ` Jisheng Zhang
  2015-01-22  5:31   ` Guenter Roeck
@ 2015-01-22 17:09   ` Doug Anderson
  2015-01-23 16:03     ` Guenter Roeck
  2015-01-26  6:22     ` Jisheng Zhang
  1 sibling, 2 replies; 15+ messages in thread
From: Doug Anderson @ 2015-01-22 17:09 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Guenter Roeck, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

Jisheng,

On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> Dear Doug,
>
> On Wed, 21 Jan 2015 15:17:22 -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.
>
> + Guenter Roeck
>
> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt: initialise
> TOP_INIT in dw_wdt_set_top()")

I will admit that I'm testing on a tree that doesn't have your patch
(I'm on a 3.14 kernel with lots of backports).  ...but I did try
cherry-picking your patch before I wrote up mine and it didn't fix my
problem.  I believe that the watchdog that's in Rockchip rk3288 must
be a slightly different version of the IP block than you're working
with.

Specifically I see the register WDT_TORR that has an offset of 0x4.
That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
"reserved".


> In fact, my original fix is as similar as your patch
>
> http://www.spinics.net/lists/arm-kernel/msg363658.html

Yup, except that I pat the watchdog before enabling it and you pat it
after...  It probably doesn't matter as long as the two instructions
are within 2.5ms of each other, but it seems nice to be safer.

-Doug

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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-22 17:09   ` Doug Anderson
@ 2015-01-23 16:03     ` Guenter Roeck
  2015-01-23 16:20       ` Doug Anderson
  2015-01-26  6:22     ` Jisheng Zhang
  1 sibling, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2015-01-23 16:03 UTC (permalink / raw)
  To: Doug Anderson, Jisheng Zhang
  Cc: Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai, Dinh Nguyen,
	linux-watchdog, linux-kernel

On 01/22/2015 09:09 AM, Doug Anderson wrote:
> Jisheng,
>
> On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>> Dear Doug,
>>
>> On Wed, 21 Jan 2015 15:17:22 -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.
>>
>> + Guenter Roeck
>>
>> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt: initialise
>> TOP_INIT in dw_wdt_set_top()")
>
> I will admit that I'm testing on a tree that doesn't have your patch
> (I'm on a 3.14 kernel with lots of backports).  ...but I did try
> cherry-picking your patch before I wrote up mine and it didn't fix my
> problem.  I believe that the watchdog that's in Rockchip rk3288 must
> be a slightly different version of the IP block than you're working
> with.
>
> Specifically I see the register WDT_TORR that has an offset of 0x4.
> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
> "reserved".
>
Not sure where that leaves us. Does that mean the driver supports different
hardware with different register sets ? Should that be documented in the driver,
and should we have (or do we need) different compatible statements for those
variants, and conditional code in the driver ?

And does it mean we need both patches, at least for some of the hardware
variants ? If so, what happens if those patches are applied and the resulting
driver runs on the other hardware ?

Thanks,
Guenter


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-23 16:03     ` Guenter Roeck
@ 2015-01-23 16:20       ` Doug Anderson
  2015-01-23 17:02         ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2015-01-23 16:20 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jisheng Zhang, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

Guenter,

On Fri, Jan 23, 2015 at 8:03 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/22/2015 09:09 AM, Doug Anderson wrote:
>>
>> Jisheng,
>>
>> On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com>
>> wrote:
>>>
>>> Dear Doug,
>>>
>>> On Wed, 21 Jan 2015 15:17:22 -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.
>>>
>>>
>>> + Guenter Roeck
>>>
>>> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt:
>>> initialise
>>> TOP_INIT in dw_wdt_set_top()")
>>
>>
>> I will admit that I'm testing on a tree that doesn't have your patch
>> (I'm on a 3.14 kernel with lots of backports).  ...but I did try
>> cherry-picking your patch before I wrote up mine and it didn't fix my
>> problem.  I believe that the watchdog that's in Rockchip rk3288 must
>> be a slightly different version of the IP block than you're working
>> with.
>>
>> Specifically I see the register WDT_TORR that has an offset of 0x4.
>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
>> "reserved".
>>
> Not sure where that leaves us. Does that mean the driver supports different
> hardware with different register sets ?

Apparently so.  I've only seen the documentation from rk3288, but it's
clearly different than what you saw.

>  Should that be documented in the
> driver,

Probably not a terrible idea.

> and should we have (or do we need) different compatible statements for those
> variants, and conditional code in the driver ?

I'm not sure we actually need any conditional code.  I've put the
other patch on rk3288 and it didn't hurt to write those reserved bits.
I also can't quite believe that the extra pat will hurt on other
hardware.


> And does it mean we need both patches, at least for some of the hardware
> variants ? If so, what happens if those patches are applied and the
> resulting
> driver runs on the other hardware ?

I think it should be fine.


Do you want me to spin my patch and add some extra comments (but
otherwise keep it roughly unchanged?).  We can get Jisheng to add his
Tested-by...

-Doug

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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-23 16:20       ` Doug Anderson
@ 2015-01-23 17:02         ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2015-01-23 17:02 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Jisheng Zhang, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

On 01/23/2015 08:20 AM, Doug Anderson wrote:
> Guenter,
>
> On Fri, Jan 23, 2015 at 8:03 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/22/2015 09:09 AM, Doug Anderson wrote:
>>>
>>> Jisheng,
>>>
>>> On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com>
>>> wrote:
>>>>
>>>> Dear Doug,
>>>>
>>>> On Wed, 21 Jan 2015 15:17:22 -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.
>>>>
>>>>
>>>> + Guenter Roeck
>>>>
>>>> This should have been fixed by dfa07141e7a792("watchdog: dw_wdt:
>>>> initialise
>>>> TOP_INIT in dw_wdt_set_top()")
>>>
>>>
>>> I will admit that I'm testing on a tree that doesn't have your patch
>>> (I'm on a 3.14 kernel with lots of backports).  ...but I did try
>>> cherry-picking your patch before I wrote up mine and it didn't fix my
>>> problem.  I believe that the watchdog that's in Rockchip rk3288 must
>>> be a slightly different version of the IP block than you're working
>>> with.
>>>
>>> Specifically I see the register WDT_TORR that has an offset of 0x4.
>>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
>>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
>>> "reserved".
>>>
>> Not sure where that leaves us. Does that mean the driver supports different
>> hardware with different register sets ?
>
> Apparently so.  I've only seen the documentation from rk3288, but it's
> clearly different than what you saw.
>
>>   Should that be documented in the
>> driver,
>
> Probably not a terrible idea.
>
>> and should we have (or do we need) different compatible statements for those
>> variants, and conditional code in the driver ?
>
> I'm not sure we actually need any conditional code.  I've put the
> other patch on rk3288 and it didn't hurt to write those reserved bits.
> I also can't quite believe that the extra pat will hurt on other
> hardware.
>
>
>> And does it mean we need both patches, at least for some of the hardware
>> variants ? If so, what happens if those patches are applied and the
>> resulting
>> driver runs on the other hardware ?
>
> I think it should be fine.
>
>
> Do you want me to spin my patch and add some extra comments (but
> otherwise keep it roughly unchanged?).  We can get Jisheng to add his
> Tested-by...
>

Yes, that would be great.

Thanks,
Guenter


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-22 17:09   ` Doug Anderson
  2015-01-23 16:03     ` Guenter Roeck
@ 2015-01-26  6:22     ` Jisheng Zhang
  2015-01-26 17:01       ` Doug Anderson
  1 sibling, 1 reply; 15+ messages in thread
From: Jisheng Zhang @ 2015-01-26  6:22 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Guenter Roeck, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

Dear Doug,

On Thu, 22 Jan 2015 09:09:28 -0800
Doug Anderson <dianders@chromium.org> wrote:

> Jisheng,
> 
> On Wed, Jan 21, 2015 at 9:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> > Dear Doug,
> >
> > On Wed, 21 Jan 2015 15:17:22 -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.
> >
> > + Guenter Roeck
> >
> > This should have been fixed by dfa07141e7a792("watchdog: dw_wdt:
> > initialise TOP_INIT in dw_wdt_set_top()")
> 
> I will admit that I'm testing on a tree that doesn't have your patch
> (I'm on a 3.14 kernel with lots of backports).  ...but I did try
> cherry-picking your patch before I wrote up mine and it didn't fix my
> problem.  I believe that the watchdog that's in Rockchip rk3288 must
> be a slightly different version of the IP block than you're working
> with.
> 
> Specifically I see the register WDT_TORR that has an offset of 0x4.
> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
> "reserved".

Could you please dump registers' value at offset 0xf4 and 0xf8 if you don't mind?

Thanks,
Jisheng

> 
> 
> > In fact, my original fix is as similar as your patch
> >
> > http://www.spinics.net/lists/arm-kernel/msg363658.html
> 
> Yup, except that I pat the watchdog before enabling it and you pat it
> after...  It probably doesn't matter as long as the two instructions
> are within 2.5ms of each other, but it seems nice to be safer.
> 
> -Doug


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-26  6:22     ` Jisheng Zhang
@ 2015-01-26 17:01       ` Doug Anderson
  2015-01-27  3:44         ` Jisheng Zhang
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2015-01-26 17:01 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Guenter Roeck, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

Jisheng,

On Sun, Jan 25, 2015 at 10:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>> Specifically I see the register WDT_TORR that has an offset of 0x4.
>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
>> "reserved".
>
> Could you please dump registers' value at offset 0xf4 and 0xf8 if you don't mind?

Those are not documented in the user manual that I have, but:

>>> r(0xff8000f4)
0x10000a02
>>> r(0xff8000f8)
0x3130332a

-Doug

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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-26 17:01       ` Doug Anderson
@ 2015-01-27  3:44         ` Jisheng Zhang
  2015-01-27  4:07           ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Jisheng Zhang @ 2015-01-27  3:44 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Guenter Roeck, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

Dear Doug,

On Mon, 26 Jan 2015 09:01:37 -0800
Doug Anderson <dianders@chromium.org> wrote:

> Jisheng,
> 
> On Sun, Jan 25, 2015 at 10:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> >> Specifically I see the register WDT_TORR that has an offset of 0x4.
> >> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
> >> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
> >> "reserved".
> >
> > Could you please dump registers' value at offset 0xf4 and 0xf8 if you
> > don't mind?
> 
> Those are not documented in the user manual that I have, but:
> 
> >>> r(0xff8000f4)
> 0x10000a02
> >>> r(0xff8000f8)
> 0x3130332a

Thanks. Now I got some information about your platform:

wdt version: v1.02a

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.

However, it doesn't hurt anything if we have an extra pat before
enabling WDT

Thanks,
Jisheng

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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-27  3:44         ` Jisheng Zhang
@ 2015-01-27  4:07           ` Guenter Roeck
  2015-01-27  4:36             ` Jisheng Zhang
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2015-01-27  4:07 UTC (permalink / raw)
  To: Jisheng Zhang, Doug Anderson
  Cc: Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai, Dinh Nguyen,
	linux-watchdog, linux-kernel

On 01/26/2015 07:44 PM, Jisheng Zhang wrote:
> Dear Doug,
>
> On Mon, 26 Jan 2015 09:01:37 -0800
> Doug Anderson <dianders@chromium.org> wrote:
>
>> Jisheng,
>>
>> On Sun, Jan 25, 2015 at 10:22 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>>>> Specifically I see the register WDT_TORR that has an offset of 0x4.
>>>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
>>>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
>>>> "reserved".
>>>
>>> Could you please dump registers' value at offset 0xf4 and 0xf8 if you
>>> don't mind?
>>
>> Those are not documented in the user manual that I have, but:
>>
>>>>> r(0xff8000f4)
>> 0x10000a02
>>>>> r(0xff8000f8)
>> 0x3130332a
>
Hi Jisheng,

This translates to ascii "103*". How does it translate to "1.02a" ?

> Thanks. Now I got some information about your platform:
>
> wdt version: v1.02a
>
> 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.
>
Any chance you can provide the bit map for the register reporting
those flags ?

Thanks,
Guenter


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

* Re: [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it
  2015-01-27  4:07           ` Guenter Roeck
@ 2015-01-27  4:36             ` Jisheng Zhang
  0 siblings, 0 replies; 15+ messages in thread
From: Jisheng Zhang @ 2015-01-27  4:36 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Doug Anderson, Wim Van Sebroeck, Heiko Stuebner, Lunxue Dai,
	Dinh Nguyen, linux-watchdog, linux-kernel

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

> On 01/26/2015 07:44 PM, Jisheng Zhang wrote:
> > Dear Doug,
> >
> > On Mon, 26 Jan 2015 09:01:37 -0800
> > Doug Anderson <dianders@chromium.org> wrote:
> >
> >> Jisheng,
> >>
> >> On Sun, Jan 25, 2015 at 10:22 PM, Jisheng Zhang <jszhang@marvell.com>
> >> wrote:
> >>>> Specifically I see the register WDT_TORR that has an offset of 0x4.
> >>>> That's the RANGE_REG in your code.  It shows bits 3:0 set the timeout
> >>>> period (0 = 0xffff and 15 = 0x7fffffff).  It shows bits 31:4 as
> >>>> "reserved".
> >>>
> >>> Could you please dump registers' value at offset 0xf4 and 0xf8 if you
> >>> don't mind?
> >>
> >> Those are not documented in the user manual that I have, but:
> >>
> >>>>> r(0xff8000f4)
> >> 0x10000a02
> >>>>> r(0xff8000f8)
> >> 0x3130332a
> >
> Hi Jisheng,
> 
> This translates to ascii "103*". How does it translate to "1.02a" ?

aha, yes. wdt version is v1.03* ;)

> 
> > Thanks. Now I got some information about your platform:
> >
> > wdt version: v1.02a
> >
> > 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.
> >
> Any chance you can provide the bit map for the register reporting
> those flags ?

will do in Doug's v2 patch thread

Thanks,
Jisheng

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

end of thread, other threads:[~2015-01-27  4:39 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-21 23:17 [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Doug Anderson
2015-01-21 23:17 ` [PATCH 2/2] watchdog: dw_wdt: Try to get a 30 second watchdog by default Doug Anderson
2015-01-21 23:36   ` Guenter Roeck
2015-01-21 23:35 ` [PATCH 1/2] watchdog: dw_wdt: pat the watchdog before enabling it Guenter Roeck
2015-01-22  5:22 ` Jisheng Zhang
2015-01-22  5:31   ` Guenter Roeck
2015-01-22 17:09   ` Doug Anderson
2015-01-23 16:03     ` Guenter Roeck
2015-01-23 16:20       ` Doug Anderson
2015-01-23 17:02         ` Guenter Roeck
2015-01-26  6:22     ` Jisheng Zhang
2015-01-26 17:01       ` Doug Anderson
2015-01-27  3:44         ` Jisheng Zhang
2015-01-27  4:07           ` Guenter Roeck
2015-01-27  4:36             ` Jisheng Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).