[U-Boot] [PATCH 1/4 v4] watchdog: Implement generic watchdog_reset() version
Michal Simek
michal.simek at xilinx.com
Fri Apr 12 08:22:13 UTC 2019
On 11. 04. 19 15:58, 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>
> Cc: Maxim Sloyko <maxims at google.com>
> Cc: Erik van Luijk <evanluijk at interact.nl>
> Cc: Ryder Lee <ryder.lee at mediatek.com>
> Cc: Weijie Gao <weijie.gao at mediatek.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: "Álvaro Fernández Rojas" <noltari at gmail.com>
> Cc: Philippe Reynes <philippe.reynes at softathome.com>
> Cc: Christophe Leroy <christophe.leroy at c-s.fr>
> ---
> This patch depends on the following patch:
>
> [1] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
>
> We have a few boards that have CONFIG_WDT (DM watchdog driver) enabled
> and CONFIG_WATCHDOG (U-Boot watchdog servicing) disabled. This might
> lead to a watchdog reboot, if the board does not boot fast enough into
> some OS (like Linux) which services the watchdog. With this patch
> now, the watchdog is started on all those boards (if found via the DT)
> and also serviced as this patch also "implies" CONFIG_WATCHDOG. If
> this is not desired, then please send a patch to disable
> CONFIG_WATCHDOG for this board. Here the list of those boards with
> their maintainers:
>
> taurus: Heiko Schocher <hs at denx.de>
> smartweb: Heiko Schocher <hs at denx.de>
> ast2500: Maxim Sloyko <maxims at google.com>
> picosam9g45: Erik van Luijk <evanluijk at interact.nl>
> mt7623n_bpir2: Ryder Lee <ryder.lee at mediatek.com> Weijie Gao <weijie.gao at mediatek.com>
> mt7629_rfb: Ryder Lee <ryder.lee at mediatek.com> Weijie Gao <weijie.gao at mediatek.com>
> bitmain_antminer_s9: Michal Simek <monstr at monstr.eu>
> sandbox64: Simon Glass <sjg at chromium.org>
> sandbox: Simon Glass <sjg at chromium.org>
> comtrend_ct5361_ram: "Álvaro Fernández Rojas" <noltari at gmail.com>
> netgear_cg3100d_ram: "Álvaro Fernández Rojas" <noltari at gmail.com>
> bcm968380gerg_ram: Philippe Reynes <philippe.reynes at softathome.com>
> bcm963158_ram: Philippe Reynes <philippe.reynes at softathome.com>
> bcm968580xref_ram: Philippe Reynes <philippe.reynes at softathome.com>
> MCR3000: Christophe Leroy <christophe.leroy at c-s.fr>
>
> Thanks,
> Stefan
>
> v4:
> - No change
>
> v3:
> - Use already available CONFIG_WATCHDOG_TIMEOUT_MSECS option vs the
> newly introduced WDT_DEFAULT_TIMEOUT default timeout value (if not
> provided via DT property).
> - Guard initr_watchdog by CONFIG_WDT instead of CONFIG_WATCHDOG &&
> CONFIG_WDT as suggested by Michal. This makes it possible to enable
> and start the U-Boot watchdog without enabling the U-Boot watchdog
> servicing.
> - Add imply CONFIG_WATCHDOG to CONFIG_WDT, as this restores the
> current behaviour to use the U-Boot watchdog servicing feature, if
> the DM watchdog is enabled (CONFIG_WDT). If this is not desired,
> CONFIG_WATCHDOG can be disabled in the board defconfig.
> - Remove CONFIG_WATCHDOG from include/configs/turris_omnia.h
>
> 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 | 41 +++++++++++++++++++
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/wdt-uclass.c | 26 ++++++++++++
> include/asm-generic/global_data.h | 4 ++
> include/configs/turris_omnia.h | 5 ---
> 11 files changed, 72 insertions(+), 224 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..d457b6715f 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,43 @@ static int initr_bedbug(void)
> }
> #endif
>
> +#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 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 (%ds timeout)\n", timeout);
> +
> + return 0;
> +}
> +#endif
> +
> static int run_main_loop(void)
> {
> #ifdef CONFIG_SANDBOX
> @@ -670,6 +708,9 @@ static init_fnc_t init_sequence_r[] = {
> #ifdef CONFIG_DM
> initr_dm,
> #endif
> +#if defined(CONFIG_WDT)
> + 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/Kconfig b/drivers/watchdog/Kconfig
> index 34e78beb2a..82080d3867 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -51,6 +51,7 @@ config ULP_WATCHDOG
> config WDT
> bool "Enable driver model for watchdog timer drivers"
> depends on DM
> + imply WATCHDOG
> help
> Enable driver model for watchdog timer. At the moment the API
> is very simple and only supports four operations:
> 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..d16f50f677 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
> +#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 */
>
> #endif /* __ASM_GENERIC_GBL_DATA_H */
> diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h
> index 038f6398eb..78e3cf2ff4 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
> -
> /* SPI NOR flash default params, used by sf commands */
> #define CONFIG_SPI_FLASH_SPANSION
>
>
Reviewed-by: Michal Simek <michal.simek at xilinx.com>
Tested-by: Michal Simek <michal.simek at xilinx.com> (on zcu100)
Maybe at some point we should extend this message to also state that it
is not serviced.
Something like:
WDT: Started without servicing (10s timeout)
Thanks,
Michal
More information about the U-Boot
mailing list