dma_channel_abort does not wait for CHAN_ABORT to go zero #923
Comments
|
See also Errata RP2040-E13 in the RP2040 datasheet, if you haven't already. |
|
Given RP2040-E13, I agree that this is documentation. The workaround recommends "Before aborting a channel, clear its interrupt enable. After aborting a channel, poll the CTRL.BUSY bit to wait for completion (not the ABORT bit), clear the spurious IRQ, and restore the interrupt enable." I suggest this could be improved too. CHAN_ABORT can be used to abort multiple channels with a single write to the register. In fact this seems like the best way to abort multiple channels that trigger each other, with other methods likely to suffer from race conditions. The workaround recommendation assumes that only a single channel is being aborted. It would be good to confirm that, even when multiple channels are aborted, it is adequate and necessary to check only the CTRL.BUSY bits for each channel in turn and not to check CHAN_ABORT. Without that clarification, it is not clear that checking only CTRL.BUSY is free of race conditions when aborting multiple channels. |
|
moving to 1.5.1 so it doesn't get forgotten (even if the docs aren't fixed there) |
Related, and for posterity, I have a DMA control+data setup, with the data channel re-starting control at the end, rather than issuing a null transfer, emulating a ring. This required me to do two CHAN_ABORT on both channels to ensure it stops. I'm thinking the DMA trigger register may already be in the DMA FIFO, making this racy even with CHAN_ABORT. To be safe, I have it in a loop: while (dma_channel_is_busy(dma_ctrl) || dma_channel_is_busy(dma_data)) {
dma_channel_abort_mask((1u << dma_ctrl) | (1u << dma_data));
} |
|
@tommie I have the exact same problem and would like to try your approach, but coming up against P.S.: For future reference, replacing this call with two single-channel |
|
@glopesdev Yepp, you're right. // Aborts several DMA channels at once, and waits for their
// completion. If one channel is triggering another, it may be
// necessary to call this multiple times to resolve the race condition
// of a trigger register being loaded into the DMA FIFO already.
static inline void dma_channel_abort_mask(uint mask) {
hw_set_bits(&dma_hw->abort, mask);
while (dma_hw->abort & mask) {
tight_loop_contents();
}
} |
The source for
dma_channel_abortfrom SDK 1.4.0:Note that while it waits for the channel's
BUSYflag to go zero, it does not wait forCHAN_ABORTto go zero.From the datasheet, emphasis mine:
Also from the datasheet:
The datasheet does not indicate whether it is necessary to wait for both
BUSYto go zero andCHAN_ABORTto go zero or if waiting for either condition is sufficient. It also does not indicate in which order code should wait for them to go zero. As mentioned, dma_channel_abort only waits forBUSYto go zero.I suggest there is either a bug in
dma_channel_abort(if it ought to be waiting for both) or the datasheet would benefit from clarification.The text was updated successfully, but these errors were encountered: