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

[Simon Glass]

Hi Rasmus,

On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> Hi Simon,
>
> Thanks for review.
>
> >> -       if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
> >> -               printf("WDT:   Not starting %s\n", dev->name);
> >> -               return;
> >> -       }
> >> -
> >> -       ret = wdt_start(dev, priv->timeout * 1000, 0);
> >> -       if (ret != 0) {
> >> -               printf("WDT:   Failed to start %s\n", dev->name);
> >> -               return;
> >> +       ret = uclass_get(UCLASS_WDT, &uc);
> >> +       if (ret) {
> >> +               printf("Error getting UCLASS_WDT: %d\n", ret);
> >
> > log_debug()as this should not happen
>
> OK. [I assume the rationale is: don't add .text which will most likely
> never be used, but allow the debug statements to be easily turned on
> per-TU if one should ever need it.]

Yes, also the error return value should give you a clue.

Please, use log_msg_ret() on your return values.

>
> >> +               return 0;
> >
> > Should return error here
>
> And have the initr sequence stop completely instead of trying to limp
> along? Why? Note that I touched upon this in the commit message: The
> existing code doesn't pass on an error from uclass_get_device*() [which
> would most likely come from an underlying probe call], and returns 0
> regardless from initr_watchdog(). I see no point changing that.

OK, I'll leave it to you. My feeling is that failure is a good way to
get a bug fixed.

>
> >> -       }
> >> -       init_watchdog_dev(gd->watchdog_dev);
> >> +       uclass_foreach_dev(dev, uc)
> >> +               device_probe(dev);
> >
> > If  watchdog fails, should we not stop? Even if we don't, I think some
> > sort of message should be shown so people know to fix it.
>
> I'd assume device_probe() would print an error message. If not, sure, I
> can add some [log_debug?] message.

No driver model never prints messages. Yes you can use log_debuig().

>
> >>
> >>         gd->flags |= GD_FLG_WDT_READY;
> >>         return 0;
> >> @@ -145,22 +121,26 @@ void watchdog_reset(void)
> >>  {
> >>         struct wdt_priv *priv;
> >>         struct udevice *dev;
> >> +       struct uclass *uc;
> >>         ulong now;
> >>
> >>         /* Exit if GD is not ready or watchdog is not initialized yet */
> >>         if (!gd || !(gd->flags & GD_FLG_WDT_READY))
> >>                 return;
> >>
> >> -       dev = gd->watchdog_dev;
> >> -       priv = dev_get_uclass_priv(dev);
> >> -       if (!priv->running)
> >> +       if (uclass_get(UCLASS_WDT, &uc))
> >>                 return;
> >>
> >> -       /* Do not reset the watchdog too often */
> >> -       now = get_timer(0);
> >> -       if (time_after_eq(now, priv->next_reset)) {
> >> -               priv->next_reset = now + priv->reset_period;
> >> -               wdt_reset(dev);
> >> +       uclass_foreach_dev(dev, uc) {
> >
> > Don't you need to use foreach_dev_probe() ?
>
> Why? They've all been probed in initr_watchdog(). And the guts of
> WATHCDOG_RESET(), which can be and is called from everywhere, is
> certainly not the place to do anything other than actually pinging the
> watchdog devices.

Then you should add a comment. You are using a low-level iterator,
then assuming the devices are probed.

>
> >> +static int wdt_post_probe(struct udevice *dev)
> >> +{
> >> +       struct wdt_priv *priv;
> >> +       int ret;
> >> +
> >> +       priv = dev_get_uclass_priv(dev);
> >> +
> >> +       if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
> >> +               printf("WDT:   Not starting %s\n", dev->name);
> >
> > log_debug()
>
> Perhaps, but this is just existing code I've moved around.

OK, well then you can leave it alone.

>
> >> +               return 0;
> >> +       }
> >> +
> >> +       ret = wdt_start(dev, priv->timeout * 1000, 0);
> >> +       if (ret != 0) {
> >> +               printf("WDT:   Failed to start %s\n", dev->name);
> >
> > log_debug()
>
> Ditto.
>
> >> +               return 0;
> >> +       }
> >
> > 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.

>
> BTW, next on my agenda is hooking in even earlier so the few boards that
> use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and
> be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely
> random places in init_sequence_f[] is a bit silly.

OK.

>
> >> +
> >> +       printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
> >> +              IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
> >
> > log_debug() I think
>
> Ditto, existing code moved around.

OK.

Regards,
Simon