Re: [PATCH v2 2/2] powerpc: introduce CONFIG_CACHE_FLUSH_WATCHDOG_THRESHOLD

On 13.04.21 21:38, Rasmus Villemoes wrote:

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 wd_limit is constant 0, the ">=" comparisons are tautologically true, hence all assignments to "flushed" are eliminated

Nitpicking: s/tautologically/autologically. BTW, I've never heard or read this word before.

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-rasm... [2] https://lists.denx.de/pipermail/u-boot/2021-April/446906.html [3] https://lists.denx.de/pipermail/u-boot/2021-April/447280.html

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

arch/powerpc/Kconfig | 1 + arch/powerpc/lib/Kconfig | 9 +++++++++ arch/powerpc/lib/cache.c | 14 ++++++++++++-- 3 files changed, 22 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..115ae8e160 100644 --- a/arch/powerpc/lib/cache.c +++ b/arch/powerpc/lib/cache.c @@ -12,6 +12,8 @@ void flush_cache(ulong start_addr, ulong size) { ulong addr, start, end;

• ulong wd_limit = CONFIG_CACHE_FLUSH_WATCHDOG_THRESHOLD;

• ulong flushed = 0;

  start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE - 1); end = start_addr + size - 1;

@@ -19,7 +21,11 @@ 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 += CONFIG_SYS_CACHELINE_SIZE;
  

• if (flushed >= wd_limit) {
  

• 	WATCHDOG_RESET();
  

• 	flushed = 0;
  

• }
  

  } /* wait for all dcbst to complete on bus */ asm volatile("sync" : : : "memory");

@@ -27,7 +33,11 @@ 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 += CONFIG_SYS_CACHELINE_SIZE;
  

• if (flushed >= wd_limit) {
  

• 	WATCHDOG_RESET();
  

• 	flushed = 0;
  

• }
  

It might make sense to pull these 2 identical code snippets into a function to not duplicate this code.

Other than this:

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan