michaelh/zephyr
1
0
Fork 0
Code Releases Activity

drivers: i2s: mcux_flexcomm: fix multiple bugs

Browse source 
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 <mjchen@google.com>
This commit is contained in:
Mike J. Chen 2023-06-27 18:34:35 -07:00 • committed by David Leach
commit f882d31ea7
1 changed files with 78 additions and 47 deletions
Download patch file Download diff file Expand all files Collapse all files

125
drivers/i2s/i2s_mcux_flexcomm.c
View file

@ -19,7 +19,7 @@
LOG_MODULE_REGISTER(i2s_mcux_flexcomm); LOG_MODULE_REGISTER(i2s_mcux_flexcomm);
#define NUM_DMA_BLOCKS 2 #define NUM_RX_DMA_BLOCKS 2
/* Device constant configuration parameters */ /* Device constant configuration parameters */
struct i2s_mcux_config { struct i2s_mcux_config {
@ -36,17 +36,32 @@ struct stream {
uint32_t channel; /* stores the channel for dma */ uint32_t channel; /* stores the channel for dma */
struct i2s_config cfg; struct i2s_config cfg;
struct dma_config dma_cfg; struct dma_config dma_cfg;
struct dma_block_config dma_block[NUM_DMA_BLOCKS];
bool last_block; bool last_block;
struct k_msgq in_queue; struct k_msgq in_queue;
void *in_msgs[CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT];
struct k_msgq out_queue; 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 i2s_mcux_data {
struct stream rx; 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; 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, 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.dest_data_size = bytes_per_word;
stream->dma_cfg.source_data_size = bytes_per_word;
/* Save configuration for get_config */ /* Save configuration for get_config */
memcpy(&stream->cfg, i2s_cfg, sizeof(struct i2s_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, 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; void *buffer;
while (k_msgq_get(&stream->in_queue, &buffer, K_NO_WAIT) == 0) { if (tx) {
k_mem_slab_free(mem_slab, &buffer); 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) { while (k_msgq_get(&stream->out_queue, &buffer, K_NO_WAIT) == 0) {
k_mem_slab_free(mem_slab, &buffer); 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 */ /* purge buffers queued in the stream */
if (drop) { 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 */ /* purge buffers queued in the stream */
if (drop) { 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, 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; const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data; 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) { if (dir == I2S_DIR_RX) {
stream = &dev_data->rx; stream = &dev_data->rx;
blk_cfg = &dev_data->rx_dma_blocks[0];
memset(blk_cfg, 0, sizeof(dev_data->rx_dma_blocks));
} else { } else {
stream = &dev_data->tx; 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]; stream->dma_cfg.head_block = blk_cfg;
memset(blk_cfg, 0, sizeof(struct dma_block_config));
if (dir == I2S_DIR_RX) { if (dir == I2S_DIR_RX) {
blk_cfg->source_address = (uint32_t)&base->FIFORD; blk_cfg->source_address = (uint32_t)&base->FIFORD;
blk_cfg->dest_address = (uint32_t)buffer[0]; blk_cfg->dest_address = (uint32_t)buffer[0];
blk_cfg->block_size = stream->cfg.block_size; blk_cfg->block_size = block_size;
blk_cfg->next_block = &stream->dma_block[1]; blk_cfg->next_block = &dev_data->rx_dma_blocks[1];
blk_cfg->dest_reload_en = 1; blk_cfg->dest_reload_en = 1;
blk_cfg = &stream->dma_block[1]; blk_cfg = &dev_data->rx_dma_blocks[1];
memset(blk_cfg, 0, sizeof(struct dma_block_config));
blk_cfg->source_address = (uint32_t)&base->FIFORD; blk_cfg->source_address = (uint32_t)&base->FIFORD;
blk_cfg->dest_address = (uint32_t)buffer[1]; blk_cfg->dest_address = (uint32_t)buffer[1];
blk_cfg->block_size = stream->cfg.block_size; blk_cfg->block_size = block_size;
stream->dma_cfg.block_count = NUM_DMA_BLOCKS;
} else { } else {
blk_cfg->dest_address = (uint32_t)&base->FIFOWR; blk_cfg->dest_address = (uint32_t)&base->FIFOWR;
blk_cfg->source_address = (uint32_t)buffer; blk_cfg->source_address = (uint32_t)buffer;
blk_cfg->block_size = stream->cfg.block_size; blk_cfg->block_size = block_size;
stream->dma_cfg.block_count = 1;
} }
stream->dma_cfg.head_block = &stream->dma_block[0];
stream->dma_cfg.user_data = (void *)dev; stream->dma_cfg.user_data = (void *)dev;
dma_config(stream->dev_dma, stream->channel, &stream->dma_cfg); 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; const struct device *dev = (const struct device *)arg;
struct i2s_mcux_data *dev_data = dev->data; struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx; struct stream *stream = &dev_data->tx;
void *buffer; struct i2s_txq_entry queue_entry;
int ret; int ret;
LOG_DBG("tx cb: %d", stream->state); 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) { if (ret == 0) {
/* transmission complete. free the buffer */ /* 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 { } else {
LOG_ERR("no buffer in output queue for channel %u", channel); 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_RUNNING:
case I2S_STATE_STOPPING: case I2S_STATE_STOPPING:
/* get the next buffer from queue */ /* 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) { if (ret == 0) {
/* config the DMA */ /* config the DMA */
i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX, (uint32_t *)buffer); i2s_mcux_config_dma_blocks(dev, I2S_DIR_TX,
k_msgq_put(&stream->out_queue, &buffer, K_NO_WAIT); (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); 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) static int i2s_mcux_tx_stream_start(const struct device *dev)
{ {
int ret = 0; int ret = 0;
void *buffer;
const struct i2s_mcux_config *cfg = dev->config; const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data; struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx; struct stream *stream = &dev_data->tx;
I2S_Type *base = cfg->base; I2S_Type *base = cfg->base;
struct i2s_txq_entry queue_entry;
/* retrieve buffer from input queue */ /* 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) { if (ret != 0) {
LOG_ERR("No buffer in input queue to start transmission"); LOG_ERR("No buffer in input queue to start transmission");
return ret; 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 */ /* 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) { if (ret != 0) {
LOG_ERR("failed to put buffer in output queue"); LOG_ERR("failed to put buffer in output queue");
return ret; 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) static int i2s_mcux_rx_stream_start(const struct device *dev)
{ {
int ret = 0; int ret = 0;
void *buffer[NUM_DMA_BLOCKS]; void *buffer[NUM_RX_DMA_BLOCKS];
const struct i2s_mcux_config *cfg = dev->config; const struct i2s_mcux_config *cfg = dev->config;
struct i2s_mcux_data *dev_data = dev->data; struct i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->rx; struct stream *stream = &dev_data->rx;
@ -606,7 +635,7 @@ static int i2s_mcux_rx_stream_start(const struct device *dev)
return -EINVAL; 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], ret = k_mem_slab_alloc(stream->cfg.mem_slab, &buffer[i],
K_NO_WAIT); K_NO_WAIT);
if (ret != 0) { 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 */ /* 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); ret = k_msgq_put(&stream->in_queue, &buffer[i], K_NO_WAIT);
if (ret != 0) { if (ret != 0) {
LOG_ERR("failed to put buffer in input queue"); 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 i2s_mcux_data *dev_data = dev->data;
struct stream *stream = &dev_data->tx; struct stream *stream = &dev_data->tx;
int ret; int ret;
struct i2s_txq_entry queue_entry = {
.mem_block = mem_block,
.size = size,
};
if (stream->state != I2S_STATE_RUNNING && if (stream->state != I2S_STATE_RUNNING &&
stream->state != I2S_STATE_READY) { stream->state != I2S_STATE_READY) {
@ -786,7 +819,7 @@ static int i2s_mcux_write(const struct device *dev, void *mem_block,
return -EIO; 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)); SYS_TIMEOUT_MS(stream->cfg.timeout));
if (ret) { if (ret) {
@ -842,13 +875,13 @@ static int i2s_mcux_init(const struct device *dev)
cfg->irq_config(dev); cfg->irq_config(dev);
/* Initialize the buffer queues */ /* Initialize the buffer queues */
k_msgq_init(&data->tx.in_queue, (char *)data->tx.in_msgs, k_msgq_init(&data->tx.in_queue, (char *)data->tx_in_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT); sizeof(struct i2s_txq_entry), CONFIG_I2S_MCUX_FLEXCOMM_TX_BLOCK_COUNT);
k_msgq_init(&data->rx.in_queue, (char *)data->rx.in_msgs, k_msgq_init(&data->rx.in_queue, (char *)data->rx_in_msgs,
sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT); 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); 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); sizeof(void *), CONFIG_I2S_MCUX_FLEXCOMM_RX_BLOCK_COUNT);
if (data->tx.dev_dma != NULL) { if (data->tx.dev_dma != NULL) {
@ -884,7 +917,6 @@ static int i2s_mcux_init(const struct device *dev)
.dma_cfg = { \ .dma_cfg = { \
.channel_direction = MEMORY_TO_PERIPHERAL, \ .channel_direction = MEMORY_TO_PERIPHERAL, \
.dma_callback = i2s_mcux_dma_tx_callback, \ .dma_callback = i2s_mcux_dma_tx_callback, \
.source_data_size = 1, \
.block_count = 1, \ .block_count = 1, \
} \ } \
}, \ }, \
@ -898,8 +930,7 @@ static int i2s_mcux_init(const struct device *dev)
.dma_cfg = { \ .dma_cfg = { \
.channel_direction = PERIPHERAL_TO_MEMORY, \ .channel_direction = PERIPHERAL_TO_MEMORY, \
.dma_callback = i2s_mcux_dma_rx_callback, \ .dma_callback = i2s_mcux_dma_rx_callback, \
.source_data_size = 1, \ .block_count = NUM_RX_DMA_BLOCKS, \
.block_count = 1, \
} \ } \
} }