Repository navigation
CMSIS: share L1 cache maintenance support across A and R profiles - #319
Conversation
|
Shall we add some checks to Core tests for the new/changed features? |
There was a problem hiding this comment.
🟡 Changes recommended
The new shared cache helper header contains a functional bug in __log2_up(), and the R-profile CP15 headers include an AArch64-only header that is likely to break AArch32 builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors CMSIS Core cache-maintenance support by extracting the common L1 cache maintenance helpers into a shared header and reusing it across Armv7-A, Armv7-R, and Armv8-R profile headers.
Changes:
- Added
CMSIS/Core/Include/a-profile/l1_cache.hto host shared L1 cache enable/disable and cache maintenance helpers. - Updated
a-profile/armv7a.hto include the sharedl1_cache.hinstead of defining the helpers inline. - Updated R-profile headers (
armv7r.h,armv8r.h) and CP15 glue headers to pull in the shared cache-maintenance implementation and required CP15 interfaces.
File summaries
| File | Description |
|---|---|
| CMSIS/Core/Include/r-profile/armv8r.h | Includes shared L1 cache maintenance header for Armv8-R profile. |
| CMSIS/Core/Include/r-profile/armv8r_cp15.h | Adjusts CP15 include wiring for Armv8-R (but currently pulls in an AArch64-only header). |
| CMSIS/Core/Include/r-profile/armv7r.h | Includes shared L1 cache maintenance header for Armv7-R profile. |
| CMSIS/Core/Include/r-profile/armv7r_cp15.h | Adjusts CP15 include wiring for Armv7-R (but currently pulls in an AArch64-only header). |
| CMSIS/Core/Include/a-profile/l1_cache.h | New shared header containing L1 cache maintenance helpers reused across profiles. |
| CMSIS/Core/Include/a-profile/armv7a.h | Switches to including the new shared L1 cache maintenance header. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 256 files - 388 256 suites - 388 0s ⏱️ - 14m 48s Results for commit 1504a1b. ± Comparison against base commit 07d7306. This pull request removes 49 and adds 56 tests. Note that renamed tests count towards both.This pull request removes 5 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
a02d9f8 to
5429093
Compare
a2a53c0 to
d12fd55
Compare
## Summary - detect all non-power-of-two inputs when rounding `log2` upward - extend the Cortex-A cache validation with even non-power-of-two cases Fixes #320. ## Problem The loop in `__log2_up()` computes `floor(log2(n))`, but the final increment only checks whether `n` is odd. Even non-powers of two such as 6 and 10 therefore remain rounded down, which can produce an incorrect cache-way shift. The new condition increments for every value that is not a power of two while preserving the existing behavior for 0, 1, exact powers of two, and the upper `uint32_t` range. ## Validation - reproduced the current failures for 6, 10, 12, and 14 - checked the updated function against an independent reference for inputs 0 through 100000 and seven upper-range edge cases - compiled the updated header as a freestanding Cortex-A9 object with Clang targeting `armv7-none-eabi` - ran `git diff --check` The CoreValidation FVP suite and physical hardware were not run because the required `cbuild` and FVP tools are not available locally. PR #319 moves this helper into a shared header but retains the same parity check, so this logic fix remains necessary; it may need a straightforward conflict resolution if that refactor lands first. LLMs were used for review and code generation in some parts of this pull request.
|
@hamtinepeng this PR needs attention |
@JonatanAntoni I’m fine rebasing the current branch, but I’d like to wait for a response to this comment first. |
|
@Masmiseim36, we are waiting for your feedback. |
I honestly lost track of this topic. Thank you for bringing it up again. |
Extract the common L1 cache maintenance implementation into l1_cache.h and reuse it for Armv7-A, Armv7-R and Armv8-R.
__L1C_MaintainDCacheSetWay() selects a cache by writing CSSELR and then reads CCSIDR to get the number of sets, the number of ways and the line size. An ISB is required between the two so that the new selection is used by the CCSIDR read (see ARM DDI 0406C.d, section B2.2). __set_CSSELR() is a bare CP15 MCR and does not imply an ISB. Co-authored-by: Jonatan Antoni <jonatan.antoni@arm.com>
|
@Masmiseim36, may I ask you to check the latest review comments and resolve the conversations if the responses are sufficient? Having all sorted is our qa gate for getting this merged. |
Extract the common L1 cache maintenance implementation into l1_cache.h and reuse it for Armv7-A, Armv7-R and Armv8-R.