Skip to content
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

DMA support for all chip variants #86

Merged
merged 7 commits into from
Oct 14, 2020
Merged

DMA support for all chip variants #86

merged 7 commits into from
Oct 14, 2020

Conversation

mciantyre
Copy link
Member

@mciantyre mciantyre commented Oct 13, 2020

The PR provides DMA support for all chip variants. DMA support is probably lower on #56's TODO list. However, the DMA implementation is more unique than the other peripherals: the RAL interface was hand-written. Hand-writing the RAL was necessary for providing a simpler API, since the svd2ral script generated a lot of symbols.

I arrived at this implementation by studying the reference manuals. In summary, the 1011 chip only has 16 DMA channels; chips that are 1015 and higher hav 32 DMA channels. When you have more than 16 DMA channels, you have the option for channel grouping. Today's DMA driver doesn't support channel grouping, so this is only a comment in the code.

The conditional compiles are hidden behind a new chip module, private to the DMA driver. We use the common symbols from the chip module throughout the implementation.

This is a backwards-compatible change, at least in the short term. We now export a new constant, CHANNEL_COUNT, which signals the number of DMA channels supported in the implementation. Crate users should favor that constant instead of hard-coded 32s.

The 1011 is the only chip that has 15 DMA channels. The rest have 32,
and support channel grouping.
Just to signal that we accounted for them varying across chip
variants.
@mciantyre mciantyre requested a review from teburd October 13, 2020 01:29
@mciantyre
Copy link
Member Author

mciantyre commented Oct 13, 2020

Draft until we figure out

  • what CHANNEL_COUNT should be if there are no feature flags
  • what to do when users do something like this in their "generic" code:
let mut dma_channels = peripherals.dma.clock(&mut peripherals.ccm.handle);

let tx_channel = dma_channels[9].take().unwrap();  // OK; 9 < 16
let mut rx_channel = dma_channels[25].take().unwrap(); // Compiler error, 25 > 16, won't work for 1011

I'm thinking that CHANNEL_COUNT is 32 by default. Since the 1011 is the only chip that has 16 channels, it'll be the exception.

For the second case, I propose that we always return an array of 32 DMA channels. But, only the lower CHANNEL_COUNT channels are initialized to Some(channel). If a user wants to remain portable across all i.MX RT variants, they will need to check for None in the upper half of the array.

We could also emit a compile-time configuration, named something like imxrt_hal_dma_limited_channels, that users could check for conditionally enabling code, or raise a compile-time error if they really need all the channels.

// In code that depends on imxrt-hal...

#[cfg(imxrt_hal_dma_limited_channels)]
compile_error!("This library requires the i.MX RT to have 32 DMA channels!");

fn do_dma_things() { /* ... */ }

I've not implemented that, because I've no easy way to test it at the moment. I'd also like to look up any best practices on this pattern. (I really want if constexpr and static_assert from C++, so users could check CHANNEL_COUNT for the same outcome...)

DMA_CHANNEL_COUNT could also be written as dma::CHANNEL_COUNT, using
the module name to qualify 'dma'.

Fix the length of the channel priority register array. It should also
be CHANNEL_COUNT long.
We only initialize the first CHANNEL_COUNT channels. If the code runs
on a 1011, that means only the lower half of the array will have
valid channels. The rest are none.

I couldn't think of an elegant way to signal this at compile time.
Users are free to use the CHANNEL_COUNT constant to do something
fancies. Or, we could consider signaling a compile-time configuration
from a build script.
@mciantyre mciantyre marked this pull request as ready for review October 14, 2020 01:07
@teburd
Copy link
Member

teburd commented Oct 14, 2020

I think that makes sense

@teburd teburd merged commit 075ecf9 into master Oct 14, 2020
@mciantyre mciantyre deleted the all-chips-dma branch October 14, 2020 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants