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

[Stefan Roese]

On 22.06.21 22:28, Rasmus Villemoes 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.]
> 
>>> +               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.
> 
>>> -       }
>>> -       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.
> 
>>>
>>>          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.
> 
>>> +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.

I would prefer to not change (remove) the currently existing output of
the status of the watchdog driver. If necessary, it could be changed to
log_info().

>>> +               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.
> 
> 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.

Nice.

>>> +
>>> +       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.

Again, please don't change this to debug level.

Thanks,
Stefan