Conversation
633e7f8 to
5e963dc
Compare
danielinux
left a comment
There was a problem hiding this comment.
Looks good, some minor visibility issues
5e963dc to
2f8ce14
Compare
There was a problem hiding this comment.
Pull request overview
Adds QSPI NOR flash support for PolarFire SoC MPFS250 and updates the RISC-V/PolarFire boot flow to better support RAM-loaded (NO_XIP) and ELF-based images.
Changes:
- Introduces MPFS250 QSPI NOR driver + EXT_FLASH integration (including SC QSPI vs MSS QSPI selection).
- Updates RISC-V trap/vector handling to switch between S-mode and M-mode via
WOLFBOOT_RISCV_MMODE. - Adjusts build/link flow for RISC-V (update loader selection based on disk config; sign ELF when
ELF=1) and documents PolarFire QSPI usage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
hal/mpfs250.c |
Implements QSPI init + transfer/read/write/erase and hooks it into hal_init() when EXT_FLASH is enabled. |
hal/mpfs250.h |
Adds SCB mailbox helpers/constants and CoreQSPI v2 register/flash command definitions under EXT_FLASH. |
hal/riscv.h |
Centralizes privilege-mode selection helpers and adds an I-cache sync helper for RISC-V. |
src/elf.c |
Uses memmove() for safer in-place ELF segment loading and syncs I-cache on RISC-V. |
src/vector_riscv.S |
Switches S-mode/M-mode selection logic to WOLFBOOT_RISCV_MMODE. |
src/boot_riscv_start.S |
Uses MODE_PREFIX from hal/riscv.h and updates mode checks to WOLFBOOT_RISCV_MMODE. |
src/boot_riscv.c |
Updates satp/MMU teardown conditional for S-mode vs M-mode. |
test-app/vector_riscv.S |
Adds RV64 trap entry/exit macros and uses sret vs mret based on WOLFBOOT_RISCV_MMODE. |
test-app/startup_riscv.c |
Sets stvec vs mtvec and reads scause vs mcause based on privilege mode. |
test-app/RISCV64-mpfs250.ld |
Adjusts placement for RAM boot / NO_XIP style execution and changes where .data/.bss live. |
arch.mk |
Chooses update loader based on disk enablement and switches signing target to .elf when ELF=1. |
docs/Targets.md |
Documents PolarFire QSPI usage and build-time selection between MSS QSPI and SC QSPI. |
config/examples/polarfire_mpfs250_qspi.config |
New example configuration for MPFS250 QSPI + NO_XIP + ELF flows. |
config/examples/polarfire_mpfs250.config |
Clarifies EXT_FLASH meaning for PolarFire MPFS250. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Verify erase (should be all 0xFF) */ | ||
| memset(pageData, 0, sizeof(pageData)); | ||
| ext_flash_read(TEST_EXT_ADDRESS, pageData, sizeof(pageData)); |
There was a problem hiding this comment.
The return value of ext_flash_read is not checked here, but is checked on line 850. For consistency and robustness, the return value should be checked and handled in case the read operation fails during erase verification.
| ext_flash_read(TEST_EXT_ADDRESS, pageData, sizeof(pageData)); | |
| ret = ext_flash_read(TEST_EXT_ADDRESS, pageData, sizeof(pageData)); | |
| if (ret < 0) { | |
| wolfBoot_printf("Erase verify read failed: Ret %d\n", ret); | |
| return ret; | |
| } |
| remaining -= chunk_len; | ||
| } | ||
|
|
||
| return len; |
There was a problem hiding this comment.
The function qspi_flash_read returns len (uint32_t) on success (line 652), but its return type is int. If len exceeds INT_MAX (2147483647), the cast to int will produce a negative value that could be misinterpreted as an error code by callers. While this is unlikely in practice for flash operations, consider either: (1) changing the return type to match the convention (return int with length or negative error), or (2) adding a check to ensure len doesn't exceed INT_MAX before returning it.
| - Both controllers share the same CoreQSPI v2 register interface. | ||
| The only difference is that SC QSPI does not need MSS clock/reset setup. | ||
|
|
||
| Example single-shot build: `cp config/examples/polarfire_mpfs250_qspi.config .config && make clean && make wolfboot.bin && hss-payload-generator -vvv -c ./hal/mpfs.yaml wolfboot.bin && make test-app/image.elf && ./tools/keytools/sign --ecc384 --sha384 test-app/image.elf wolfboot_signing_private_key.der 1` |
There was a problem hiding this comment.
The PR description mentions a new tooling script tools/scripts/mpfs_program.sh, but this file is not included in the PR changes and is not referenced in the documentation. If this script is intended to be part of this PR, it should be added. Otherwise, update the PR description to remove this reference.
| @@ -62,6 +113,8 @@ | |||
| mret | |||
There was a problem hiding this comment.
The 32-bit trap_exit macro has a hardcoded mret instruction but does not check the WOLFBOOT_RISCV_MMODE define. This is inconsistent with the 64-bit trap_exit macro (lines 65-69) which conditionally uses sret or mret based on privilege mode. If a 32-bit RISC-V system runs in supervisor mode, this will cause an illegal instruction exception. Add the same conditional logic as the 64-bit version.
Summary
Key Changes
config/examples/polarfire_mpfs250_qspi.confighal/mpfs250.c(MSS QSPI + SC SPI paths)arch.mkfor update loader selection + ELF signingdocs/Targets.mdtools/scripts/mpfs_program.shTesting