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

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.villemoes@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