[PATCH v2 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()
Simon Glass
sjg at chromium.org
Tue Jun 22 15:31:50 CEST 2021
Hi Rasmus,
On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes at 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 at 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
More information about the U-Boot
mailing list