Fixes for Vybrid SPI DMA implementation

Current DMA implementation had a bug where the DMA transfer would exit the loop in dspi_transfer_one_message after the completion of a single transfer. This results in a multi message transfer submitted with SPI_IOC_MESSAGE to terminate incorrectly without an error.

Quoted 4 lines · Sanchayan Maity

Current DMA implementation had a bug where the DMA transfer would exit the loop in dspi_transfer_one_message after the completion of a single transfer. This results in a multi message transfer submitted with SPI_IOC_MESSAGE to terminate incorrectly without an error.

Please don't resend already applied patches. If there are any changes needed please send incremental changes on top of what's already applied.

Hello Mark,

Quoted 6 lines · Mark Brown

Quoted 4 lines · Sanchayan Maity

Current DMA implementation had a bug where the DMA transfer would exit the loop in dspi_transfer_one_message after the completion of a single transfer. This results in a multi message transfer submitted with SPI_IOC_MESSAGE to terminate incorrectly without an error.

Quoted 2 lines

Please don't resend already applied patches. If there are any changes needed please send incremental changes on top of what's already applied.

This is not a resend of an applied patch. The whole series applies on top of your topic/fsl-dspi branch and has fixes for the SPI DMA as incremental changes

Quoted 3 lines · maitysanchayan at gmail.com

This is not a resend of an applied patch. The whole series applies on top of your topic/fsl-dspi branch and has fixes for the SPI DMA as incremental changes

Sorry. I take that back. I now see you applied the patch and I got the applied mail after I replied.

Current DMA implementation was not handling the continuous selection format viz. SPI chip select would be deasserted even between sequential serial transfers. Use the cs_change variable and correctly set or reset the CONT bit accordingly for case where peripherals require the chip select to be asserted between sequential transfers.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/spi/spi-fsl-dspi.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/spi/spi-fsl-dspi.c +0 -0

index b1ee1f5..41422cd 100644

Quoted 16 lines · Sanchayan Maity

Current DMA implementation was not handling the continuous selection format viz. SPI chip select would be deasserted even between sequential serial transfers. Use the cs_change variable and correctly set or reset the CONT bit accordingly for case where peripherals require the chip select to be asserted between sequential transfers.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/spi/spi-fsl-dspi.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/spi/spi-fsl-dspi.c +0 -0

index b1ee1f5..41422cd 100644

drivers/spi/spi-fsl-dspi.c static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi) · line 268

Quoted 8 lines

drivers/spi/spi-fsl-dspi.c +2 -0

@@ -261,6 +261,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	dspi->dma->tx_dma_buf[i] = SPI_PUSHR_TXDATA(val) |
 					SPI_PUSHR_PCS(dspi->cs) |
 					SPI_PUSHR_CTAS(0);
+	if (!dspi->cs_change)
+		dspi->dma->tx_dma_buf[i] |= SPI_PUSHR_CONT;
 	dspi->tx += tx_word + 1;
 
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,

Other transfer mode use:

if ((dspi->cs_change) && (!dspi->len))

        dspi_pushr &= ~SPI_PUSHR_CONT;

which indicates that they only clear SPI_PUSHR_CONT at the very end of a transfer... The DMA code currently deselects after every DMA transfer if dspi->cs_change is set.

Maybe we should use the helper dspi_data_to_pushr to fill the DMA buffer and _clear_ SPI_PUSHR_CONT if necessary like the other transfer modes do... Then we can use the for loop to fill the complete buffer and get rid of some code dupplication.

I see that dspi_data_to_pushr does move len too, which we did not in the DMA case. dspi->len gets incremented only on successful DMA transfer in dspi_dma_xfer. However, I wonder if that is not even a bug: We increment dspi->tx always, but len only on success. This makes len go off sync with regards to the tx pointer which does not help anybody. So lets get rid of the update code in dspi_dma_xfer

Currently dmaengine_prep_slave_single was being called with length set to the complete DMA buffer size. This resulted in unwanted bytes being transferred to the SPI register leading to clock and MOSI lines having unwanted data even after chip select got deasserted and the required bytes having been transferred.

While at it also clean up the use of curr_xfer_len which is central to the DMA setup, from bytes to DMA transfers for every use.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/spi/spi-fsl-dspi.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/spi/spi-fsl-dspi.c +5 -8

index 41422cd..08882f7 100644
@@ -151,6 +151,7 @@ static const struct fsl_dspi_devtype_data ls2085a_data = {
 };
 
 struct fsl_dspi_dma {
+	/* Length of transfer in words of DSPI_FIFO_SIZE */
 	u32 curr_xfer_len;
 
 	u32 *tx_dma_buf;

@@ -217,15 +218,13 @@ static void dspi_rx_dma_callback(void *arg)
 	struct fsl_dspi *dspi = arg;
 	struct fsl_dspi_dma *dma = dspi->dma;
 	int rx_word;
-	int i, len;
+	int i;
 	u16 d;
 
 	rx_word = is_double_byte_mode(dspi);
 
-	len = rx_word ? (dma->curr_xfer_len / 2) : dma->curr_xfer_len;
-
 	if (!(dspi->dataflags & TRAN_STATE_RX_VOID)) {
-		for (i = 0; i < len; i++) {
+		for (i = 0; i < dma->curr_xfer_len; i++) {
 			d = dspi->dma->rx_dma_buf[i];
 			rx_word ? (*(u16 *)dspi->rx = d) :
 						(*(u8 *)dspi->rx = d);

@@ -242,14 +241,12 @@ static int dspi_next_xfer_dma_submit(struct
 fsl_dspi *dspi)
 	struct device *dev = &dspi->pdev->dev;
 	int time_left;
 	int tx_word;
-	int i, len;
+	int i;
 	u16 val;
 
 	tx_word = is_double_byte_mode(dspi);
 
-	len = tx_word ? (dma->curr_xfer_len / 2) : dma->curr_xfer_len;
-
-	for (i = 0; i < len - 1; i++) {
+	for (i = 0; i < dma->curr_xfer_len - 1; i++) {
 		val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
 		dspi->dma->tx_dma_buf[i] =

drivers/spi/spi-fsl-dspi.c +13 -9

 			SPI_PUSHR_TXDATA(val) | SPI_PUSHR_PCS(dspi->cs) |

@@ -267,7 +264,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
 					dma->tx_dma_phys,
-					DSPI_DMA_BUFSIZE, DMA_MEM_TO_DEV,
+					dma->curr_xfer_len *
+					DMA_SLAVE_BUSWIDTH_4_BYTES,
+					DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!dma->tx_desc) {
 		dev_err(dev, "Not able to get desc for DMA xfer\n");

@@ -283,7 +282,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 	dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx,
 					dma->rx_dma_phys,
-					DSPI_DMA_BUFSIZE, DMA_DEV_TO_MEM,
+					dma->curr_xfer_len *
+					DMA_SLAVE_BUSWIDTH_4_BYTES,
+					DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!dma->rx_desc) {
 		dev_err(dev, "Not able to get desc for DMA xfer\n");

@@ -330,17 +331,17 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 	struct device *dev = &dspi->pdev->dev;
 	int curr_remaining_bytes;
 	int bytes_per_buffer;
-	int tx_word;
+	int word = 1;
 	int ret = 0;
 
-	tx_word = is_double_byte_mode(dspi);
+	if (is_double_byte_mode(dspi))
+		word = 2;
 	curr_remaining_bytes = dspi->len;
+	bytes_per_buffer = DSPI_DMA_BUFSIZE / DSPI_FIFO_SIZE;
 	while (curr_remaining_bytes) {
 		/* Check if current transfer fits the DMA buffer */
-		dma->curr_xfer_len = curr_remaining_bytes;
-		bytes_per_buffer = DSPI_DMA_BUFSIZE /
-				(DSPI_FIFO_SIZE / (tx_word ? 2 : 1));
-		if (curr_remaining_bytes > bytes_per_buffer)
+		dma->curr_xfer_len = curr_remaining_bytes / word;
+		if (dma->curr_xfer_len > bytes_per_buffer)
 			dma->curr_xfer_len = bytes_per_buffer;
 
 		ret = dspi_next_xfer_dma_submit(dspi);

@@ -349,7 +350,7 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 			goto exit;
 
 		} else {
-			curr_remaining_bytes -= dma->curr_xfer_len;
+			curr_remaining_bytes -= dma->curr_xfer_len * word;
 			if (curr_remaining_bytes < 0)
 				curr_remaining_bytes = 0;
 			dspi->len = curr_remaining_bytes;

From 1eaccf210c59e04eb6e9b5469a60d6609c95ac61 Mon Sep 17 00:00:00 2001

Code cleanup for improving code readability and error path fixes and cleanup removing use of devm_kfree.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/spi/spi-fsl-dspi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/spi/spi-fsl-dspi.c +12 -4

index 08882f7..2987a16 100644
@@ -226,8 +226,10 @@ static void dspi_rx_dma_callback(void *arg)
 	if (!(dspi->dataflags & TRAN_STATE_RX_VOID)) {
 		for (i = 0; i < dma->curr_xfer_len; i++) {
 			d = dspi->dma->rx_dma_buf[i];
-			rx_word ? (*(u16 *)dspi->rx = d) :
-						(*(u8 *)dspi->rx = d);
+			if (rx_word)
+				*(u16 *)dspi->rx = d;
+			else
+				*(u8 *)dspi->rx = d;
 			dspi->rx += rx_word + 1;
 		}
 	}

@@ -247,14 +249,20 @@ static int dspi_next_xfer_dma_submit(struct
 fsl_dspi *dspi)
 	tx_word = is_double_byte_mode(dspi);
 
 	for (i = 0; i < dma->curr_xfer_len - 1; i++) {
-		val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
+		if (tx_word)
+			val = *(u16 *) dspi->tx;
+		else
+			val = *(u8 *) dspi->tx;
 		dspi->dma->tx_dma_buf[i] =
 			SPI_PUSHR_TXDATA(val) | SPI_PUSHR_PCS(dspi->cs) |
 			SPI_PUSHR_CTAS(0) | SPI_PUSHR_CONT;
 		dspi->tx += tx_word + 1;
 	}
 
-	val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
+	if (tx_word)
+		val = *(u16 *) dspi->tx;
+	else
+		val = *(u8 *) dspi->tx;
 	dspi->dma->tx_dma_buf[i] = SPI_PUSHR_TXDATA(val) |
 					SPI_PUSHR_PCS(dspi->cs) |

drivers/spi/spi-fsl-dspi.c +4 -2

 					SPI_PUSHR_CTAS(0);

@@ -430,9 +438,11 @@ static int dspi_request_dma(struct fsl_dspi
 *dspi, phys_addr_t phy_addr)
 	return 0;
 
 err_slave_config:
-	devm_kfree(dev, dma->rx_dma_buf);
+	dma_free_coherent(dev, DSPI_DMA_BUFSIZE,
+			dma->rx_dma_buf, dma->rx_dma_phys);
 err_rx_dma_buf:
-	devm_kfree(dev, dma->tx_dma_buf);
+	dma_free_coherent(dev, DSPI_DMA_BUFSIZE,
+			dma->tx_dma_buf, dma->tx_dma_phys);
 err_tx_dma_buf:
 	dma_release_channel(dma->chan_tx);

err_tx_channel:

Quoted 2 lines · Sanchayan Maity

Code cleanup for improving code readability and error path fixes and cleanup removing use of devm_kfree.

Two things in one, not very nice. Especially the dma_free_coherent is really a bug and the other is a cleanup. Can you make a separate patch for the bug?

As for the cleanup, I don't like the one line conditions too, but I don't think it is worth a patch. At least the TX path should be solved with my suggestion in patch 2.

--
Stefan

Quoted 62 lines

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/spi/spi-fsl-dspi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/spi/spi-fsl-dspi.c +12 -4

index 08882f7..2987a16 100644
@@ -226,8 +226,10 @@ static void dspi_rx_dma_callback(void *arg)
 	if (!(dspi->dataflags & TRAN_STATE_RX_VOID)) {
 		for (i = 0; i < dma->curr_xfer_len; i++) {
 			d = dspi->dma->rx_dma_buf[i];
-			rx_word ? (*(u16 *)dspi->rx = d) :
-						(*(u8 *)dspi->rx = d);
+			if (rx_word)
+				*(u16 *)dspi->rx = d;
+			else
+				*(u8 *)dspi->rx = d;
 			dspi->rx += rx_word + 1;
 		}
 	}

@@ -247,14 +249,20 @@ static int dspi_next_xfer_dma_submit(struct
 fsl_dspi *dspi)
 	tx_word = is_double_byte_mode(dspi);
 
 	for (i = 0; i < dma->curr_xfer_len - 1; i++) {
-		val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
+		if (tx_word)
+			val = *(u16 *) dspi->tx;
+		else
+			val = *(u8 *) dspi->tx;
 		dspi->dma->tx_dma_buf[i] =
 			SPI_PUSHR_TXDATA(val) | SPI_PUSHR_PCS(dspi->cs) |
 			SPI_PUSHR_CTAS(0) | SPI_PUSHR_CONT;
 		dspi->tx += tx_word + 1;
 	}
 
-	val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
+	if (tx_word)
+		val = *(u16 *) dspi->tx;
+	else
+		val = *(u8 *) dspi->tx;
 	dspi->dma->tx_dma_buf[i] = SPI_PUSHR_TXDATA(val) |
 					SPI_PUSHR_PCS(dspi->cs) |

drivers/spi/spi-fsl-dspi.c +4 -2

 					SPI_PUSHR_CTAS(0);

@@ -430,9 +438,11 @@ static int dspi_request_dma(struct fsl_dspi
 *dspi, phys_addr_t phy_addr)
 	return 0;
 
 err_slave_config:
-	devm_kfree(dev, dma->rx_dma_buf);
+	dma_free_coherent(dev, DSPI_DMA_BUFSIZE,
+			dma->rx_dma_buf, dma->rx_dma_phys);
 err_rx_dma_buf:
-	devm_kfree(dev, dma->tx_dma_buf);
+	dma_free_coherent(dev, DSPI_DMA_BUFSIZE,
+			dma->tx_dma_buf, dma->tx_dma_phys);
 err_tx_dma_buf:
 	dma_release_channel(dma->chan_tx);

err_tx_channel: