Add DMA channels #49
Add DMA channels #49
Conversation
|
Added SPI DMA transfer and receive capabilities. The |
|
This is super, I need to look at the DMA controller on this chip closer before I can really comment well on this, will do my best to get a thorough review here soon-ish |
|
There's an enhanced UART DMA example in the |
|
I think this is ready for review. There's two working examples in the SPI and UART are DMA-capable peripherals. If we want any other DMA-capable peripherals, or any other DMA features, let me know! I might play with I2C and DMA, but unless someone has a real need for DMA-capable I2C, I don't consider it a blocker for DMA. |
|
Fantastic, I'll spend sometime this afternoon reviewing. This is fantastic, and will certainly be needed once I get to the SAI and USB audio stuff, which I want to use on a board design I've been working on |
|
First once again, I want to say how awesome of an addition this is. That said I see a few things that could perhaps be improved through either inline commentary or self describing function names that perhaps better describe whats going on. Particularly the 4 functions that setup the TCD registers with source/destination addresses, offsets, last address "add" values, things like that. The ref manual doesn't really describe why its setting all the registers itself in its examples, I had to look at the register descriptions. That's a lot of in depth digging to better understand the ~5-10 lines of register manipulation that happen there. It'd be nice if things were self descriptive in the code there. I know what it all does today as I write this, but won't likely remember a few months from now. New people looking at that code won't either. I think overall this is a great first round on DMA. There's features like you mentioned that are missing. One feature I particularly like in this DMA controller is its ability to automatically read or write from a ring buffer using the MOD field. Its not quite as awesome as "here's my N buffers, read from them or write to them" as the buffer must be contiguous in memory, but still something I'd likely use myself in the future. It's not entirely clear how things might change to use that feature here. the set_source/set_destination don't likely need it, but perhaps the set_destination_buffer, set_source_buffer could. This isn't something that needs to be addressed in this PR but I guess I'd like to hear your thoughts on how we might expand this API to support the MOD feature. I'd like to add a ticket then after to make sure we don't forget. Lastly a DMA powered memcpy is always a nice addition, and this DMA controller seems to support it. Also not something that needs to be addressed here, but perhaps we can write a ticket so we don't forget. |
| impl Element for u64 { | ||
| const DATA_TRANSFER_ID: u16 = 3; | ||
| } | ||
|
|
There was a problem hiding this comment.
I suppose to support the 32-byte burst feature here something perhaps a little more special might be needed? Would likely need to be setup to look like
impl Element for u8[32]
or
impl Element for u16[16]
And so on down the line of uint types I'd guess?
There was a problem hiding this comment.
The SSIZE documentation in the reference manual says that the burst is "4 beats of 64 bits." I'd be partial to only support [u64; 4], as it describes the "beats" of the burst.
Am I misinterpreting the 32-byte burst?
Tested in the teensy4-rs project, showing both DMA RX and TX capabilities.
Effort made me realize that the UART implementation is hard-coded for UART2's DMA signals. Fixing this in the next diff...
Builder might let us evolve the possible configurations without breaking users. Helper functions are helpful when you don't want to type our your explicit type.
If a user wants to transfer one `u16`, the math that computed CITER and BITER would be zero. The invalid major loop count would trigger a DMA error. We always want at least one major loop, so ensure that there at least one major loop when transfering / receiving buffers. The commit passes the `dma_uart.rs` and `dma_spi.rs` examples in the `teensy4-rs` project. It matches e84f551 in that project.
CITER and BITER reflect the number of major loop iterations. When used in the set_*_buffer methods, it reflects the total number of elements in the buffer. It should not account for the element size; that's handled by NBYTES.
We may want to set the number of transfer iterations independent of the source / destination buffer size. This lets a higher-level DMA abstraction specify the number of transfer iterations independent of the buffer size. This could be used for a hypothetical peripheral- to-peripheral transfer, which would need to specify how many elements to transfer across the peripherals.
There's only an unsafe interface right now. I'm exploring approaches for a safe interface for in-memory DMA transfers.
Definitely needed when preparing a transfer. May be needed when canceling a transfer.
Also fix doctest typo in Unclocked example
|
Thanks for the review. The latest update
The |
- should set disable on completion, since we need to explicitly disable the channel, which we do as of this commit - move start() call before fence. Unsure if this has an impact at the moment, but the reference manual example seemed to group all TCD operations next to each other - call complete on the provided buffers At this commit, `Memcpy` doesn't work. Or, it didn't work in my testing on a Teensy 4. I modified the `Memcpy` implementation to call `start()` on the DMA channel in a busy-loop on `is_complete()`, and that has the effect of driving the transfer to completion. My current thinking is that we need to use a single major loop for in- memory DMA transfers, similar to what's documented in the reference manual. There's no external peripheral to request DMA services, so we need to move all the data in one call to `start()`.
Just some code cleanup, in preparation for possible changes in the DMA module.
This spreads the channel setup throughout the peripheral and buffer modules. We define a `Transfer` struct, which names and documents the values that go into defining a transfer. We separate minor loop iterations from major loop iterations, which let us support a memcpy with a single DMA service request. The memcpy example I tested copied memory across two circular buffers. I found that I was both incorrectly specifying the modulo value, and not correctly aligning my buffer. Both modulo, and the alignment, need to account for the size of the element! The former is fixed here; the latter can be enforced by an error at run-time.
The runtime check wasn't accounting for the element size. It now checks for the element size. Updated the docs. We can't reliably run the mis-alignment example, since there's a chance that the linker could place the buffer at a memory address that satisfies the alignment. The test could sporadically fail. We instead check that the example compiles. Added docs that describe the variability of this runtime-check approach.
Case in point: runtime checks might miss incorrectly-specified code that happens to work.
|
Update to the update:
I think this address all thoughts from the first review. Ready for round two! |
- Return the buffers if there was a transfer error - Handle calls to `complete()` that aren't ready
|
I'm fine with this, a few nitpicks don't block a merge in my mind. Fine to merge when you are ready |
DMA ERQ is to enable hardware requests, not general DMA channel service. I renamed the set_enable() method to set_hardware_enable() to make it more distinct. This means we have to change how we cancel a DMA memcpy. I added capabilities to halt DMA transfers, and cancel a transfer. I reduced the `Memcpy` API to signal both completion and cancel through the `complete()` method. Tested memcpy cancellation in the Teensy 4 repo.
|
Sounds good. I'll do a self-review, and merge this branch within a week. Let me know if there's anything else that might help with SAI support. |
The DAM multiplexer can treat a DMA channel as "always on." When a channel is always on, it doesn't need explicit re-activation, or service requests, to drive the major loop to zero. This means we can accomplish a memory copy across multiple major loops, not just one, large major loop, without needed explicit software re-activation. At this commit, all DMA transfers run through the DMA multiplexer. We have a consistent method for disabling transfers; we disable the 'enable' flag in ERQ. This undoes the changes to the enable method introduced in the previous commit.
Users should see the RAL's DMA peripherals as in-use by the HAL. Users will be required to unsafely steal the peripherals if they want to bypass the HAL.
Taking care of it now before I forget again! Closes #27.
The PR adds DMA channels, and a set of traits that lets us add DMA capabilities for iMXRT peripherals. I extend the UART peripheral with the new DMA capabilities.
There's a rough example in the
teensy4-rsrepo showing how we can use DMA to coordinate UART transfers and receives. It needs to be cleaned up and documented, but it seems to work!There are a bunch of DMA capabilities that this PR does not support, like
If you think we need these DMA capabilities, or have ideas on how we can add them in, let me know!
The PR closes #43 once we extend the SPI peripheral with the new DMA capabilities, and we test SPI DMA transfers.