[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Christophe Leroy]

On 02/20/2020 05:43 AM, Stefan Roese wrote:
> Hi Christophe
> 
> On 19.02.20 20:21, Christophe Leroy wrote:
>>
>>
>> Le 25/04/2019 à 09:17, Stefan Roese a écrit :
>>> This patch tries to implement a generic watchdog_reset() function that
>>> can be used by all boards that want to service the watchdog device in
>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
>>>
>>> Without this approach, new boards or platforms needed to implement a
>>> board specific version of this functionality, mostly copy'ing the same
>>> code over and over again into their board or platforms code base.
>>>
>>> With this new generic function, the scattered other functions are now
>>> removed to be replaced by the generic one. The new version also enables
>>> the configuration of the watchdog timeout via the DT "timeout-sec"
>>> property (if enabled via CONFIG_OF_CONTROL).
>>>
>>> This patch also adds a new flag to the GD flags, to flag that the
>>> watchdog is ready to use and adds the pointer to the watchdog device
>>> to the GD. This enables us to remove the global "watchdog_dev"
>>> variable, which was prone to cause problems because of its potentially
>>> very early use in watchdog_reset(), even before the BSS is cleared.
>>>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>> Cc: Heiko Schocher <hs at denx.de>
>>> Cc: Tom Rini <trini at konsulko.com>
>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>> Cc: "Marek Behún" <marek.behun at nic.cz>
>>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>> Cc: Maxim Sloyko <maxims at google.com>
>>> Cc: Erik van Luijk <evanluijk at interact.nl>
>>> Cc: Ryder Lee <ryder.lee at mediatek.com>
>>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: "Álvaro Fernández Rojas" <noltari at gmail.com>
>>> Cc: Philippe Reynes <philippe.reynes at softathome.com>
>>> Cc: Christophe Leroy <christophe.leroy at c-s.fr>
>>> Cc: Chris Packham <chris.packham at alliedtelesis.co.nz>
>>> Reviewed-by: Michal Simek <michal.simek at xilinx.com>
>>> Tested-by: Michal Simek <michal.simek at xilinx.com> (on zcu100)
>>> ---
>>
>> [...]
>>
>>> diff --git a/drivers/watchdog/wdt-uclass.c 
>>> b/drivers/watchdog/wdt-uclass.c
>>> index 23b7e3360d..bbfac4f0f9 100644
>>> --- a/drivers/watchdog/wdt-uclass.c
>>> +++ b/drivers/watchdog/wdt-uclass.c
>>> @@ -10,6 +10,8 @@
>>>   #include <dm/device-internal.h>
>>>   #include <dm/lists.h>
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>   int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>   {
>>>       const struct wdt_ops *ops = device_get_ops(dev);
>>> @@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>>>       return ret;
>>>   }
>>> +#if defined(CONFIG_WATCHDOG)
>>> +/*
>>> + * 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
>>> + * it in this function.
>>> + */
>>> +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 (now > next_reset) {
>>> +        next_reset = now + 1000;    /* reset every 1000ms */
>>> +        wdt_reset(gd->watchdog_dev);
>>> +    }
>>
>> This is a problem for the MPC8xx.
>>
>> When running with a MPC8xx at 132MHz clock, the watchdog will fire 
>> about 1s after the last refresh. So the above makes the board unusable.
> 
> So you need a shorted delay between the wdt_reset() calls? Is this
> correct? We could introduce a new Kconfig option which defaults to
> 1000 (ms) and you can "select" a shorter value for MPC8xx.

Exactly. However, why is this limitation needed at all ? Why is it a 
problem to refresh more often ?


Christophe