watchdog/imx_watchdog: do not set WCR_WDW

PATCH New Sebastian Andrzej Siewior 2 participants 7 messages Mar 3, 2015 – Mar 26, 2015

Subsystems (2)

Arm i.MX and Layerscape Watchdog device drivers

lore

Share & tools

lore

Sebastian Andrzej Siewior

author

2015-03-03 16:45 UTC #0 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").

While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 drivers/watchdog/imx_watchdog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/watchdog/imx_watchdog.c +1 -2

index d5993b4d26d6..1d18d4b269f8 100644
@@ -20,7 +20,6 @@ struct watchdog_regs {
 #define WCR_WDE		0x04	/* WDOG enable */
 #define WCR_WDT		0x08
 #define WCR_SRS		0x10
-#define WCR_WDW		0x80
 #define SET_WCR_WT(x)	(x << 8)
 
 #ifdef CONFIG_IMX_WATCHDOG

@@ -47,7 +46,7 @@ void hw_watchdog_init(void)
 #endif
 	timeout = (CONFIG_WATCHDOG_TIMEOUT_MSECS / 500) - 1;
 	writew(WCR_WDZST | WCR_WDBG | WCR_WDE | WCR_WDT | WCR_SRS |
-		WCR_WDW | SET_WCR_WT(timeout), &wdog->wcr);
+		SET_WCR_WT(timeout), &wdog->wcr);
 	hw_watchdog_reset();
 }
 #endif

Signature · 1 line

2.1.4

Stefano Babic

2015-03-25 08:03 UTC #1 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

Hi Sebastian,

(sorry for late answer)

Quoted 5 lines · Sebastian Andrzej Siewior

with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").

Just some questions. The described behavior is coherent with the description in the manual. When WDW is set, the watchdog is stopped while the processor is in wait mode.
Current imx2_wdt.c driver in kernel does not care about this bit, but it looks like that only the kernel and/or the application can decide if the watchdog should run when the system is in WAIT or not. The bootloader cannot decide, and it should set the bit for its own usage.

Should this setup be done by the kernel driver instead of by the bootloader ?

Quoted 3 lines

While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.

I can agree or disagree. This depends on the application.

Differently as the WDZST, that can be set only once, and it is set by *both* U-Boot and kernel (setting in kernel has then no influence), the WDW can be even disabled in kernel.

Quoted 20 lines

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 drivers/watchdog/imx_watchdog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/watchdog/imx_watchdog.c +0 -1

index d5993b4d26d6..1d18d4b269f8 100644
@@ -20,7 +20,6 @@ struct watchdog_regs {
 #define WCR_WDE		0x04	/* WDOG enable */
 #define WCR_WDT		0x08
 #define WCR_SRS		0x10
-#define WCR_WDW		0x80
 #define SET_WCR_WT(x)	(x << 8)
 
 #ifdef CONFIG_IMX_WATCHDOG

drivers/watchdog/imx_watchdog.c void hw_watchdog_init(void) · line 52

Quoted 8 lines

drivers/watchdog/imx_watchdog.c +1 -1

@@ -47,7 +46,7 @@ void hw_watchdog_init(void)
 #endif
 	timeout = (CONFIG_WATCHDOG_TIMEOUT_MSECS / 500) - 1;
 	writew(WCR_WDZST | WCR_WDBG | WCR_WDE | WCR_WDT | WCR_SRS |
-		WCR_WDW | SET_WCR_WT(timeout), &wdog->wcr);
+		SET_WCR_WT(timeout), &wdog->wcr);
 	hw_watchdog_reset();
 }
 #endif

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Sebastian Andrzej Siewior

author

2015-03-25 12:15 UTC #2 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

Quoted 1 line · * Stefano Babic | 2015-03-25 09:03:06 [+0100]:

Hi Sebastian,

Hi Stefano,

Quoted 1 line

(sorry for late answer)

no worries.

Quoted 10 lines · Sebastian Andrzej Siewior

Quoted 5 lines
with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").
Just some questions. The described behavior is coherent with the description in the manual. When WDW is set, the watchdog is stopped while the processor is in wait mode.

I fail to notice how it could make any sense.

Quoted 7 lines

Current imx2_wdt.c driver in kernel does not care about this bit, but it looks like that only the kernel and/or the application can decide if the watchdog should run when the system is in WAIT or not. The bootloader cannot decide, and it should set the bit for its own usage.
Should this setup be done by the kernel driver instead of by the bootloader ?

It is a one-time-bit. Once it is set (or not) it can not be changed later. It makes no difference for the bootloader. And the kernel may remain idle for longer than one jiffy with NO_HZ but it will still wake up within a second if I remember correctly. Eitherway the user will usually trigger the WD in timeout/2 so atleast that timer will wake up the kernel from idle.
Having the WDT not account the idle time makes it hard for the user to figure out how much time has been accounted by the WDT so far.

Quoted 9 lines

Quoted 3 lines
While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.
I can agree or disagree. This depends on the application.
Differently as the WDZST, that can be set only once, and it is set by *both* U-Boot and kernel (setting in kernel has then no influence), the WDW can be even disabled in kernel.

I patched the kernel as well. Once I get through here I would also push patches there. The thing I don't like is that the kernel pings the WDT on its own. While it may make sense for some people I would like to atleast configure it :)

Sebastian

Stefano Babic

2015-03-25 13:08 UTC #3 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

Hi Sebastian,

Quoted 6 lines · Sebastian Andrzej Siewior

Quoted 4 lines
Just some questions. The described behavior is coherent with the description in the manual. When WDW is set, the watchdog is stopped while the processor is in wait mode.
I fail to notice how it could make any sense.

Well, I have not said it makes sense - only that the behavior is expected ;-)

Quoted 11 lines

Quoted 8 lines
Current imx2_wdt.c driver in kernel does not care about this bit, but it looks like that only the kernel and/or the application can decide if the watchdog should run when the system is in WAIT or not. The bootloader cannot decide, and it should set the bit for its own usage.
Should this setup be done by the kernel driver instead of by the bootloader ?
It is a one-time-bit. Once it is set (or not) it can not be changed later. It makes no difference for the bootloader.

mmhhh..Sorry, it looks like I read the wrong line in the manual. This is a write once bit exactly as WDZST. My assumption were wrong. We cannot disable in kernel if the bootloader has already activated it.

Quoted 7 lines

And the kernel may
remain idle for longer than one jiffy with NO_HZ but it will still wake up within a second if I remember correctly. Eitherway the user will usually trigger the WD in timeout/2 so atleast that timer will wake up the kernel from idle.
Having the WDT not account the idle time makes it hard for the user to figure out how much time has been accounted by the WDT so far.

I have no objections here - I start from the wrong assumption that the bit could be cleared by the kernel, but it is not.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.
I can agree or disagree. This depends on the application.

Quoted 8 lines

Quoted 3 lines
Differently as the WDZST, that can be set only once, and it is set by *both* U-Boot and kernel (setting in kernel has then no influence), the WDW can be even disabled in kernel.
I patched the kernel as well. Once I get through here I would also push patches there. The thing I don't like is that the kernel pings the WDT on its own. While it may make sense for some people I would like to atleast configure it :)

Agree, patch put in my merge queue.

Acked-by Stefano Babic <[email protected]>

Best regards,
Stefano

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Stefano Babic

2015-03-25 15:53 UTC #4 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

In reply to Sebastian Andrzej Siewior

Quoted 11 lines · Sebastian Andrzej Siewior

with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").
While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
---

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Sebastian Andrzej Siewior

author

2015-03-26 08:02 UTC #5 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

Quoted 7 lines · Stefano Babic

Quoted 6 lines · Sebastian Andrzej Siewior
with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").

Quoted 7 lines

Quoted 5 lines
While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
---
Applied to u-boot-imx, thanks !

Thanks. What just came to mind: In case you do suspend-to-mem. Would the watchdog trigger then? I am not sure and I have no way to test?

Quoted 2 lines

Best regards,
Stefano Babic

Sebastian

Stefano Babic

2015-03-26 08:27 UTC #6 lore raw

PATCH 1/1 watchdog/imx_watchdog: do not set WCR_WDW

Hi Sebastian,

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 10 lines · Stefano Babic
Quoted 9 lines · Sebastian Andrzej Siewior
with WCR_WDW set, the watchdog won't trigger if we bootet linux and idle around while the watchdog is not triggered. It seems the timer makes progress very slowly if at all. I managed to remain 20minutes alive while the timeout was set to 60secs. It reboots within 60secs if I start a busyloop in userland (something like "while (1) { }").
While I don't see a reason why the WDT should not be running while the CPU is in idle, I'm dropping this bit.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
---
Applied to u-boot-imx, thanks !
Thanks. What just came to mind: In case you do suspend-to-mem. Would the watchdog trigger then? I am not sure and I have no way to test?

I have also not tested, but in my understanding this is ruled by the WDZST. This is also a write-once bit, and suspend the watchdog timer during low power modes.

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================