nrf-nrf54l: add basic nrf54l support #4393
Conversation
dmitry-tarnyagin
force-pushed
the
dev/nrf54l
branch
from
65a1946 to
da387cc
Compare
dmitry-tarnyagin
force-pushed
the
dev/nrf54l
branch
3 times, most recently
from
703977a to
74d937f
Compare
|
Checks passed, ready for review. |
There was a problem hiding this comment.
Thanks for the PR! A lot of stuff, so this will take some time to review in detail. After a first look I have several questions in the review comments. Overall, there's more code duplication than I expected, and I'm not sure about using the extend crate, which makes the nrf54 very 'bolted on' instead of doing the necessary rewrite to make the HAL flexible to support nrf54. The other thing is the use of enums for instance types, where the usual pattern has been to have an Instance trait that gets implemented for FooX, and potentially type-reased when converting to AnyFoo.
|
|
||
| #[repr(transparent)] | ||
| #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] | ||
| pub struct SpiFrequencyVals(pub u32); |
There was a problem hiding this comment.
Why define the values here instead of using them from nrf-pac for nrf54l? Is that because they're missing from the pac? If so, maybe it can be fixed in the pac.
There was a problem hiding this comment.
They are quite incompatible, unfortunately. The nRF54L has a prescaler register, and the integer value is interpreted as the core-clock-to-SCK divisor. Other nRFs use magic constants (reflected in the PAC) and a frequency register instead of a prescaler. The SpiFrequencyVals here is an attempt to maintain a common API for all the chips.
| } | ||
|
|
||
| /// Buffered UARTE driver. | ||
| pub struct BufferedUarte<'d, U: UarteInstance> { |
There was a problem hiding this comment.
What is the reason for adding a separate BufferedUarte implementation for nrf54l?
There was a problem hiding this comment.
UARTE blocks are implemented quite differently in nRF54L. In other nRFs, the number of transferred bytes can be counted as the number of RXRDY events. It’s a bit racy, since RXRDY means “received,” not “written to memory,” but it usually works—the inconsistency window is only a few 16 MHz cycles.
In nRF54, the UARTE block (or rather the EasyDMA block) implements buffering. Bytes are collected into u32 words before being sent to memory. The number of RXRDY events no longer matches the number of transferred bytes, and bytes in the internal buffer aren’t transferred to memory until the DMA END event, which can never happen if a packed size is not multiple-of-four.
The only way to flush the RX buffer is to trigger a STOP task and wait for DMA END. Fortunately, Nordic has implemented an RX timeout as a separate event, so the strategy is: wait for timeout → force stop → restart RX.
The common buffered_uarte implementation uses a PPI network to count transferred bytes via RXRDY events. nRF54L uses the timeout‐stop‐restart strategy and doesn’t need external PPI machinery at all.
An attempt to keep both implementations in the same code failed under a tangle of #[cfg]s and parallel code, so there are now two separate files.
embassy-nrf/src/buffered_uarte.rs
Outdated
| @@ -39,6 +39,7 @@ pub(crate) struct State { | |||
| rx_started_count: AtomicU8, | |||
| rx_ended_count: AtomicU8, | |||
| rx_ppi_ch: AtomicU8, | |||
| rx_ppi_regs: AtomicPtr<()>, | |||
There was a problem hiding this comment.
Could this be some kind of Ppi instance type (or AnyPpi if you want to erase the type) instead of a raw pointer?
There was a problem hiding this comment.
It can be completely removed as it’s not relevant to the nRF54. It was part of a naive attempt to just tune the buffered_uarte for the nRF54.
| pub enum DppiInstance { | ||
| /// DPPI_00 instance | ||
| Dppi00, | ||
| /// DPPI_20 instance | ||
| Dppi20, | ||
| /// DPPI_30 instance | ||
| Dppi30, | ||
| } |
There was a problem hiding this comment.
Could this instead be built into some type erased AnyPpi type (similar to AnyPin)?
There was a problem hiding this comment.
..Update: The PPI implementation closely resembles the existing implementation of GPIO. Here, the DPPI instance is a direct analogue of a GPIO port. I think it’s quite in-style :).
Existing AnyChannel and AnyGroup are already extended with controller instances.
|
@lulf I’ll clean up the original buffered_uarte.rs, removing rx_ppi_regs and/if any other leftover development code. I think it’s fine to bolt on nRF54L for now. The massive change in the nRF54 register binary interface is probably due to the new DMA.RX.MATCH functionality, which is useless because of errata 44. Right now, I don’t want to ruin the existing embassy-nrf code with tons of if #[cfg(feature = "_nrf54l")] { If a dependency on the extend crate is problematic, the compatibility layer can be implemented manually using traits / impl traits for. Again, thanks for the feedback! Please share any further comments and suggestions. Much appreciated! Regards, Dmitry |
Here we have implemented basic support for the following peripheral blocks of the nRF54L: * uarte, buffered_uarte * twim, twis * spim, spis * saadc * pwm * dppic, instantiated for dppic 00, 20, 30 * gpiote, instantiated for gpiote 20, 30 * timer Signed-off-by: Dmitry Tarnyagin <[email protected]> Signed-off-by: Vegard Eriksen <[email protected]>
dmitry-tarnyagin
force-pushed
the
dev/nrf54l
branch
from
74d937f to
afb11a2
Compare
dmitry-tarnyagin
force-pushed
the
dev/nrf54l
branch
from
afb11a2 to
e9ee938
Compare
|
non-54l buffered_uarte.rs is reverted. |
Here we have implemented basic support for the following peripheral blocks of the nRF54L: