Implement support for XipCS1 pin function. #873
Conversation
| @@ -168,6 +168,7 @@ pub(crate) fn set_function<P: PinId>(pin: &P, function: DynFunction) { | |||
| DynFunction::Pio1 => FUNCSEL_A::PIO1, | |||
| DynFunction::Pio2 => FUNCSEL_A::PIO2, | |||
| DynFunction::Clock => FUNCSEL_A::GPCK, | |||
| DynFunction::XipCs1 => FUNCSEL_A::GPCK, | |||
There was a problem hiding this comment.
The rp235x-pac is missing function select definition for XipCS1:
https://github.com/rp-rs/rp235x-pac/blob/main/svd/RP2350.yaml#L321
But FUNCSEL_A::GPCK has the same value (9). We should add a variant specific for XipCS1.
There was a problem hiding this comment.
OK I'll go add a ticket for that
There was a problem hiding this comment.
It's not an easy fix, sadly.
There was a problem hiding this comment.
We should probably limit which pins you can set with ::Clock and ::XipCs1, to avoid surprises.
There was a problem hiding this comment.
Aren't pins that can be set as XipCs1 already being limited by:
https://github.com/rp-rs/rp-hal/pull/873/files#diff-514435c8bc1dac690be86445ca032b00cea05a617a1fef193197402cf99d94e0R174
(XipCs1, Bank0, 0 | 8 | 19 | 47) => true,This check is a bit confusing 😅.
pin_valid_func!(bank0 as Gpio, [XipCs1], [0, 8, 19, 47]);?
There was a problem hiding this comment.
Yup, I missed that. It seems fine.
| @@ -48,6 +48,8 @@ pub enum DynFunction { | |||
| /// The 'Clock' function. This can be used to input or output clock references to or from the | |||
| /// rp235x. | |||
| Clock, | |||
| /// The XIP CS1 function. | |||
| XipCs1, | |||
There was a problem hiding this comment.
Maybe this is the same DynFunction as Clock? They have the same FUNCSEL value.
There was a problem hiding this comment.
I am not sure I understand your comment.
Are you proposing instead of having a DynFunction for XipCs1 it reuses Clock as they have same FUNCSEL value?
Would that mean also changing/removing? https://github.com/rp-rs/rp-hal/pull/873/files#diff-514435c8bc1dac690be86445ca032b00cea05a617a1fef193197402cf99d94e0R211:
pin_valid_func!(bank0 as Gpio, [XipCs1], [0, 8, 19, 47]);I am not sure this would be a correct change as although both Clock and XipCs1 use the same FUNCSEL value; the pins that can be Clock and the pins that can be XipCs1 are not the same.
If the plan is not to change rp235x-pac I think that we keep both DynFunction::Clock and DynFunction::XipCs1 and simply point them to the same FUNCSEL_A::GPCK as currently done on: https://github.com/rp-rs/rp-hal/pull/873/files#diff-e4e5afea6f39b9d83036b039760824fc1f6ad0138f1106bdaa18d11d4b932577R171
This way the api exposed by the HAL is sound and matches expected behavior and the discrepancy inside rp235x-pac 's FUNCSEL_A is not exposed.
Unless I am misunderstanding your comment.
There was a problem hiding this comment.
Are you proposing instead of having a DynFunction for XipCs1 it reuses Clock as they have same FUNCSEL value?
I was. But I think you're right - it's probably better to keep them separate.
|
Hey @thejpster let me know if there is anything else you would like to be changed or if you would like to wait until rp-rs/rp235x-pac#7 gets addressed. thanks |
Altaflux
changed the title
|
Looks ok. I'd like to try it though. Do you have an example? |
|
I am not sure how to test it without a Pimoroni Pico Plus 2. I use this functionality on my project to enable its PSRAM. https://github.com/Altaflux/gb-rp2350/blob/main/src/main.rs#L375 If you have a Pimoroni Pico Plus 2 I am more than happy to build a sample project for the PSRAM only for you to test if you want to. |
I do. I used it to run the Neotron OS demo at RustConf, as it happens. |
Implement support for XipCS1 pin function which is necessary to implement support for external ram or flash.
Note: The rp235x-pac is missing function select definition for XipCS1:
https://github.com/rp-rs/rp235x-pac/blob/main/svd/RP2350.yaml#L321
But
FUNCSEL_A::GPCKhas the same value (9). We should add a variant specific for XipCS1.