Skip to content

Conversation

Dat-NguyenDuy
Copy link
Contributor

@Dat-NguyenDuy Dat-NguyenDuy commented Aug 9, 2023

This extends NXP current eDMA shim driver for supporting eDMA v3 on S32K344

@zephyrbot
Copy link

zephyrbot commented Aug 9, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@8cc344e zephyrproject-rtos/hal_nxp@6d91c17 (master) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Aug 9, 2023
@mmahadevan108 mmahadevan108 requested review from hakehuang and removed request for decsny August 15, 2023 16:06
@mmahadevan108
Copy link
Contributor

@Dat-NguyenDuy, please take a look at the CI failures.

#define DEV_DMAMUX_BASE(dev, idx) ((DMAMUX_Type *)DEV_CFG(dev)->dmamux_base[idx])
#define DEV_DMAMUX_IDX(dev, ch) (ch / DEV_CFG(dev)->channels_per_mux)

#ifdef CONFIG_SOC_SERIES_S32K3_M7
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this code needs to be SOC dependent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is SoC dependency, i'm considering it's worth to add a dt properties for it

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add a binding property to remove the SoC dependency in the driver, e.g.

#define DEV_DMAMUX_CHANNEL(dev, ch) ((ch % DEV_CFG(dev)->channels_per_mux) ^ (DEV_CFG(dev)->mux_channel_mask))

and use a default of 0 which would be the logical way to access the DMA CHCFGn registers. The SoCs that have a different arrangement can change the mux-channel-mask (feel free to suggest a better name) as needed, like in the case of K344 to 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated

IRQ_CONFIG(n, 16, dma_mcux_edma_error_irq_handler); \
\
LOG_DBG("install irq done"); \
#ifdef CONFIG_DMA_MCUX_SINGLE_IRQ_PER_CHANNEL
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this macro will install an interrupt handler per each channel of the EDMA. Existing NXP SOCs using the legacy EDMA also have a per channel interrupt, but have a combined error interrupt. Would it be possible to enable per channel interrupts for all EDMA instances, and only enable the error interrupt servicing in the channel interrupt when this Kconfig was set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, it's possible, i just re-pushed the update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danieldegrasse could you pls revisit if this one looks fine to you

@hakehuang
Copy link
Contributor

I am trigger CI for all NXP related EDMA boards, will feadback once ready

regression test done. no issues found.

manuargue
manuargue previously approved these changes Sep 26, 2023
@Dat-NguyenDuy
Copy link
Contributor Author

Dat-NguyenDuy commented Sep 26, 2023

Minor update #ifdef DMA_MCUX_HAS_CHANNEL_GAP --> #if DMA_MCUX_HAS_CHANNEL_GAP

hakehuang
hakehuang previously approved these changes Sep 26, 2023
Copy link
Contributor

@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.

Test PASS on NXP regression for RT series

manuargue
manuargue previously approved these changes Sep 26, 2023
@mmahadevan108
Copy link
Contributor

@Dat-NguyenDuy , thank you for the PR. Overall this looks great. My request would be not leave the current EDMA driver Kconfigs and device tree binding names unchanged to maintain backwards compatibility.

Copy link
Contributor

@mmahadevan108 mmahadevan108 left a comment

Choose a reason for hiding this comment

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

Request to leave the EDMA Kconfigs and device tree binding names unchanged.

@manuargue
Copy link
Member

Request to leave the EDMA Kconfigs and device tree binding names unchanged.

@mmahadevan108 I'm a bit puzzled, I had the impression we agreed naming existing binding v2 and the new one v3 to follow the corresponding DMA versions (#61311 (comment)). We should add this to release notes to make sure the changes are visible to users.

Add new dt binding for edma v3 that inherits whole dt
properties from current version. One more property is
added for SoCs that don't have separate error interrupt
id, use same id with channel interrupt

Signed-off-by: Dat Nguyen Duy <[email protected]>
On S32K344, the offset in memory map between each channel
is 0x4000 for most channels, but there is specific case is
between channel 11 and 12 which is 0x1D4000 instead. As a
consequence, 32 channels are divided to two parts: one
starts from channel 0 -> 11. The other is from channel 128
to 145. The channel gap is from 12 -> 127.

For user and data structures in shim driver, the channel's
value comes from 0 --> 31. Above constraint will be counted
when interact with the mcux sdk

Beside that, the DMAMUX register in this platform is very
specific, not in identical with DMAMUX channel, so shim
driver is updated to cover this case

Signed-off-by: Dat Nguyen Duy <[email protected]>
@Dat-NguyenDuy Dat-NguyenDuy dismissed stale reviews from manuargue and hakehuang via dc50609 September 27, 2023 03:37
@Dat-NguyenDuy
Copy link
Contributor Author

Request to leave the EDMA Kconfigs and device tree binding names unchanged.

I re-pushed to leave current EDMA Kconfig and dts binding

@mmahadevan108
Copy link
Contributor

Please update west.yml

@mmahadevan108 mmahadevan108 dismissed their stale review September 27, 2023 13:55

Changes made

Enable dma support for mr_canhubk3 board

Signed-off-by: Dat Nguyen Duy <[email protected]>
Add devicetree overlay for enabling dma test for mr_canhubk3 board

Signed-off-by: Dat Nguyen Duy <[email protected]>
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Sep 27, 2023
@Dat-NguyenDuy
Copy link
Contributor Author

Please update west.yml
I've updated

@mmahadevan108
Copy link
Contributor

Request to leave the EDMA Kconfigs and device tree binding names unchanged.

@mmahadevan108 I'm a bit puzzled, I had the impression we agreed naming existing binding v2 and the new one v3 to follow the corresponding DMA versions (#61311 (comment)). We should add this to release notes to make sure the changes are visible to users.

@manuargue , the general guidance from the Zephyr project is to limit breaking backward compatibility as much as possible. In this case, we are sticking with the SDMA naming convention for the driver folders.

@hakehuang
Copy link
Contributor

regression test is done. no issue found in rt platforms with this change

@hakehuang hakehuang self-requested a review September 27, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: DMA Direct Memory Access area: SPI SPI bus area: UART Universal Asynchronous Receiver-Transmitter manifest manifest-hal_nxp platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants