Re: [PATCH] watchdog: allow overriding the rate-limiting logic

[Stefan Roese]

Hi Rasmus,

(added Christophe to Cc)

On 12.03.20 12:40, Rasmus Villemoes wrote:

On the MPC8309-derived board I'm currently working on, the current
rate-limiting logic in the generic watchdog_reset function poses two
problems:

First, the hard-coded limit of 1000ms is too large for the
external GPIO-triggered watchdog we have.

I remember a discussion a few weeks ago with Christophe, where you
pointed out, that there is already the official "hw_margin_ms" DT
property for this delay here. Why don't you introduce it here so that
all boards can use it as well?

Second, in the SPL, the decrementer interrupt is not enabled, so the
get_timer() call calls always returns 0, so wdt_reset() is never
actually called. Enabling that interrupt (i.e. calling
interrupt_init() somewhere in my early board code) is a bit
problematic, since U-Boot proper gets loaded to address 0, hence
overwriting exception vectors - and the reason I even need to care
about the watchdog in the SPL is that the signature verification takes
a rather long time, so just disabling interrupts before loading U-Boot
proper doesn't work.

Both problems can be solved by allowing the board to override the
rate-limiting logic. For example, in my case I will implement the
function in terms of get_ticks() (i.e. the time base registers on
PPC) which do not depend on interrupts, and use a threshold of
gd->bus_clk / (4*16) ticks (~62ms).

I would assume that you might run into multiple issues, when your timer
infrastructure is not working correctly in SPL. Can't you instead "fix"
this by using this get_ticks() option for the get_timer() functions in
SPL so that you can use the common functions here and in all other
places in SPL as well?

Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---

This is on top of https://patchwork.ozlabs.org/patch/1242772/, but
it's obviously trivial to do on master instead.

  drivers/watchdog/wdt-uclass.c | 21 ++++++++++++++-------
  1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 309a0e9c5b..ad53e86b80 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -67,6 +67,19 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
  }

#if defined(CONFIG_WATCHDOG)

+__weak int watchdog_reset_ratelimit(void)
+{
+       static ulong next_reset;
+       ulong now;
+
+       now = get_timer(0);
+       if (time_after(now, next_reset)) {
+               next_reset = now + 1000;        /* reset every 1000ms */
+               return 1;
+       }
+       return 0;
+}
+
  /*
   * Called by macro WATCHDOG_RESET. This function be called *very* early,
   * so we need to make sure, that the watchdog driver is ready before using
@@ -74,19 +87,13 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
   */
  void watchdog_reset(void)
  {
-       static ulong next_reset;
-       ulong 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 + 1000;        /* reset every 1000ms */
+       if (watchdog_reset_ratelimit())
                wdt_reset(gd->watchdog_dev);
-       }
  }
  #endif

Thanks,
Stefan