From 5b1d2ad5a84b2e440a767b4309e01aae872d3fe5 Mon Sep 17 00:00:00 2001 From: "Mike J. Chen" Date: Tue, 27 Jun 2023 18:34:35 -0700 Subject: [PATCH] drivers: i2s: mcux_flexcomm: fix multiple bugs Fix for bugs described in: https://github.com/zephyrproject-rtos/zephyr/issues/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 --- drivers/i2s/i2s_mcux_flexcomm.c | 125 ++++++++++++++++++++------------ 1 file changed, 78 insertions(+), 47 deletions(-) diff --git a/drivers/i2s/i2s_mcux_flexcomm.c b/drivers/i2s/i2s_mcux_flexcomm.c index 7550dd7237ffb43..c48d6565f9d9e15 100644 --- a/drivers/i2s/i2s_mcux_flexcomm.c +++ b/drivers/i2s/i2s_mcux_flexcomm.c @@ -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 { @@ -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, @@ -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)); @@ -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); @@ -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); } } @@ -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; @@ -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); @@ -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); } @@ -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); } @@ -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; @@ -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; @@ -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) { @@ -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"); @@ -778,7 +808,10 @@ 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) { @@ -786,7 +819,7 @@ static int i2s_mcux_write(const struct device *dev, void *mem_block, 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) { @@ -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) { @@ -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, \ } \ }, \ @@ -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, \ } \ }