Re: [PATCH v4 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

Stefan Roese
2021-08-03T08:28:52Z

Hi Rasmus,

On 02.08.21 17:00, Rasmus Villemoes 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, and call the init_watchdog_dev helper for each.
>
> 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 | 56 ++++++++++++++++++++-----------
> include/asm-generic/global_data.h | 6 ----
> 2 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 358fc68e27..0ce8b3a425 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
>
> 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;
> + struct udevice *dev;
> + struct uclass *uc;
> + int ret;
> +
> + ret = uclass_get(UCLASS_WDT, &uc);
> + if (ret) {
> + log_debug("Error getting UCLASS_WDT: %d\n", ret);
> + return 0;
> + }
> +
> + uclass_foreach_dev(dev, uc) {
> + ret = device_probe(dev);
> + if (ret) {
> + log_debug("Error probing %s: %d\n", dev->name, ret);
> + continue;
> }
> + init_watchdog_dev(dev);
> }
> - init_watchdog_dev(gd->watchdog_dev);
>
> gd->flags |= GD_FLG_WDT_READY;
> return 0;
> @@ -157,22 +161,34 @@ 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);
> + /*
> + * All devices bound to the wdt uclass should have been probed
> + * in initr_watchdog(). But just in case something went wrong,
> + * check device_active() before accessing the uclass private
> + * data.
> + */
> + uclass_foreach_dev(dev, uc) {
> + if (!device_active(dev))
> + continue;
> + 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
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index e55070303f..28d749538c 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -447,12 +447,6 @@ struct global_data {
> */
> fdt_addr_t translation_offset;
> #endif
> -#if CONFIG_IS_ENABLED(WDT)
> - /**
> - * @watchdog_dev: watchdog device
> - */
> - struct udevice *watchdog_dev;
> -#endif

After applying the patchset v4 to current master, I see this error when
building for "x530":

board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
struct global_data'} has no member named 'watchdog_dev'
125 | wdt_stop(gd->watchdog_dev);
| ^~
make[1]: *** [scripts/Makefile.build:254:
board/alliedtelesis/x530/x530.o] Error 1

Perhaps we need a common function now to stop all watchdogs, which can
be called from such places?

Thanks,
Stefan