[U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Simon Goldschmidt]

Hi Stefan,

Am 25.04.2019 um 09:17 schrieb Stefan Roese:
> This patch tries to implement a generic watchdog_reset() function that
> can be used by all boards that want to service the watchdog device in
> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
> 
> Without this approach, new boards or platforms needed to implement a
> board specific version of this functionality, mostly copy'ing the same
> code over and over again into their board or platforms code base.
> 
> With this new generic function, the scattered other functions are now
> removed to be replaced by the generic one. The new version also enables
> the configuration of the watchdog timeout via the DT "timeout-sec"
> property (if enabled via CONFIG_OF_CONTROL).
> 
> This patch also adds a new flag to the GD flags, to flag that the
> watchdog is ready to use and adds the pointer to the watchdog device
> to the GD. This enables us to remove the global "watchdog_dev"
> variable, which was prone to cause problems because of its potentially
> very early use in watchdog_reset(), even before the BSS is cleared.
> 
> Signed-off-by: Stefan Roese <sr at denx.de>

<snip>

> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -133,6 +133,9 @@ typedef struct global_data {
>   	struct spl_handoff *spl_handoff;
>   # endif
>   #endif
> +#if defined(CONFIG_WDT)
> +	struct udevice *watchdog_dev;
> +#endif
>   } gd_t;
>   #endif
>   
> @@ -161,5 +164,6 @@ typedef struct global_data {
>   #define GD_FLG_ENV_DEFAULT	0x02000 /* Default variable flag	   */
>   #define GD_FLG_SPL_EARLY_INIT	0x04000 /* Early SPL init is done	   */
>   #define GD_FLG_LOG_READY	0x08000 /* Log system is ready for use	   */
> +#define GD_FLG_WDT_READY	0x10000 /* Watchdog is ready for use	   */

Sorry to warm up a thread that is more than 4 months old, but I just 
stumbled accross this line when searching for space in 'gd':

The comment some lines above in global_data.h clearly states that the 
top 16 bits of flags are reserved for arch-specific flags, and your 
patch here uses the lowest of these 16 arch-specific flags for generic code.

Is this a problem? Does any arch code use the upper 16 bits? I would 
have thought you'd at least need to adjust the comment to reflect your 
new usage...

The reason I ask is that I'd need a place to put some (~5?) 
'is_initialized' bits for some code running in SPL in the 'board_init_f' 
code where BSS shouldn't be used. gd->flags would be ideal for that, but 
I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...

Regards,
Simon

>   
>   #endif /* __ASM_GENERIC_GBL_DATA_H */
> diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h
> index c7805cf36b..0e65a12345 100644
> --- a/include/configs/turris_omnia.h
> +++ b/include/configs/turris_omnia.h
> @@ -29,11 +29,6 @@
>   #define CONFIG_SPL_I2C_MUX
>   #define CONFIG_SYS_I2C_MVTWSI
>   
> -/* Watchdog support */
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -# define CONFIG_WATCHDOG
> -#endif
> -
>   /*
>    * SDIO/MMC Card Configuration
>    */
> diff --git a/include/wdt.h b/include/wdt.h
> index e9a7c5355a..aa77d3e9b4 100644
> --- a/include/wdt.h
> +++ b/include/wdt.h
> @@ -6,6 +6,9 @@
>   #ifndef _WDT_H_
>   #define _WDT_H_
>   
> +#include <dm.h>
> +#include <dm/read.h>
> +
>   /*
>    * Implement a simple watchdog uclass. Watchdog is basically a timer that
>    * is used to detect or recover from malfunction. During normal operation
> @@ -103,4 +106,42 @@ struct wdt_ops {
>   	int (*expire_now)(struct udevice *dev, ulong flags);
>   };
>   
> +#if defined(CONFIG_WDT)
> +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
> +#define CONFIG_WATCHDOG_TIMEOUT_MSECS	(60 * 1000)
> +#endif
> +#define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
> +
> +static inline int initr_watchdog(void)
> +{
> +	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +
> +	/*
> +	 * 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;
> +		}
> +	}
> +
> +	if (CONFIG_IS_ENABLED(OF_CONTROL)) {
> +		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> +					       WATCHDOG_TIMEOUT_SECS);
> +	}
> +
> +	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	gd->flags |= GD_FLG_WDT_READY;
> +	printf("WDT:   Started with%s servicing (%ds timeout)\n",
> +	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
> +
> +	return 0;
> +}
> +#endif
> +
>   #endif  /* _WDT_H_ */
>