remind-me-later opened a new pull request, #20461:
URL: https://github.com/apache/nuttx/pull/20461
## Summary
With `CONFIG_STM32_ICACHE=y`, any read of the STM32H5 OTP, read-only (UID,
flash size, package) or high-cycle data (EDATA) flash areas raises a precise
bus fault. These areas only accept 16/32-bit accesses (RM0481 Table 77), and
the manual requires the MPU to disable local cacheability for them
(RM0481 7.3.2).
The current code works around this piecemeal:
- #19978 disables the ICACHE around `stm32_get_uniqueid()`;
- `flash_read_eccsafe16()` in `stm32h563xx_flash.c` does the same for the
OTP and EDATA word reads;
- `nucleo-h563zi` maps the 4 KB OTP/RO area non-cacheable in its board code
(72e468de8f, from #20122).
Everything else still faults, including the in-tree `stm32_otp_read()`
(plain 16-bit loads) and any application or out-of-tree board reading these
areas. Nothing handles a bootloader that hands over with the ICACHE enabled,
and flash programming runs with the ICACHE enabled.
The series, one commit per step:
1. **Fix unused warnings.** An ICACHE build with no ICACHE region configured
warns about an unused `stm32_icache_setup_region()` and an unused
`regval`. Independent of the rest.
2. **Map `0x08fff000-0x09017fff` non-cacheable.** The OTP, RO and EDATA areas
are contiguous, so one MPU region (Normal non-cacheable, execute-never)
covers them. `STM32_ICACHE` now selects `ARM_MPU`, so
`stm32_mpuinitialize()` has reset and enabled the MPU before the ICACHE is
enabled. The `nucleo-h563zi` board region is removed in the same commit:
the Armv8-M MPU faults on an address that matches two regions, so keeping
both would make OTP and UID reads fault on that board (verified on
hardware, see Testing 5).
3. **Drop the per-call-site ICACHE disables.** With the region in place, the
disable/enable pairs in `stm32_get_uniqueid()` and
`flash_read_eccsafe16()` are redundant. Each pair invalidated the whole
ICACHE, in `flash_read_eccsafe16()` with interrupts disabled and twice per
32-bit OTP read.
4. **Start the ICACHE from a clean state.** On first use the driver disables
and invalidates the ICACHE (a bootloader may have left it enabled) before
touching the associativity and region registers (writable only while
`EN=0`), waits for a pending invalidate before enabling (RM0481 8.4.5),
and
bounds every `BUSYF` wait. `stm32_enable_icache()` returns `OK` or
`-ETIMEDOUT`; on a timeout the ICACHE is left disabled. When
`CONFIG_STM32_ICACHE` is not set, `__start` disables an ICACHE left
enabled
by a bootloader.
5. **Disable the ICACHE while program memory is modified.** An enabled ICACHE
does not manage write transactions and flags cacheable writes as errors
(`ICACHE_SR.ERRF`); RM0481 8.4.5 recommends modifying the memory with the
ICACHE disabled. On master, stale ICACHE lines also make
`up_progmem_write()` and `up_progmem_eraseblock()` fail their read-back
and erase checks with `-EIO` when the block was read through the ICACHE
before (Testing 2). `up_progmem_eraseblock()` and
`up_progmem_write()` now disable the ICACHE, which invalidates it, and
restore it afterwards, logging if it cannot be re-enabled.
6. **Documentation.** The STM32H5 peripheral table notes that the ICACHE
uses the MPU.
The steps are bundled because the ICACHE is only safe to enable with all of
them in place; each commit builds and runs on its own, and the series is not
a breaking change (see Impact).
## Impact
- **New feature:** no. Existing feature changed: yes, the STM32H5 ICACHE
driver (below).
- **Config/build:** `STM32_ICACHE` now selects `ARM_MPU`. The option is only
available on STM32H5 (`STM32_HAVE_ICACHE`), so other families are
unaffected. The ICACHE uses one additional MPU region, allocated with
`mpu_configure_region()`. No in-tree defconfig sets
`CONFIG_STM32_ICACHE=y`.
- **Boards:** `nucleo-h563zi` loses `stm32_mpu.c` and
`stm32_mpu_configure_otp()`; the arch region replaces it and also covers
EDATA. The removed region was read-only (`MPU_RBAR_AP_RORO`); the new one
is read-write, so the OTP and EDATA write paths keep working.
- **API:** `stm32_enable_icache()` returns `int` instead of `void`. Existing
callers ignore the result and keep working. `stm32_disable_icache()` stays
`void`: it clears `EN` before it waits, so the ICACHE is off even if the
invalidate times out.
- **Behavior:**
- UID, OTP and EDATA reads no longer touch the ICACHE; they bypass it
through the MPU region.
- `stm32_disable_icache()` now waits for the invalidate that `EN=0`
starts, and `stm32_enable_icache()` waits for a pending one. Both waits
are bounded (`STM32_ICACHE_BUSY_TIMEOUT`, a margin because RM0481 gives
no invalidate duration). After this series their only in-tree callers
are `__start` and the flash erase/write paths.
- `__start` disables an ICACHE left enabled when `CONFIG_STM32_ICACHE` is
not set.
- Flash erase/write briefly runs with the ICACHE disabled.
- **Which writers are covered.** Only `up_progmem_write()` and
`up_progmem_eraseblock()` write the main flash array, through stores to the
cacheable `0x08xxxxxx` range, so only they need the ICACHE suspended. The
OTP writers (`stm32_otp_write()`, `stm32_otp_word_write16/32()`) and the
EDATA writers (`stm32_flash_edata_write()`, `edata_erase()`) store into
`0x08fff000-0x09017fff`, which the new region makes non-cacheable, so they
bypass the ICACHE and need no handling of their own. The option-byte
functions (`stm32_flash_optmodify()`, `stm32_flash_swapbanks()`,
`stm32_flash_edata_configure()`) only use control registers.
- **MPU/HardFault:** `stm32_mpuinitialize()` enables the MPU with
`HFNMIENA=0`, so the region does not apply inside the HardFault/NMI
handlers. Before this series `stm32_get_uniqueid()` disabled the ICACHE
itself, so a UID read from a fault handler worked; now it would fault.
Nothing in the tree reads the UID from a fault handler.
- **Docs:** the STM32H5 platform page notes that the ICACHE uses the MPU
(separate commit).
- **Security:** no.
- **Compatibility:** not a breaking change. Callers that ignore the new
`stm32_enable_icache()` return value build and behave as before. With
`CONFIG_STM32_ICACHE=y` the MPU is now enabled and one region is used.
Out-of-tree boards that map the OTP area themselves (as `nucleo-h563zi`
did) must drop that region, or OTP/UID reads take a MemManage fault.
## Testing
**Host:** Ubuntu 22.04.5, Intel Core i7-12800H, arm-none-eabi-gcc 10.2.1
(GNU Arm Embedded 10-2020-q4).
**Builds:** `nucleo-h563zi:nsh` with `CONFIG_STM32_PROGMEM`,
`CONFIG_STM32H5_OTP_WORD` and `CONFIG_STM32_EDATA` added, since no in-tree
configuration enables the ICACHE or these options: every commit with
`CONFIG_STM32_ICACHE=y`, and the tip with the ICACHE off and with
`CONFIG_STM32_ICACHE_DIRECT`. No warnings in the touched files.
**Hardware:** a custom STM32H563 board (2 MB flash), flashed over SWD. The
board is out of tree, so it cannot be built from this repository; the
in-tree `nucleo-h563zi` builds above cover the same code.
**Test setup.** Images were built from master (baseline) and from the tip of
this series, on top of the board's `nsh` configuration with:
- `CONFIG_STM32_ICACHE` set or not, as each test says;
- `CONFIG_STM32_PROGMEM=y` and `CONFIG_STM32H5_OTP_WORD=y`, needed by the
self-tests;
- the NSH `mh` command enabled (`CONFIG_NSH_DISABLE_MH` not set).
Tests 1 and 4 use only NSH and a debugger. Tests 2, 3 and 5 use two small
self-tests (not part of this PR) added to the board's `stm32_bringup()`, so
they run before NSH starts and print to the console through `syslog`:
- **progmem self-test** (test 2): erases the last 8 KB flash block (block
255 on a 2 MB part), reads its first word, which also loads that line into
the ICACHE, programs 16 bytes with `up_progmem_write()`, reads the first
word again, erases the block again and reads it once more. Every step
prints the result and the expected value, then `PASS` or `FAIL`.
`up_progmem_write()` reads the data back itself after programming; that
check is what returns `-EIO` on master.
- **UID/OTP self-test** (test 3): calls `stm32_get_uniqueid()`, reads the
first 32 OTP words with `stm32_otp_word_read16()` (ECC-safe), then reads
the same 64 bytes with `stm32_otp_read()` (plain 16-bit loads) and
compares the two. The ECC-safe read runs first because a blank OTP word
raises the flash ECC NMI on a plain load, ICACHE or not.
- **Overlap variant** (test 5): the UID/OTP self-test, with an
`mpu_configure_region()` call identical to the removed `nucleo-h563zi`
one added before the reads.
Registers were read with gdb over SWD after each run: `CFSR` `0xe000ed28`,
`HFSR` `0xe000ed2c`, `MMFAR` `0xe000ed34`, `BFAR` `0xe000ed38`, `MPU_CTRL`
`0xe000ed94`, `ICACHE_CR` `0x40030400`, `ICACHE_SR` `0x40030404`.
*1. ICACHE enabled by `CONFIG_STM32_ICACHE=y` — read of the read-only area
from NSH (`mh 08fff800 8`).*
| | baseline (master) | this PR |
|---|---|---|
| console | `Assertion failed panic`, `PC` at the `ldrh` in `cmd_mh`, `R3 =
08fff800` | four half-words printed |
| `CFSR` | `0x00008200` (PRECISERR, BFARVALID) | `0x00000000` |
| `HFSR` | `0x40000000` (FORCED) | `0x00000000` |
| `BFAR` | `0x08fff800` | not valid |
| `ICACHE_CR` | `0x5` (enabled) | `0x5` (enabled) |
| `MPU_CTRL` | `0x0` | `0x5` |
Baseline, same fault signature as #19978:
```
nsh> mh 08fff800 8
dump_assert_info: Assertion failed panic: at file: :0 task: <noname> ...
up_dump_register: R0: 00000000 R1: 00000010 R2: 00000000 R3: 08fff800
```
This PR:
```
nsh> mh 08fff800 8
0x8fff800 = 0x****
0x8fff802 = 0x****
0x8fff804 = 0x****
0x8fff806 = 0x****
```
*2. Erase and write program memory with the ICACHE enabled (progmem
self-test).* The baseline result depends on the build: in one image the
block read back correctly and only `ERRF` was set; in another (master
`7e5cf3d5d0`, repeated twice) `up_progmem_write()` failed its read-back
because it hit the erased data cached by the read just before.
| | baseline, image A | baseline, image B | this PR |
|---|---|---|---|
| `up_progmem_write()` | `ret=16` | `ret=-5` (-EIO), `p[0]` still `ffffffff`
| `ret=16`, `p[0]=11111111` |
| self-test | PASS | FAIL | PASS |
| `ICACHE_SR` after the self-test | `0x00000006` (BSYENDF, **ERRF**) |
`0x00000006` | `0x00000000` |
Baseline image B:
```
SELFTEST: ICACHE enabled=1
SELFTEST: erase #1 ret=8192 (want 8192)
SELFTEST: after erase p[0]=ffffffff (want ffffffff)
SELFTEST: write ret=-5 (want 16)
SELFTEST: after write p[0]=ffffffff (want 11111111)
```
To confirm the cause, image B was run under gdb with breakpoints on every
`-EIO` in `up_progmem_write()`. It stopped at the read-back mismatch
(`written = -EIO` after the `*fp++ != *rp++` comparison), with `FLASH_NSSR`
and `FLASH_ECCDETR` both `0`, so programming itself succeeded. A debugger
read of the block, which does not go through the ICACHE, showed the
programmed data (`11111111 22222222 33333333 44444444`). After setting
`ICACHE_CR.CACHEINV` from gdb and continuing, the self-test's next CPU read
returned `11111111` instead of `ffffffff`. The erase check is affected the
same way: in that run, the following `up_progmem_eraseblock()` returned
`-EIO` because its erased-range check read the now cached `11111111`:
```
SELFTEST: write ret=-5 (want 16)
SELFTEST: after write p[0]=11111111 (want 11111111)
SELFTEST: erase #2 ret=-5 (want 8192)
SELFTEST: after erase p[0]=11111111 (want ffffffff)
```
*3. UID and OTP reads through the driver APIs with the ICACHE enabled
(UID/OTP self-test).*
| | baseline (master) | this PR |
|---|---|---|
| `stm32_get_uniqueid()` | OK (ICACHE disabled around it) | OK (no ICACHE
handling) |
| `stm32_otp_word_read16()` x32 | OK (ICACHE disabled around it) | OK (no
ICACHE handling) |
| `stm32_otp_read()` | panic, `CFSR 0x8200`, `BFAR 0x08fff000` | `ret=0`,
same data as `word_read16` |
| `ICACHE_SR` afterwards | — | `0x00000000` |
Baseline:
```
OTPTEST: ICACHE_CR=00000005
OTPTEST: stm32_get_uniqueid done, nonzero=1
OTPTEST: word_read16 x32 err=0
dump_assert_info: Assertion failed panic: at file: :0 task: <noname> ...
up_dump_register: R0: 00000000 R1: 7ffffffe R2: 200041d8 R3: 08fff000
```
This PR:
```
OTPTEST: ICACHE_CR=00000005
OTPTEST: stm32_get_uniqueid done, nonzero=1
OTPTEST: word_read16 x32 err=0
OTPTEST: stm32_otp_read ret=0 mismatch=0
OTPTEST: ICACHE_SR=00000000
OTPTEST: PASS
```
*4. ICACHE left enabled by a bootloader, `CONFIG_STM32_ICACHE` not set.* The
ICACHE is enabled through the debugger after reset and before the image
starts, standing in for a bootloader hand-over, once direct-mapped
(`ICACHE_CR = 0x1`) and once n-way (`0x5`). With OpenOCD and gdb:
```
(gdb) monitor reset halt
(gdb) set {unsigned int}0x40030400 = 1
(gdb) monitor resume
```
Then the self-tests run at boot and `mh 08fff800 8` is entered in NSH.
| | baseline (master) | this PR |
|---|---|---|
| progmem self-test `up_progmem_write()` | `ret=-5` (-EIO), `p[0]` still
`ffffffff` | `ret=16`, PASS |
| UID/OTP self-test | not run | PASS |
| `mh 08fff800 8` | panic, `CFSR 0x8200`, `BFAR 0x08fff800` | four
half-words printed |
| `ICACHE_CR` after boot | `0x1` (still enabled) | `0x0` (`0x4` for the
n-way poke): `EN` cleared by `__start` |
*5. Overlapping MPU regions.* To check why the `nucleo-h563zi` region has to
go, the UID/OTP self-test was run on this PR after first adding a region
identical to the removed one (`0x08fff000`, 4 KB, `XN | SH_NO | AP_RORO`,
non-cacheable). The first UID
read takes a MemManage fault, even though both regions allow reads:
```
OTPTEST: adding overlapping OTP region (nucleo)
OTPTEST: ICACHE_CR=00000005
dump_assert_info: Assertion failed panic: at file: :0 task: <noname> ...
up_dump_register: R0: 20004228 R1: 00000008 R2: 00000000 R3: 08fff800
```
`CFSR 0x00000082` (DACCVIOL, MMARVALID), `MMFAR 0x08fff800`, `HFSR 0`.
**Revisions:** "this PR" is the tip of this series. The baselines are
master `7e5cf3d5d0` (table 2 image B, table 3) and `acc7d9bd3c` (tables 1
and 4, table 2 image A).
**Not tested:** other STM32H5 variants; `nucleo-h563zi` on hardware (built
only; the overlap behavior is shown in table 5); EDATA reads and writes
(they use the same helper and the same MPU region as the OTP); OTP writes;
the `-ETIMEDOUT` paths (no way to make `BUSYF` stick); the invalidate
latency; the ICACHE interrupt options (`STM32_ICACHE_INV_INT`,
`STM32_ICACHE_ERR_INT`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
