NevynUK commented on PR #20125:
URL: https://github.com/apache/nuttx/pull/20125#issuecomment-5814446015
Rebased onto current master and force-pushed. The branch is now 9 commits,
27 files, +2345/-390 — down from 34 files / +2964/-390 at submission.
## What changed
**Rebased onto master** (272 commits), replayed with no conflicts. An earlier
rebase had picked up #20126, which conflicted in `irq.h`: it claims frame
slot
32 for `REG_INT_THRESH_NDX` and moves `REG_INT_CTX_NDX` to 33, where
`REG_MCAUSE_NDX` had been hard-coded to 33. `REG_MCAUSE_NDX` is now derived
as
`(REG_INT_CTX_NDX + 1)` so it tracks upstream rather than racing it. Verified
in the disassembly rather than on paper — under CLIC + protected the frame is
`INT_THRESH`=128, `INT_CTX`=132, `MCAUSE`=136.
**Three debug features removed**, each to its own future PR if wanted:
| removed | why |
| --- | --- |
| `RISCV_FRAME_TRACE` | @xiaoxiang781216 — it changes common code and
warrants separate review |
| `ESPRESSIF_P4DBG` | bring-up scaffolding; see below |
| `ESPRESSIF_PMP_EARLY_SNAPSHOT` | @eren-terzioglu — see below |
Removing `P4DBG` takes `esp_idle.c`, `esp_irq.c`, `esp_timerisr.c` and
`esp32p4_bringup.c` out of the PR entirely — they return to upstream byte for
byte. That also clears 12 of the 17 nxstyle failures the Check job was
reporting, since checkpatch examines every file a commit range touches and
`esp_idle.c` was only in the range because P4DBG modified it. The remaining 5
are pre-existing violations in `esp_start.c`, which stays in the PR for real
protected-boot reasons.
## Responses to review
**@xiaoxiang781216 — no `CONFIG_ARCH_CHIP_ESP32P4` in common code.** Done.
Generic RISC-V code now gates on `CONFIG_ARCH_RV_HAVE_CLIC` from #20126, and
`ARCH_CHIP_ESP32P4` selects it. There is no chip name left in shared code.
Selecting that symbol alone was not enough. #20126 drives the threshold via
CSR
0x347, which pre-v3 ESP32-P4 does not implement — reading it raises an
illegal
instruction, and the faulting read is inside `exception_common`, so the trap
is
unrecoverable. It boot-looped the shipping flat `nsh` as well as `knsh`. The
threshold is now routed to the memory-mapped register at 0x20800008 behind
`ARCH_RV_CLIC_INTTHRESH_MMIO`, defined in the chip's Kconfig so shared code
keys on a capability rather than a chip. Regression-checked on an ESP32-C6
(non-CLIC): `nuttx_enter_critical` compiles byte-identically to upstream.
**@eren-terzioglu — `esp_pmp_early_snapshot` vs the esp common layer.** You
are
right that this duplicated existing code: `riscv_pmp.c` already has
`pmp_read_region_cfg()`, `pmp_read_addr()` and `pmp_read()`, while this read
the
sixteen `pmpaddr` CSRs inline. It was a bring-up diagnostic that answered one
question — what locks the PMP entries before NuttX gains control — and that
answer is now recorded in the `ESPRESSIF_KERNEL_OWNS_PMP` help text. Removed.
If a dump is wanted later, a generic `riscv_pmp_dump()` beside those
primitives,
mirroring ARM's `mpu_dump_region()`, is the right shape, in its own PR
against
the common layer.
**@tmedicci — PMP CSRs are hart-local / SMP.** Documented as single core
only.
`configure_mpu()` runs on CPU0 startup and the configuration has only been
exercised on one core.
**@tmedicci — `CONFIG_MMU_PAGE_SIZE` in the pass-1 linker script.** Fixed.
The
script deliberately skips `common.ld` because the HAL headers are not on the
include path for the pass-1 user link, and it already reproduces
`RESERVE_RTC_MEM` and `MSPI_WORKAROUND_SIZE` locally for that reason;
`CONFIG_MMU_PAGE_SIZE` was missed, so `IDROM_SEG_SIZE` evaluated as `0 <<
10`.
It is now defined locally under `#ifndef` with the 64 KiB value, and
`extern_ram_seg` reports 64 MB, matching the flat build. Say the word if you
would prefer the 1024-page figure stated explicitly instead.
## Still to do
These are acknowledged and not yet addressed:
- @tmedicci — validate the user image metadata in `load_header()` before
`configure_mmu()` consumes it
- @tmedicci — review the synthetic frame paths in
`supervisor/riscv_syscall.S`
against the restored `mcause` slot
## Testing
M5Stack Tab5, ESP32-P4 rev v1.0. Clean builds from scratch, zero compiler
warnings, on the exact tree pushed here.
| config | ostest | ramtest |
| --- | --- | --- |
| `esp32p4-tab5:knsh` | status 0, 154 sections, 0 errors | 18/18 |
| `esp32p4-tab5:nsh` | status 0, 162 sections, 0 errors | 18/18 |
`ostest` does not prove the PMP grant on its own, so the PSRAM window is also
exercised from user mode with `ramtest` at its base, middle and top
(0x48000000, 0x49000000, 0x49ff0000). `TESTING_OSTEST` and `TESTING_RAMTEST`
are not in the flat `nsh` defconfig and were enabled by hand for that run
only.
--
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]
