drivers: i2s: i2s_mcux_flexcomm: multiple bugs #59803

mjchen0 · 2023-06-28T01:31:09Z

Describe the bug
While testing a driver for the speaker amp on the RT595EVK, I found multiple bugs in the i2s_mcux_flexcomm driver:

the size passed in i2s_write() is ignored. it is always transferring the mem_block size configured for the stream. so if the block passed to i2s_write() was only partially filled, garbage could be sent out to i2s.
if the values for CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT were to be set to non-defaults and not the same, the msg queue buffers would be incorrectly sized.
related to bug drivers: dma: dma_mcux_lpc: uses wrong transfer size for I2S #59802, the source_data_size and dest_data_size should be set to be the same because the DMA controller can only set one size for each transfer.

I've not checked the history but I believe these bugs have existed for a while (since Zephyr v3.3 at least).

To Reproduce
Since the driver for the TFA9896 isn't currently in Zephyr, it's not easy to reproduce with existing configs. I ported the TFA9896 driver from the NXP SDK for RT595EVK to Zephyr and found these bugs while testing the driver.

Expected behavior

size for i2s_write should be honored
CONFIG for TX_BLOCK_COUNT and RX_BLOCK_COUNT should work when they are not the same value
source_data_size and dest_data_size should be set to be the same

Impact
When DMA driver is changed to address bug #59802, new asserts will cause I2S driver to fail.
The other bugs can be worked around but are annoying and incorrect.

Environment (please complete the following information):

Zephyr v3.4.0 release

Fix for bugs described in: zephyrproject-rtos#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

Fix for bugs described in: #59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

Fix for bugs described in: zephyrproject-rtos/zephyr#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime (cherry picked from commit f882d31) Original-Signed-off-by: Mike J. Chen <mjchen@google.com> GitOrigin-RevId: f882d31 Change-Id: Ic6d1eb111ff878070d5ee4558731ee3993ff247c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4702331 Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com> Tested-by: Fabio Baltieri <fabiobaltieri@google.com> Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com> Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>

Fix for bugs described in: zephyrproject-rtos/zephyr#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

Fix for bugs described in: zephyrproject-rtos#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

Fix for bugs described in: zephyrproject-rtos/zephyr#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

Fix for bugs described in: zephyrproject-rtos#59803 1. the size argument passed to i2s_write() was being ignored. change the code so that the size is queued with the tx mem_block and the dma transfer is configured with this size. 2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that the queue buffers are allocated correctly when the two config values are not the same 3. set source_data_size and dest_data_size to be the same since the DMA controller can only set one size per DMA transfer. the driver was already computing a dest_data_size but always passing 1 for the source_data_size. For I2S RX case, I think source_data_size should be set to the expected FIFO read size instead of dest_data_size. Also some smaller improvements like: * don't allocate two dma_blocks for tx in the static dev_mem when it only needs one * memset both rx_dma_blocks together instead of separtely * set dma_cfg block_count for tx and rx statically instead of at runtime Signed-off-by: Mike J. Chen <mjchen@google.com>

mjchen0 added the bug The issue is a bug, or the PR is fixing a bug label Jun 28, 2023

henrikbrixandersen added platform: NXP NXP area: I2S labels Jul 4, 2023

jhedberg assigned mjchen0 Jul 4, 2023

jhedberg added the priority: low Low impact/importance bug label Jul 4, 2023

mmahadevan108 mentioned this issue Jul 17, 2023

fix bugs in mcux i2s and dma #59832

Merged

dleach02 closed this as completed in #59832 Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: i2s: i2s_mcux_flexcomm: multiple bugs #59803

drivers: i2s: i2s_mcux_flexcomm: multiple bugs #59803

mjchen0 commented Jun 28, 2023

drivers: i2s: i2s_mcux_flexcomm: multiple bugs #59803

drivers: i2s: i2s_mcux_flexcomm: multiple bugs #59803

Comments

mjchen0 commented Jun 28, 2023