Re: [RFC PATCH] watchdog: base rate-limiting on get_ticks() rather than get_timer()

On 05.06.20 13:16, Rasmus Villemoes wrote:

On powerpc, get_timer() is implemented using a volatile variable that gets incremented from the decrementer interrupt handler. Hence, when interrupts are disabled, time as measured by get_timer() ceases to pass.

Interrupts are necessarily disabled during bootm (see the comment in bootm_disable_interrupts() for why). But after interrupts are disabled, there's still lots of work to do - e.g. decompressing the kernel image to the right load address. Unfortunately, the rate-limiting logic in wdt_uclass.c's watchdog_reset function means that WATCHDOG_RESET() becomes a no-op, since get_timer() never progresses past the next_reset.

This, combined with an external gpio-triggered watchdog that must be petted at least every 800ms, means our board gets reset before booting into linux.

Now, at least on powerpc, get_ticks() continues to return sensible results whether or not interrupts are enabled. So this fixes the above problem for our board - but I don't know if get_ticks() can be assumed to always work.

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

This is what I had in mind. I also considered making it a config knob (possibly just auto-selected based on $ARCH) whether to use get_timer() or get_ticks(), but that becomes quite ugly.

I hesitate a bit, moving with this generic code from get_timer() to get_ticks() for all boards. Did you test this patch on other platforms, like some ARM boards?

Please note that I don't reject it - just asking.

Thanks, Stefan

drivers/watchdog/wdt-uclass.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 4cdb7bd64c..7be4e9b5bc 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -16,14 +16,15 @@ DECLARE_GLOBAL_DATA_PTR;

#define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)

-/*

• • Reset every 1000ms, or however often is required as indicated by a
• • hw_margin_ms property.
• */

-static ulong reset_period = 1000; +static u64 ratelimit_ticks;

int initr_watchdog(void) {

• /*

• * Reset every 1000ms, or however often is required as indicated by a
  

• * hw_margin_ms property.
  

• */
  

• u32 reset_period = 1000; u32 timeout = WATCHDOG_TIMEOUT_SECS;

  /*

@@ -48,6 +49,7 @@ int initr_watchdog(void) 4 * reset_period) / 4; }

• ratelimit_ticks = usec_to_tick(reset_period * 1000); wdt_start(gd->watchdog_dev, timeout * 1000, 0); gd->flags |= GD_FLG_WDT_READY; printf("WDT: Started with%s servicing (%ds timeout)\n",

@@ -117,17 +119,17 @@ int wdt_expire_now(struct udevice *dev, ulong flags) */ void watchdog_reset(void) {

• static ulong next_reset;
• ulong now;

• static u64 last_reset;

• u64 now;

  /* Exit if GD is not ready or watchdog is not initialized yet */ if (!gd || !(gd->flags & GD_FLG_WDT_READY)) return;

  /* Do not reset the watchdog too often */

• now = get_timer(0);
• if (time_after(now, next_reset)) {

• next_reset = now + reset_period;
  

• now = get_ticks();
• if (now - last_reset >= ratelimit_ticks) {

• last_reset = now;
  

  wdt_reset(gd->watchdog_dev); } }

Viele Grüße, Stefan