Skip to content

Conversation

AnnsAnns
Copy link
Member

@AnnsAnns AnnsAnns commented Sep 30, 2025

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

@github-actions github-actions bot added Area: doc Area: Documentation Area: build system Area: Build system Area: boards Area: Board ports Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Sep 30, 2025
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 30, 2025
@crasbe
Copy link
Contributor

crasbe commented Sep 30, 2025

Would you mind creating a "DELETEME" commit that adds the two boards to the .murdock file, so that the CI tests actually are meaningful? :)

@AnnsAnns AnnsAnns requested a review from kaspar030 as a code owner September 30, 2025 14:30
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Sep 30, 2025
@riot-ci
Copy link

riot-ci commented Sep 30, 2025

Murdock results

FAILED

cca4d4f DELETEME

Build failures (21)
Application Target Toolchain Runtime (s) Worker
examples/basic/default rpi-pico-2-riscv gnu 1.83 mobi7
tests/bench/sizeof_coretypes rpi-pico-2-riscv gnu 1.65 mobi7
tests/core/msg_avail rpi-pico-2-riscv gnu 2.26 mobi6
tests/core/mutex_order rpi-pico-2-riscv gnu 2.25 mobi6
tests/core/sched_change_priority rpi-pico-2-riscv gnu 1.79 mobi7
tests/core/thread_flags_group rpi-pico-2-riscv gnu 1.79 tatooine
tests/core/thread_msg_block_w_queue rpi-pico-2-riscv gnu 2.07 mobi6
tests/core/thread_msg_bus rpi-pico-2-riscv gnu 2.22 mobi6
tests/drivers/disp_dev rpi-pico-2-riscv gnu 1.90 mobi7
tests/pkg/jsmn rpi-pico-2-riscv gnu 1.76 tatooine
tests/pkg/mcufont rpi-pico-2-riscv gnu 1.57 tatooine
tests/sys/cb_mux rpi-pico-2-arm gnu 2.48 skyleaf
tests/sys/event_source rpi-pico-2-riscv gnu 2.29 tud-1
tests/sys/malloc rpi-pico-2-riscv gnu 1.92 skyleaf
tests/sys/od rpi-pico-2-riscv gnu 2.25 mobi6
tests/sys/shell_coreclk rpi-pico-2-riscv gnu 1.93 skyleaf
tests/sys/ssp rpi-pico-2-riscv gnu 1.63 mobi7
tests/sys/struct_tm_utility rpi-pico-2-riscv gnu 2.09 mobi6
tests/sys/vfs_iterate_mount rpi-pico-2-riscv gnu 1.91 skyleaf
tests/turo rpi-pico-2-riscv gnu 1.70 mobi7

and 297 more build failures...

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 🎉 Some comments from briefly skimming over the changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the exact same file as in boards/rpi-pico-2-riscv?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just wasn't too sure how to handle the boards, technically I could also do the rpi-pico-2-common strategy but also like, there is barely any code here (Though I guess doing that would futureproof it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a rpi-pico-2-common folder something that makes sense in this context currently?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For boards, the common parts are in the subfolder boards/common, so in this case boards/common/rpi-pico-2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a common boards folder 👍

Comment on lines 79 to 84
// case UART_PARITY_EVEN:
// io_reg_atomic_set(&dev->UARTLCR_H, UART0_UARTLCR_H_EPS_Msk | UART0_UARTLCR_H_PEN_Msk);
// break;
// case UART_PARITY_ODD:
// io_reg_atomic_set(&dev->UARTLCR_H, UART0_UARTLCR_H_PEN_Msk);
// break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually argue those are fine, if they are not actual comments, but more or less temporarily commenting out part of the code. The question is rather why it is commented out, an additional comment would help :)

Copy link
Member Author

@AnnsAnns AnnsAnns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the reviews (esp. so quickly 🐎), I will try to go through all the points mentioned and fix them 🐸 👉👉

Comment on lines 79 to 84
// case UART_PARITY_EVEN:
// io_reg_atomic_set(&dev->UARTLCR_H, UART0_UARTLCR_H_EPS_Msk | UART0_UARTLCR_H_PEN_Msk);
// break;
// case UART_PARITY_ODD:
// io_reg_atomic_set(&dev->UARTLCR_H, UART0_UARTLCR_H_PEN_Msk);
// break;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh quite frankly because I never tested these configurations and there were still issues with different define names between cpu/rp2040 and RP2350.h

@crasbe crasbe added the CI: no fast fail don't abort PR build after first error label Oct 1, 2025

if ((mcause & ~MCAUSE_INT) <= 0xb) {
const char *error_messages[] = {
"Instruction alignment: Does not occur on RP2350, because 16-bit compressed instructions are implemented, and it is impossible to jump to a byte-aligned address.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Instruction alignment: Does not occur on RP2350, because 16-bit compressed instructions are implemented, and it is impossible to jump to a byte-aligned address.",
"Instruction alignment: Does not occur on RP2350, because "
"16-bit compressed instructions are implemented, and it is "
"impossible to jump to a byte-aligned address.",

Perhaps you can split the other lines up too? C allows you to do that:
https://stackoverflow.com/a/797351

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the entire function to be a bit less hacky (The error messages were straight out of the rp2350 documentation, the new one follows the official RISC-V specs https://riscv.github.io/riscv-isa-manual/snapshot/privileged/#mcause)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be fancy, you can also add a comment with that link to the code 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy fancy

@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 1, 2025

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.

@crasbe
Copy link
Contributor

crasbe commented Oct 2, 2025

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)

@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 6, 2025

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.

@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 6, 2025

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:

@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 6, 2025

Its hard to keep track with Github but I think I should have covered all feedback, thank you for all the reviews so far 🙏

Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think though you can do an intermediate squash already. Or you can squash everything at the very end, however you prefer.

Hopefully I'll get around testing it on real hardware soon.

CFLAGS += -D$(CPU_MODEL)
CFLAGS += -DROM_START_ADDR=$(ROM_START_ADDR)
CFLAGS += -DRAM_START_ADDR=$(RAM_START_ADDR)
CFLAGS += -Wno-error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CFLAGS += -Wno-error

@@ -1,33 +1,17 @@
CFLAGS += -Wno-pedantic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CFLAGS += -Wno-pedantic

I guess?

*
* @file
* @brief CPU initialization implementation for the RP2350
* @file cpu.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @file cpu.c
* @file

I think this is not necessary.

@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 7, 2025

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])

@AnnsAnns AnnsAnns force-pushed the rp2350_common_the_third_and_final branch from ceef12d to 8a0ad0e Compare October 8, 2025 11:23
@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 8, 2025

That rebase went like, wrong wrong wow, the amount of fixups overwhelmed it. I will fix it and re-push it.

@AnnsAnns AnnsAnns force-pushed the rp2350_common_the_third_and_final branch from 8a0ad0e to 2058fce Compare October 8, 2025 11:46
@AnnsAnns AnnsAnns force-pushed the rp2350_common_the_third_and_final branch from 2058fce to cca4d4f Compare October 8, 2025 12:00
@AnnsAnns
Copy link
Member Author

AnnsAnns commented Oct 8, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants