[PATCH v2 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

On 28.06.21 09:44, Rasmus Villemoes wrote:
> On 26/06/2021 20.32, Simon Glass wrote:
>> Hi Rasmus,
>>
>> On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
>> <rasmus.villemoes at prevas.dk> wrote:
>>>
> 
>>>> I don't think it is good to start it in the post-probe. Can you do it
>>>> separately, afterwards?
>>>
>>> Eh, yes, of course I could do this in the loop in initr_watchdog() where
>>> I probe all watchdog devices, but the end result is exactly the same,
>>> and it seemed that this was a perfect fit for DM since it provided this
>>> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
>>> and if it is, that precisely means the developer wanted to start
>>> handling the device(s) ASAP.
>>>
>>>> Probing is supposed to just probe it and it
>>>> should be safe to do that without side effects.
>>>
>>> I agree in general, but watchdog devices are a bit special compared to
>>> some random USB controller or LCD display or spi master - those devices
>>> generally don't do anything unless asked to by the CPU, while a watchdog
>>> device is the other way around, it does its thing _unless_ the CPU asks
>>> it not to (often enough). Which in turn, I suppose, is the whole reason
>>> wdt-uclass has its own hook into the initr sequence - one needs to probe
>>> and possibly start handling the watchdog(s) ASAP.
>>
>> It still needs a 'start' method to make it start. Having it start on
>> probe means the board will reset at the command line if the device is
>> probed. Yuck.
> 
> No, because while sitting in the command line waiting for user input,
> WATCHDOG_RESET() is called something like a million times per second (or
> at least extremely often).

We have some rate-limiting in place since a few years (reset_period).

> For the most common case of there only being
> one (or zero) DM watchdogs, I'm not changing anything at all about how
> things behave. I'm just expanding the handling done in the wdt-uclass
> provided functions initr_watchdog() and watchdog_reset() to all DM
> watchdogs, making things more consistent. And there's
> CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe
> function into a no-op.
> 
> As I said, yes, I can move the call of the post_probe function into the
> loop in initr_watchdog (and then it wouldn't be called post_probe, but
> probably named something including auto_start). In practice, that won't
> change anything.
> 
> Stefan, what do you think? I think this is the only contentious point at
> this time, so I'll do whatever you think is right, then resend the
> patches with Simon's other feedback incorporated.

I'm fine with this post_probe() implementation but have no strong
feelings about this. So if Simon (or someone else) does not object, then
please continue this way.

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan