RIOT-OS
RP2350: Add RISCV, Unify RP2350, Update UART, Add XH3IRQ Interrupt Controller
Contribution description
After two failed attempts, I finally managed to get a shared and unified setup working for the RP2350. As opposed to the other approaches I took, where I tried to fully unify both the RISCV and ARM versions, this version takes a different approach similar to cortexm_common or riscv_common where each CPU still exists as a separate entity, e.g. rp2350_arm and rp2350_riscv but uses the shared rp2350_common folder.
This PR supersedes both #21745 and #21746, as in it:
- adds support for the Hazard3 CPU used on the RP2350
- adds support for the XH3IRQ Interrupt Controller used by the Hazard3. While fairly similar to CLIC/PLIC, the Hazard3 on the RP2350 actually has a custom interrupt controller to "mimick" the way interrupts work on the Cortex ARM side, even having support for direct vector table jumps (Not enabled here since I wanted to still use the trap_entry/trap_handler of riscv_common.
- updates the uart
- enables interrupts on the CortexM33
- unifies both the RISCV and ARM side of the RP2350
- renames rp2350 (arm version) to rp2350_arm
- renames rpi-pico-2 (arm version) to rpi-pico-2-arm
- fixes debugging completely (GDB now runs smoothly both on ARM and RISCV)
However, as of now, it does not yet add the multicore example. All the points raised in the other PRs also still hold true.
Testing procedure
Remember to specify USEMODULE += stdio_uart if you wish to print via UART.
For my testing I mostly modified examples/basic/blinky to also print. On RISCV you can also use xh3irq_force_irq to force interrupts for testing purposes, for example, on each blinky loop, if you feel like it :)
RISCV
make PROGRAMMER=openocd QUIET=0 BOARD=rpi-pico-2-riscv flash flashes the pico 2 in RISCV mode using openocd, you can also use make PROGRAMMER=picotool QUIET=0 BOARD=rpi-pico-2-riscv flash to flash it using picotool.
ARM
make PROGRAMMER=openocd QUIET=0 BOARD=rpi-pico-2-arm flash flashes the pico 2 in RISCV mode using openocd, you can also use make PROGRAMMER=picotool QUIET=0 BOARD=rpi-pico-2-arm flash to flash it using picotool.
Issues/PRs references
Would you mind creating a "DELETEME" commit that adds the two boards to the .murdock file, so that the CI tests actually are meaningful? :)
Murdock results
:heavy_check_mark: PASSED
3eaf9d5208d1683b01b1971f2820b2bb7014ea75 fixup! rp2350/riscv: multicore support
| Success | Failures | Total | Runtime |
|---|---|---|---|
| 320 | 0 | 320 | 02m:19s |
Artifacts
Moving the register addresses out of the assembly caused the assembler to complain. Since that is the only location they are used I moved them back into the function and simply explained them within the function.
I resolved some of the review comments that were fixed, but here are the ones that either haven't been addressed yet or where I'm not sure if they should be marked as resolved yet.
https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2391828261 https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2391859578 https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2391881875 https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2392372415 https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2392381003 https://github.com/RIOT-OS/RIOT/pull/21753#discussion_r2394888094
The reason for the failed build is that the RP2350.h also defines BIT0, but in a struct:
https://github.com/raspberrypi/pico-sdk/blob/a1438dff1d38bd9c65dbd693f0e5db4b9ae91779/src/rp2_common/cmsis/stub/CMSIS/Device/RP2350/Include/RP2350.h#L674-L695
I guess there are two ways around that: either do a lot of patches in RIOT or do a single patch of the RP2350.h for a peripheral that is not used (yet) 🤔😅
The reason for the failed build is that the
RP2350.halso definesBIT0, but in a struct:https://github.com/raspberrypi/pico-sdk/blob/a1438dff1d38bd9c65dbd693f0e5db4b9ae91779/src/rp2_common/cmsis/stub/CMSIS/Device/RP2350/Include/RP2350.h#L674-L695
I guess there are two ways around that: either do a lot of patches in RIOT or do a single patch of the RP2350.h for a peripheral that is not used (yet) 🤔😅
bleeeeh 😵💫 Presumably because of using bit.h now?
The reason for the failed build is that the
RP2350.halso definesBIT0, but in a struct:https://github.com/raspberrypi/pico-sdk/blob/a1438dff1d38bd9c65dbd693f0e5db4b9ae91779/src/rp2_common/cmsis/stub/CMSIS/Device/RP2350/Include/RP2350.h#L674-L695 I guess there are two ways around that: either do a lot of patches in RIOT or do a single patch of the RP2350.h for a peripheral that is not used (yet) 🤔😅
bleeeeh 😵💫 Presumably because of using bit.h now?
I guess. But it's good that it happens now, because the RP2350 "BIT" stuff is not at all the same as the bit.h stuff. That might have caused weird errors.
Also you can use RIOT's patch feature to patch the file, similar to what we did with the CMakeLists of the picotool.
buechse@skyleaf:~/RIOTstuff/riot-vanilla/RIOT$ BUILD_IN_DOCKER=1 BOARD=rpi-pico-2-riscv make -C tests/core/thread_flags_group -j
make: Entering directory '/home/buechse/RIOTstuff/riot-vanilla/RIOT/tests/core/thread_flags_group'
Launching build container using image "docker.io/riot/riotbuild@sha256:21c416fbbb94a99c3d9c76341baf5a9740608b1d1af89967127c7a171248fd7b".
docker run --rm --tty --user $(id -u) --platform linux/amd64 -v '/usr/share/zoneinfo/Europe/Berlin:/etc/localtime:ro' -v '/home/buechse/RIOTstuff/riot-vanilla/RIOT:/data/riotbuild/riotbase:delegated' -v '/home/buechse/.cargo/registry:/data/riotbuild/.cargo/registry:delegated' -v '/home/buechse/.cargo/git:/data/riotbuild/.cargo/git:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'BUILD_IN_DOCKER=/data/riotbuild/riotbase/tests/core/thread_flags_group/1' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles' -e 'BOARD=rpi-pico-2-riscv' -e 'DISABLE_MODULE=' -e 'DEFAULT_MODULE=test_utils_interactive_sync test_utils_print_stack_usage' -e 'FEATURES_REQUIRED=' -e 'FEATURES_BLACKLIST=' -e 'FEATURES_OPTIONAL=' -e 'USEMODULE=core_thread_flags core_thread_flags_group' -e 'USEPKG=' -w '/data/riotbuild/riotbase/tests/core/thread_flags_group/' 'docker.io/riot/riotbuild@sha256:21c416fbbb94a99c3d9c76341baf5a9740608b1d1af89967127c7a171248fd7b' make -j
Building application "tests_thread_flags_group" for "rpi-pico-2-riscv" with CPU "rp2350_riscv".
"make" -C /data/riotbuild/riotbase/pkg/mpaland-printf/
"make" -C /data/riotbuild/riotbase/build/pkg/mpaland-printf -f /data/riotbuild/riotbase/pkg/mpaland-printf/mpaland-printf.mk
"make" -C /data/riotbuild/riotbase/pkg/picosdk/
"make" -C /data/riotbuild/riotbase/boards
"make" -C /data/riotbuild/riotbase/boards/rpi-pico-2-riscv
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/core/lib
"make" -C /data/riotbuild/riotbase/cpu/rp2350_riscv
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/boards/common/init
"make" -C /data/riotbuild/riotbase/cpu/riscv_common
"make" -C /data/riotbuild/riotbase/cpu/rp2350_common
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys/auto_init
"make" -C /data/riotbuild/riotbase/sys/div
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/sys/libc
"make" -C /data/riotbuild/riotbase/sys/malloc_thread_safe
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/preprocessor
"make" -C /data/riotbuild/riotbase/sys/stdio
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/cpu/riscv_common/periph
"make" -C /data/riotbuild/riotbase/sys/test_utils/interactive_sync
"make" -C /data/riotbuild/riotbase/cpu/rp2350_common/periph
"make" -C /data/riotbuild/riotbase/sys/test_utils/print_stack_usage
"make" -C /data/riotbuild/riotbase/sys/tsrb
In file included from /data/riotbuild/riotbase/core/thread_flags_group.c:21:
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:58:15: error: expected identifier or '(' before numeric constant
58 | #define BIT0 0x00000001 /**< Bit 0 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:59:15: error: expected identifier or '(' before numeric constant
59 | #define BIT1 0x00000002 /**< Bit 1 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:60:15: error: expected identifier or '(' before numeric constant
60 | #define BIT2 0x00000004 /**< Bit 2 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:61:15: error: expected identifier or '(' before numeric constant
61 | #define BIT3 0x00000008 /**< Bit 3 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:62:15: error: expected identifier or '(' before numeric constant
62 | #define BIT4 0x00000010 /**< Bit 4 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:63:15: error: expected identifier or '(' before numeric constant
63 | #define BIT5 0x00000020 /**< Bit 5 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:64:15: error: expected identifier or '(' before numeric constant
64 | #define BIT6 0x00000040 /**< Bit 6 set define */
| ^~~~~~~~~~
/data/riotbuild/riotbase/core/lib/include/bitarithm.h:65:15: error: expected identifier or '(' before numeric constant
65 | #define BIT7 0x00000080 /**< Bit 7 set define */
| ^~~~~~~~~~
make[2]: *** [/data/riotbuild/riotbase/Makefile.base:157: /data/riotbuild/riotbase/tests/core/thread_flags_group/bin/rpi-pico-2-riscv/core/thread_flags_group.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/data/riotbuild/riotbase/Makefile.base:31: ALL--/data/riotbuild/riotbase/core] Error 2
make: *** [/data/riotbuild/riotbase/Makefile.include:764: application_tests_thread_flags_group.module] Error 2
make: *** [/home/buechse/RIOTstuff/riot-vanilla/RIOT/makefiles/docker.inc.mk:386: ..in-docker-container] Error 2
make: Leaving directory '/home/buechse/RIOTstuff/riot-vanilla/RIOT/tests/core/thread_flags_group'
Works (or rather fails) for me.
It appears like the culprit was similar to the issue I had before, the riscv interrupt entry expects the scheduler to be active before going into interrupts, since we initialized the rp2350 uart, etc. before that it could cause the uart to trigger and already queue before exiting the cpu_init function. Simply enabling interrupts as the last part of the cpu_init appears to mitigate that issue.
I resolved some of the review comments that were fixed, but here are the ones that either haven't been addressed yet or where I'm not sure if they should be marked as resolved yet.
#21753 (comment) #21753 (comment) #21753 (comment) #21753 (comment) #21753 (comment) #21753 (comment)
Tracking:
- [x] #21753 (comment)
- [x] #21753 (comment)
- [x] #21753 (comment)
- [x] #21753 (comment)
- [x] #21753 (comment)
- [x] #21753 (comment)
Its hard to keep track with Github but I think I should have covered all feedback, thank you for all the reviews so far :pray:
Will do a squash tomorrow, I also updated the UART to be fairly feature complete now after a full day of debugging a really weird UART reading issue only to find out that the pins were swapped on my adapter (but for some reason UART writing still worked, its really cursed and I dont want to question it anymore [Thank you to Teufelchen for surviving this insanity with me])
That rebase went like, wrong wrong wow, the amount of fixups overwhelmed it. I will fix it and re-push it.
Not really sure what triggers this but rebase kept creating multiple identical commits in the process of rebasing.
After lunch I'll diff the pushed commits just to ensure that I didn't accidentally regress anything in the process.
Git diff tells me the pre-rebase and after rebase branches are now identical again (I also fixed a redefine on ARM) :) [Pre-rebase is still available via https://github.com/AnnsAnns/RIOT/tree/blubbranch just in case though]
RE: PMP (Quoting out my bachelor thesis just for clarity why I removed PMP again, we actually came across this issue organically and were really confused by the bug till I found the errata)
Errata RP2350-E6 breaks the PMP specification conformity. The standard ordering for PMP permissions is X, W, R (execute, write, read). The Hazard3 incorrectly interprets the ordering as R, W, X (read, write, execute). The Hazard3 core v1.1 revision fixed this issue in April 2024. The Raspberry Pi foundation decided to not include this fix in newer RP2350 revisions, instead opting to keep the errata. Based on the errata description, it can be assumed that this was done to maintain compatibility with existing software, as it states that the issue was fixed through “Documentation”. This means that the PMP implementation in RIOT OS will not work correctly on any RP2350 device.
I rebased on master to fix a conflict in CI (that was ironically caused by a PR I approved myself even :melting_face:), didn't actually squash anything don't worry :+1:
Just because it also breaks the Github visualization of the history, the last pre-review commit was https://github.com/RIOT-OS/RIOT/pull/21753/commits/656d092c00986d4ef8490a358afefa7e16bb7948
The unreviewed changes start at the fixup commit after that https://github.com/RIOT-OS/RIOT/pull/21753/commits/3b96eb3baca6dc9e34537d0b1ac4654463f22787
Also since multicore can't/isn't being tested right now, here is some proof of it running: (ARM here, RISC-V was also tested)