* [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 related [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 related [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 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.