Uh oh!
There was an error while loading. Please reload this page.
Added Microchip PolarFire SoC target, BSP framework, and CI/CD pipeline - #48
Added Microchip PolarFire SoC target, BSP framework, and CI/CD pipeline#48AmmarOkla12772 wants to merge 3 commits into
Conversation
fdesbiens
commented
Aug 24, 2026
Hi @akifejaz. I will perform the main review, but please have a look. This is a rather popular board in RISC-V circles. |
fdesbiens
left a comment
There was a problem hiding this comment.
Nice work getting a 64-bit RISC-V target up. The multi-hart bring-up, the CLINT tick math, and the FPU/ABI configuration are all correct, which is genuinely not easy on RV64 — those are the parts I expected to find problems in and didn't.
That said, I'm requesting changes. There are two separate categories below, and I'd like the first sorted out before I go through the second, because the branch's git state currently makes the rest hard to evaluate.
Part 1 — Git state: please fix this first
1.1 The branch silently reverts merged work on dev
The branch has dev as an ancestor, but the merge resolution kept the older side for several files. I simulated the merge with git merge-tree: merging this PR into dev today produces zero conflicts and silently reverts all of the following. Nothing would flag it at merge time, which is why I want it fixed at the source.
| File | What is lost |
|---|---|
SECURITY.md | The entire rewrite from #43 — coordinated-disclosure instructions, the "do not report through public issues" warning, the private advisory link, the supported-versions table, and the release-cadence section |
README.md | The sentence noting that iot-devkit will eventually be archived |
.gitignore | *.gdb, *.log, and the two STM32F767ZI-Nucleo vendor-SDK entries added by #39. Without those, a full CubeF7 SDK downloaded by fetch_sdk.sh shows up as untracked and is easy to commit by accident |
.gitmodules | branch = dev on all four libs/* submodules |
1.2 Three submodules are rolled backwards
Verified against upstream:
| Submodule | Change |
|---|---|
libs/filex | 7 commits behind |
libs/netxduo | 16 commits behind |
libs/usbx | 15 commits behind |
(libs/threadx is untouched.)
Combined with the branch = dev removal in .gitmodules, this also breaks scripts/update-libs.sh: all four upstream repos have master as their default branch, so after this PR git submodule update --remote would silently start tracking master instead of dev. The script would still exit 0 — it would just quietly do the wrong thing.
1.3 The NUCLEO_F401RE target is duplicated, not moved
326 files are added and zero are deleted. STMicroelectronics/NUCLEO_F401RE/ (281 files) stays in the tree andtargets/STMicroelectronics/NUCLEO_F401RE/ (273 files) is added. Roughly 254 of those are byte-identical, including about 124,000 lines of vendored STM32CubeF4. Git's rename detection finds nothing even at a 40% threshold, so this lands as a copy, leaving two divergent copies of one board with two build systems — only one of which CI builds.
Here's the thing that makes this worth fixing rather than tolerating: the two trees actually differ by only 19 files, 128 insertions, 1039 deletions. Done as a real git mv, the CubeF4 vendor tree shows up as renames and the reviewable diff collapses from ~124,000 lines to about 1,200.
1.4 Please split this into two PRs
The relocation and the new target are independent pieces of work, and bundling them is what makes this PR hard to review. Please separate them:
PR A — Shared BSP framework + PolarFire SoC target (the new work)
bsp/include/bsp/— the abstract interfacecmake/utilities.cmake— the one genuinely target-agnostic helpertemplates/target/docs/architecture.mdtargets/Microchip/POLARFIRE_ICICLE_RENODE/.github/workflows/ci.ymlwith the RISC-V build and Renode jobs only
PR B — Relocate NUCLEO_F401RE into targets/ and adopt the BSP framework
git mv STMicroelectronics/NUCLEO_F401RE targets/STMicroelectronics/NUCLEO_F401RE, then apply the BSP rework as edits on top so the vendor tree stays renamesapps/threadx_demo/main.c— only the F401RE consumes it; PolarFire has its ownapp/main.ccmake/FindCMSIS.cmake,cmake/FindSTM32HAL.cmake,cmake/arm-gcc-cortex-m4.cmake,cmake/arm-gcc-cortex-toolchain.cmake— all four are ARM/STM32-specific- the ARM build job added to CI
Put PR A up first — it's the substantive one. PR B then becomes a small, mechanical follow-up I can approve quickly.
1.5 Restoring the unrelated files
For whichever branch you keep working on:
git checkout origin/dev -- SECURITY.md README.md .gitignore .gitmodules libs/
git commit -m "Restored dev state for files unrelated to this PR"Part 2 — Correctness
2.1 BLOCKER — PLIC registers are addressed at double the base
targets/Microchip/POLARFIRE_ICICLE_RENODE/lib/bsp/include/plic.h:17-19 adds PLIC_BASE (0x0C000000) to offsets that are already absolute:
| Macro | Computes | Should be |
|---|---|---|
PLIC_HART1_M_THRESHOLD_REG | 0x18202000 | 0x0C202000 |
PLIC_HART1_M_CLAIM_REG | 0x18202004 | 0x0C202004 |
enable reg (plic.c:22) | 0x18002108 | 0x0C002108 |
The target README documents 0x0C202000, and plic.c:18 writes the priority array as PLIC_BASE + irq * 4, which is correct — so the doubling is inconsistent within the same file.
Effect: the threshold is never set, IRQ 91 is never enabled in Hart 1's M-mode enable bitmap, and plic_claim() reads unmapped space. In Renode these are no-ops against a non-existent peripheral; on real silicon the load from 0x18202004 is an access fault. MMUART1 external interrupts can never be delivered even though MIE_MEIE is set in tx_initialize_low_level.S:70.
Worth understanding why this passed your testing, because the reason is structural rather than careless: plic_init() is called from board.c:20, but nothing in the demo ever depends on a PLIC-delivered interrupt. uart_putc polls LSR_THRE in a busy loop, and the system tick comes from the CLINT machine timer, not the PLIC. So the whole PLIC path is initialised and then never exercised — the demo behaves identically whether these three registers are right or wrong.
That's the useful lesson here: initialising a peripheral proves nothing about whether the initialisation is correct. Either drive something through the PLIC (interrupt-driven UART RX would be the natural choice) or drop it from this PR until something needs it. Dead-but-plausible init code is worse than no init code, because it reads as tested.
2.2 BLOCKER — heap and stack occupy the same 16 KB
app/common/linker/linker.ld:50-53:
. = ALIGN(16);
__stack_top = . + 0x4000; /* stack grows DOWN from here */
__end = .; /* heap grows UP from here */
_end = .;The location counter never advances past the stack, so __end/_end land at the stack's bottom. Two consumers are affected:
_sbrk(app/common/startup/newlib_stubs.c:24-30) starts the heap at&__end— inside the stack. The firstmalloc(newlib's stdio buffer) returns memory the boot stack is actively using. Its only guard isBSP_RAM_END(0xC0000000, the far end of 1 GiB), so nothing catches it._tx_initialize_low_levelstores_endinto_tx_initialize_unused_memory, so anytx_application_defineusingfirst_unused_memory— asapps/threadx_demo/main.cdoes, and astemplates/teaches new contributors to do — carves ThreadX pools out of the live stack.
Suggested fix:
. = ALIGN(16);
. = . + 0x4000;
__stack_top = .;
__end = .;
_end = .;2.3 .bss zero loop can overrun
linker.ld applies ALIGN(16) to the section start, but __bss_end = . follows *(COMMON) with no alignment. entry.S:32 stores 8 bytes at a time while t0 < t1, so a .bss size that isn't a multiple of 8 writes up to 7 bytes past __bss_end — into __end/stack. Please add . = ALIGN(8); before __bss_end.
2.4 gp is never initialised
linker.ld places .sdata/.sbss but never defines __global_pointer$, and _start never loads gp. This currently works only because GNU ld skips gp-relaxation when the symbol is absent. Please add the standard sequence:
.option push.option norelaxla gp, __global_pointer$.option pop2.5 Synchronous exceptions are silently ignored
lib/bsp/src/trap.c:22 only acts when mcause bit 63 is set. An illegal instruction or failed load falls through, mret returns to the faulting instruction, and it faults again — a silent, output-free infinite loop with no diagnostic. MIE_MSIE is enabled at tx_initialize_low_level.S:70 but cause 3 is likewise unhandled.
mepc and mtval are already taken as parameters and then discarded with (void), so the intent was there. At minimum, print mcause/mepc/mtval and halt.
2.6 CI cannot gate this PR, and has never run
.github/workflows/ci.yml:5-7 triggers on pull_request: branches: [main, master] and push: [main, master, 'feat/**']. dev is the integration branch and this PR targets it, so the pipeline does not run here and will not run on any future PR into dev. The only check currently on this PR is the Eclipse ECA.
That means every "builds successfully through the CI pipeline" claim in the description reflects local runs only. Please add dev to both trigger lists so I can see it go green on the PR.
2.7 The Renode test can hang CI indefinitely
scripts/test_renode.py:70: the while time.time() - start_time < timeout_seconds guard is only evaluated between reads, but proc.stdout.readline() blocks. If Renode stalls, fails to load the ELF, or the firmware sits in the unhandled-trap loop from 2.5, the 20-second timeout never fires and the job runs to GitHub's 6-hour limit instead of failing. Please use a reader thread with a queue, select, or a hard subprocess deadline.
Related: polarfire_demo.resc:12 uses showAnalyzer mmuart1 while CI runs --plain --disable-gui. Worth confirming UART output actually reaches stdout headlessly, since the whole assertion greps for it.
2.8 _sbrk is not thread-safe, and the two copies disagree
The PolarFire stub bounds-checks against BSP_RAM_END; the F401RE one (targets/STMicroelectronics/NUCLEO_F401RE/lib/bsp/src/newlib_stubs.c:24-37) does not — it advances heap unconditionally and never returns (void*)-1, despite BSP_RAM_END being defined right next to it. On 96 KB of SRAM an oversized malloc succeeds and returns a pointer past the end of RAM.
Neither implements __malloc_lock/__malloc_unlock, so heap_ptr is an unguarded static. Latent today, but this ships as the copy-me template.
2.9 Stack size versus full newlib on RV64
app/main.c:19 sets DEMO_STACK_SIZE to 2048, and reporter_thread_entry calls snprintf. The RISC-V build never passes --specs=nano.specs — that flag appears only in cmake/arm-gcc-cortex-toolchain.cmake:68 — so PolarFire links full newlib vfprintf, which routinely uses 1.5–2.5 KB of stack on RV64/lp64d. With no TX_ENABLE_STACK_CHECKING, an overflow corrupts the adjacent analyzer_stack/queue_area globals silently. Either add --specs=nano.specs or raise the reporter's stack.
Part 3 — Code quality suggestions
These are my preferences rather than documented rules — CONTRIBUTING.md doesn't cover them today, which is mine to fix, not yours.
3.1 Check the ThreadX return values
app/main.c discards the return value of every tx_* call — tx_byte_pool_create, tx_queue_create, tx_event_flags_create, all three tx_thread_create, plus tx_queue_send and tx_event_flags_set in the thread bodies. For an RTOS sample that people copy as a starting point, handling these matters more than usual: a silent TX_SIZE_ERROR from tx_thread_create currently produces a demo that boots and simply never runs that thread. I try to follow MISRA C where practical in this codebase (17.7 here), and samples are the code most likely to be imitated.
3.2 Test coverage
The Renode assertion script is a good addition — genuinely nice to see a new target arrive with automated validation. It currently asserts on two log strings. Since the BSP abstraction is explicitly meant to be reused, some coverage of the BSP contract itself would strengthen it.
Part 4 — Medium
- Dead resources, and telemetry that contradicts them.
byte_pool/memory_area(16 KB) are created and never allocated from, yetreporter_thread_entryprints"Memory Area Active".vibration_g,simulated_vib(constant0.5f) andALARM_HIGH_VIBare never exercised. Either wire them up or remove them — as written the demo advertises capabilities it doesn't have. - Queue over-reads 4 bytes.
sizeof(SENSOR_DATA)is 12;DEMO_QUEUE_MSG_WORDSrounds up to 2 ULONGs (16 bytes), so everytx_queue_sendcopies 4 bytes past the struct.queue_areaalso budgets asizeof(void*)per slot that ThreadX doesn't use. hwtimer_ack()(hwtimer.c:21) advancesmtimecmpfrommtimecmpwith no catch-up clamp. A missed tick leaves it behindmtimeand the timer re-fires continuously until it catches up. Consider clamping tomtime + TICK_CYCLESwhen it falls behind.- Duplicated constants.
CLINT_TIME_FREQ_HZ(hwtimer.h:20) duplicatesBSP_CLINT_RTC_FREQ_HZ(board_config.h), andTHREADX_TICK_RATE_HZ 100ULL(hwtimer.h:21) hardcodes what should derive fromTX_TIMER_TICKS_PER_SECOND.app/main.ccompounds this withtx_thread_sleep(50)commented as "500ms". If any one of these changes the others silently desync.architecture.mdpresentsboard_config.has the single place for board settings — worth honouring that here. #ifdefvs#if. PolarFire'sbsp_console.c:19andbsp_led.c:18use#ifdef BSP_HAS_CONSOLE/BSP_HAS_LED, whiletemplates/target/lib/bsp/src/bsp_led.c:18correctly uses#if. Both flags are#define ... 1, so setting either to0in the PolarFire BSP disables nothing. The template has it right; the reference implementation should match.file(GLOB THREADX_COMMON_SOURCES ...)(app/CMakeLists.txt:11) withoutCONFIGURE_DEPENDS— stale builds after a submodule bump. Separately, PolarFire compiles ThreadX itself while F401RE linksazrtos::threadx; two mechanisms for one dependency in one repo is worth reconciling.renode-latestis unpinned (ci.yml:94), which contradicts the "pinned toolchain for reproducible CI builds" claim — the RISC-V GCC is pinned. Neither download is checksum-verified.APP_CONFIGis dead.targets/STMicroelectronics/NUCLEO_F401RE/app/CMakeLists.txt:16computesCONFIG_DIRand never uses it; sources hardcodeapps/threadx_demo/main.c.build.sh -c <name>still accepts the flag and prints"Building application with configuration: X", so an invalid config builds the default and reports success.board_init()andbsp_board_init()both calluart_init()— harmless double-init, but worth picking one owner.
Part 5 — Minor
docs/architecture.mddocumentsbsp_console_write(const char *data, int length); the header declaressize_t. The doc's tree also showssamplexFORK/ (repository root)and omitsSTMicroelectronics/andOpenHW/.newlib_stubs.cusesuintptr_twithout including<stdint.h>— it currently resolves transitively through<sys/stat.h>on some newlib configs. Please include it explicitly._sbrkdoesn't handle negativeincr, which newlib does use to shrink the heap.build.shdoesshift 2on-c|--configwithout checking$#, so a trailing--configfails underset -e..datais never copied from LMA to VMA inentry.S. Fine for a Renode ELF load where LMA == VMA, but it will not survive a real Icicle boot via HSS. Worth a comment noting the constraint.
What I checked and found correct
So this isn't all negative — I verified these specifically and they're right:
- FPU handling.
-march=rv64gc -mabi=lp64ddefines__riscv_float_abi_double,tx_initialize_low_level.Ssetsmstatus.FS, and the risc-v64 port savesft*/fa*on interrupt entry andfs0-fs11on solicited switch. Float state survives context switches correctly. - Hart-1 gating in
entry.Sand the parking loop for auxiliary harts. mtimecmpfor Hart 1 atCLINT + 0x4008.- The tick math: 1 MHz CLINT / 100 Hz = 10,000 cycles = 10 ms.
- Licence headers are present and consistent on all 26 new source files, matching the form used by the majority of the existing tree.
- The BSP abstraction is a good shape —
board.h/led.h/console.hare the right level, and keeping the application free of vendor MMIO is exactly what I was hoping for here.
Suggested path forward
- Split into PR A and PR B as described in 1.4, and restore the unrelated files (1.5). Between them this removes well over 100,000 lines from what has to be reviewed.
- In PR A, fix the PLIC base addressing and the linker script (2.1, 2.2) — these are the two that stop the target working as described.
- Add
devto the CI triggers so the pipeline actually runs on the PR and I can watch it go green. - Work through the rest of Part 2, then Parts 3–5, as follow-ups. A few of those (2.8's
_sbrkbounds check, Part 4's deadAPP_CONFIG) live in the F401RE tree and belong in PR B.
Ping me as soon as PR A is up and I'll turn the review around quickly. Don't read the length of this as a verdict on the work — the design is sound and most of what is above is mechanical. The two that actually need thought are the PLIC addressing and the linker script; the rest is cleanup.
If there's one theme to take from this, it's the pattern behind 2.1, and the dead byte_pool and vibration path in Part 4: each is a capability that gets set up and then never exercised, so nothing can tell you whether it works. The PLIC registers are wrong, the byte pool is never allocated from, and vibration_g never varies — yet the demo runs identically and prints "Memory Area Active" regardless. Whenever you add something to a sample, it's worth asking what observable behaviour would change if it were broken. If the answer is "nothing", either wire it up or leave it out.
fdesbiens
commented
Aug 28, 2026
Superseded by #49. |
Overview
This PR adds support for the Microchip PolarFire SoC Icicle Kit under
targets/Microchip/POLARFIRE_ICICLE_RENODE/.The goal is to provide a reusable 64-bit RISC-V ThreadX target and condition-monitoring demo running in Renode, while establishing a common BSP abstraction for future targets and automated build and simulation validation through GitHub Actions.
Changes
u54_1) for ThreadX execution and parking auxiliary application harts_write(),_sbrk(), and_exit()TX_QUEUETX_EVENT_FLAGS_GROUPValidation
Tested locally using Renode and the configured cross-compilation toolchains:
Toolchain
Notes
The PolarFire target uses the reusable BSP interfaces for application-level hardware access, keeping the condition-monitoring application independent of platform-specific MMIO details. The CI pipeline provides automated build and headless Renode validation to detect build or runtime regressions.