drivers: dma: dma_mcux_lpc: fix multiple bugs with continuous mode

There are multiple bugs related to continuous/circular mode.
Continuous/circular mode is where the DMA runs continuously
until the stop API is called, instead of auto-stopping on
completion on a single transfer. After a stop, the DMA
can then be reconfigured/restarted.

1. Fix bug where stop didn't actually stop. This can cause memory
   corruption if the user thought the DMA stopped and repurposed
   the dest memory, but in fact the DMA is still writing to it.
   The bug was due the incorrect usage of the DMA controller busy
   state. The DMA controller is busy only when a transfer is
   actively in progress, but the driver needed to stop even if
   the transfer is not active but is only enabled (and may become
   active on a subsequent trigger event). Change so that data->busy
   doesn't use the DMA controller busy state but tracks the enable
   state. Also, to make it doubly safe, make stop function always stop
   regardless of data->busy state because it is alway safe/correct
   to do so.

2. Fix race condition where a stop request from another ISR might race
   with a DMA completion interrupt, and the DMA completion callback
   gets invoked after the DMA has already been stopped. The fix
   is to unregister the callback with the sdk DMA driver, so the
   ISR still runs and clear the interrupt without invoking the
   callback. There is potentially still a race if the interrupt
   is restarted before the ISR fires, so the callback might be
   called too early. However, the Zephyr DMA driver doesn't
   have the channel level details that the SDK driver does and
   it cannot clear just the channel interrupt.

Also a couple of general fixes/improvements:

a. Use interrupt B for end of transfer (single transfer or end
   of block list). Use interrupt A for interrupts of a block
   in the middle of a transfer or for continuous/circular transfers.
   This fixes the dma callback so it can properly report
   DMA_STATUS_BLOCK vs DMA_STATUS_COMPLETE.

b. Reorder some fields in struct channel_data to pack a little
   better in memory

Signed-off-by: Mike J. Chen <[email protected]>

Author: mjchen0

drivers/dma/dma_mcux_lpc.c

@@ -44,13 +44,13 @@ struct channel_data {
 	const struct device *dev;
 	void *user_data;
 	dma_callback_t dma_callback;
+	dma_descriptor_t *curr_descriptor;
+	uint32_t width;
 	enum dma_channel_direction dir;
 	uint8_t src_inc;
 	uint8_t dst_inc;
-	dma_descriptor_t *curr_descriptor;
 	uint8_t num_of_descriptors;
 	bool descriptors_queued;
-	uint32_t width;
 	bool busy;
 };
 
@@ -93,14 +93,20 @@ static void nxp_lpc_dma_callback(dma_handle_t *handle, void *param,
 
 	if (intmode == kDMA_IntError) {
 		DMA_AbortTransfer(handle);
+		data->busy = false;
 	} else if (intmode == kDMA_IntA) {
+		/* this is interrupt for a block that is not the
+		 * last so leave busy flag set.
+		 */
 		ret = DMA_STATUS_BLOCK;
 	} else {
+		/* this is interrupt for end of a block list
+		 * or a single transfer.
+		 */
+		data->busy = false;
 		ret = DMA_STATUS_COMPLETE;
 	}
 
-	data->busy = DMA_ChannelIsBusy(data->dma_handle.base, channel);
-
 	if (data->dma_callback) {
 		data->dma_callback(data->dev, data->user_data, channel, ret);
 	}
@@ -238,7 +244,7 @@ static int dma_mcux_lpc_queue_descriptors(struct channel_data *data,
 			enable_b_interrupt = 0;
 		} else {
 			/* Use intB when this is the end of the block list and transfer */
-			if (last_block) {
+			if (last_block && !setup_extra_descriptor) {
 				enable_a_interrupt = 0;
 				enable_b_interrupt = 1;
 			} else {
@@ -661,8 +667,8 @@ static int dma_mcux_lpc_configure(const struct device *dev, uint32_t channel,
 					block_config->block_size);
 		}
 	} else {
-		/* Enable interrupt for the descriptor */
-		xfer_config = DMA_CHANNEL_XFER(0UL, 0UL, 1UL, 0UL,
+		/* Enable interrupt for the descriptor. Use int_b */
+		xfer_config = DMA_CHANNEL_XFER(0UL, 0UL, 0UL, 1UL,
 				width,
 				src_inc,
 				dst_inc,
@@ -796,11 +802,16 @@ static int dma_mcux_lpc_start(const struct device *dev, uint32_t channel)
 	struct dma_mcux_lpc_dma_data *dev_data = dev->data;
 	int8_t virtual_channel = dev_data->channel_index[channel];
 	struct channel_data *data = DEV_CHANNEL_DATA(dev, virtual_channel);
+	dma_handle_t *p_handle = DEV_DMA_HANDLE(dev, virtual_channel);
 
 	LOG_DBG("START TRANSFER");
 	LOG_DBG("DMA CTRL 0x%x", DEV_BASE(dev)->CTRL);
 	data->busy = true;
-	DMA_StartTransfer(DEV_DMA_HANDLE(dev, virtual_channel));
+	/* In case of a restart after a stop, reinstall the DMA callback
+	 * that was removed by the stop.
+	 */
+	DMA_SetCallback(p_handle, nxp_lpc_dma_callback, (void *)data);
+	DMA_StartTransfer(p_handle);
 	return 0;
 }
 
@@ -809,14 +820,28 @@ static int dma_mcux_lpc_stop(const struct device *dev, uint32_t channel)
 	struct dma_mcux_lpc_dma_data *dev_data = dev->data;
 	int8_t virtual_channel = dev_data->channel_index[channel];
 	struct channel_data *data = DEV_CHANNEL_DATA(dev, virtual_channel);
+	dma_handle_t *p_handle = DEV_DMA_HANDLE(dev, virtual_channel);
 
-	if (!data->busy) {
-		return 0;
-	}
-	DMA_AbortTransfer(DEV_DMA_HANDLE(dev, virtual_channel));
-	DMA_DisableChannel(DEV_BASE(dev), channel);
+	/* Abort/disable even if not busy. it's safe and avoids
+	 * any risk of data->busy not being accurate.
+	 */
+	DMA_AbortTransfer(p_handle);
+	DMA_DisableChannel(DEV_BASE(dev), p_handle->channel);
 
 	data->busy = false;
+
+	/* Handle race condition where if this is called from an ISR
+	 * and the DMA channel completion interrupt becomes pending
+	 * before we complete the abort and disable, then the DMA ISR
+	 * may run and invoke the callback may run after we return,
+	 * which is unexpected by user since from their perspective
+	 * the stop was already completed. We cannot just checking
+	 * and clear the pending IRQ since it is shared by other
+	 * channels. This way, the pending DMA ISR will still run,
+	 * but should just clear the interrupt and not invoke
+	 * the callback.
+	 */
+	DMA_SetCallback(p_handle, NULL, NULL);
 	return 0;
 }
 
@@ -837,8 +862,8 @@ static int dma_mcux_lpc_reload(const struct device *dev, uint32_t channel,
 
 		p_handle = DEV_DMA_HANDLE(dev, virtual_channel);
 
-		/* Only one buffer, enable interrupt */
-		xfer_config = DMA_CHANNEL_XFER(0UL, 0UL, 1UL, 0UL,
+		/* Only one buffer, enable interrupt b */
+		xfer_config = DMA_CHANNEL_XFER(0UL, 0UL, 0UL, 1UL,
 					data->width,
 					data->src_inc,
 					data->dst_inc,