Skip to content

mcux-sdk: edma: support memory offset for different master and correct ELINKNO #528

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yongxu-wang15
Copy link

No description provided.

@@ -8,6 +8,9 @@
#include <stdbool.h>

#include "fsl_edma_rev2.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a short description for this commit explaining what is the issue this is trying to tackle?

Copy link
Author

Choose a reason for hiding this comment

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

sure, have updated

@yongxu-wang15 yongxu-wang15 force-pushed the mcu-sdk-support-memory-offest-for-edma-rev2-driver branch 2 times, most recently from 91d4818 to 8e2196d Compare April 17, 2025 07:43
@dbaluta
Copy link
Collaborator

dbaluta commented Apr 17, 2025

I'm fine with this but keep in mind to separate patches per logical change. Once you feel the need to add a list of things that this commit is doing that's a clear sign that you need to split the PR into multiple patches.

I won't insist on this one because it looks pretty simple and straightforward.

Copy link
Collaborator

@LaurentiuM1234 LaurentiuM1234 left a comment

Choose a reason for hiding this comment

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

very good catches, thank you! can you please add a commit description detailing the address translation issue? I think it's extremely important to have this logged for future reference.

@@ -74,7 +74,7 @@ extern "C" {
#define EDMA_TCD_ATTR_DSIZE(x) ((x) & EDMA_TCD_ATTR_SSIZE_DSIZE_MASK)
#define EDMA_TCD_ATTR_SSIZE(x) (((x) & EDMA_TCD_ATTR_SSIZE_DSIZE_MASK) << 8)

#define EDMA_TCD_CITER_ELINKNO_MASK 0xf
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch, thx! mind also doing this change for EDMA_TCD_BITER_ELINKNO_MASK?

Copy link
Author

Choose a reason for hiding this comment

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

sure, have updated

@@ -8,6 +8,9 @@
#include <stdbool.h>

#include "fsl_edma_rev2.h"
#if defined FSL_FEATURE_MEMORY_HAS_ADDRESS_OFFSET && FSL_FEATURE_MEMORY_HAS_ADDRESS_OFFSET
#include "fsl_memory.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

would you mind making this e separate commit with a more detailed description? I'd be interested in learning more about this.

is this related to the fact that DTCM is mapped at different addresses for M7 and EDMA and thus we require a translation? if so, please mention this in the commit description.

Copy link
Author

Choose a reason for hiding this comment

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

yes, for examples In imx95, DTCM has different memory maps for M33 or M7core, which are defined in modules/hal/nxp/mcux/mcux-sdk/devices/MIMX9596, and other devices.
image

@@ -237,6 +240,11 @@ status_t EDMA_ConfigureTransfer(edma_config_t *cfg, int channel,
break;
}

#if defined FSL_FEATURE_MEMORY_HAS_ADDRESS_OFFSET && FSL_FEATURE_MEMORY_HAS_ADDRESS_OFFSET
saddr = MEMORY_ConvertMemoryMapAddress((uint32_t)(saddr), kMEMORY_Local2DMA);
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be useful to add a comment here stating that translation will only be performed if saddr/daddr are from DTCM

@dleach02
Copy link
Member

Can you clarify which platforms are impacted by this? The changes are made to the legacy driver side of things and not SDK-NG

Also, there isn't a Zephyr side PR so how or when do you need this integrated?

- This api convert the address between system mapped address
  and native mapped address.
- There maybe offset between subsystem native address
  and system address for some memory (such as iMX95 DTCM)

Signed-off-by: Yongxu Wang <yongxu.wang@nxp.com>
Signed-off-by: Yongxu Wang <yongxu.wang@nxp.com>
Signed-off-by: Yongxu Wang <yongxu.wang@nxp.com>
@yongxu-wang15 yongxu-wang15 force-pushed the mcu-sdk-support-memory-offest-for-edma-rev2-driver branch from 8e2196d to 57e9a25 Compare April 22, 2025 03:19
@yongxu-wang15
Copy link
Author

hi @dleach02 , this patch is for dma_nxp_edma driver, and mainly to support m2m test case for imx95 board, the another pull request is zephyrproject-rtos/zephyr#88748.
and I want to merge this pr after the another zephyr patch can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants