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

[Stefan Roese <sr@denx.de>]

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>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   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
>   #ifdef CONFIG_GENERATE_ACPI_TABLE
>   	/**
>   	 * @acpi_ctx: ACPI context pointer
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de