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