[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