Initial rp235x support #3243
Initial rp235x support #3243
Conversation
Examples have been run, but there is not yet a test suite.
|
CI build fails because currently neither the 2040 nor rp235x are selected by default, so there is no pac. I'm not sure what the best way to handle that is. If it's built with -F rp2040 -F rt-2040 or -F rp235x -F rt-235x it should pass. Tests fail because there are no tests.
|
|
I will review fully soon, just one quick thought for now: I think we should add rp235x to Chiptool doesn't have builtin support for that. Either we could add something, or we could just make chiptool generate the two sub-pacs and write lib.rs manually to include either one or the other. |
|
rp-pac#5 now supports both the 2040 and 2350. I had assumed it'd be hard to do, but it wasn't too bad. I just updated the update.sh to make both and hand wrote a tiny lib.rs. Features did indeed clean up with the single pac. |
|
ci.sh fails, but I don't understand why. Building embassy-rp with (as far as I know) the same args works. |
embassy-rp/src/spi.rs
Outdated
| if let Some(pin) = &clk { | ||
| pin.gpio().ctrl().write(|w| w.set_funcsel(1)); | ||
| pin.pad_ctrl().write(|w| { | ||
| #[cfg(feature = "rp235x")] |
There was a problem hiding this comment.
Looks little suspicious. Is cfg intended here or before whole write? (Same for other three same blocks below ...)
There was a problem hiding this comment.
This is correct, only the 235x has isolation latches for the gpio pads. The remainder of the pad config is the same.
|
i've done some hacking on the PAC, mostly to arrayify/deduplicate more stuff. The PAC builds ~2x faster now. Cherry-pick this commit into your branch to fix CI. 1ff027b . BTW I'd request for next PRs you enable the "allow maintainers to push to the branch" thing, it's nice to allow maintainers to do small fixes like that directly on the PR. |
embassy-rp/src/flash.rs
Outdated
| #[inline(never)] | ||
| #[link_section = ".data.ram_func"] | ||
| #[cfg(feature = "rp235x")] | ||
| unsafe fn write_flash_inner(_addr: u32, _len: u32, _data: Option<&[u8]>, _ptrs: *const FlashFunctionPointers) {} |
There was a problem hiding this comment.
please don't leave empty functions if unimplemented. Either add a todo!() or ideally make the API inaccessible for the user.
There was a problem hiding this comment.
I've left this as TODO, flash will be my next pr.
| impl_pin!(PIN_28, Bank::Bank0, 28); | ||
| impl_pin!(PIN_29, Bank::Bank0, 29); | ||
|
|
||
| #[cfg(feature = "rp235xb")] |
There was a problem hiding this comment.
this needs adjusting (probably renaming...) BANK0_PIN_COUNT / BANK0_WAKERS, or async wait_for_* will panic due to OOB.
There was a problem hiding this comment.
I've done the cfg bits, but didn't rename anything. Pi still calls all 48 pins on the B bank 0. I think it's because of the gpio co-processor that lets you access all 48 pins in a single cycle. On the other hand, that's not implemented here yet...
There was a problem hiding this comment.
Pi still calls all 48 pins on the B bank 0.
Huh, okay.
The rp235x doc test requires an unfortunate workaround using a private feature, "_test", in order compile.
|
If it's not an issue to use the git dep for the pac then I think this is ready. |
Dirbaio
left a comment
Dirbaio
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is awesome. I've got some rp2350's in the mail, can't wait to test it out 🙃
|
Hello! Just a reminder that under the terms of the licences under which my HAL was placed, you have to include the copyright line from my MIT file along with the paragraphs below it in any copies or substantial portions of the software. As this PR lifts several modules wholesale, I’d say that counts. If I had included a NOTICES file you would also have to carry that, but I didn’t include one. |
|
@thejpster can you send a PR? |
|
Not from my phone. Maybe in a week after my vacation. |
|
4 participants
There are many places that could reasonably be bikesheded, as I'm the only person who's spent any time with this code and names are important. I especially want input on the feature names and organization.
Features not yet supported:
Examples have been run locally during development for sanity checks, but there is not yet a test suite, as probe-rs does not yet support the rp2350.
Many thanks to @thejpster for doing all the hard parts and for writing the start block and binary info code.
This depends on rp-pac#5. rp23-pac should probably be moved to the embassy-rs org.
Marked draft at least until rp-pac and rp23-pac are updated/moved/released and the related cargo.tomls are updated.