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
