[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.villemoes at 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