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

[Rasmus Villemoes]

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.

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

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

Thanks,
Rasmus