-
Notifications
You must be signed in to change notification settings - Fork 2.1k
RP2350: Add RISCV, Unify RP2350, Update UART, Add XH3IRQ Interrupt Controller #21753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
RP2350: Add RISCV, Unify RP2350, Update UART, Add XH3IRQ Interrupt Controller #21753
Conversation
Would you mind creating a "DELETEME" commit that adds the two boards to the |
Murdock results❌ FAILED cca4d4f DELETEME Build failures (21)
Artifacts |
There was a problem hiding this 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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
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 👍
cpu/rp2350_common/periph/uart.c
Outdated
// 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; |
There was a problem hiding this comment.
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 :)
There was a problem hiding this 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 🐸 👉👉
cpu/rp2350_common/periph/uart.c
Outdated
// 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; |
There was a problem hiding this comment.
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
cpu/riscv_common/irq_arch.c
Outdated
|
||
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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fancy fancy
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. #21753 (comment) |
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. |
Tracking: |
Its hard to keep track with Github but I think I should have covered all feedback, thank you for all the reviews so far 🙏 |
There was a problem hiding this 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.
cpu/rp2350_arm/Makefile.include
Outdated
CFLAGS += -D$(CPU_MODEL) | ||
CFLAGS += -DROM_START_ADDR=$(ROM_START_ADDR) | ||
CFLAGS += -DRAM_START_ADDR=$(RAM_START_ADDR) | ||
CFLAGS += -Wno-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFLAGS += -Wno-error |
cpu/rp2350_common/Makefile.include
Outdated
@@ -1,33 +1,17 @@ | |||
CFLAGS += -Wno-pedantic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFLAGS += -Wno-pedantic |
I guess?
cpu/rp2350_common/cpu.c
Outdated
* | ||
* @file | ||
* @brief CPU initialization implementation for the RP2350 | ||
* @file cpu.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @file cpu.c | |
* @file |
I think this is not necessary.
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]) |
ceef12d
to
8a0ad0e
Compare
That rebase went like, wrong wrong wow, the amount of fixups overwhelmed it. I will fix it and re-push it. |
8a0ad0e
to
2058fce
Compare
2058fce
to
cca4d4f
Compare
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. |
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
andrp2350_riscv
but uses the sharedrp2350_common
folder.This PR supersedes both #21745 and #21746, as in it:
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 usexh3irq_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 usemake 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 usemake PROGRAMMER=picotool QUIET=0 BOARD=rpi-pico-2-arm flash
to flash it using picotool.Issues/PRs references