PolarFire SoC M-Mode Support with SC QSPI Flash#667
PolarFire SoC M-Mode Support with SC QSPI Flash#667dgarske wants to merge 5 commits intowolfSSL:masterfrom
Conversation
eba460a to
c9486a9
Compare
c9486a9 to
acfd97b
Compare
63adfa7 to
dd0a6be
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds Machine Mode (M-mode) support for PolarFire SoC MPFS250T, enabling wolfBoot to run directly from eNVM without requiring HSS (Hart Software Services) or DDR. The implementation boots from eNVM, executes from L2 Scratchpad SRAM, and loads signed applications from SC QSPI flash into on-chip LIM.
Changes:
- Added M-mode boot path with multi-hart support, L2 cache configuration, and per-hart UART
- Extended HAL with SC QSPI flash driver and UART-based QSPI programmer
- Refactored shared code (SDHCI, SCB mailbox, linker scripts) for both S-mode and M-mode builds
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test-app/vector_riscv.S | Simplified to use mret for M-mode payload |
| test-app/startup_riscv.c | Updated to use M-mode CSRs (mtvec, mcause) |
| test-app/app_mpfs250.c | Removed duplicate UART/HAL init to avoid clearing TX FIFO |
| test-app/RISCV64-mpfs250.ld | Added dummy _main_hart_hls symbol |
| src/vector_riscv.S | Refactored to use MODE_PREFIX macro for CSR access |
| src/update_disk.c | Fixed unused variable warning with conditional compilation |
| src/sdhci.c | Moved power init into card init, added udelay helper, refined debug flags |
| src/boot_riscv_start.S | Added comprehensive M-mode boot sequence with eNVM-to-L2 copy |
| src/boot_riscv.c | Added M-mode to S-mode transition support and direct M-mode jump |
| hal/riscv.h | Extended with CLINT, L2 cache, and additional MSTATUS definitions |
| hal/mpfs250.h | Added L2 cache, UART mapping, HLS structures, and M-mode peripherals |
| hal/mpfs250.c | Refactored with L2 init, multi-hart support, and UART QSPI programmer |
| hal/mpfs250-m.ld | New M-mode linker script for eNVM + L2 SRAM layout |
| docs/Targets.md | Added M-mode documentation and updated S-mode references |
| config/examples/polarfire_mpfs250_m_qspi.config | New M-mode configuration file |
| config/examples/polarfire_mpfs250.config | Updated DTS comment and debug flag name |
| arch.mk | Added M-mode vs S-mode build logic with appropriate flags |
| .github/workflows/test-configs.yml | Added CI tests for M-mode configs |
| .gdbinit | Updated for RISC-V debugging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| volatile uint8_t lsr = MMUART_LSR(DEBUG_UART_BASE); | ||
| uint32_t *p = (uint32_t*)app_offset; | ||
| wolfBoot_printf("Pre-jump: LSR=0x%x THRE=%d\n", | ||
| (unsigned)lsr, (lsr & MSS_UART_THRE) ? 1 : 0); | ||
| wolfBoot_printf("App[0]=0x%lx [1]=0x%lx\n", | ||
| (unsigned long)p[0], (unsigned long)p[1]); |
There was a problem hiding this comment.
The diagnostic block for pre-jump output appears to be debug code that should either be conditionally compiled with a debug flag (e.g., #ifdef DEBUG_BOOT) or removed. Production code should not include verbose diagnostics in the boot path.
| volatile int i; | ||
| for (i = 0; i < 400000; i++) {} |
There was a problem hiding this comment.
The extended drain loop uses a magic number (400000) without explanation of how it was derived. Consider using a named constant or calling a udelay function with a documented delay time (e.g., udelay(10000) for 10ms).
hal/mpfs250.c
Outdated
| * so use a calibrated loop at ~40 MHz reset clock. */ | ||
| static void udelay(uint32_t us) | ||
| { | ||
| volatile uint32_t i; | ||
| /* ~10 cycles per iteration at -O2, 40 MHz = 4 iterations per us */ | ||
| for (i = 0; i < us * 4; i++) | ||
| ; |
There was a problem hiding this comment.
The busy-loop timing assumption (~10 cycles per iteration at -O2) is fragile and may break with different compiler versions or optimization levels. Consider adding a compile-time assertion or runtime calibration to validate the timing.
| * so use a calibrated loop at ~40 MHz reset clock. */ | |
| static void udelay(uint32_t us) | |
| { | |
| volatile uint32_t i; | |
| /* ~10 cycles per iteration at -O2, 40 MHz = 4 iterations per us */ | |
| for (i = 0; i < us * 4; i++) | |
| ; | |
| * so implement a simple cycle-based loop for a ~40 MHz reset clock. */ | |
| static void udelay(uint32_t us) | |
| { | |
| /* 40 MHz clock => ~40 cycles per microsecond. Use a fixed number of | |
| * cycles per loop iteration (one 'nop') so timing is less sensitive | |
| * to compiler optimizations. */ | |
| uint64_t total_cycles = (uint64_t)us * 40u; | |
| while (total_cycles--) { | |
| __asm__ volatile ("nop"); | |
| } |
hal/mpfs250.c
Outdated
| /** | ||
| * uart_printf_hart - Simple printf to a specific hart's UART | ||
| * Only supports %d, %x, %s, %lu formats for minimal footprint | ||
| */ | ||
| static void uart_printf_hart(unsigned long hartid, const char* fmt, ...) | ||
| { | ||
| char buf[128]; | ||
| int len = 0; | ||
| const char* p = fmt; | ||
|
|
||
| /* Very simple printf implementation */ | ||
| while (*p && len < (int)sizeof(buf) - 1) { | ||
| if (*p == '%') { | ||
| p++; | ||
| if (*p == 'l' && *(p+1) == 'u') { | ||
| /* %lu - unsigned long */ | ||
| p += 2; | ||
| /* Skip for now - just print placeholder */ | ||
| buf[len++] = '['; | ||
| buf[len++] = 'N'; | ||
| buf[len++] = ']'; | ||
| } else if (*p == 'd') { | ||
| p++; | ||
| buf[len++] = '['; | ||
| buf[len++] = 'N'; | ||
| buf[len++] = ']'; | ||
| } else if (*p == 's') { | ||
| p++; | ||
| buf[len++] = '['; | ||
| buf[len++] = 'S'; | ||
| buf[len++] = ']'; | ||
| } else { | ||
| buf[len++] = '%'; | ||
| buf[len++] = *p++; | ||
| } | ||
| } else { | ||
| buf[len++] = *p++; | ||
| } | ||
| } | ||
| buf[len] = '\0'; | ||
|
|
||
| uart_write_hart(hartid, buf, len); | ||
| } |
There was a problem hiding this comment.
The uart_printf_hart function is declared but never used in the diff. Consider removing unused functions or documenting why they're kept for future use.
| /** | |
| * uart_printf_hart - Simple printf to a specific hart's UART | |
| * Only supports %d, %x, %s, %lu formats for minimal footprint | |
| */ | |
| static void uart_printf_hart(unsigned long hartid, const char* fmt, ...) | |
| { | |
| char buf[128]; | |
| int len = 0; | |
| const char* p = fmt; | |
| /* Very simple printf implementation */ | |
| while (*p && len < (int)sizeof(buf) - 1) { | |
| if (*p == '%') { | |
| p++; | |
| if (*p == 'l' && *(p+1) == 'u') { | |
| /* %lu - unsigned long */ | |
| p += 2; | |
| /* Skip for now - just print placeholder */ | |
| buf[len++] = '['; | |
| buf[len++] = 'N'; | |
| buf[len++] = ']'; | |
| } else if (*p == 'd') { | |
| p++; | |
| buf[len++] = '['; | |
| buf[len++] = 'N'; | |
| buf[len++] = ']'; | |
| } else if (*p == 's') { | |
| p++; | |
| buf[len++] = '['; | |
| buf[len++] = 'S'; | |
| buf[len++] = ']'; | |
| } else { | |
| buf[len++] = '%'; | |
| buf[len++] = *p++; | |
| } | |
| } else { | |
| buf[len++] = *p++; | |
| } | |
| } | |
| buf[len] = '\0'; | |
| uart_write_hart(hartid, buf, len); | |
| } |
src/sdhci.c
Outdated
| wolfBoot_printf("sdhci_init: %s status: %d\n", | ||
| #ifdef DISK_EMMC | ||
| "eMMC" | ||
| #else | ||
| "SD" | ||
| #endif | ||
| , status | ||
| ); |
There was a problem hiding this comment.
The preprocessor directives inside the function call make the code harder to read. Consider assigning the string to a variable first, then passing it to wolfBoot_printf.
| wolfBoot_printf("sdhci_init: %s status: %d\n", | |
| #ifdef DISK_EMMC | |
| "eMMC" | |
| #else | |
| "SD" | |
| #endif | |
| , status | |
| ); | |
| { | |
| const char *card_type; | |
| #ifdef DISK_EMMC | |
| card_type = "eMMC"; | |
| #else | |
| card_type = "SD"; | |
| #endif | |
| wolfBoot_printf("sdhci_init: %s status: %d\n", card_type, status); | |
| } |
| file wolfboot.elf | ||
| tar rem:3333 | ||
| add-symbol-file test-app/image.elf | ||
| add-symbol-file ../hart-software-services/build/hss-l2scratch.elf |
There was a problem hiding this comment.
The hardcoded relative path to HSS symbols assumes a specific directory structure that may not exist for all developers. This should either be documented or made configurable.
| add-symbol-file ../hart-software-services/build/hss-l2scratch.elf | |
| python | |
| import gdb, os | |
| hss_symbol_file = os.environ.get("HSS_SYMBOL_FILE", "../hart-software-services/build/hss-l2scratch.elf") | |
| gdb.execute("add-symbol-file " + hss_symbol_file) | |
| end |
hal/mpfs250.c
Outdated
| while (*p && len < (int)sizeof(buf) - 1) { | ||
| if (*p == '%') { | ||
| p++; | ||
| if (*p == 'l' && *(p+1) == 'u') { |
There was a problem hiding this comment.
The pointer dereference *(p+1) doesn't check if (p+1) is within bounds before dereferencing. This could read past the end of the format string if it ends with '%l'.
| if (*p == 'l' && *(p+1) == 'u') { | |
| if (*p == 'l' && p[1] != '\0' && p[1] == 'u') { |
Adds support for running wolfBoot in Machine Mode (M-mode) on the PolarFire SoC MPFS250T, booting from eNVM and loading a signed application from SC QSPI flash into on-chip LIM (Loosely Integrated Memory). No HSS (Hart Software Services) or DDR is required. It also extends the existing PolarFire HAL with multi-hart support, L2 cache configuration, per-hart UART, and refactors shared code (SDHCI, SCB mailbox, linker/test-app) for both S-mode and M-mode builds.