[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

Michal Simek michal.simek at xilinx.com
Fri Apr 5 12:07:03 UTC 2019


On 04. 04. 19 13:23, Stefan Roese wrote:
> 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>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Heiko Schocher <hs at denx.de>
> Cc: Michal Simek <michal.simek at xilinx.com>
> Cc: "Marek BehĂșn" <marek.behun at nic.cz>
> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> ---
> This patch depends on the following patches:
> 
> [1] watchdog: Move watchdog_dev to data section (BSS may not be cleared)
> https://patchwork.ozlabs.org/patch/1075500/
> 
> [2] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
> 
> I would like to see [2] applied in this release, since its a real fix on
> some of the platforms with minimal chances of breakage.
> 
> This patch now is a bigger rework and is intended for the next merge
> window. Please review and test if possible. Comments welcome.
> 
> Thanks,
> Stefan
> 
>  arch/mips/mach-mt7620/cpu.c                   | 36 ---------
>  board/CZ.NIC/turris_mox/turris_mox.c          | 30 --------
>  board/CZ.NIC/turris_omnia/turris_omnia.c      | 35 ---------
>  .../microblaze-generic/microblaze-generic.c   | 40 ----------
>  board/xilinx/zynq/board.c                     | 39 ----------
>  board/xilinx/zynqmp/zynqmp.c                  | 39 ----------
>  drivers/watchdog/wdt-uclass.c                 | 74 +++++++++++++++++++
>  include/asm-generic/global_data.h             |  4 +
>  8 files changed, 78 insertions(+), 219 deletions(-)
> 
> diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c
> index fe74f26a54..fcd0484a6d 100644
> --- a/arch/mips/mach-mt7620/cpu.c
> +++ b/arch/mips/mach-mt7620/cpu.c
> @@ -69,28 +69,6 @@ int print_cpuinfo(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = get_timer(0);
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		next_reset = now + 1000;	/* reset every 1000ms */
> -		wdt_reset(watchdog_dev);
> -	}
> -}
> -#endif
> -
>  int arch_misc_init(void)
>  {
>  	/*
> @@ -103,19 +81,5 @@ int arch_misc_init(void)
>  	flush_dcache_range(gd->bd->bi_memstart,
>  			   gd->bd->bi_memstart + gd->ram_size - 1);
>  
> -#ifdef CONFIG_WATCHDOG
> -	/* Init watchdog */
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 60000, 0);	/* 60 seconds */
> -	printf("Watchdog: Started\n");
> -#endif
> -
>  	return 0;
>  }
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
> index 96cb9c7e5c..8a4872343b 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -119,41 +119,11 @@ int board_fix_fdt(void *blob)
>  }
>  #endif
>  
> -#ifdef CONFIG_WDT_ARMADA_37XX
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -void watchdog_reset(void)
> -{
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 100000;
> -	}
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	/* address of boot parameters */
>  	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>  
> -#ifdef CONFIG_WDT_ARMADA_37XX
> -	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -		printf("Cannot find Armada 3720 watchdog!\n");
> -	} else {
> -		printf("Enabling Armada 3720 watchdog (3 minutes timeout).\n");
> -		wdt_start(watchdog_dev, 180000, 0);
> -	}
> -#endif
> -
>  	return 0;
>  }
>  
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index c7f6479a0c..8101cfd88a 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -364,25 +364,12 @@ static bool disable_mcu_watchdog(void)
>  }
>  #endif
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  int board_init(void)
>  {
>  	/* adress of boot parameters */
>  	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
>  
>  #ifndef CONFIG_SPL_BUILD
> -# ifdef CONFIG_WDT_ORION
> -	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -		puts("Cannot find Armada 385 watchdog!\n");
> -	} else {
> -		puts("Enabling Armada 385 watchdog.\n");
> -		wdt_start(watchdog_dev, (u32) 25000000 * 120, 0);
> -	}
> -# endif
> -
>  	if (disable_mcu_watchdog())
>  		puts("Disabled MCU startup watchdog.\n");
>  
> @@ -392,28 +379,6 @@ int board_init(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -	static ulong next_reset = 0;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> -
>  int board_late_init(void)
>  {
>  #ifndef CONFIG_SPL_BUILD
> diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
> index 28c9efa3a2..ba82292e35 100644
> --- a/board/xilinx/microblaze-generic/microblaze-generic.c
> +++ b/board/xilinx/microblaze-generic/microblaze-generic.c
> @@ -24,10 +24,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
> -
>  ulong ram_base;
>  
>  int dram_init_banksize(void)
> @@ -43,44 +39,8 @@ int dram_init(void)
>  	return 0;
>  };
>  
> -#ifdef CONFIG_WDT
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -#if !defined(CONFIG_SPL_BUILD)
> -	ulong now;
> -	static ulong next_reset;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -#endif /* !CONFIG_SPL_BUILD */
> -}
> -#endif /* CONFIG_WDT */
> -
>  int board_late_init(void)
>  {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	watchdog_dev = NULL;
> -
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
>  #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYSRESET_MICROBLAZE)
>  	int ret;
>  
> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> index ea26aad16f..6857f2c0b8 100644
> --- a/board/xilinx/zynq/board.c
> +++ b/board/xilinx/zynq/board.c
> @@ -18,10 +18,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F)
>  int board_early_init_f(void)
>  {
> @@ -31,19 +27,6 @@ int board_early_init_f(void)
>  
>  int board_init(void)
>  {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -# endif
> -
>  	return 0;
>  }
>  
> @@ -127,25 +110,3 @@ int dram_init(void)
>  	return 0;
>  }
>  #endif
> -
> -#if defined(CONFIG_WATCHDOG)
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index db27247850..1c12891b82 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -24,10 +24,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \
>      !defined(CONFIG_SPL_BUILD)
>  static xilinx_desc zynqmppl = XILINX_ZYNQMP_DESC;
> @@ -340,44 +336,9 @@ int board_init(void)
>  	}
>  #endif
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -#endif
> -
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> -
>  int board_early_init_r(void)
>  {
>  	u32 val;
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 23b7e3360d..a9330cba0a 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -10,6 +10,12 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  
> +#ifndef WDT_DEFAULT_TIMEOUT
> +#define WDT_DEFAULT_TIMEOUT	60
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>  {
>  	const struct wdt_ops *ops = device_get_ops(dev);
> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>  	return ret;
>  }
>  
> +#if defined(CONFIG_WATCHDOG)

This is not enough because you can enable it just for full u-boot but
not for SPL. It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
macros.

Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build


> +/*
> + * Called by macro WATCHDOG_RESET. This function be called *very* early,
> + * so we need to make sure, that the watchdog driver is ready before using
> + * it in this function.
> + */
> +void watchdog_reset(void)
> +{
> +	static ulong next_reset;
> +	ulong now;
> +
> +	/* Exit if GD is not ready or watchdog is not initialized yet */
> +	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
> +		return;
> +
> +	/* Do not reset the watchdog too often */
> +	now = get_timer(0);
> +	if (now > next_reset) {
> +		next_reset = now + 1000;	/* reset every 1000ms */
> +		wdt_reset(gd->watchdog_dev);
> +	}
> +}
> +
> +static void watchdog_start(void)
> +{
> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	/*
> +	 * Use only the first watchdog device in U-Boot to trigger the
> +	 * watchdog reset
> +	 */
> +	if (gd->watchdog_dev) {

I hope that gd structure is cleared somewhere.

> +		debug("Only one watchdog device used!\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Init watchdog: This will call the probe function of the
> +	 * watchdog driver, enabling the use of the device
> +	 */
> +	if (uclass_get_device(UCLASS_WDT, 0,
> +			      (struct udevice **)&gd->watchdog_dev)) {
> +		debug("Watchdog: Not found!\n");
> +		return;
> +	}

I think you should c&p what I have done in zynq/zynqmp. It means check
watchdog0 alias first because this is supposed to be the first primary
watchdog.

zynq/zynqmp/mb code
       if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
               debug("Watchdog: Not found by seq!\n");
               if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
                       puts("Watchdog: Not found!\n");
                       return 0;
               }
       }



> +
> +	if (CONFIG_IS_ENABLED(OF_CONTROL)) {
> +		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> +					       WDT_DEFAULT_TIMEOUT);
> +	}

You should remove them from drivers.

drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
dev_read_u32_default(dev, "timeout-sec",
drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
dev_read_u32_default(dev, "timeout-sec",


> +
> +	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	gd->flags |= GD_FLG_WDT_READY;
> +	printf("WDT:   Started (%ds timeout)\n", timeout);
> +}
> +#else
> +static void watchdog_start(void)
> +{
> +}
> +#endif
> +
>  static int wdt_post_bind(struct udevice *dev)
>  {
>  #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> @@ -82,6 +149,13 @@ static int wdt_post_bind(struct udevice *dev)
>  		reloc_done++;
>  	}
>  #endif
> +
> +	/*
> +	 * Start the watchdog service in U-Boot if selected via
> +	 * CONFIG_WATCHDOG
> +	 */
> +	watchdog_start();
> +
>  	return 0;
>  }
>  
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 78dcf40bff..3cb583cbb1 100644
> --- 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
> +#ifdef CONFIG_WATCHDOG
> +	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	   */
>  
>  #endif /* __ASM_GENERIC_GBL_DATA_H */
> 

Thanks,
Michal


More information about the U-Boot mailing list