[U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Maxim Sloyko maxims@google.com Cc: Erik van Luijk evanluijk@interact.nl Cc: Ryder Lee ryder.lee@mediatek.com Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Glass sjg@chromium.org Cc: "Álvaro Fernández Rojas" noltari@gmail.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Christophe Leroy christophe.leroy@c-s.fr Cc: Chris Packham chris.packham@alliedtelesis.co.nz Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100) --- 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@denx.de smartweb: Heiko Schocher hs@denx.de ast2500: Maxim Sloyko maxims@google.com picosam9g45: Erik van Luijk evanluijk@interact.nl mt7623n_bpir2: Ryder Lee ryder.lee@mediatek.com Weijie Gao weijie.gao@mediatek.com mt7629_rfb: Ryder Lee ryder.lee@mediatek.com Weijie Gao weijie.gao@mediatek.com bitmain_antminer_s9: Michal Simek monstr@monstr.eu sandbox64: Simon Glass sjg@chromium.org sandbox: Simon Glass sjg@chromium.org comtrend_ct5361_ram: "Álvaro Fernández Rojas" noltari@gmail.com netgear_cg3100d_ram: "Álvaro Fernández Rojas" noltari@gmail.com bcm968380gerg_ram: Philippe Reynes philippe.reynes@softathome.com bcm963158_ram: Philippe Reynes philippe.reynes@softathome.com bcm968580xref_ram: Philippe Reynes philippe.reynes@softathome.com MCR3000: Christophe Leroy christophe.leroy@c-s.fr
Thanks, Stefan
v5: - Remove board specific x530 watchdog support and implement this in this generic apprioach as well. For this, we need to support the watchdog in SPL now. Move the watchdog init function initr_watchdog() into wdt.h to not duplicate this code.
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 ---------------- board/alliedtelesis/x530/x530.c | 36 +--------------- .../microblaze-generic/microblaze-generic.c | 40 ------------------ board/xilinx/zynq/board.c | 39 ------------------ board/xilinx/zynqmp/zynqmp.c | 39 ------------------ common/board_r.c | 4 ++ common/spl/spl.c | 5 +++ drivers/watchdog/Kconfig | 1 + drivers/watchdog/wdt-uclass.c | 26 ++++++++++++ include/asm-generic/global_data.h | 4 ++ include/configs/turris_omnia.h | 5 --- include/wdt.h | 41 +++++++++++++++++++ 14 files changed, 82 insertions(+), 259 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 0287f23283..4c08f810a2 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, 120000, 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/alliedtelesis/x530/x530.c b/board/alliedtelesis/x530/x530.c index 6934fd8017..97dbed79dd 100644 --- a/board/alliedtelesis/x530/x530.c +++ b/board/alliedtelesis/x530/x530.c @@ -25,10 +25,6 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_NVS_LOCATION 0xf4800000 #define CONFIG_NVS_SIZE (512 << 10)
-#ifdef CONFIG_WATCHDOG -static struct udevice *watchdog_dev; -#endif - static struct serdes_map board_serdes_map[] = { {PEX0, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, {DEFAULT_SERDES, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, @@ -80,10 +76,6 @@ struct mv_ddr_topology_map *mv_ddr_topology_map_get(void)
int board_early_init_f(void) { -#ifdef CONFIG_WATCHDOG - watchdog_dev = NULL; -#endif - /* Configure MPP */ writel(0x00001111, MVEBU_MPP_BASE + 0x00); writel(0x00000000, MVEBU_MPP_BASE + 0x04); @@ -99,13 +91,6 @@ int board_early_init_f(void)
void spl_board_init(void) { -#ifdef CONFIG_WATCHDOG - int ret; - - ret = uclass_get_device(UCLASS_WDT, 0, &watchdog_dev); - if (!ret) - wdt_start(watchdog_dev, 120000, 0); -#endif }
int board_init(void) @@ -128,29 +113,10 @@ int board_init(void) void arch_preboot_os(void) { #ifdef CONFIG_WATCHDOG - wdt_stop(watchdog_dev); + wdt_stop(gd->watchdog_dev); #endif }
-#ifdef CONFIG_WATCHDOG -void watchdog_reset(void) -{ - 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 - static int led_7seg_init(unsigned int segments) { int node; 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 5189925beb..c840e92d9c 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; @@ -344,44 +340,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 1ad44bbe3f..150e8cd424 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;
@@ -677,6 +678,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/common/spl/spl.c b/common/spl/spl.c index 88d4b8a9bf..0a6a47c202 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -22,6 +22,7 @@ #include <linux/compiler.h> #include <fdt_support.h> #include <bootcount.h> +#include <wdt.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -600,6 +601,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_board_init(); #endif
+#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && defined(CONFIG_WDT) + initr_watchdog(); +#endif + if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF)) dram_init_banksize();
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 9d7f503b69..aa8e725573 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 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_ */
Now that we have a generic DT property "timeout-sec" handling, the driver specific implementation can be dropped.
This patch also changes the timeout restriction to the min and max values (clipping). Before this patch, the value provided via "timeout-sec" was used if the parameter was too high or low. Now the driver specific min and max values are used instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100) --- v5: - No Change
v4: - No Change
v3: - Divide timeout in _start() by 1000 to get value in seconds
v2: - New patch
drivers/watchdog/cdns_wdt.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/watchdog/cdns_wdt.c b/drivers/watchdog/cdns_wdt.c index fc85fbcec2..6a608b6371 100644 --- a/drivers/watchdog/cdns_wdt.c +++ b/drivers/watchdog/cdns_wdt.c @@ -10,6 +10,7 @@ #include <dm.h> #include <wdt.h> #include <clk.h> +#include <div64.h> #include <linux/io.h>
DECLARE_GLOBAL_DATA_PTR; @@ -23,7 +24,6 @@ struct cdns_regs {
struct cdns_wdt_priv { bool rst; - u32 timeout; struct cdns_regs *regs; };
@@ -142,10 +142,10 @@ static int cdns_wdt_start(struct udevice *dev, u64 timeout, ulong flags) return -1; }
- if ((timeout < CDNS_WDT_MIN_TIMEOUT) || - (timeout > CDNS_WDT_MAX_TIMEOUT)) { - timeout = priv->timeout; - } + /* Calculate timeout in seconds and restrict to min and max value */ + do_div(timeout, 1000); + timeout = max_t(u64, timeout, CDNS_WDT_MIN_TIMEOUT); + timeout = min_t(u64, timeout, CDNS_WDT_MAX_TIMEOUT);
debug("%s: CLK_FREQ %ld, timeout %lld\n", __func__, clk_f, timeout);
@@ -235,12 +235,9 @@ static int cdns_wdt_ofdata_to_platdata(struct udevice *dev) if (IS_ERR(priv->regs)) return PTR_ERR(priv->regs);
- priv->timeout = dev_read_u32_default(dev, "timeout-sec", - CDNS_WDT_DEFAULT_TIMEOUT); - priv->rst = dev_read_bool(dev, "reset-on-timeout");
- debug("%s: timeout %d, reset %d\n", __func__, priv->timeout, priv->rst); + debug("%s: reset %d\n", __func__, priv->rst);
return 0; }
On 25.04.19 09:17, Stefan Roese wrote:
Now that we have a generic DT property "timeout-sec" handling, the driver specific implementation can be dropped.
This patch also changes the timeout restriction to the min and max values (clipping). Before this patch, the value provided via "timeout-sec" was used if the parameter was too high or low. Now the driver specific min and max values are used instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100)
Applied to u-boot-marvell/master.
Thanks, Stefan
With the generic watchdog driver now implemented, this patch removes some legacy stuff from the MPC8xx watchdog driver and its Kconfig integration. CONFIG_MPC8xx_WATCHDOG is completely removed and hw_watchdog_reset() is made static, as the watchdog will now get serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Christophe Leroy christophe.leroy@c-s.fr --- v5: - No change
v4: - New patch
arch/powerpc/Kconfig | 2 +- arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++--- drivers/watchdog/Kconfig | 1 - drivers/watchdog/Makefile | 2 +- drivers/watchdog/mpc8xx_wdt.c | 4 +--- 5 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c727d9162c..0b1629ba62 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -35,7 +35,7 @@ config MPC8xx bool "MPC8xx" select BOARD_EARLY_INIT_F imply CMD_REGINFO - imply MPC8xx_WATCHDOG + imply WDT_MPC8xx
endchoice
diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig index b0e90a0f20..3e8ea38529 100644 --- a/arch/powerpc/cpu/mpc8xx/Kconfig +++ b/arch/powerpc/cpu/mpc8xx/Kconfig @@ -25,9 +25,9 @@ config MPC885
endchoice
-config MPC8xx_WATCHDOG - bool "Watchdog" - select HW_WATCHDOG +#config MPC8xx_WATCHDOG +# bool "Watchdog" +# select HW_WATCHDOG
config 8xx_GCLK_FREQ int "CPU GCLK Frequency" diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index aa8e725573..3bce0aa0b8 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -151,7 +151,6 @@ config WDT_MT7621 config WDT_MPC8xx bool "MPC8xx watchdog timer support" depends on WDT && MPC8xx - select CONFIG_MPC8xx_WATCHDOG help Select this to enable mpc8xx watchdog timer
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index d901240ad1..40b2f4bc66 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -24,6 +24,6 @@ obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_WDT_ORION) += orion_wdt.o obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o -obj-$(CONFIG_MPC8xx_WATCHDOG) += mpc8xx_wdt.o +obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o obj-$(CONFIG_WDT_MTK) += mtk_wdt.o diff --git a/drivers/watchdog/mpc8xx_wdt.c b/drivers/watchdog/mpc8xx_wdt.c index c24c2a9da6..675b62d8b3 100644 --- a/drivers/watchdog/mpc8xx_wdt.c +++ b/drivers/watchdog/mpc8xx_wdt.c @@ -10,7 +10,7 @@ #include <asm/cpm_8xx.h> #include <asm/io.h>
-void hw_watchdog_reset(void) +static void hw_watchdog_reset(void) { immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;
@@ -18,7 +18,6 @@ void hw_watchdog_reset(void) out_be16(&immap->im_siu_conf.sc_swsr, 0xaa39); /* write magic2 */ }
-#ifdef CONFIG_WDT_MPC8xx static int mpc8xx_wdt_start(struct udevice *dev, u64 timeout, ulong flags) { immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; @@ -66,4 +65,3 @@ U_BOOT_DRIVER(wdt_mpc8xx) = { .of_match = mpc8xx_wdt_ids, .ops = &mpc8xx_wdt_ops, }; -#endif /* CONFIG_WDT_MPC8xx */
On 25.04.19 09:17, Stefan Roese wrote:
With the generic watchdog driver now implemented, this patch removes some legacy stuff from the MPC8xx watchdog driver and its Kconfig integration. CONFIG_MPC8xx_WATCHDOG is completely removed and hw_watchdog_reset() is made static, as the watchdog will now get serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Christophe Leroy christophe.leroy@c-s.fr
Applied to u-boot-marvell/master.
Thanks, Stefan
Hi Stefan,
On 04/25/2019 07:17 AM, Stefan Roese wrote:
With the generic watchdog driver now implemented, this patch removes some legacy stuff from the MPC8xx watchdog driver and its Kconfig integration. CONFIG_MPC8xx_WATCHDOG is completely removed and hw_watchdog_reset() is made static, as the watchdog will now get serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Christophe Leroy christophe.leroy@c-s.fr
v5:
- No change
v4:
- New patch
arch/powerpc/Kconfig | 2 +- arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++--- drivers/watchdog/Kconfig | 1 - drivers/watchdog/Makefile | 2 +- drivers/watchdog/mpc8xx_wdt.c | 4 +--- 5 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c727d9162c..0b1629ba62 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -35,7 +35,7 @@ config MPC8xx bool "MPC8xx" select BOARD_EARLY_INIT_F imply CMD_REGINFO
- imply MPC8xx_WATCHDOG
imply WDT_MPC8xx
endchoice
diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig index b0e90a0f20..3e8ea38529 100644 --- a/arch/powerpc/cpu/mpc8xx/Kconfig +++ b/arch/powerpc/cpu/mpc8xx/Kconfig @@ -25,9 +25,9 @@ config MPC885
endchoice
-config MPC8xx_WATCHDOG
- bool "Watchdog"
- select HW_WATCHDOG
+#config MPC8xx_WATCHDOG +# bool "Watchdog" +# select HW_WATCHDOG
If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() (arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the board with restart every second.
#ifndef CONFIG_HW_WATCHDOG /* deactivate watchdog if not enabled in config */ out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & ~SYPCR_SWE); #endif
Should this be changed to use CONFIG_WATCHDOG instead ?
config 8xx_GCLK_FREQ int "CPU GCLK Frequency" diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index aa8e725573..3bce0aa0b8 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -151,7 +151,6 @@ config WDT_MT7621 config WDT_MPC8xx bool "MPC8xx watchdog timer support" depends on WDT && MPC8xx
- select CONFIG_MPC8xx_WATCHDOG
This is brought back by a later patch from Patrice. Unwanted I guess ?
Christophe
Hi Christophe,
On 19.02.20 20:15, Christophe Leroy wrote:
Hi Stefan,
On 04/25/2019 07:17 AM, Stefan Roese wrote:
With the generic watchdog driver now implemented, this patch removes some legacy stuff from the MPC8xx watchdog driver and its Kconfig integration. CONFIG_MPC8xx_WATCHDOG is completely removed and hw_watchdog_reset() is made static, as the watchdog will now get serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Christophe Leroy christophe.leroy@c-s.fr
v5:
- No change
v4:
- New patch
arch/powerpc/Kconfig | 2 +- arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++--- drivers/watchdog/Kconfig | 1 - drivers/watchdog/Makefile | 2 +- drivers/watchdog/mpc8xx_wdt.c | 4 +--- 5 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c727d9162c..0b1629ba62 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -35,7 +35,7 @@ config MPC8xx bool "MPC8xx" select BOARD_EARLY_INIT_F imply CMD_REGINFO - imply MPC8xx_WATCHDOG + imply WDT_MPC8xx endchoice diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig index b0e90a0f20..3e8ea38529 100644 --- a/arch/powerpc/cpu/mpc8xx/Kconfig +++ b/arch/powerpc/cpu/mpc8xx/Kconfig @@ -25,9 +25,9 @@ config MPC885 endchoice -config MPC8xx_WATCHDOG - bool "Watchdog" - select HW_WATCHDOG +#config MPC8xx_WATCHDOG +# bool "Watchdog" +# select HW_WATCHDOG
If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() (arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the board with restart every second.
#ifndef CONFIG_HW_WATCHDOG /* deactivate watchdog if not enabled in config */ out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & ~SYPCR_SWE); #endif
Should this be changed to use CONFIG_WATCHDOG instead ?
Yes, that sound reasonable.
Thanks, Stefan
On 02/20/2020 05:37 AM, Stefan Roese wrote:
Hi Christophe,
On 19.02.20 20:15, Christophe Leroy wrote:
Hi Stefan,
On 04/25/2019 07:17 AM, Stefan Roese wrote:
With the generic watchdog driver now implemented, this patch removes some legacy stuff from the MPC8xx watchdog driver and its Kconfig integration. CONFIG_MPC8xx_WATCHDOG is completely removed and hw_watchdog_reset() is made static, as the watchdog will now get serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Christophe Leroy christophe.leroy@c-s.fr
v5:
- No change
v4:
- New patch
arch/powerpc/Kconfig | 2 +- arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++--- drivers/watchdog/Kconfig | 1 - drivers/watchdog/Makefile | 2 +- drivers/watchdog/mpc8xx_wdt.c | 4 +--- 5 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c727d9162c..0b1629ba62 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -35,7 +35,7 @@ config MPC8xx bool "MPC8xx" select BOARD_EARLY_INIT_F imply CMD_REGINFO - imply MPC8xx_WATCHDOG + imply WDT_MPC8xx endchoice diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig index b0e90a0f20..3e8ea38529 100644 --- a/arch/powerpc/cpu/mpc8xx/Kconfig +++ b/arch/powerpc/cpu/mpc8xx/Kconfig @@ -25,9 +25,9 @@ config MPC885 endchoice -config MPC8xx_WATCHDOG - bool "Watchdog" - select HW_WATCHDOG +#config MPC8xx_WATCHDOG +# bool "Watchdog" +# select HW_WATCHDOG
If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() (arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the board with restart every second.
#ifndef CONFIG_HW_WATCHDOG /* deactivate watchdog if not enabled in config */ out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & ~SYPCR_SWE); #endif
Should this be changed to use CONFIG_WATCHDOG instead ?
Yes, that sound reasonable.
Looking at it more closely, I think there is something going wrong.
watchdog_reset() in wdt-uclass.c bails out without pinging the watchdog until GD_FLG_WDT_READY is set. But this is set in initr_watchdog(). It means that all WATCHDOG_RESET() until then are now ignored. This means all the phase during which Uboot execute from firmware until relocation do not service the watchdog anymore. The watchdog is ON since the CPU reset and must be serviced until someone decides to disable it, in which case it can't be re-enabled later. This means we can't disable it early to re-enable it once DM is ready.
And if I understand correctly doc/README.watchdog, that's exactly what CONFIG_HW_WATCHDOG is made for.
So I think that we just can't switch mpc8xx to CONFIG_WATCHDOG (include/wdt.c clears makes the difference between Hardware watchdogs and software watchdogs), it must remain with CONFIG_HW_WATCHDOG
Christophe
With the 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).
The watchdog servicing is enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Heiko Schocher hs@denx.de Cc: Eugen Hristev eugen.hristev@microchip.com --- v5: - No change
v4: - New patch (as this at91 watchdog changes just hit mainline)
arch/arm/mach-at91/clock.c | 45 ------------------- arch/arm/mach-at91/include/mach/at91_wdt.h | 2 - .../gardena-smart-gateway-at91sam_defconfig | 2 - drivers/watchdog/at91sam9_wdt.c | 8 ---- 4 files changed, 57 deletions(-)
diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c index 1d3df2c09d..8344daeb39 100644 --- a/arch/arm/mach-at91/clock.c +++ b/arch/arm/mach-at91/clock.c @@ -14,8 +14,6 @@
#define EN_UPLL_TIMEOUT 500
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; - void at91_periph_clk_enable(int id) { struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC; @@ -123,46 +121,3 @@ void at91_pllicpr_init(u32 icpr)
writel(icpr, &pmc->pllicpr); } - -/* 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); - } -} - -int arch_early_init_r(void) -{ - struct at91_wdt_priv *priv; - - /* 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; - } - } - - priv = dev_get_priv(watchdog_dev); - if (!priv) { - printf("Watchdog: priv not available!\n"); - return 0; - } - - wdt_start(watchdog_dev, priv->timeout * 1000, 0); - printf("Watchdog: Started\n"); - - return 0; -} diff --git a/arch/arm/mach-at91/include/mach/at91_wdt.h b/arch/arm/mach-at91/include/mach/at91_wdt.h index a8fc73b3d1..8ef8e007d7 100644 --- a/arch/arm/mach-at91/include/mach/at91_wdt.h +++ b/arch/arm/mach-at91/include/mach/at91_wdt.h @@ -28,7 +28,6 @@ typedef struct at91_wdt { struct at91_wdt_priv { void __iomem *regs; u32 regval; - u32 timeout; };
#endif @@ -51,6 +50,5 @@ struct at91_wdt_priv {
/* Hardware timeout in seconds */ #define WDT_MAX_TIMEOUT 16 -#define WDT_DEFAULT_TIMEOUT 2
#endif diff --git a/configs/gardena-smart-gateway-at91sam_defconfig b/configs/gardena-smart-gateway-at91sam_defconfig index b395a5a175..956897dc7f 100644 --- a/configs/gardena-smart-gateway-at91sam_defconfig +++ b/configs/gardena-smart-gateway-at91sam_defconfig @@ -23,7 +23,6 @@ CONFIG_USE_BOOTARGS=y CONFIG_BOOTARGS="console=ttyS0,115200 earlyprintk mtdparts=atmel_nand:256k(bootstrap)ro,768k(uboot)ro,256k(env_redundant),256k(env),512k(dtb),6M(kernel)ro,-(rootfs) rootfstype=ubifs ubi.mtd=6 root=ubi0:rootfs rw" CONFIG_SYS_CONSOLE_IS_IN_ENV=y CONFIG_SYS_CONSOLE_INFO_QUIET=y -CONFIG_ARCH_EARLY_INIT_R=y CONFIG_SPL_SYS_MALLOC_SIMPLE=y CONFIG_SPL_SEPARATE_BSS=y # CONFIG_TPL_BANNER_PRINT is not set @@ -76,7 +75,6 @@ CONFIG_TIMER=y CONFIG_SPL_TIMER=y CONFIG_ATMEL_PIT_TIMER=y # CONFIG_SYS_WHITE_ON_BLACK is not set -CONFIG_WATCHDOG=y CONFIG_WDT=y CONFIG_WDT_AT91=y # CONFIG_UBIFS_SILENCE_MSG is not set diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c index 000769d46d..48433cc158 100644 --- a/drivers/watchdog/at91sam9_wdt.c +++ b/drivers/watchdog/at91sam9_wdt.c @@ -107,14 +107,6 @@ static int at91_wdt_probe(struct udevice *dev) if (!priv->regs) return -EINVAL;
-#if CONFIG_IS_ENABLED(OF_CONTROL) - priv->timeout = dev_read_u32_default(dev, "timeout-sec", - WDT_DEFAULT_TIMEOUT); - debug("%s: timeout %d", __func__, priv->timeout); -#else - priv->timeout = WDT_DEFAULT_TIMEOUT; -#endif - debug("%s: Probing wdt%u\n", __func__, dev->seq);
return 0;
On 25.04.19 09:17, Stefan Roese wrote:
With the 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).
The watchdog servicing is enabled via CONFIG_WATCHDOG.
Signed-off-by: Stefan Roese sr@denx.de Cc: Heiko Schocher hs@denx.de Cc: Eugen Hristev eugen.hristev@microchip.com
Applied to u-boot-marvell/master.
Thanks, Stefan
On 25.04.19 09:17, 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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Maxim Sloyko maxims@google.com Cc: Erik van Luijk evanluijk@interact.nl Cc: Ryder Lee ryder.lee@mediatek.com Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Glass sjg@chromium.org Cc: "Álvaro Fernández Rojas" noltari@gmail.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Christophe Leroy christophe.leroy@c-s.fr Cc: Chris Packham chris.packham@alliedtelesis.co.nz Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100)
Applied to u-boot-marvell/master.
Thanks, Stefan
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@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_ */
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT)
If c) is not an option, then I would prefer to implement b). Here we could also only add this new "flags_arch" variable for arch's that implement such flags (e.g. x86 right now). I could work on such a patch, if we agree on this solution.
Any comments / suggestions?
Thanks, Stefan
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese sr@denx.de wrote:
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
Probably we can drop warm boot.
arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT)
If c) is not an option, then I would prefer to implement b). Here we could also only add this new "flags_arch" variable for arch's that implement such flags (e.g. x86 right now). I could work on such a patch, if we agree on this solution.
Any comments / suggestions?
I'm not keen on arch-specific flags here. They should be in gd->arch... if needed. global_data is current used by generic code.
So maybe move the x86 flag to the generic header?
Regards, Simon
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese sr@denx.de wrote:
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
Probably we can drop warm boot.
Bin, do you think so as well?
arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT)
If c) is not an option, then I would prefer to implement b). Here we could also only add this new "flags_arch" variable for arch's that implement such flags (e.g. x86 right now). I could work on such a patch, if we agree on this solution.
Any comments / suggestions?
I'm not keen on arch-specific flags here. They should be in gd->arch... if needed. global_data is current used by generic code.
So maybe move the x86 flag to the generic header?
Yes, I also think that those flags should not be sprinkled in different headers but collected in the generic header. I'll prepare a patch to move the x86 flags and remove the comment about the upper 16 bits usage. We can remove the x86 warm boot flag later, if this isn't used at all.
Thanks, Stefan
Hi Stefan,
On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese sr@denx.de wrote:
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese sr@denx.de wrote:
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
Probably we can drop warm boot.
Bin, do you think so as well?
I believe we can drop these 2 flags completely. Currently usage of warm/cold boot flags is only limited to coreboot codes.
arch/x86/cpu/coreboot/coreboot.c::last_stage_init()
if (gd->flags & GD_FLG_COLD_BOOT) timestamp_add_to_bootstage();
timestamp_add_to_bootstage() will never be called for coreboot.
arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT)
If c) is not an option, then I would prefer to implement b). Here we could also only add this new "flags_arch" variable for arch's that implement such flags (e.g. x86 right now). I could work on such a patch, if we agree on this solution.
Any comments / suggestions?
I'm not keen on arch-specific flags here. They should be in gd->arch... if needed. global_data is current used by generic code.
So maybe move the x86 flag to the generic header?
Yes, I also think that those flags should not be sprinkled in different headers but collected in the generic header. I'll prepare a patch to move the x86 flags and remove the comment about the upper 16 bits usage. We can remove the x86 warm boot flag later, if this isn't used at all.
Regards, Bin
Hi Bin,
On 15.08.19 16:19, Bin Meng wrote:
Hi Stefan,
On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese sr@denx.de wrote:
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese sr@denx.de wrote:
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
Probably we can drop warm boot.
Bin, do you think so as well?
I believe we can drop these 2 flags completely. Currently usage of warm/cold boot flags is only limited to coreboot codes.
arch/x86/cpu/coreboot/coreboot.c::last_stage_init()
if (gd->flags & GD_FLG_COLD_BOOT) timestamp_add_to_bootstage();timestamp_add_to_bootstage() will never be called for coreboot.
Why is this the case? Will GD_FLG_COLD_BOOT never be set for the coreboot target?
Thanks, Stefan
Hi Stefan,
On Fri, Aug 16, 2019 at 1:11 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 15.08.19 16:19, Bin Meng wrote:
Hi Stefan,
On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese sr@denx.de wrote:
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese sr@denx.de wrote:
Hi Simon,
(added Simon Glass and Bin to Cc)
On 13.08.19 22:16, Simon Goldschmidt wrote:
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@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.
I totally missed this comment.
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...
As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file.
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...
And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits:
$ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */
This should definitely be fixed. I see 3 options right now:
a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely
I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on:
Probably we can drop warm boot.
Bin, do you think so as well?
I believe we can drop these 2 flags completely. Currently usage of warm/cold boot flags is only limited to coreboot codes.
arch/x86/cpu/coreboot/coreboot.c::last_stage_init()
if (gd->flags & GD_FLG_COLD_BOOT) timestamp_add_to_bootstage();timestamp_add_to_bootstage() will never be called for coreboot.
Why is this the case? Will GD_FLG_COLD_BOOT never be set for the coreboot target?
GD_FLG_COLD_BOOT is only set in the 16-bit start code while on coreboot it boots directly from the 32-bit start code.
Regards, Bin
Le 25/04/2019 à 09:17, Stefan Roese a écrit :
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Maxim Sloyko maxims@google.com Cc: Erik van Luijk evanluijk@interact.nl Cc: Ryder Lee ryder.lee@mediatek.com Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Glass sjg@chromium.org Cc: "Álvaro Fernández Rojas" noltari@gmail.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Christophe Leroy christophe.leroy@c-s.fr Cc: Chris Packham chris.packham@alliedtelesis.co.nz Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100)
[...]
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);- }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
Is that really necessary to restrict the refresh like this in the core part of Watchdog driver ?
+} +#endif
- static int wdt_post_bind(struct udevice *dev) { #if defined(CONFIG_NEEDS_MANUAL_RELOC)
Thanks, Christophe
Hi Christophe
On 19.02.20 20:21, Christophe Leroy wrote:
Le 25/04/2019 à 09:17, Stefan Roese a écrit :
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Maxim Sloyko maxims@google.com Cc: Erik van Luijk evanluijk@interact.nl Cc: Ryder Lee ryder.lee@mediatek.com Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Glass sjg@chromium.org Cc: "Álvaro Fernández Rojas" noltari@gmail.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Christophe Leroy christophe.leroy@c-s.fr Cc: Chris Packham chris.packham@alliedtelesis.co.nz Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100)
[...]
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); + }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx.
Would this work for you?
Is that really necessary to restrict the refresh like this in the core part of Watchdog driver ?
It seems to work for most cases (all until I hear of this MPC8xx issue).
Thanks, Stefan
On 02/20/2020 05:43 AM, Stefan Roese wrote:
Hi Christophe
On 19.02.20 20:21, Christophe Leroy wrote:
Le 25/04/2019 à 09:17, Stefan Roese a écrit :
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Maxim Sloyko maxims@google.com Cc: Erik van Luijk evanluijk@interact.nl Cc: Ryder Lee ryder.lee@mediatek.com Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Glass sjg@chromium.org Cc: "Álvaro Fernández Rojas" noltari@gmail.com Cc: Philippe Reynes philippe.reynes@softathome.com Cc: Christophe Leroy christophe.leroy@c-s.fr Cc: Chris Packham chris.packham@alliedtelesis.co.nz Reviewed-by: Michal Simek michal.simek@xilinx.com Tested-by: Michal Simek michal.simek@xilinx.com (on zcu100)
[...]
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); + }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx.
Exactly. However, why is this limitation needed at all ? Why is it a problem to refresh more often ?
Christophe
On 20.02.20 07:38, Christophe Leroy wrote:
<snip>
+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); + }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx.
Exactly. However, why is this limitation needed at all ? Why is it a problem to refresh more often ?
Very likely its not. What is a reasonable value for your platform? 100 or 500ms? I think we could change it to default to a shorter value, but such a change should go in early in the merge window, so that other platforms have a bit of time to test it.
Please feel free to send a patch for this and please add a comment to explain, why the delay is this "short".
Thanks, Stefan
On 20/02/2020 07.43, Stefan Roese wrote:
On 20.02.20 07:38, Christophe Leroy wrote:
<snip>
+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); + }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx.
Exactly. However, why is this limitation needed at all ? Why is it a problem to refresh more often ?
Very likely its not. What is a reasonable value for your platform? 100 or 500ms? I think we could change it to default to a shorter value, but such a change should go in early in the merge window, so that other platforms have a bit of time to test it.
Please feel free to send a patch for this and please add a comment to explain, why the delay is this "short".
IMO, this should come from DT. For a gpio watchdog (which for some reason U-Boot doesn't have a generic driver for) the linux kernel uses a hw_margin_ms property that tells the core how often the watchdog must be pinged - that could be generalized to apply for all, with 1000ms as a default if not set. And I've seen boards with a gpio watchdog with a timeout of 200 ms.
Also, I'm wondering why that generic _reset only handles one watchdog device? I can easily imagine needing to reset both, say, an external gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop over all DM watchdogs, and have the next_reset/hw_margin etc. metadata live with the watchdog device instead of in static variable/build-time constants?
Rasmus
On 20.02.20 08:43, Rasmus Villemoes wrote:
On 20/02/2020 07.43, Stefan Roese wrote:
On 20.02.20 07:38, Christophe Leroy wrote:
<snip>
+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); + }
This is a problem for the MPC8xx.
When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable.
So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx.
Exactly. However, why is this limitation needed at all ? Why is it a problem to refresh more often ?
Very likely its not. What is a reasonable value for your platform? 100 or 500ms? I think we could change it to default to a shorter value, but such a change should go in early in the merge window, so that other platforms have a bit of time to test it.
Please feel free to send a patch for this and please add a comment to explain, why the delay is this "short".
IMO, this should come from DT.
Yes, I agree. I just wanted to keep the change "simple" here with this approach.
For a gpio watchdog (which for some reason U-Boot doesn't have a generic driver for) the linux kernel uses a hw_margin_ms property that tells the core how often the watchdog must be pinged - that could be generalized to apply for all, with 1000ms as a default if not set. And I've seen boards with a gpio watchdog with a timeout of 200 ms.
Ah, okay. Its good to know, that Linux already has such a DT property.
Also, I'm wondering why that generic _reset only handles one watchdog device? I can easily imagine needing to reset both, say, an external gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop over all DM watchdogs, and have the next_reset/hw_margin etc. metadata live with the watchdog device instead of in static variable/build-time constants?
I agree, that the current WDT handling can be extended to support multiple (different) WDT instances. Currently there don't seem to be any (I might be incorrect here) board that need such a feature though. I would suggest to implement such a feature, once its really needed.
Thanks, Stefan
participants (6)
-
Bin Meng -
Christophe Leroy -
Rasmus Villemoes -
Simon Glass -
Simon Goldschmidt -
Stefan Roese