Hi Rasmus, On Thu, 27 May 2021 at 16:00, Rasmus Villemoes <rasmus.villem...@prevas.dk> wrote: > > A board can have and make use of more than one watchdog device, say > one built into the SOC and an external gpio-petted one. Having > wdt-uclass only handle the first is both a little arbitrary and > unexpected. > > So change initr_watchdog() so we visit (probe) all DM watchdog > devices. This essentially boils down to making the init_watchdog_dev > function into a .post_probe method. > > Similarly let watchdog_reset() loop over the whole uclass - each > having their own ratelimiting metadata, and a separate "is this device > running" flag. > > This gets rid of the watchdog_dev member of struct global_data. We > do, however, still need the GD_FLG_WDT_READY set in > initr_watchdog(). This is because watchdog_reset() can get called > before DM is ready, and I don't think we can call uclass_get() that > early. > > The current code just returns 0 if "getting" the first device fails - > that can of course happen because there are no devices, but it could > also happen if its ->probe call failed. In keeping with that, continue > with the handling of the remaining devices even if one fails to > probe. This is also why we cannot use uclass_probe_all(). > > If desired, it's possible to later add a per-device "u-boot,autostart" > boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART > per-device. > > Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> > --- > drivers/watchdog/wdt-uclass.c | 87 ++++++++++++++++--------------- > include/asm-generic/global_data.h | 6 --- > 2 files changed, 46 insertions(+), 47 deletions(-) > > diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c > index 026b6d1eb4..e062a75574 100644 > --- a/drivers/watchdog/wdt-uclass.c > +++ b/drivers/watchdog/wdt-uclass.c > @@ -25,44 +25,20 @@ struct wdt_priv { > bool running; > }; > > -static void init_watchdog_dev(struct udevice *dev) > +int initr_watchdog(void) > { > - struct wdt_priv *priv; > + struct udevice *dev; > + struct uclass *uc; > int ret; > > - priv = dev_get_uclass_priv(dev); > - > - 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 > + return 0; Should return error here > } > > - printf("WDT: Started %s with%s servicing (%ds timeout)\n", > dev->name, > - IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); > -} > - > -int initr_watchdog(void) > -{ > - /* > - * Init watchdog: This will call the probe function of the > - * watchdog driver, enabling the use of the device > - */ > - if (uclass_get_device_by_seq(UCLASS_WDT, 0, > - (struct udevice **)&gd->watchdog_dev)) { > - debug("WDT: Not found by seq!\n"); > - if (uclass_get_device(UCLASS_WDT, 0, > - (struct udevice **)&gd->watchdog_dev)) { > - printf("WDT: Not found!\n"); > - return 0; > - } > - } > - 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. > > 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() ? > + priv = dev_get_uclass_priv(dev); > + if (!priv->running) > + continue; > + /* 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); > + } > } > } > #endif > @@ -214,11 +194,36 @@ static int wdt_pre_probe(struct udevice *dev) > return 0; > } > > +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() > + return 0; > + } > + > + ret = wdt_start(dev, priv->timeout * 1000, 0); > + if (ret != 0) { > + printf("WDT: Failed to start %s\n", dev->name); log_debug() > + return 0; > + } I don't think it is good to start it in the post-probe. Can you do it separately, afterwards? Probing is supposed to just probe it and it should be safe to do that without side effects. > + > + printf("WDT: Started %s with%s servicing (%ds timeout)\n", > dev->name, > + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); log_debug() I think > + > + return 0; > +} > + > UCLASS_DRIVER(wdt) = { > .id = UCLASS_WDT, > .name = "watchdog", > .flags = DM_UC_FLAG_SEQ_ALIAS, > .post_bind = wdt_post_bind, > .pre_probe = wdt_pre_probe, > + .post_probe = wdt_post_probe, > .per_device_auto = sizeof(struct wdt_priv), > }; > diff --git a/include/asm-generic/global_data.h > b/include/asm-generic/global_data.h > index 47921d27b1..b554016628 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -445,12 +445,6 @@ struct global_data { > */ > fdt_addr_t translation_offset; > #endif > -#if CONFIG_IS_ENABLED(WDT) > - /** > - * @watchdog_dev: watchdog device > - */ > - struct udevice *watchdog_dev; > -#endif > #ifdef CONFIG_GENERATE_ACPI_TABLE > /** > * @acpi_ctx: ACPI context pointer > -- > 2.31.1 > Regards, Simon