Skip to content

Commit

Permalink
drivers: i2s: mcux_flexcomm: fix multiple bugs
Browse files Browse the repository at this point in the history 
Fix for bugs described in:
#59803

1. the size argument passed to i2s_write() was being ignored.
   change the code so that the size is queued with the
   tx mem_block and the dma transfer is configured with this
   size.

2. change how CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT and
   CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT are used so that
   the queue buffers are allocated correctly when the two
   config values are not the same

3. set source_data_size and dest_data_size to be the same
   since the DMA controller can only set one size per
   DMA transfer. the driver was already computing a dest_data_size
   but always passing 1 for the source_data_size.
   For I2S RX case, I think source_data_size should be
   set to the expected FIFO read size instead of dest_data_size.

Also some smaller improvements like:
* don't allocate two dma_blocks for tx in the static dev_mem
  when it only needs one
* memset both rx_dma_blocks together instead of separtely
* set dma_cfg block_count for tx and rx statically instead
  of at runtime

Signed-off-by: Mike J. Chen <[email protected]>
  • Loading branch information
@mjchen0
mjchen0 committed Jul 11, 2023
1 parent 7fba7d3 commit 5b1d2ad
Showing 1 changed file with 78 additions and 47 deletions.
125 changes: 78 additions & 47 deletions drivers/i2s/i2s_mcux_flexcomm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

LOG_MODULE_REGISTER(i2s_mcux_flexcomm);

#define NUM_DMA_BLOCKS 2
#define NUM_RX_DMA_BLOCKS 2

/* Device constant configuration parameters */
struct i2s_mcux_config {
Expand All @@ -36,17 +36,32 @@ struct stream {
uint32_t channel; /* stores the channel for dma */
struct i2s_config cfg;
struct dma_config dma_cfg;
struct dma_block_config dma_block[NUM_DMA_BLOCKS];
bool last_block;
struct k_msgq in_queue;
void *in_msgs[CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT];
struct k_msgq out_queue;
void *out_msgs[CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT];
};

struct i2s_txq_entry {
void *mem_block;
size_t size;
};

struct i2s_mcux_data {
struct stream rx;
void *rx_in_msgs[CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT];
void *rx_out_msgs[CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT];
struct dma_block_config rx_dma_blocks[NUM_RX_DMA_BLOCKS];

struct stream tx;
/* For tx, the in queue is for requests generated by
* the i2s_write() API call, and size must be tracked
* separate from the buffer size.
* The out_queue is for tracking buffers that should
* be freed once the DMA is done transferring it.
*/
struct i2s_txq_entry tx_in_msgs[CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT];
void *tx_out_msgs[CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT];
struct dma_block_config tx_dma_block;
};

static int i2s_mcux_flexcomm_cfg_convert(uint32_t base_frequency,
Expand Down Expand Up @@ -266,6 +281,7 @@ static int i2s_mcux_configure(const struct device *dev, enum i2s_dir dir,
}

stream->dma_cfg.dest_data_size = bytes_per_word;
stream->dma_cfg.source_data_size = bytes_per_word;

/* Save configuration for get_config */
memcpy(&stream->cfg, i2s_cfg, sizeof(struct i2s_config));
Expand All @@ -275,12 +291,21 @@ static int i2s_mcux_configure(const struct device *dev, enum i2s_dir dir,
}

static inline void i2s_purge_stream_buffers(struct stream *stream,
struct k_mem_slab *mem_slab)
struct k_mem_slab *mem_slab,
bool tx)
{
void *buffer;

while (k_msgq_get(&stream->in_queue, &buffer, K_NO_WAIT) == 0) {
k_mem_slab_free(mem_slab, &buffer);
if (tx) {
struct i2s_txq_entry queue_entry;

while (k_msgq_get(&stream->in_queue, &queue_entry, K_NO_WAIT) == 0) {
k_mem_slab_free(mem_slab, &queue_entry.mem_block);
}
} else {
while (k_msgq_get(&stream->in_queue, &buffer, K_NO_WAIT) == 0) {
k_mem_slab_free(mem_slab, &buffer);
}
}
while (k_msgq_get(&stream->out_queue, &buffer, K_NO_WAIT) == 0) {
k_mem_slab_free(mem_slab, &buffer);
Expand Down Expand Up @@ -324,7 +349,7 @@ static void i2s_mcux_tx_stream_disable(const struct device *dev, bool drop)

/* purge buffers queued in the stream */
if (drop) {
i2s_purge_stream_buffers(stream, stream->cfg.mem_slab);
i2s_purge_stream_buffers(stream, stream->cfg.mem_slab, true);
}
}

Expand All @@ -351,12 +376,13 @@ static void i2s_mcux_rx_stream_disable(const struct device *dev, bool drop)

/* purge buffers queued in the stream */
if (drop) {
i2s_purge_stream_buffers(stream, stream->cfg.mem_slab);
i2s_purge_stream_buffers(stream, stream->cfg.mem_slab, false);
}
}

static void i2s_mcux_config_dma_blocks(const struct device *dev,
enum i2s_dir dir, uint32_t *buffer)
enum i2s_dir dir, uint32_t *buffer,
size_t block_size)
{
const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data;
Expand All @@ -366,36 +392,34 @@ static void i2s_mcux_config_dma_blocks(const struct device *dev,

if (dir == I2S_DIR_RX) {
stream = &dev_data->rx;
blk_cfg = &dev_data->rx_dma_blocks[0];
memset(blk_cfg, 0, sizeof(dev_data->rx_dma_blocks));
} else {
stream = &dev_data->tx;
blk_cfg = &dev_data->tx_dma_block;
memset(blk_cfg, 0, sizeof(dev_data->tx_dma_block));
}

blk_cfg = &stream->dma_block[0];
memset(blk_cfg, 0, sizeof(struct dma_block_config));
stream->dma_cfg.head_block = blk_cfg;

if (dir == I2S_DIR_RX) {

blk_cfg->source_address = (uint32_t)&base->FIFORD;
blk_cfg->dest_address = (uint32_t)buffer[0];
blk_cfg->block_size = stream->cfg.block_size;
blk_cfg->next_block = &stream->dma_block[1];
blk_cfg->block_size = block_size;
blk_cfg->next_block = &dev_data->rx_dma_blocks[1];
blk_cfg->dest_reload_en = 1;

blk_cfg = &stream->dma_block[1];
memset(blk_cfg, 0, sizeof(struct dma_block_config));

blk_cfg = &dev_data->rx_dma_blocks[1];
blk_cfg->source_address = (uint32_t)&base->FIFORD;
blk_cfg->dest_address = (uint32_t)buffer[1];
blk_cfg->block_size = stream->cfg.block_size;

stream->dma_cfg.block_count = NUM_DMA_BLOCKS;
blk_cfg->block_size = block_size;
} else {
blk_cfg->dest_address = (uint32_t)&base->FIFOWR;
blk_cfg->source_address = (uint32_t)buffer;
blk_cfg->block_size = stream->cfg.block_size;
stream->dma_cfg.block_count = 1;
blk_cfg->block_size = block_size;
}

stream->dma_cfg.head_block = &stream->dma_block[0];
stream->dma_cfg.user_data = (void *)dev;

dma_config(stream->dev_dma, stream->channel, &stream->dma_cfg);
Expand Down Expand Up @@ -425,14 +449,15 @@ static void i2s_mcux_dma_tx_callback(const struct device *dma_dev, void *arg,
const struct device *dev = (const struct device *)arg;
struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx;
void *buffer;
struct i2s_txq_entry queue_entry;
int ret;

LOG_DBG("tx cb: %d", stream->state);
ret = k_msgq_get(&stream->out_queue, &buffer, K_NO_WAIT);

ret = k_msgq_get(&stream->out_queue, &queue_entry.mem_block, K_NO_WAIT);
if (ret == 0) {
/* transmission complete. free the buffer */
k_mem_slab_free(stream->cfg.mem_slab, &buffer);
k_mem_slab_free(stream->cfg.mem_slab, &queue_entry.mem_block);
} else {
LOG_ERR("no buffer in output queue for channel %u", channel);
}
Expand All @@ -449,11 +474,13 @@ static void i2s_mcux_dma_tx_callback(const struct device *dma_dev, void *arg,
case I2S_STATE_RUNNING:
case I2S_STATE_STOPPING:
/* get the next buffer from queue */
ret = k_msgq_get(&stream->in_queue, &buffer, K_NO_WAIT);
ret = k_msgq_get(&stream->in_queue, &queue_entry, K_NO_WAIT);
if (ret == 0) {
/* config the DMA */
i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX, (uint32_t *)buffer);
k_msgq_put(&stream->out_queue, &buffer, K_NO_WAIT);
i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX,
(uint32_t *)queue_entry.mem_block,
queue_entry.size);
k_msgq_put(&stream->out_queue, &queue_entry.mem_block, K_NO_WAIT);
dma_start(stream->dev_dma, stream->channel);
}

Expand Down Expand Up @@ -549,23 +576,25 @@ static void i2s_mcux_dma_rx_callback(const struct device *dma_dev, void *arg,
static int i2s_mcux_tx_stream_start(const struct device *dev)
{
int ret = 0;
void *buffer;
const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx;
I2S_Type *base = cfg->base;
struct i2s_txq_entry queue_entry;

/* retrieve buffer from input queue */
ret = k_msgq_get(&stream->in_queue, &buffer, K_NO_WAIT);
ret = k_msgq_get(&stream->in_queue, &queue_entry, K_NO_WAIT);
if (ret != 0) {
LOG_ERR("No buffer in input queue to start transmission");
return ret;
}

i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX, (uint32_t *)buffer);
i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX,
(uint32_t *)queue_entry.mem_block,
queue_entry.size);

/* put buffer in output queue */
ret = k_msgq_put(&stream->out_queue, &buffer, K_NO_WAIT);
ret = k_msgq_put(&stream->out_queue, &queue_entry.mem_block, K_NO_WAIT);
if (ret != 0) {
LOG_ERR("failed to put buffer in output queue");
return ret;
Expand All @@ -589,7 +618,7 @@ static int i2s_mcux_tx_stream_start(const struct device *dev)
static int i2s_mcux_rx_stream_start(const struct device *dev)
{
int ret = 0;
void *buffer[NUM_DMA_BLOCKS];
void *buffer[NUM_RX_DMA_BLOCKS];
const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->rx;
Expand All @@ -606,7 +635,7 @@ static int i2s_mcux_rx_stream_start(const struct device *dev)
return -EINVAL;
}

for (int i = 0; i < NUM_DMA_BLOCKS; i++) {
for (int i = 0; i < NUM_RX_DMA_BLOCKS; i++) {
ret = k_mem_slab_alloc(stream->cfg.mem_slab, &buffer[i],
K_NO_WAIT);
if (ret != 0) {
Expand All @@ -615,10 +644,11 @@ static int i2s_mcux_rx_stream_start(const struct device *dev)
}
}

i2s_mcux_config_dma_blocks(dev, I2S_DIR_RX, (uint32_t *)buffer);
i2s_mcux_config_dma_blocks(dev, I2S_DIR_RX, (uint32_t *)buffer,
stream->cfg.block_size);

/* put buffers in input queue */
for (int i = 0; i < NUM_DMA_BLOCKS; i++) {
for (int i = 0; i < NUM_RX_DMA_BLOCKS; i++) {
ret = k_msgq_put(&stream->in_queue, &buffer[i], K_NO_WAIT);
if (ret != 0) {
LOG_ERR("failed to put buffer in input queue");
Expand Down Expand Up @@ -778,15 +808,18 @@ static int i2s_mcux_write(const struct device *dev, void *mem_block,
struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx;
int ret;

struct i2s_txq_entry queue_entry = {
.mem_block = mem_block,
.size = size,
};

if (stream->state != I2S_STATE_RUNNING &&
stream->state != I2S_STATE_READY) {
LOG_ERR("invalid state (%d)", stream->state);
return -EIO;
}

ret = k_msgq_put(&stream->in_queue, &mem_block,
ret = k_msgq_put(&stream->in_queue, &queue_entry,
SYS_TIMEOUT_MS(stream->cfg.timeout));

if (ret) {
Expand Down Expand Up @@ -842,13 +875,13 @@ static int i2s_mcux_init(const struct device *dev)
cfg->irq_config(dev);

/* Initialize the buffer queues */
k_msgq_init(&data->tx.in_queue, (char *)data->tx.in_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT);
k_msgq_init(&data->rx.in_queue, (char *)data->rx.in_msgs,
k_msgq_init(&data->tx.in_queue, (char *)data->tx_in_msgs,
sizeof(struct i2s_txq_entry), CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT);
k_msgq_init(&data->rx.in_queue, (char *)data->rx_in_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT);
k_msgq_init(&data->tx.out_queue, (char *)data->tx.out_msgs,
k_msgq_init(&data->tx.out_queue, (char *)data->tx_out_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT);
k_msgq_init(&data->rx.out_queue, (char *)data->rx.out_msgs,
k_msgq_init(&data->rx.out_queue, (char *)data->rx_out_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT);

if (data->tx.dev_dma != NULL) {
Expand Down Expand Up @@ -884,7 +917,6 @@ static int i2s_mcux_init(const struct device *dev)
.dma_cfg = { \
.channel_direction = MEMORY_TO_PERIPHERAL, \
.dma_callback = i2s_mcux_dma_tx_callback, \
.source_data_size = 1, \
.block_count = 1, \
} \
}, \
Expand All @@ -898,8 +930,7 @@ static int i2s_mcux_init(const struct device *dev)
.dma_cfg = { \
.channel_direction = PERIPHERAL_TO_MEMORY, \
.dma_callback = i2s_mcux_dma_rx_callback, \
.source_data_size = 1, \
.block_count = 1, \
.block_count = NUM_RX_DMA_BLOCKS, \
} \
}

Expand Down

0 comments on commit 5b1d2ad

Please sign in to comment.