Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dma: Scatter gather test #44041

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Conversation

teburd
Copy link
Collaborator

@teburd teburd commented Mar 21, 2022

Adds a scatter gather test against memory to memory transfers.

Based on #43984 so the updated designware driver can be built on both test cases in #43883

DesignWare DMA passes both tests, NXP's eDMA does not seem to pass the scatter gather test and its not clear as to why, maybe @hakehuang wouldn't mind taking a look?

Marked DNM until #43984 is merged

@teburd teburd requested a review from nashif as a code owner March 21, 2022 17:36
@teburd teburd requested a review from hakehuang March 21, 2022 17:36
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Mar 21, 2022
@teburd teburd added the DNM This PR should not be merged (Do Not Merge) label Mar 21, 2022
@danieldegrasse
Copy link
Collaborator

danieldegrasse commented Mar 21, 2022

@hakehuang the changes to fsl_edma in MCUXpresso SDK 2.11 (#43826) and the updates to the EDMA driver in #42750 enable this test to pass on an RT1060.

@dleach02 @DerekSnell FYI- These look like the same issues we've been seeing with the EDMA driver, although I didn't investigate too closely (just tried updating the HAL and our EDMA driver)

@teburd teburd force-pushed the dma_test_sg branch 2 times, most recently from 32dd04e to 0fba197 Compare March 21, 2022 22:47
@teburd
Copy link
Collaborator Author

teburd commented Mar 21, 2022

@hakehuang the changes to fsl_edma in MCUXpresso SDK 2.11 (#43826) and the updates to the EDMA driver in #42750 enable this test to pass on an RT1060.

@dleach02 @DerekSnell FYI- These look like the same issues we've been seeing with the EDMA driver, although I didn't investigate too closely (just tried updating the HAL and our EDMA driver)

Thanks for taking a look at this @danieldegrasse and confirming that there are fixes required for this to work on eDMA. I've pushed a change to this PR to disable it for all platforms initially, in the updated designware DMA PR I've enabled it for the appropriate intel adsp platforms. When the NXP changes are merged we can enable it there as well.

@teburd teburd self-assigned this Mar 21, 2022
@hakehuang
Copy link
Collaborator

@danieldegrasse, I port the hal fix to this PR, however it does not work at my side for rt1064 board, can you help to double check? maybe my port has problem, thanks in advance.

*** Booting Zephyr OS build zephyr-v3.0.0-1521-g0fba19714e60  ***
Running test suite dma_m2m_sg_test
===================================================================
START - test_dma_m2m_sg
DMA memory to memory transfer started on DMA_0
Preparing DMA Controller
dma block 0 block_size 64, source addr 80000000, dest addr 80000040
set next block pointer to 0x80000b8c
dma block 1 block_size 64, source addr 80000000, dest addr 80000080
Configuring the scatter-gather transfer on channel 0
Starting the transfer on channel 0 and waiting completion
callback status 1
I: channel 0 error status is 0x2

@danieldegrasse
Copy link
Collaborator

@hakehuang Did you pull in the updates to the EDMA driver in #42750? I saw errors like that when I only had updated the HAL.

@hakehuang
Copy link
Collaborator

hakehuang commented Mar 22, 2022

@hakehuang Did you pull in the updates to the EDMA driver in #42750? I saw errors like that when I only had updated the HAL.

Thanks @danieldegrasse , there is one key update at

#ifdef CONFIG_HAS_MCUX_CACHE

#ifdef CONFIG_DMA_MCUX_USE_DTCM_FOR_DMA_DESCRIPTORS

#if DT_NODE_HAS_STATUS(DT_CHOSEN(zephyr_dtcm), okay)
#define EDMA_TCDPOOL_CACHE_ATTR __dtcm_noinit_section
#else /* DT_NODE_HAS_STATUS(DT_CHOSEN(zephyr_dtcm), okay) */
#error Selected DTCM for MCUX DMA descriptors but no DTCM section.
#endif /* DT_NODE_HAS_STATUS(DT_CHOSEN(zephyr_dtcm), okay) */

#elif defined(CONFIG_NOCACHE_MEMORY)
#define EDMA_TCDPOOL_CACHE_ATTR __nocache
#else
#error tcdpool could not be located in cacheable memory, a requirement for proper EDMA operation.
#endif /* CONFIG_DMA_MCUX_USE_DTCM_FOR_DMA_DESCRIPTORS */

#else /* CONFIG_HAS_MCUX_CACHE */

#define EDMA_TCDPOOL_CACHE_ATTR

#endif /* CONFIG_HAS_MCUX_CACHE */

static __aligned(32) EDMA_TCDPOOL_CACHE_ATTR edma_tcd_t
tcdpool[DT_INST_PROP(0, dma_channels)][CONFIG_DMA_TCD_QUEUE_SIZE];

which fixes my issues here

hakehuang
hakehuang previously approved these changes Mar 23, 2022
Copy link
Collaborator

@hakehuang hakehuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need wait for #42750 merged. others looks OK for me and test pass on my rt series board

danieldegrasse
danieldegrasse previously approved these changes Mar 23, 2022
@laurenmurphyx64
Copy link
Contributor

laurenmurphyx64 commented Mar 23, 2022

I don't think it matters a whole lot, especially considering you just got 2 approvals on this and updating the PR would dismiss them and put you back in review purgatory, but the copyrights for the new files should be 2022 instead of 2021. You might consider fixing the copyright comments in another little PR after this one gets merged.

danieldegrasse
danieldegrasse previously approved these changes Mar 23, 2022
@hakehuang hakehuang self-requested a review March 24, 2022 03:03
@teburd teburd removed the DNM This PR should not be merged (Do Not Merge) label Mar 24, 2022
@teburd
Copy link
Collaborator Author

teburd commented Mar 24, 2022

@hakehuang please do take a look, should be good to merge now

Adds a scatter gather test against memory to memory transfers. Initially
excludes all platforms as they are all failing.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@nashif nashif closed this Mar 25, 2022
@nashif nashif reopened this Mar 25, 2022
dma_cfg.user_data = (struct device *)dma;
#else
dma_cfg.user_data = NULL;
#endif /* CONFIG_DMAMUX_STM32 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conditional really necessary ? I don't see the callback using the data passsed here. The driver should not do anything with the user pointer as well.

I may have missed something though.

dma_block_cfgs[i].source_gather_en = 1U;
dma_block_cfgs[i].block_size = XFER_SIZE;
dma_block_cfgs[i].source_address = (uint32_t)(tx_data);
dma_block_cfgs[i].dest_address = (uint32_t)(rx_data[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't these address be 64bit if CONFIG_DMA_64BIT is selected ? Btw, why don't we use uintptr_t for these address instead of having it conditionally set ? (not related with this particular patch, just a question)

Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, looks good to me.

@carlescufi carlescufi merged commit 2ec3222 into zephyrproject-rtos:main Mar 28, 2022
@teburd teburd deleted the dma_test_sg branch March 28, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants