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.

Sign up for GitHub

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: dma_nxp_edma: fix for dynamic IRQ handling and support for per-channel PDs #82431

Merged
merged 8 commits into from
Dec 13, 2024
+376 −142
Merged

dma: dma_nxp_edma: fix for dynamic IRQ handling and support for per-channel PDs #82431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a48789
manifest: bump up hal_nxp revision
LaurentiuM1234 Dec 2, 2024
80f4d20
dma: dma_nxp_edma: refactor state transitioning
LaurentiuM1234 Nov 22, 2024
a231d87
dma: dma_nxp_edma: perform IRQ enable/disable on channel request/release
LaurentiuM1234 Nov 22, 2024
1172a3e
dma: dma_nxp_edma: make sure channel is inactive when releasing
LaurentiuM1234 Dec 2, 2024
5599511
dma: dma_nxp_edma: add support for managing per-channel PDs
LaurentiuM1234 Nov 22, 2024
4006c15
dts: xtensa: add imx8qm and imx8qxp DTSI variants
LaurentiuM1234 Oct 25, 2024
6f7204a
dts: xtensa: nxp_imx8: move up the definition of system-controller
LaurentiuM1234 Oct 25, 2024
67a0c66
dts: xtensa: nxp_imx8: add edma power domains
LaurentiuM1234 Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion boards/nxp/imx8qm_mek/imx8qm_mek_mimx8qm6_adsp.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

/dts-v1/;

#include <nxp/nxp_imx8.dtsi>
#include <nxp/nxp_imx8qm.dtsi>
#include "imx8qm_mek_mimx8qm6_adsp-pinctrl.dtsi"

/ {
Expand Down
6 changes: 1 addition & 5 deletions boards/nxp/imx8qxp_mek/imx8qxp_mek_mimx8qx6_adsp.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

/dts-v1/;

#include <nxp/nxp_imx8.dtsi>
#include <nxp/nxp_imx8qxp.dtsi>
#include "imx8qxp_mek_mimx8qx6_adsp-pinctrl.dtsi"

/ {
Expand All @@ -31,7 +31,3 @@
pinctrl-0 = <&sai1_default>;
pinctrl-names = "default";
};

&irqsteer {
reg = <0x51080000 DT_SIZE_K(64)>;
};
134 changes: 102 additions & 32 deletions drivers/dma/dma_nxp_edma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "dma_nxp_edma.h"

#define EDMA_ACTIVE_TIMEOUT 50

/* TODO list:
* 1) Support for requesting a specific channel.
* 2) Support for checking if DMA transfer is pending when attempting config. (?)
Expand All @@ -28,6 +30,11 @@ static void edma_isr(const void *parameter)
cfg = chan->dev->config;
data = chan->dev->data;

if (chan->state == CHAN_STATE_RELEASING || chan->state == CHAN_STATE_INIT) {
/* skip, not safe to access channel register space */
return;
}

if (!EDMA_ChannelRegRead(data->hal_cfg, chan->id, EDMA_TCD_CH_INT)) {
/* skip, interrupt was probably triggered by another channel */
return;
Expand Down Expand Up @@ -270,11 +277,11 @@ static int edma_config(const struct device *dev, uint32_t chan_id,
chan->cyclic_buffer = false;
}

/* change channel's state to CONFIGURED */
ret = channel_change_state(chan, CHAN_STATE_CONFIGURED);
if (ret < 0) {
LOG_ERR("failed to change channel %d state to CONFIGURED", chan_id);
return ret;
/* check if transition to CONFIGURED is allowed */
if (!channel_allows_transition(chan, CHAN_STATE_CONFIGURED)) {
LOG_ERR("chan %d transition from %d to CONFIGURED not allowed",
chan_id, chan->state);
return -EPERM;
}

ret = get_transfer_type(dma_cfg->channel_direction, &transfer_type);
Expand Down Expand Up @@ -335,6 +342,8 @@ static int edma_config(const struct device *dev, uint32_t chan_id,
/* dump register status - for debugging purposes */
edma_dump_channel_registers(data, chan_id);

chan->state = CHAN_STATE_CONFIGURED;

return 0;
}

Expand Down Expand Up @@ -389,7 +398,6 @@ static int edma_suspend(const struct device *dev, uint32_t chan_id)
struct edma_data *data;
const struct edma_config *cfg;
struct edma_channel *chan;
int ret;

data = dev->data;
cfg = dev->config;
Expand All @@ -403,11 +411,11 @@ static int edma_suspend(const struct device *dev, uint32_t chan_id)

edma_dump_channel_registers(data, chan_id);

/* change channel's state to SUSPENDED */
ret = channel_change_state(chan, CHAN_STATE_SUSPENDED);
if (ret < 0) {
LOG_ERR("failed to change channel %d state to SUSPENDED", chan_id);
return ret;
/* check if transition to SUSPENDED is allowed */
if (!channel_allows_transition(chan, CHAN_STATE_SUSPENDED)) {
LOG_ERR("chan %d transition from %d to SUSPENDED not allowed",
chan_id, chan->state);
return -EPERM;
}

LOG_DBG("suspending channel %u", chan_id);
Expand All @@ -416,6 +424,8 @@ static int edma_suspend(const struct device *dev, uint32_t chan_id)
EDMA_ChannelRegUpdate(data->hal_cfg, chan_id,
EDMA_TCD_CH_CSR, 0, EDMA_TCD_CH_CSR_ERQ_MASK);

chan->state = CHAN_STATE_SUSPENDED;

return 0;
}

Expand All @@ -439,11 +449,11 @@ static int edma_stop(const struct device *dev, uint32_t chan_id)

prev_state = chan->state;

/* change channel's state to STOPPED */
ret = channel_change_state(chan, CHAN_STATE_STOPPED);
if (ret < 0) {
LOG_ERR("failed to change channel %d state to STOPPED", chan_id);
return ret;
/* check if transition to STOPPED is allowed */
if (!channel_allows_transition(chan, CHAN_STATE_STOPPED)) {
LOG_ERR("chan %d transition from %d to STOPPED not allowed",
chan_id, chan->state);
return -EPERM;
}

LOG_DBG("stopping channel %u", chan_id);
Expand All @@ -459,11 +469,8 @@ static int edma_stop(const struct device *dev, uint32_t chan_id)
/* disable HW requests */
EDMA_ChannelRegUpdate(data->hal_cfg, chan_id, EDMA_TCD_CH_CSR, 0,
EDMA_TCD_CH_CSR_ERQ_MASK);

out_release_channel:

irq_disable(chan->irq);

/* clear the channel MUX so that it can used by a different peripheral.
*
* note: because the channel is released during dma_stop() that means
Expand All @@ -482,6 +489,8 @@ static int edma_stop(const struct device *dev, uint32_t chan_id)

edma_dump_channel_registers(data, chan_id);

chan->state = CHAN_STATE_STOPPED;

return 0;
}

Expand All @@ -490,7 +499,6 @@ static int edma_start(const struct device *dev, uint32_t chan_id)
struct edma_data *data;
const struct edma_config *cfg;
struct edma_channel *chan;
int ret;

data = dev->data;
cfg = dev->config;
Expand All @@ -502,21 +510,21 @@ static int edma_start(const struct device *dev, uint32_t chan_id)
return -EINVAL;
}

/* change channel's state to STARTED */
ret = channel_change_state(chan, CHAN_STATE_STARTED);
if (ret < 0) {
LOG_ERR("failed to change channel %d state to STARTED", chan_id);
return ret;
/* check if transition to STARTED is allowed */
if (!channel_allows_transition(chan, CHAN_STATE_STARTED)) {
LOG_ERR("chan %d transition from %d to STARTED not allowed",
chan_id, chan->state);
return -EPERM;
}

LOG_DBG("starting channel %u", chan_id);

irq_enable(chan->irq);

/* enable HW requests */
EDMA_ChannelRegUpdate(data->hal_cfg, chan_id,
EDMA_TCD_CH_CSR, EDMA_TCD_CH_CSR_ERQ_MASK, 0);

chan->state = CHAN_STATE_STARTED;

return 0;
}

Expand Down Expand Up @@ -577,19 +585,80 @@ static int edma_get_attribute(const struct device *dev, uint32_t type, uint32_t

static bool edma_channel_filter(const struct device *dev, int chan_id, void *param)
{
int *requested_channel;
struct edma_channel *chan;
int ret;

if (!param) {
return false;
}

requested_channel = param;
if (*(int *)param != chan_id) {
return false;
}

chan = lookup_channel(dev, chan_id);
if (!chan) {
return false;
}

if (*requested_channel == chan_id && lookup_channel(dev, chan_id)) {
return true;
if (chan->pd_dev) {
ret = pm_device_runtime_get(chan->pd_dev);
if (ret < 0) {
LOG_ERR("failed to PM get channel %d PD dev: %d",
chan_id, ret);
return false;
}
}

irq_enable(chan->irq);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You add the irq_enable() on dma_chan_filter() - which is a different function than dma_request_channel() as mentioned in the commit message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, the commit message is supposed to refer to dma_request_channel and dma_release_channel. The filter function is called as part of dma_request_channel.


return true;
}

static void edma_channel_release(const struct device *dev, uint32_t chan_id)
{
struct edma_channel *chan;
struct edma_data *data;
int ret;

chan = lookup_channel(dev, chan_id);
if (!chan) {
return;
}

data = dev->data;

if (!channel_allows_transition(chan, CHAN_STATE_RELEASING)) {
LOG_ERR("chan %d transition from %d to RELEASING not allowed",
chan_id, chan->state);
return;
}

/* channel needs to be INACTIVE before transitioning */
if (!WAIT_FOR(!EDMA_CHAN_IS_ACTIVE(data, chan),
EDMA_ACTIVE_TIMEOUT, k_busy_wait(1))) {
LOG_ERR("timed out while waiting for chan %d to become inactive",
chan->id);
return;
}

/* start the process of disabling IRQ and PD */
chan->state = CHAN_STATE_RELEASING;

#ifdef CONFIG_NXP_IRQSTEER
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you disable the irq only for CONFIG_NXP_IRQSTEER, in other cases what happens?
Since, above, in edma_channel_filter() you enable the irq for "all".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is somewhat the way we used to do things. We used to call irq_enable() in dma_config(), resulting in irq_enable being called multiple times with no irq_disable() so the approach should be fine. There's no reference counting for non-irqstr-based SoCs.

irq_disable(chan->irq);
#endif /* CONFIG_NXP_IRQSTEER */

if (chan->pd_dev) {
ret = pm_device_runtime_put(chan->pd_dev);
if (ret < 0) {
LOG_ERR("failed to PM put channel %d PD dev: %d",
chan_id, ret);
}
}

return false;
/* done, proceed with next state */
chan->state = CHAN_STATE_INIT;
}

static const struct dma_driver_api edma_api = {
Expand All @@ -602,6 +671,7 @@ static const struct dma_driver_api edma_api = {
.get_status = edma_get_status,
.get_attribute = edma_get_attribute,
.chan_filter = edma_channel_filter,
.chan_release = edma_channel_release,
};

static edma_config_t *edma_hal_cfg_get(const struct edma_config *cfg)
Expand Down
Loading
Loading