* [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 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 ` 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 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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@vger.kernel.org, linux-kernel@vger.kernel.org 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