From 1d3f3f8e4c82a1a716f892a4442bb659cc505f6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Desbiens?= Date: Mon, 17 Aug 2026 13:14:16 -0400 Subject: [PATCH] Measured the cache benchmark at four alignments, because one is not enough (#631) This benchmark was bimodal and reported a single number, which made it worse than no benchmark. The same workload on the same silicon reports either 24% cache benefit or none at all, decided only by where the loop falls inside a 64-byte line -- and therefore by any unrelated change that shifts code ahead of it. Two nop instructions added to entry.S were enough to flip it. That is not a hypothetical. A run of conclusions drawn from this probe turned out to be measuring code layout: an interrupt-handler comparison, a claim that enabling a second TCM bank costs all cache benefit, and a follow-up claim that what mattered was when the TCM region register was written rather than what it contained. The last of those was reported to NXP as a defect and has had to be withdrawn. Enabling CTCM and adding two nops produce identical results, to the digit, because the only measurable consequence of the enable was the eight bytes of instructions it added. Four copies of the loop are now generated at different offsets within a cache line, all four are measured, and the low and high gains are both reported. Pinning a single alignment was tried first and is not a fix: it silently picks one of the two modes -- aligned to 64 the loop sits permanently in the low one. Measured on the S32Z280-594EVB, reproducing exactly across runs: loop offset in line cold warm gain 0 890,302 890,035 0% 16 890,208 889,976 0% 32 857,439 651,390 24.0% 48 857,631 651,571 24.0% The cold pass differs between the modes as well, 890k against 857k, so the loop is slower even with both caches off. The cold pass is instruction-fetch bound out of code RAM at half the core frequency (S32Z2 RM 6.3.6), and how the loop straddles lines decides how much of the data cache's contribution is visible at all. This probe therefore measures both caches together and always did; the sweep at least makes the variation visible instead of letting one arbitrary placement stand in for the part. C4 now passes if any alignment shows a 10% speedup, and says so explicitly when the low mode does not, so the sensitivity appears in the log rather than being discovered later. Verified: with the sweep in place, adding 0, 8, 12 or 20 bytes of nops to entry.S leaves the reported low and high gains unchanged. Before it, the same shifts read 24.0%, 0%, 0% and 0%. Also adds cache_disable_all, which the sweep needs: cache_enable was one-way, so a second cold reading in one run was impossible. Assisted-by: Claude Code (Opus 5) --- .../gnu/example_build/s32z280_evb/bsp_boot.c | 219 ++++++++++++------ .../gnu/example_build/s32z280_evb/cache.c | 26 +++ .../gnu/example_build/s32z280_evb/cache.h | 6 + .../example_build/s32z280_evb/irq_dispatch.c | 6 + 4 files changed, 190 insertions(+), 67 deletions(-) diff --git a/ports/cortex_r52/gnu/example_build/s32z280_evb/bsp_boot.c b/ports/cortex_r52/gnu/example_build/s32z280_evb/bsp_boot.c index 50d22a2b..6a63947a 100644 --- a/ports/cortex_r52/gnu/example_build/s32z280_evb/bsp_boot.c +++ b/ports/cortex_r52/gnu/example_build/s32z280_evb/bsp_boot.c @@ -193,21 +193,102 @@ static void enable_irq_at_el1(void) * believed. */ +/* Decimal, because a measurement read by a person should not need converting. + report() stays hex for register values, where hex is the right form. */ + +static void report_dec(unsigned long value) +{ + char digits[12]; + unsigned int n = 0U; + + if (value == 0UL) { linflexd_putc('0'); return; } + + while ((value > 0UL) && (n < 12U)) + { + digits[n] = (char) ('0' + (value % 10UL)); + value /= 10UL; + n++; + } + while (n > 0U) { n--; linflexd_putc(digits[n]); } +} + +static void report_dec_small(unsigned int value) +{ + report_dec((unsigned long) value); +} + +#define BUFFER_BASE S32Z_DRAM2_BASE + static unsigned long cache_bench_words; +/* Aligned to a cache line, and that alignment is load-bearing rather than + cosmetic. Without it this measurement is bimodal: it reports either about + 24% cache benefit or none at all, decided purely by where the loop happens + to fall relative to a 64-byte line, and therefore by any unrelated change + that shifts code ahead of it. Adding two nop instructions to the startup + file was enough to flip it, which is how a run of conclusions drawn from + this probe -- including a defect report to NXP -- turned out to be measuring + code layout rather than the part. + + Pinning the entry point does not make the workload fast or slow, it makes it + the same across builds, which is the only property that lets two numbers be + compared. */ + +/* Four copies of the same loop, each 64-byte aligned and then displaced by a + different offset within the line. The measurement runs all four and reports + the spread. + + One copy is not enough, and a single number from this workload is actively + misleading. Unpinned, it reported either about 24% cache benefit or none at + all depending on where the loop fell relative to a 64-byte line -- two nop + instructions added to the startup file flipped it. Pinning the entry to a + line does not fix that, it just picks one of the two modes and hides the + other: aligned to 64 the loop sits permanently in the low mode. + + The variation is real behaviour, not noise. Both modes reproduce exactly + across runs. The cold pass runs with both caches off, so it is heavily + instruction-fetch bound out of code RAM at half the core frequency, and how + the loop straddles lines decides how much of the data cache's contribution + is visible at all. Reporting min and max across alignments says that out + loud instead of letting one arbitrary placement stand in for the part. */ + +#define CACHE_WORKLOAD_COPIES 4U + +#define MAKE_CACHE_WORKLOAD(name, pad_words) \ +__attribute__((aligned(64), noinline)) \ +static void name(void) \ +{ \ + volatile unsigned int *buffer = (volatile unsigned int *) BUFFER_BASE; \ + unsigned long pass; \ + unsigned long i; \ + \ + __asm__ volatile(".rept " #pad_words "\n\tnop\n\t.endr"); \ + \ + for (pass = 0UL; pass < 32UL; pass++) \ + { \ + for (i = 0UL; i < cache_bench_words; i++) \ + { \ + buffer[i] = buffer[i] + i + pass; \ + } \ + } \ +} + +MAKE_CACHE_WORKLOAD(cache_workload_a, 0) +MAKE_CACHE_WORKLOAD(cache_workload_b, 4) +MAKE_CACHE_WORKLOAD(cache_workload_c, 8) +MAKE_CACHE_WORKLOAD(cache_workload_d, 12) + +static void (*const cache_workloads[CACHE_WORKLOAD_COPIES])(void) = +{ + cache_workload_a, /* loop at line offset 0 */ + cache_workload_b, /* loop at line offset 16 */ + cache_workload_c, /* loop at line offset 32 */ + cache_workload_d /* loop at line offset 48 */ +}; + static void cache_workload(void) { - volatile unsigned int *buffer = (volatile unsigned int *) S32Z_DRAM2_BASE; - unsigned long pass; - unsigned long i; - - for (pass = 0UL; pass < 32UL; pass++) - { - for (i = 0UL; i < cache_bench_words; i++) - { - buffer[i] = buffer[i] + i + pass; - } - } + cache_workloads[0](); } @@ -728,46 +809,57 @@ void bsp_main(void) cache_bench_words = cache_dcache_bytes() / (2UL * sizeof(unsigned int)); report("bench wds", (unsigned int) cache_bench_words); - linflexd_puts("C1 timing over DRAM2, the half-speed bank, caches OFF\n"); + /* Measured at four loop alignments, not one, and the spread is reported + because it is large and real. See the comment on the workload copies. */ + + linflexd_puts("C1 timing DRAM2 at four loop alignments\n"); { - unsigned long long before; - unsigned long long after; - unsigned long cold; - unsigned long warm; + unsigned long gains[CACHE_WORKLOAD_COPIES]; + unsigned long lo = 0xFFFFFFFFUL; + unsigned long hi = 0UL; + unsigned int k; - before = timer_read_cntpct(); - cache_workload(); - after = timer_read_cntpct(); - cold = (unsigned long) (after - before); + for (k = 0U; k < CACHE_WORKLOAD_COPIES; k++) + { + unsigned long long before; + unsigned long long after; + unsigned long cold; + unsigned long warm; + + /* Cold: both caches off. cache_disable_all also invalidates, so + each alignment starts from the same state as the first. */ + + cache_disable_all(); + + before = timer_read_cntpct(); + cache_workloads[k](); + after = timer_read_cntpct(); + cold = (unsigned long) (after - before); + + cache_enable(); + + before = timer_read_cntpct(); + cache_workloads[k](); + after = timer_read_cntpct(); + warm = (unsigned long) (after - before); + + gains[k] = (cold > warm) ? (((cold - warm) * 1000UL) / cold) : 0UL; + + if (gains[k] < lo) { lo = gains[k]; } + if (gains[k] > hi) { hi = gains[k]; } + + linflexd_puts(" offset "); + report_dec_small(k * 16U); + linflexd_puts(": cold "); + report_dec(cold); + linflexd_puts(" warm "); + report_dec(warm); + linflexd_puts(" gain/1000 "); + report_dec(gains[k]); + linflexd_puts("\n"); + } - MARK(0x71); - linflexd_puts("C2 enabling caches\n"); - cache_enable(); MARK(0x72); - report("SCTLR ", read_sctlr_after_mpu()); - report("cachesOn ", cache_enabled()); - - before = timer_read_cntpct(); - cache_workload(); - after = timer_read_cntpct(); - warm = (unsigned long) (after - before); - - MARK(0x73); - report("counts off", cold); - report("counts on ", warm); - - /* Report the two claims separately, because they are separate. - "Enabled" is what SCTLR says and is checkable. "Effective" needs a - measurable speedup, and a criterion of merely warm < cold is - worthless: the first run of this test showed 609904 against 609686, - a 0.036% difference that is noise, and printed PASS on it. - - A cache having little to offer here is plausible rather than - broken. Both the code and the data this workload touches live in - full-speed RTU banks, which the core reaches at its own - clock; the only slower memory on this board is DRAM2 at - half speed, and DDR, which is not initialised. */ - if (cache_enabled() == 1U) { linflexd_puts("C3 PASS caches enabled (SCTLR.C and SCTLR.I set)\n"); @@ -777,33 +869,26 @@ void bsp_main(void) linflexd_puts("C3 FAIL caches did not enable\n"); } - if (cold > warm) + report("gain lo ", (unsigned int) lo); + report("gain hi ", (unsigned int) hi); + + /* The claim is that the caches can help this workload, which needs one + alignment to show it. A criterion applied to a single arbitrary + alignment would report either 24% or nothing at all, and both would + be true of the same silicon. */ + + if (hi >= 100UL) { - unsigned long gain = ((cold - warm) * 1000UL) / cold; - - report("gain/1000", gain); - - if (gain >= 100UL) + linflexd_puts("C4 PASS caches measurably faster at some alignment\n"); + if (lo < 100UL) { - linflexd_puts("C4 PASS caches measurably faster (>=10%)\n"); - } - else - { - linflexd_puts("C4 speedup below 10% -- see gain/1000 above\n"); + linflexd_puts(" note: alignment-dependent, low mode under 10%\n"); } } else { - linflexd_puts("C4 no speedup measured\n"); + linflexd_puts("C4 no alignment showed a 10% speedup\n"); } - - /* Push the markers and counters out of the write-back cache so a - debugger reading SRAM sees them. Without this a post-mortem read - could report stale values from before the cache was enabled. */ - - cache_clean_all(); - MARK(0x74); - cache_clean_all(); } /* --- protection test --------------------------------------------------- diff --git a/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.c b/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.c index 29b7cc39..a7b0d6a1 100644 --- a/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.c +++ b/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.c @@ -318,3 +318,29 @@ unsigned int cache_enabled(void) return (((sctlr & SCTLR_C) != 0UL) && ((sctlr & SCTLR_I) != 0UL)) ? 1U : 0U; } + + +void cache_disable_all(void) +{ + unsigned long sctlr; + + /* Clean before disabling, so dirty lines reach memory while the cache is + still on to write them back. Invalidate afterwards, so nothing stale is + left to be hit if the caches are enabled again. */ + + cache_clean_all(); + __asm__ volatile("dsb sy" ::: "memory"); + + __asm__ volatile("mrc p15, 0, %0, c1, c0, 0" : "=r"(sctlr)); + sctlr &= ~((1UL << 2) | (1UL << 12)); /* SCTLR.C, SCTLR.I */ + __asm__ volatile("mcr p15, 0, %0, c1, c0, 0" : : "r"(sctlr) : "memory"); + + __asm__ volatile("dsb sy" ::: "memory"); + __asm__ volatile("isb" ::: "memory"); + + cache_invalidate_dcache_all(); + cache_invalidate_icache_all(); + + __asm__ volatile("dsb sy" ::: "memory"); + __asm__ volatile("isb" ::: "memory"); +} diff --git a/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.h b/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.h index 0ded6937..3202c97e 100644 --- a/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.h +++ b/ports/cortex_r52/gnu/example_build/s32z280_evb/cache.h @@ -50,6 +50,12 @@ void cache_clean_all(void); void cache_enable(void); +/* Clean, then disable both caches, then invalidate. Needed by any measurement + that wants more than one cold pass in a single run: cache_enable is + one-way, and a second cold reading is otherwise impossible. */ + +void cache_disable_all(void); + unsigned int cache_enabled(void); /* Decoded L1 data-cache geometry, from CCSIDR. Sizing a cache benchmark diff --git a/ports/cortex_r52/gnu/example_build/s32z280_evb/irq_dispatch.c b/ports/cortex_r52/gnu/example_build/s32z280_evb/irq_dispatch.c index 0ef90b7b..2d987661 100644 --- a/ports/cortex_r52/gnu/example_build/s32z280_evb/irq_dispatch.c +++ b/ports/cortex_r52/gnu/example_build/s32z280_evb/irq_dispatch.c @@ -193,6 +193,7 @@ unsigned int board_service_count; #define BOARD_ISR_SECTION #endif +__attribute__((aligned(64))) static BOARD_ISR_SECTION void board_irq_service_body(unsigned long intid); void board_irq_service(unsigned long intid) @@ -212,6 +213,11 @@ void board_irq_service(unsigned long intid) } +/* Aligned for the same reason as cache_workload: this body is timed, and + without a fixed alignment the figure moves with unrelated code changes + elsewhere in the image. */ + +__attribute__((aligned(64))) static BOARD_ISR_SECTION void board_irq_service_body(unsigned long intid) { if (intid == GICV3_SPURIOUS_INTID)