Skip to content

Commit

Permalink
dma: dma_nxp_edma: refactor state transitioning
Browse files Browse the repository at this point in the history 
The channel state transitions are currently performed at the
beginning of each of the functions that triggers them
(e.g: edma_start(), edma_stop(), etc...).  The main issue with
this approach is the fact if there's any failures after the state
transition then the channel will be in the target state without
performing the required steps for it.

For instance, during edma_config(), if any of the functions after
the state transition (the channel_change_state() call) fails
(e.g: get_transfer_type()) fails then the state of the channel
will be CONFIGURED even if not all the required steps were performed
(e.g: setting the MUX, configuring the transfer, etc...).

To fix this, split the state transition into two steps:

	1) Check if the transition is possible.
	2) Do the transition.

First step should be done before any configurations to make sure
that we should be performing them in the first place, while the
second step should be performed after all configurations, thus
guaranteeing that all the required steps for the target state were
performed before transitioning to it.

Signed-off-by: Laurentiu Mihalcea <[email protected]>
  • Loading branch information
@LaurentiuM1234
LaurentiuM1234 committed Dec 11, 2024
1 parent 6bd3f96 commit 39cb688
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 36 deletions.
50 changes: 28 additions & 22 deletions drivers/dma/dma_nxp_edma.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,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 +335,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 +391,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 +404,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 +417,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 +442,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 Down Expand Up @@ -482,6 +485,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 +495,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,11 +506,11 @@ 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);
Expand All @@ -517,6 +521,8 @@ static int edma_start(const struct device *dev, uint32_t chan_id)
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
23 changes: 9 additions & 14 deletions drivers/dma/dma_nxp_edma.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,52 +221,47 @@ struct edma_config {
bool contiguous_channels;
};

static inline int channel_change_state(struct edma_channel *chan,
enum channel_state next)
static inline bool channel_allows_transition(struct edma_channel *chan,
enum channel_state next)
{
enum channel_state prev = chan->state;

LOG_DBG("attempting to change state from %d to %d for channel %d", prev, next, chan->id);

/* validate transition */
switch (prev) {
case CHAN_STATE_INIT:
if (next != CHAN_STATE_CONFIGURED) {
return -EPERM;
return false;
}
break;
case CHAN_STATE_CONFIGURED:
if (next != CHAN_STATE_STARTED &&
next != CHAN_STATE_CONFIGURED) {
return -EPERM;
return false;
}
break;
case CHAN_STATE_STARTED:
if (next != CHAN_STATE_STOPPED &&
next != CHAN_STATE_SUSPENDED) {
return -EPERM;
return false;
}
break;
case CHAN_STATE_STOPPED:
if (next != CHAN_STATE_CONFIGURED) {
return -EPERM;
return false;
}
break;
case CHAN_STATE_SUSPENDED:
if (next != CHAN_STATE_STARTED &&
next != CHAN_STATE_STOPPED) {
return -EPERM;
return false;
}
break;
default:
LOG_ERR("invalid channel previous state: %d", prev);
return -EINVAL;
return false;
}

/* transition OK, proceed */
chan->state = next;

return 0;
return true;
}

static inline int get_transfer_type(enum dma_channel_direction dir, uint32_t *type)
Expand Down

0 comments on commit 39cb688

Please sign in to comment.