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

Michal Simek michal.simek at xilinx.com
Tue Apr 9 10:45:49 UTC 2019


On 08. 04. 19 11:28, 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: Heiko Schocher <hs at denx.de>
> Cc: Tom Rini <trini at konsulko.com>
> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
> driver already reading it from the DT (cdns & at91) in this patch.
> The reason for this is, that in the cdns case, the removal also brings
> a functional change, which I wanted to do in a separate patch. And
> in the at91 case its because there are updates to this driver already 
> queued in the at91 next git branch which would most likely cause merge
> conflict. I'll send a cleanup patch for this driver later after these
> patches have been applied.
> 
> Thanks,
> Stefan
> 
> v2:
> - Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
>   This way its only called once, after the DM subsystem has bound all
>   watchdog drivers. This enables the use of the currently implemented
>   logic of using the correct watchdog in U-Boot (via alias etc).
> 
>  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 ------------------
>  common/board_r.c                              | 40 +++++++++++++++++++
>  drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>  include/asm-generic/global_data.h             |  4 ++
>  9 files changed, 70 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/common/board_r.c b/common/board_r.c
> index 472987d5d5..d80f16c3ed 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -48,6 +48,7 @@
>  #include <linux/compiler.h>
>  #include <linux/err.h>
>  #include <efi_loader.h>
> +#include <wdt.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>  }
>  #endif
>  
> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)

This is not correct.
Here should be just CONFIG_WDT.

Because there are cases where you want just start watchdog in u-boot but
never service it by u-boot.


> +#ifndef WDT_DEFAULT_TIMEOUT
> +#define WDT_DEFAULT_TIMEOUT	60
> +#endif
> +
> +static int initr_watchdog(void)
> +{
> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	/*
> +	 * 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",
> +					       WDT_DEFAULT_TIMEOUT);
> +	}
> +
> +	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	gd->flags |= GD_FLG_WDT_READY;
> +	printf("WDT:   Started (%ds timeout)\n", timeout);
> +
> +	return 0;
> +}
> +#endif
> +
>  static int run_main_loop(void)
>  {
>  #ifdef CONFIG_SANDBOX
> @@ -670,6 +707,9 @@ static init_fnc_t init_sequence_r[] = {
>  #ifdef CONFIG_DM
>  	initr_dm,
>  #endif
> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)

ditto.

> +	initr_watchdog,
> +#endif
>  #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
>  	defined(CONFIG_SANDBOX)
>  	board_init,	/* Setup chipselects */
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 23b7e3360d..bbfac4f0f9 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -10,6 +10,8 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  
> +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 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>  	return ret;
>  }
>  
> +#if defined(CONFIG_WATCHDOG)
> +/*
> + * 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);
> +	}
> +}
> +#endif
> +
>  static int wdt_post_bind(struct udevice *dev)
>  {
>  #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> 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

CONFIG_WDT here.

nit: don't know if you should use #if defined() or if CONFIG_IS_ENABLED
here.

> +	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 */
> 

M


More information about the U-Boot mailing list