From 7dd2097e21342f1b8dc332ed4f5711b2a4b64419 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Tue, 13 Apr 2021 16:43:20 +0200 Subject: [PATCH 1/5] watchdog: use time_after_eq() in watchdog_reset() Some boards don't work with the rate-limiting done in the generic watchdog_reset() provided by wdt-uclass. For example, on powerpc, get_timer() ceases working during bootm since interrupts are disabled before the kernel image gets decompressed, and when the decompression takes longer than the watchdog device allows (or enough of the budget that the kernel doesn't get far enough to assume responsibility for petting the watchdog), the result is a non-booting board. As a somewhat hacky workaround (because DT is supposed to describe hardware), allow specifying hw_margin_ms=0 in device tree to effectively disable the ratelimiting and actually ping the watchdog every time watchdog_reset() is called. For that to work, the "has enough time passed" check just needs to be tweaked a little to allow the now==next_reset case as well. Suggested-by: Christophe Leroy Signed-off-by: Rasmus Villemoes Reviewed-by: Stefan Roese --- drivers/watchdog/wdt-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 0603ffbd36..2687135296 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -148,7 +148,7 @@ void watchdog_reset(void) /* Do not reset the watchdog too often */ now = get_timer(0); - if (time_after(now, next_reset)) { + if (time_after_eq(now, next_reset)) { next_reset = now + reset_period; wdt_reset(gd->watchdog_dev); } From 2156016294c7cb005bba3d8b7c7f13bc09d7824c Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 14 Apr 2021 09:18:21 +0200 Subject: [PATCH 2/5] timer: mpc83xx_timer: fix build with CONFIG_{HW_, }WATCHDOG The code, which is likely copied from arch/powerpc/lib/interrupts.c, lacks a fallback definition of CONFIG_SYS_WATCHDOG_FREQ and refers to a non-existing timestamp variable - obviously priv->timestamp is meant. Signed-off-by: Rasmus Villemoes --- drivers/timer/mpc83xx_timer.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c index f4f6e90387..9adb4b4cab 100644 --- a/drivers/timer/mpc83xx_timer.c +++ b/drivers/timer/mpc83xx_timer.c @@ -20,6 +20,10 @@ DECLARE_GLOBAL_DATA_PTR; +#ifndef CONFIG_SYS_WATCHDOG_FREQ +#define CONFIG_SYS_WATCHDOG_FREQ (CONFIG_SYS_HZ / 2) +#endif + /** * struct mpc83xx_timer_priv - Private data structure for MPC83xx timer driver * @decrementer_count: Value to which the decrementer register should be re-set @@ -171,7 +175,7 @@ void timer_interrupt(struct pt_regs *regs) priv->timestamp++; #if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) - if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) + if ((priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) WATCHDOG_RESET(); #endif /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */ From 933ada560b678726835566fbff8a044fa9800175 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 14 Apr 2021 09:18:22 +0200 Subject: [PATCH 3/5] allow opting out of WATCHDOG_RESET() from timer interrupt Having WATCHDOG_RESET() called automatically from the timer interrupt runs counter to the idea of a watchdog device - if the board runs into an infinite loops with interrupts still enabled, the watchdog will never fire. When using CONFIG_(SPL_)WDT, the watchdog_reset function is a lot more complicated than just poking a few SOC-specific registers - it involves accessing all kinds of global data, and if the interrupt happens at the wrong time (say, in the middle of an WATCHDOG_RESET() call from ordinary code), that can end up corrupting said global data. Allow the board to opt out of calling WATCHDOG_RESET() from the timer interrupt handler by setting CONFIG_SYS_WATCHDOG_FREQ to 0 - as that setting is currently nonsensical (it would be compile-time divide-by-zero), it cannot affect any existing boards. Add documentation for both the existing and extended meaning of CONFIG_SYS_WATCHDOG_FREQ. Signed-off-by: Rasmus Villemoes --- README | 9 +++++++++ arch/m68k/lib/time.c | 2 +- arch/powerpc/lib/interrupts.c | 2 +- drivers/timer/mpc83xx_timer.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/README b/README index a565748e43..ad13092bbb 100644 --- a/README +++ b/README @@ -747,6 +747,15 @@ The following options need to be configured: SoC, then define this variable and provide board specific code for the "hw_watchdog_reset" function. + CONFIG_SYS_WATCHDOG_FREQ + Some platforms automatically call WATCHDOG_RESET() + from the timer interrupt handler every + CONFIG_SYS_WATCHDOG_FREQ interrupts. If not set by the + board configuration file, a default of CONFIG_SYS_HZ/2 + (i.e. 500) is used. Setting CONFIG_SYS_WATCHDOG_FREQ + to 0 disables calling WATCHDOG_RESET() from the timer + interrupt. + - Real-Time Clock: When CONFIG_CMD_DATE is selected, the type of the RTC diff --git a/arch/m68k/lib/time.c b/arch/m68k/lib/time.c index cbe29e72a8..ebb2ac54db 100644 --- a/arch/m68k/lib/time.c +++ b/arch/m68k/lib/time.c @@ -71,7 +71,7 @@ void dtimer_interrupt(void *not_used) timestamp++; #if defined(CONFIG_WATCHDOG) || defined (CONFIG_HW_WATCHDOG) - if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) { + if (CONFIG_SYS_WATCHDOG_FREQ && (timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) { WATCHDOG_RESET (); } #endif /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */ diff --git a/arch/powerpc/lib/interrupts.c b/arch/powerpc/lib/interrupts.c index 73f270002c..5ba4cd0c13 100644 --- a/arch/powerpc/lib/interrupts.c +++ b/arch/powerpc/lib/interrupts.c @@ -80,7 +80,7 @@ void timer_interrupt(struct pt_regs *regs) timestamp++; #if defined(CONFIG_WATCHDOG) || defined (CONFIG_HW_WATCHDOG) - if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) + if (CONFIG_SYS_WATCHDOG_FREQ && (timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) WATCHDOG_RESET (); #endif /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */ diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c index 9adb4b4cab..952293195f 100644 --- a/drivers/timer/mpc83xx_timer.c +++ b/drivers/timer/mpc83xx_timer.c @@ -175,7 +175,7 @@ void timer_interrupt(struct pt_regs *regs) priv->timestamp++; #if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) - if ((priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) + if (CONFIG_SYS_WATCHDOG_FREQ && (priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) WATCHDOG_RESET(); #endif /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */ From ec4cc0edf9c73ce65eaf669ad1b6b445c42844a0 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 21 Apr 2021 11:16:02 +0200 Subject: [PATCH 4/5] powerpc: lib: remove leftover CONFIG_5xx CONFIG_5xx hasn't existed since commit 502589777416 (powerpc, 5xx: remove support for 5xx). Remove this last mention of it. Reviewed-by: Stefan Roese Signed-off-by: Rasmus Villemoes --- arch/powerpc/lib/cache.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c index 3c3c470bbb..3e487f50fe 100644 --- a/arch/powerpc/lib/cache.c +++ b/arch/powerpc/lib/cache.c @@ -11,7 +11,6 @@ void flush_cache(ulong start_addr, ulong size) { -#ifndef CONFIG_5xx ulong addr, start, end; start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE - 1); @@ -33,5 +32,4 @@ void flush_cache(ulong start_addr, ulong size) asm volatile("sync" : : : "memory"); /* flush prefetch queue */ asm volatile("isync" : : : "memory"); -#endif } From 729c1fe656913f0d5b09e986fec9976020a3363c Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 21 Apr 2021 11:16:03 +0200 Subject: [PATCH 5/5] powerpc: introduce CONFIG_CACHE_FLUSH_WATCHDOG_THRESHOLD When flush_cache() is called during boot on our ~7M kernel image, the hundreds of thousands of WATCHDOG_RESET calls end up adding significantly to boottime. Flushing a single cache line doesn't take many microseconds, so doing these calls for every cache line is complete overkill. The generic watchdog_reset() provided by wdt-uclass.c actually contains some rate-limiting logic that should in theory mitigate this, but alas, that rate-limiting must be disabled on powerpc because of its get_timer() implementation - get_timer() works just fine until interrupts are disabled, but it just so happens that the "big" flush_cache() call happens in the part of bootm where interrupts are indeed disabled. [1] [2] [3] I have checked with objdump that the generated code doesn't change when this option is left at its default value of 0: gcc is smart enough to see that the ">=" comparison is tautologically true, hence all assignments to "flushed" are eliminated as dead stores. On our board, setting the option to something like 65536 ends up reducing total boottime by about 0.8 seconds. [1] https://patchwork.ozlabs.org/project/uboot/patch/20200605111657.28773-1-rasmus.villemoes@prevas.dk/ [2] https://lists.denx.de/pipermail/u-boot/2021-April/446906.html [3] https://lists.denx.de/pipermail/u-boot/2021-April/447280.html Reviewed-by: Stefan Roese Signed-off-by: Rasmus Villemoes --- arch/powerpc/Kconfig | 1 + arch/powerpc/lib/Kconfig | 9 +++++++++ arch/powerpc/lib/cache.c | 15 +++++++++++++-- 3 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 arch/powerpc/lib/Kconfig diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 6a2e88fed2..133447648c 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -49,5 +49,6 @@ source "arch/powerpc/cpu/mpc83xx/Kconfig" source "arch/powerpc/cpu/mpc85xx/Kconfig" source "arch/powerpc/cpu/mpc86xx/Kconfig" source "arch/powerpc/cpu/mpc8xx/Kconfig" +source "arch/powerpc/lib/Kconfig" endmenu diff --git a/arch/powerpc/lib/Kconfig b/arch/powerpc/lib/Kconfig new file mode 100644 index 0000000000..b30b5edf7c --- /dev/null +++ b/arch/powerpc/lib/Kconfig @@ -0,0 +1,9 @@ +config CACHE_FLUSH_WATCHDOG_THRESHOLD + int "Bytes to flush between WATCHDOG_RESET calls" + default 0 + help + The flush_cache() function periodically, and by default for + every cache line, calls WATCHDOG_RESET(). When flushing a + large area, that may add a significant amount of + overhead. This option allows you to set a threshold for how + many bytes to flush between each WATCHDOG_RESET call. diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c index 3e487f50fe..19162511ce 100644 --- a/arch/powerpc/lib/cache.c +++ b/arch/powerpc/lib/cache.c @@ -9,9 +9,20 @@ #include #include +static ulong maybe_watchdog_reset(ulong flushed) +{ + flushed += CONFIG_SYS_CACHELINE_SIZE; + if (flushed >= CONFIG_CACHE_FLUSH_WATCHDOG_THRESHOLD) { + WATCHDOG_RESET(); + flushed = 0; + } + return flushed; +} + void flush_cache(ulong start_addr, ulong size) { ulong addr, start, end; + ulong flushed = 0; start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE - 1); end = start_addr + size - 1; @@ -19,7 +30,7 @@ void flush_cache(ulong start_addr, ulong size) for (addr = start; (addr <= end) && (addr >= start); addr += CONFIG_SYS_CACHELINE_SIZE) { asm volatile("dcbst 0,%0" : : "r" (addr) : "memory"); - WATCHDOG_RESET(); + flushed = maybe_watchdog_reset(flushed); } /* wait for all dcbst to complete on bus */ asm volatile("sync" : : : "memory"); @@ -27,7 +38,7 @@ void flush_cache(ulong start_addr, ulong size) for (addr = start; (addr <= end) && (addr >= start); addr += CONFIG_SYS_CACHELINE_SIZE) { asm volatile("icbi 0,%0" : : "r" (addr) : "memory"); - WATCHDOG_RESET(); + flushed = maybe_watchdog_reset(flushed); } asm volatile("sync" : : : "memory"); /* flush prefetch queue */