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

[Simon Glass]

Hi Rasmus,

On Mon, 28 Jun 2021 at 01:44, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> 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). 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.

>From a driver model perspective, it is good practice to have a 'start'
method separate from probing. I understand that you are saying that
the watchdog should always be enabled, but we can handle that with a
separate method to start it. I don't think it needs to be started in
the post-probe method. We can just start it later. Imagine you probe
the device but then go into SDRAM init that hangs the CPU for a few
seconds. We need a way to signal that we want the watchdog to start,
so the board owner has a choice as to when this happens.

I also understand this is not quite how things work at present and I'm
fine with copying the existing behaviour. It's just that you are
introducing a driver model interface and I have pretty strong views
about the separation between probe at start, with that.

Regards,
Simon