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

[Stefan Roese]

On 20.02.20 08:43, Rasmus Villemoes wrote:
> On 20/02/2020 07.43, Stefan Roese wrote:
>> On 20.02.20 07:38, Christophe Leroy wrote:
>>
>> <snip>
>>
>>>>>> +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 ?
>>
>> Very likely its not. What is a reasonable value for your platform? 100
>> or 500ms? I think we could change it to default to a shorter value, but
>> such a change should go in early in the merge window, so that other
>> platforms have a bit of time to test it.
>>
>> Please feel free to send a patch for this and please add a comment to
>> explain, why the delay is this "short".
> 
> IMO, this should come from DT.

Yes, I agree. I just wanted to keep the change "simple" here with this
approach.

> For a gpio watchdog (which for some
> reason U-Boot doesn't have a generic driver for) the linux kernel uses a
> hw_margin_ms property that tells the core how often the watchdog must be
> pinged - that could be generalized to apply for all, with 1000ms as a
> default if not set. And I've seen boards with a gpio watchdog with a
> timeout of 200 ms.

Ah, okay. Its good to know, that Linux already has such a DT property.

> Also, I'm wondering why that generic _reset only handles one watchdog
> device? I can easily imagine needing to reset both, say, an external
> gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop
> over all DM watchdogs, and have the next_reset/hw_margin etc. metadata
> live with the watchdog device instead of in static variable/build-time
> constants?

I agree, that the current WDT handling can be extended to support
multiple (different) WDT instances. Currently there don't seem to be
any (I might be incorrect here) board that need such a feature though.
I would suggest to implement such a feature, once its really needed.

Thanks,
Stefan