On 04. 04. 19 13:23, Stefan Roese wrote:
> This patch tries to implement a generic watchdog_reset() function that
> can be used by all boards that want to service the watchdog device in
> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
>
> Without this approach, new boards or platforms needed to implement a
> board specific version of this functionality, mostly copy'ing the same
> code over and over again into their board or platforms code base.
>
> With this new generic function, the scattered other functions are now
> removed to be replaced by the generic one. The new version also enables
> the configuration of the watchdog timeout via the DT "timeout-sec"
> property (if enabled via CONFIG_OF_CONTROL).
>
> This patch also adds a new flag to the GD flags, to flag that the
> watchdog is ready to use and adds the pointer to the watchdog device
> to the GD. This enables us to remove the global "watchdog_dev"
> variable, which was prone to cause problems because of its potentially
> very early use in watchdog_reset(), even before the BSS is cleared.
>
> Signed-off-by: Stefan Roese <s...@denx.de>
> Cc: Tom Rini <tr...@konsulko.com>
> Cc: Simon Glass <s...@chromium.org>
> Cc: Heiko Schocher <h...@denx.de>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: "Marek Behún" <marek.be...@nic.cz>
> Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
> ---
> This patch depends on the following patches:
>
> [1] watchdog: Move watchdog_dev to data section (BSS may not be cleared)
> https://patchwork.ozlabs.org/patch/1075500/
>
> [2] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
>
> I would like to see [2] applied in this release, since its a real fix on
> some of the platforms with minimal chances of breakage.
>
> This patch now is a bigger rework and is intended for the next merge
> window. Please review and test if possible. Comments welcome.
>
> Thanks,
> Stefan
>
> arch/mips/mach-mt7620/cpu.c | 36 ---------
> board/CZ.NIC/turris_mox/turris_mox.c | 30 --------
> board/CZ.NIC/turris_omnia/turris_omnia.c | 35 ---------
> .../microblaze-generic/microblaze-generic.c | 40 ----------
> board/xilinx/zynq/board.c | 39 ----------
> board/xilinx/zynqmp/zynqmp.c | 39 ----------
> drivers/watchdog/wdt-uclass.c | 74 +++++++++++++++++++
> include/asm-generic/global_data.h | 4 +
> 8 files changed, 78 insertions(+), 219 deletions(-)
>
> diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c
> index fe74f26a54..fcd0484a6d 100644
> --- a/arch/mips/mach-mt7620/cpu.c
> +++ b/arch/mips/mach-mt7620/cpu.c
> @@ -69,28 +69,6 @@ int print_cpuinfo(void)
> return 0;
> }
>
> -#ifdef CONFIG_WATCHDOG
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> - static ulong next_reset;
> - ulong now;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = get_timer(0);
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - next_reset = now + 1000; /* reset every 1000ms */
> - wdt_reset(watchdog_dev);
> - }
> -}
> -#endif
> -
> int arch_misc_init(void)
> {
> /*
> @@ -103,19 +81,5 @@ int arch_misc_init(void)
> flush_dcache_range(gd->bd->bi_memstart,
> gd->bd->bi_memstart + gd->ram_size - 1);
>
> -#ifdef CONFIG_WATCHDOG
> - /* Init watchdog */
> - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> - debug("Watchdog: Not found by seq!\n");
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - puts("Watchdog: Not found!\n");
> - return 0;
> - }
> - }
> -
> - wdt_start(watchdog_dev, 60000, 0); /* 60 seconds */
> - printf("Watchdog: Started\n");
> -#endif
> -
> return 0;
> }
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c
> b/board/CZ.NIC/turris_mox/turris_mox.c
> index 96cb9c7e5c..8a4872343b 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -119,41 +119,11 @@ int board_fix_fdt(void *blob)
> }
> #endif
>
> -#ifdef CONFIG_WDT_ARMADA_37XX
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -void watchdog_reset(void)
> -{
> - static ulong next_reset;
> - ulong now;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = timer_get_us();
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - wdt_reset(watchdog_dev);
> - next_reset = now + 100000;
> - }
> -}
> -#endif
> -
> int board_init(void)
> {
> /* address of boot parameters */
> gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>
> -#ifdef CONFIG_WDT_ARMADA_37XX
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - printf("Cannot find Armada 3720 watchdog!\n");
> - } else {
> - printf("Enabling Armada 3720 watchdog (3 minutes timeout).\n");
> - wdt_start(watchdog_dev, 180000, 0);
> - }
> -#endif
> -
> return 0;
> }
>
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c
> b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index c7f6479a0c..8101cfd88a 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -364,25 +364,12 @@ static bool disable_mcu_watchdog(void)
> }
> #endif
>
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
> int board_init(void)
> {
> /* adress of boot parameters */
> gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
>
> #ifndef CONFIG_SPL_BUILD
> -# ifdef CONFIG_WDT_ORION
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - puts("Cannot find Armada 385 watchdog!\n");
> - } else {
> - puts("Enabling Armada 385 watchdog.\n");
> - wdt_start(watchdog_dev, (u32) 25000000 * 120, 0);
> - }
> -# endif
> -
> if (disable_mcu_watchdog())
> puts("Disabled MCU startup watchdog.\n");
>
> @@ -392,28 +379,6 @@ int board_init(void)
> return 0;
> }
>
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> - static ulong next_reset = 0;
> - ulong now;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = timer_get_us();
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - wdt_reset(watchdog_dev);
> - next_reset = now + 1000;
> - }
> -# endif
> -}
> -#endif
> -
> int board_late_init(void)
> {
> #ifndef CONFIG_SPL_BUILD
> diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c
> b/board/xilinx/microblaze-generic/microblaze-generic.c
> index 28c9efa3a2..ba82292e35 100644
> --- a/board/xilinx/microblaze-generic/microblaze-generic.c
> +++ b/board/xilinx/microblaze-generic/microblaze-generic.c
> @@ -24,10 +24,6 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
> -
> ulong ram_base;
>
> int dram_init_banksize(void)
> @@ -43,44 +39,8 @@ int dram_init(void)
> return 0;
> };
>
> -#ifdef CONFIG_WDT
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -#if !defined(CONFIG_SPL_BUILD)
> - ulong now;
> - static ulong next_reset;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = timer_get_us();
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - wdt_reset(watchdog_dev);
> - next_reset = now + 1000;
> - }
> -#endif /* !CONFIG_SPL_BUILD */
> -}
> -#endif /* CONFIG_WDT */
> -
> int board_late_init(void)
> {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> - watchdog_dev = NULL;
> -
> - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> - debug("Watchdog: Not found by seq!\n");
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - puts("Watchdog: Not found!\n");
> - return 0;
> - }
> - }
> -
> - wdt_start(watchdog_dev, 0, 0);
> - puts("Watchdog: Started\n");
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
> #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYSRESET_MICROBLAZE)
> int ret;
>
> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> index ea26aad16f..6857f2c0b8 100644
> --- a/board/xilinx/zynq/board.c
> +++ b/board/xilinx/zynq/board.c
> @@ -18,10 +18,6 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
> #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F)
> int board_early_init_f(void)
> {
> @@ -31,19 +27,6 @@ int board_early_init_f(void)
>
> int board_init(void)
> {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> - debug("Watchdog: Not found by seq!\n");
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - puts("Watchdog: Not found!\n");
> - return 0;
> - }
> - }
> -
> - wdt_start(watchdog_dev, 0, 0);
> - puts("Watchdog: Started\n");
> -# endif
> -
> return 0;
> }
>
> @@ -127,25 +110,3 @@ int dram_init(void)
> return 0;
> }
> #endif
> -
> -#if defined(CONFIG_WATCHDOG)
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> - static ulong next_reset;
> - ulong now;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = timer_get_us();
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - wdt_reset(watchdog_dev);
> - next_reset = now + 1000;
> - }
> -# endif
> -}
> -#endif
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index db27247850..1c12891b82 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -24,10 +24,6 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
> #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \
> !defined(CONFIG_SPL_BUILD)
> static xilinx_desc zynqmppl = XILINX_ZYNQMP_DESC;
> @@ -340,44 +336,9 @@ int board_init(void)
> }
> #endif
>
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> - debug("Watchdog: Not found by seq!\n");
> - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> - puts("Watchdog: Not found!\n");
> - return 0;
> - }
> - }
> -
> - wdt_start(watchdog_dev, 0, 0);
> - puts("Watchdog: Started\n");
> -#endif
> -
> return 0;
> }
>
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> - static ulong next_reset;
> - ulong now;
> -
> - if (!watchdog_dev)
> - return;
> -
> - now = timer_get_us();
> -
> - /* Do not reset the watchdog too often */
> - if (now > next_reset) {
> - wdt_reset(watchdog_dev);
> - next_reset = now + 1000;
> - }
> -# endif
> -}
> -#endif
> -
> int board_early_init_r(void)
> {
> u32 val;
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 23b7e3360d..a9330cba0a 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -10,6 +10,12 @@
> #include <dm/device-internal.h>
> #include <dm/lists.h>
>
> +#ifndef WDT_DEFAULT_TIMEOUT
> +#define WDT_DEFAULT_TIMEOUT 60
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
> {
> const struct wdt_ops *ops = device_get_ops(dev);
> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
> return ret;
> }
>
> +#if defined(CONFIG_WATCHDOG)
This is not enough because you can enable it just for full u-boot but
not for SPL. It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
macros.
Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build
> +/*
> + * Called by macro WATCHDOG_RESET. This function be called *very* early,
> + * so we need to make sure, that the watchdog driver is ready before using
> + * it in this function.
> + */
> +void watchdog_reset(void)
> +{
> + static ulong next_reset;
> + ulong now;
> +
> + /* Exit if GD is not ready or watchdog is not initialized yet */
> + if (!gd || !(gd->flags & GD_FLG_WDT_READY))
> + return;
> +
> + /* Do not reset the watchdog too often */
> + now = get_timer(0);
> + if (now > next_reset) {
> + next_reset = now + 1000; /* reset every 1000ms */
> + wdt_reset(gd->watchdog_dev);
> + }
> +}
> +
> +static void watchdog_start(void)
> +{
> + u32 timeout = WDT_DEFAULT_TIMEOUT;
> +
> + /*
> + * Use only the first watchdog device in U-Boot to trigger the
> + * watchdog reset
> + */
> + if (gd->watchdog_dev) {
I hope that gd structure is cleared somewhere.
> + debug("Only one watchdog device used!\n");
> + return;
> + }
> +
> + /*
> + * Init watchdog: This will call the probe function of the
> + * watchdog driver, enabling the use of the device
> + */
> + if (uclass_get_device(UCLASS_WDT, 0,
> + (struct udevice **)&gd->watchdog_dev)) {
> + debug("Watchdog: Not found!\n");
> + return;
> + }
I think you should c&p what I have done in zynq/zynqmp. It means check
watchdog0 alias first because this is supposed to be the first primary
watchdog.
zynq/zynqmp/mb code
if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
debug("Watchdog: Not found by seq!\n");
if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
puts("Watchdog: Not found!\n");
return 0;
}
}
> +
> + if (CONFIG_IS_ENABLED(OF_CONTROL)) {
> + timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> + WDT_DEFAULT_TIMEOUT);
> + }
You should remove them from drivers.
drivers/watchdog/at91sam9_wdt.c:119: priv->timeout =
dev_read_u32_default(dev, "timeout-sec",
drivers/watchdog/cdns_wdt.c:238: priv->timeout =
dev_read_u32_default(dev, "timeout-sec",
> +
> + wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> + gd->flags |= GD_FLG_WDT_READY;
> + printf("WDT: Started (%ds timeout)\n", timeout);
> +}
> +#else
> +static void watchdog_start(void)
> +{
> +}
> +#endif
> +
> static int wdt_post_bind(struct udevice *dev)
> {
> #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> @@ -82,6 +149,13 @@ static int wdt_post_bind(struct udevice *dev)
> reloc_done++;
> }
> #endif
> +
> + /*
> + * Start the watchdog service in U-Boot if selected via
> + * CONFIG_WATCHDOG
> + */
> + watchdog_start();
> +
> return 0;
> }
>
> diff --git a/include/asm-generic/global_data.h
> b/include/asm-generic/global_data.h
> index 78dcf40bff..3cb583cbb1 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -133,6 +133,9 @@ typedef struct global_data {
> struct spl_handoff *spl_handoff;
> # endif
> #endif
> +#ifdef CONFIG_WATCHDOG
> + struct udevice *watchdog_dev;
> +#endif
> } gd_t;
> #endif
>
> @@ -161,5 +164,6 @@ typedef struct global_data {
> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */
> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done
> */
> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */
> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */
>
> #endif /* __ASM_GENERIC_GBL_DATA_H */
>
Thanks,
Michal
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
