Hi Rasmus,
On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
<rasmus.villem...@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
