fix bugs in mcux i2s and dma #59832

mjchen0 · 2023-06-28T18:01:27Z

dleach02 · 2023-07-13T23:27:14Z

@hakehuang can you run a regression test on RT595

hakehuang · 2023-07-14T03:42:00Z

the existing testcases i2s_speed can not catch such issue, as I can run pass with i2s_speed test with(zephyr-v3.4.0-976-ge17597821848)/without(zephyr-v3.4.0-974-g7fba7d39575) this fixes

*** Booting Zephyr OS build zephyr-v3.4.0-974-g7fba7d395750 ***
Running TESTSUITE drivers_i2s_speed
===================================================================
START - test_i2s_transfer_long
20 TX blocks sent
20 RX blocks received
 PASS - test_i2s_transfer_long in 0.120 seconds
===================================================================
START - test_i2s_transfer_short
0->OK
1->OK
2->OK
1<-OK
2<-OK
3<-OK
 PASS - test_i2s_transfer_short in 0.021 seconds
===================================================================
TESTSUITE drivers_i2s_speed succeeded
Running TESTSUITE drivers_i2s_speed_both_rxtx
===================================================================
START - test_i2s_dir_both_transfer_long
I2S_DIR_BOTH value is not supported.
 SKIP - test_i2s_dir_both_transfer_long in 0.004 seconds
===================================================================
START - test_i2s_dir_both_transfer_short
I2S_DIR_BOTH value is not supported.
 SKIP - test_i2s_dir_both_transfer_short in 0.004 seconds
===================================================================
TESTSUITE drivers_i2s_speed_both_rxtx succeeded

------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [drivers_i2s_speed]: pass = 2, fail = 0, skip = 0, total = 2 duration = 0.141 seconds
 - PASS - [drivers_i2s_speed.test_i2s_transfer_long] duration = 0.120 seconds
 - PASS - [drivers_i2s_speed.test_i2s_transfer_short] duration = 0.021 seconds

SUITE SKIP -   0.00% [drivers_i2s_speed_both_rxtx]: pass = 0, fail = 0, skip = 2, total = 2 duration = 0.008 seconds
 - SKIP - [drivers_i2s_speed_both_rxtx.test_i2s_dir_both_transfer_long] duration = 0.004 seconds
 - SKIP - [drivers_i2s_speed_both_rxtx.test_i2s_dir_both_transfer_short] duration = 0.004 seconds

------ TESTSUITE SUMMARY END ------

===================================================================
PROJECT EXECUTION SUCCESSFUL
I: Device flexcomm@109000 inited
I: Device flexcomm@107000 inited
*** Booting Zephyr OS build zephyr-v3.4.0-976-ge17597821848 ***
Running TESTSUITE drivers_i2s_speed
===================================================================
START - test_i2s_transfer_long
20 TX blocks sent
20 RX blocks received
 PASS - test_i2s_transfer_long in 0.033 seconds
===================================================================
START - test_i2s_transfer_short
0->OK
1->OK
2->OK
1<-OK
2<-OK
3<-OK
 PASS - test_i2s_transfer_short in 0.008 seconds
===================================================================
TESTSUITE drivers_i2s_speed succeeded
Running TESTSUITE drivers_i2s_speed_both_rxtx
===================================================================
START - test_i2s_dir_both_transfer_long
I2S_DIR_BOTH value is not supported.
 SKIP - test_i2s_dir_both_transfer_long in 0.004 seconds
===================================================================
START - test_i2s_dir_both_transfer_short
I2S_DIR_BOTH value is not supported.
 SKIP - test_i2s_dir_both_transfer_short in 0.004 seconds
===================================================================
TESTSUITE drivers_i2s_speed_both_rxtx succeeded

------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [drivers_i2s_speed]: pass = 2, fail = 0, skip = 0, total = 2 duration = 0.041 seconds
 - PASS - [drivers_i2s_speed.test_i2s_transfer_long] duration = 0.033 seconds
 - PASS - [drivers_i2s_speed.test_i2s_transfer_short] duration = 0.008 seconds

SUITE SKIP -   0.00% [drivers_i2s_speed_both_rxtx]: pass = 0, fail = 0, skip = 2, total = 2 duration = 0.008 seconds
 - SKIP - [drivers_i2s_speed_both_rxtx.test_i2s_dir_both_transfer_long] duration = 0.004 seconds
 - SKIP - [drivers_i2s_speed_both_rxtx.test_i2s_dir_both_transfer_short] duration = 0.004 seconds

------ TESTSUITE SUMMARY END ------

===================================================================
PROJECT EXECUTION SUCCESSFUL

mmahadevan108 · 2023-07-14T20:49:00Z

drivers/dma/dma_mcux_lpc.c

+	 * for source and dest. So assert that the source
+	 * and dest sizes are the same.
+	 */
+	assert(config->dest_data_size == config->source_data_size);


This assert may cause failures in some of the other drivers that use this driver. I would suggest removing it and updating the comment.

If the caller has the dest_data_size == source_data_size, then the assert won't trip, and it won't trip if asserts are not enabled.

If the caller has different values for src and dest sizes, then it's a problem they need to think about and fix.
The old driver (prior to the change to support chaining for long transfers) used MIN. The driver
that supports the long transfers used MAX. This changing of behavior already broke existing drivers silently (e.g. the I2S usage). I think it'd be better to detect such problems as soon as possible, instead of silently doing something they might not be expecting and possibly setting the size the opposite of what the caller intended.

I know we will have to update the SPI driver to address this. Let me know if you would like me to send you a patch.

I can add that. I thought I had checked the SPI driver, since we use that too, but I think we didn't catch it because our usage of SPI is always using 1-byte dest_data_size (src_data_size is fixed at 1).

I think also the UART driver will be fine because destination and source data sizes will both be 1 always

mmahadevan108 · 2023-07-17T14:09:59Z

drivers/dma/dma_mcux_lpc.c

@@ -492,6 +498,9 @@ static int dma_mcux_lpc_configure(const struct device *dev, uint32_t channel,
 				dst_inc,
 				block_config->block_size);
 	}
+	/* DMA controller requires that the address be aligned to transfer size */


I am not aware of such a alignment requirement. Were you seeing an issue?
There is a requirement for the DMA descriptors which this driver addresses.

In the RT500 Reference Manual, section 16.4.2.4 says:

16.4.2.4 Address alignment for data transfers
Transfers of 16 bit width require an address alignment to a multiple of 2 bytes. Transfers of 32 bit width require an address alignment to a multiple of 4 bytes. Transfers of 8 bit width can be at any address

dbaluta · 2023-07-17T14:42:37Z

drivers/i2s/i2s_mcux_flexcomm.c

@@ -19,7 +19,7 @@



Ideally you should add only one patch per issue addressed. This way the code would be much easier to understand and review.

There are exceptions if the changes are tightly related. Not sure if this the case.

The primary bug is the change/regression in the DMA driver. But the fix to the DMA driver includes an assert, which is there to prevent future mistakes from happening silently. That assert would immediately break the I2S driver, which is why the two commits are combined in this PR.

I could take out the I2S one, but it would have to be merged first. I'm still relatively new to Zephyr (and github) development so I don't know if pull requests can be dependent on one another. Let me know if there's still a preference in having the I2S change be a separate Pull Request.

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 <[email protected]>

mmahadevan108 · 2023-07-17T18:39:09Z

@mjchen0. Thank you for submitting the PR. The changes look fine to me. Could you help fix the CI compliance failures.

The MCUX DMA controller only supports a single data_size for a DMA transfer, not separate ones for source and dest. An older version of the DMA driver used dest_data_size as the DMA transfer size, but the current one uses MIN(dest/source) as the trasnfer size, which breaks case when SPI wants to do 2-byte transfers. Signed-off-by: Mike J. Chen <[email protected]>

Fix for bug: zephyrproject-rtos#59802 The DMA controller only supports one transfer size, but the Zephyr DMA driver api allows specifying a source_data_size and dest_data_size which might be different. An old version was always using dest_data_size for the transfer size (variable is called "width"), but a recent change made the driver use the MIN for the source and dest data sizes. The MIN choice breaks the I2S driver because it always set source_data_size to 1, but dest_data_size was typically 4 for like two-channel 16-bit PCM data. So the old driver worked using dest_data_size, but the new driver broke I2S using MIN since source_data_size was 1. To prevent confusion, change the DMA driver to assert that source_data_size and dest_data_size are the same. Also assert that the source_address and dest_address for each block_config are properly aligned for the transfer size, since that is a documentated requirement for the DMA controller. Also rename max_xfer to max_xfer-bytes to be more clear what the units are, and use this value in many places that are comparing block_size in bytes rather than converting block_size to words by dividing by width and then comparing to NXP_LPC_DMA_MAX_XFER. Signed-off-by: Mike J. Chen <[email protected]>

mjchen0 requested review from mmahadevan108 and dleach02 as code owners June 28, 2023 18:01

zephyrbot added platform: NXP NXP area: DMA Direct Memory Access area: I2S labels Jun 28, 2023

zephyrbot requested review from anangl, danieldegrasse, dbaluta, decsny, DerekSnell, EmilioCBen, iuliana-prodan, manuargue, teburd and yvanderv June 28, 2023 18:02

zephyrbot assigned dleach02 Jun 28, 2023

mjchen0 force-pushed the work branch from ed05d5e to 1aeaaf3 Compare June 28, 2023 18:39

mjchen0 force-pushed the work branch from 1aeaaf3 to e175978 Compare July 11, 2023 19:53

mmahadevan108 reviewed Jul 14, 2023

View reviewed changes

mmahadevan108 reviewed Jul 17, 2023

View reviewed changes

dbaluta reviewed Jul 17, 2023

View reviewed changes

mjchen0 force-pushed the work branch from e175978 to 67eb3a7 Compare July 17, 2023 17:48

mjchen0 requested a review from tbursztyka as a code owner July 17, 2023 17:48

zephyrbot added the area: SPI SPI bus label Jul 17, 2023

mjchen0 added 2 commits July 17, 2023 14:44

mjchen0 force-pushed the work branch from 67eb3a7 to fcf0b63 Compare July 17, 2023 21:44

mmahadevan108 approved these changes Jul 18, 2023

View reviewed changes

decsny approved these changes Jul 18, 2023

View reviewed changes

EmilioCBen approved these changes Jul 18, 2023

View reviewed changes

dleach02 merged commit 7839eb5 into zephyrproject-rtos:main Jul 19, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bugs in mcux i2s and dma #59832

fix bugs in mcux i2s and dma #59832

mjchen0 commented Jun 28, 2023 •

edited by mmahadevan108

Loading

dleach02 commented Jul 13, 2023

hakehuang commented Jul 14, 2023 •

edited

Loading

mmahadevan108 Jul 14, 2023

mjchen0 Jul 14, 2023

mmahadevan108 Jul 15, 2023

mjchen0 Jul 17, 2023

decsny Jul 18, 2023

mmahadevan108 Jul 17, 2023 •

edited

Loading

mjchen0 Jul 17, 2023

dbaluta Jul 17, 2023

mjchen0 Jul 17, 2023

mmahadevan108 commented Jul 17, 2023 •

edited

Loading

fix bugs in mcux i2s and dma #59832

fix bugs in mcux i2s and dma #59832

Conversation

mjchen0 commented Jun 28, 2023 • edited by mmahadevan108 Loading

dleach02 commented Jul 13, 2023

hakehuang commented Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmahadevan108 Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmahadevan108 commented Jul 17, 2023 • edited Loading

mjchen0 commented Jun 28, 2023 •

edited by mmahadevan108

Loading

hakehuang commented Jul 14, 2023 •

edited

Loading

mmahadevan108 Jul 17, 2023 •

edited

Loading

mmahadevan108 commented Jul 17, 2023 •

edited

Loading