Hi Rasmus,
On Thu, 27 May 2021 at 16:00, Rasmus Villemoes rasmus.villemoes@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.villemoes@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