drivers: mcux: multiple bug fixes and improvements to dma driver and it's users #91530
+458
−360
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
platform: NXP Drivers
github-actions
bot
requested review from
anangl,
dbaluta,
dcpleung,
decsny,
dleach02,
manuargue,
mmahadevan108,
Raymond0225,
tbursztyka and
teburd
mjchen0
force-pushed
the
mcux_spi_dma_explicit_direction
branch
from
85fe72f to
56d41ee
Compare
|
@hakehuang can you run regression on the HW range? |
Board testing PASS on v4.1.0-5619-g56d41ee3db5b |
|
@mjchen0 please address the merge conflict |
…y read." This reverts commit a3530d6. This change incorrect if chip-select is via a GPIO pin, released by spi_context_cs_control(), and not the controller default chip-select pin that can be released using a final FIFOWR. When GPIO is used for chip-select control, the chip-select could be released too soon because the transfer callback is invoked when the last byte is put into the TX FIFO but that byte might not yet have clocked out yet on the SPI pins. The rx part of the transfer is really what completes the transfer so ignoring it is incorrect. We could try to special case and use ignore only when spi_context_cs_control() does nothing, but since it is a very small optimization, it doesn't seem worth the extra complexity. Signed-off-by: Mike J. Chen <[email protected]>
The current driver implementation would block even when the async API was invoked, so it wasn't really async. This CL also fully chains the DMA transfer using multiple dma blocks and makes the number of dma blocks available a config value. The increase in number of dma blocks is needed so that a spi_buf_set that has many entries can be converted into chained dma transfers with the last transfer in a separate block that will set the EOT flag. Also make some improvements: 1) When doing single cnt transfer, don't use the SDK driver but use a new fucntion spi_mcux_transfer_single_word(). It's much more efficient and does not use an ISR like the SDK function does just to send one word. 2) Fix calls to spi_context_update_tx/rx() so that the correct word size is passed in, instead of previously being hardcoded to 1. This only matters when word size is two. Evaluate the word_size_bytes and word_size_bits once and store the values in data instead of computing it multiple times in various parts of the driver 3) When CONFIG_SPI_MCUX_FLEXCOMM_DMA is defined, we do not use the IRQ handler, so add #ifdefs to compile that code out. This reduces code size. Signed-off-by: Mike J. Chen <[email protected]>
In spi_mcux_transfer_next_packet(), if the next transfer start fails, add calls to spi_context_cs_control() to release cs and spi_context_complete() with error code -EIO. Signed-off-by: Mike J. Chen <[email protected]>
The spi_mcux_flexcomm driver uses a special last DMA blk_cfg to trigger a release of the SPI chip select. This transfer is always a 4-byte transfer, regardless of the width specified during dma_configure(). The way the spi_mcux_flexcomm driver communicated this special transfer was kind of a hack, where the dma_mcux_lpc driver would assume that when a blk_cfg with source_addr_adj and dest_addr_adj both set to NO_CHANGE was for this SPI_TX special case. However, this is an unsafe hack since it is perfectly valid to have dma use cases for both src/dest_addr_adj to be NO_CHANGE that is not for SPI_TX. One example is when transmitting a fixed/repeating value to a periperhal address (e.g. send 100 bytes of the same value from a single memory address over SPI). This CL introduces a dma_mcux_lpc specific dma channel_direction which the two drivers now use to cleary request this special transfer case. Signed-off-by: Mike J. Chen <[email protected]>
The dma driver was determining src_inc and dst_inc from the config of the first block buffer and ignoring the config flags for any additional buffers in the chain, which could lead to incorrect transfers (e.g. in a multiple rx buffer case, if the first buffer was to receive to NULL, but the subsequent buffers were not NULL, the bug would manifest as all transfers being made with dst_inc of 0). Change the driver to setup each dma descriptor according to the addr_adj flag of each block_buffer. Add check that peripheral transfers have addr_adj set to NO_CHANGE instead of assuming it, to help catch errors. Also now check for invalid addr_adj request of decrement, which this controller doesn't support. Signed-off-by: Mike J. Chen <[email protected]>
There are multiple bugs related to continuous/circular mode. Continuous/circular mode is where the DMA runs continuously until the stop API is called, instead of auto-stopping on completion on a single transfer. After a stop, the DMA can then be reconfigured/restarted. 1. Fix bug where stop didn't actually stop. This can cause memory corruption if the user thought the DMA stopped and repurposed the dest memory, but in fact the DMA is still writing to it. The bug was due the incorrect usage of the DMA controller busy state. The DMA controller is busy only when a transfer is actively in progress, but the driver needed to stop even if the transfer is not active but is only enabled (and may become active on a subsequent trigger event). Change so that data->busy doesn't use the DMA controller busy state but tracks the enable state. Also, to make it doubly safe, make stop function always stop regardless of data->busy state because it is alway safe/correct to do so. 2. Fix race condition where a stop request from another ISR might race with a DMA completion interrupt, and the DMA completion callback gets invoked after the DMA has already been stopped. The fix is to unregister the callback with the sdk DMA driver, so the ISR still runs and clear the interrupt without invoking the callback. There is potentially still a race if the interrupt is restarted before the ISR fires, so the callback might be called too early. However, the Zephyr DMA driver doesn't have the channel level details that the SDK driver does and it cannot clear just the channel interrupt. Also a couple of general fixes/improvements: a. Use interrupt B for end of transfer (single transfer or end of block list). Use interrupt A for interrupts of a block in the middle of a transfer or for continuous/circular transfers. This fixes the dma callback so it can properly report DMA_STATUS_BLOCK vs DMA_STATUS_COMPLETE. b. Reorder some fields in struct channel_data to pack a little better in memory Signed-off-by: Mike J. Chen <[email protected]>
mjchen0
force-pushed
the
mcux_spi_dma_explicit_direction
branch
from
56d41ee to
b0b9f6d
Compare
|
dleach02
approved these changes
Jul 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are multiple CL's in this PR and each has it's own detailed description.
In general, these CLs are related in fixing issues with chained and continuous DMA.
Also fixes a number of issues with SPI driver.