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

[Stefan Roese]

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 at 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

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