Fixes for Vybrid SPI DMA implementation

PATCH In review Sanchayan Maity 2 participants 6 messages Nov 22, 2016

Subsystems (2)

SPI Freescale dspi driver

lore

Share & tools

lore

Sanchayan Maity

author

2016-11-22 07:01 UTC #0 lore raw

v3 cover Fixes for Vybrid SPI DMA implementation

Hello,

This v3 set of patches have fixes for Vybrid SPI DMA implementation and is rebased on top of latest topic/fsl-dspi.

http://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/log/?h=topic/fsl-dspi

The patches have been tested on a Toradex Colibri Vybrid VF61 module and now incoporate feedback from Stefan on version 2 of patchset.

Changes since v2:
1. Drop the patch "Fix SPI transfer issue when using multiple SPI_IOC_MESSAGE" since it's now applied and rebase the whole patchset
2. Second patch in this series "Fix continuous selection format" now fixes the issue using an existing function and handling it similar to existing EOQ mode and also cleaning up nicely the transmit code path.
3. Third patch now just fixes the incorrect freeing of DMA allocated buffers and drops the minor clean up patch from earlier series completely due to the clean up from 2 above.

Changes since v1:
1. Place the continuous selection format patch second in order and remove code duplication
2. Improve the use of curr_xfer_len and instead of converting from bytes to DMA transfers in every use, do it at a single place. Accordingly change it's use at other places
3. Code cleanup patch has less to clean with change above

v2:
https://www.spinics.net/lists/arm-kernel/msg543941.html

v1:
http://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1274632.html

Thanks & Regards,
Sanchayan.

Sanchayan Maity (3):
spi: spi-fsl-dspi: Fix incorrect DMA setup
spi: spi-fsl-dspi: Fix continuous selection format
spi: spi-fsl-dspi: Fix incorrect freeing of DMA allocated buffers

 drivers/spi/spi-fsl-dspi.c | 59 +++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

Signature · 1 line

2.10.2

Sanchayan Maity

author

2016-11-22 07:01 UTC #1 lore raw

PATCH v3 1/3 spi: spi-fsl-dspi: Fix incorrect DMA setup

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]>

Reviewed-by Stefan Agner <[email protected]>

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

drivers/spi/spi-fsl-dspi.c +18 ~~-17~~

index b1ee1f5..911aadb 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] =
 			SPI_PUSHR_TXDATA(val) | SPI_PUSHR_PCS(dspi->cs) |

@@ -265,7 +262,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");

@@ -281,7 +280,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");

@@ -328,17 +329,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);

@@ -347,7 +348,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;

Signature · 1 line

2.10.2

Sanchayan Maity

author

2016-11-22 07:01 UTC #2 lore raw

PATCH v3 2/3 spi: spi-fsl-dspi: Fix continuous selection format

In reply to 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 existing dspi_data_to_pushr function to restructure the transmit code path and set or reset the CONT bit on same lines as code path in EOQ mode does. This correctly implements continuous selection format while also correcting and cleaning up the transmit code path.

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

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

drivers/spi/spi-fsl-dspi.c +6 ~~-14~~

index 911aadb..8af3151 100644
@@ -196,6 +196,8 @@ struct fsl_dspi {
 	struct fsl_dspi_dma	*dma;
 };
 
+static u32 dspi_data_to_pushr(struct fsl_dspi *dspi, int tx_word);
+
 static inline int is_double_byte_mode(struct fsl_dspi *dspi)
 {
 	unsigned int val;

@@ -242,24 +244,15 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	int time_left;
 	int tx_word;
 	int i;
-	u16 val;
 
 	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;
-		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;
+	for (i = 0; i < dma->curr_xfer_len; i++) {
+		dspi->dma->tx_dma_buf[i] = dspi_data_to_pushr(dspi, tx_word);
+		if ((dspi->cs_change) && (!dspi->len))
+			dspi->dma->tx_dma_buf[i] &= ~SPI_PUSHR_CONT;
 	}
 
-	val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
-	dspi->dma->tx_dma_buf[i] = SPI_PUSHR_TXDATA(val) |
-					SPI_PUSHR_PCS(dspi->cs) |
-					SPI_PUSHR_CTAS(0);
-	dspi->tx += tx_word + 1;
-
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
 					dma->tx_dma_phys,
 					dma->curr_xfer_len *

@@ -351,7 +344,6 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 			curr_remaining_bytes -= dma->curr_xfer_len * word;
 			if (curr_remaining_bytes < 0)
 				curr_remaining_bytes = 0;
-			dspi->len = curr_remaining_bytes;
 		}
 	}

Signature · 1 line

2.10.2

Mark Brown

maintainer

2016-11-22 17:26 UTC #3 lore raw

Applied "spi: spi-fsl-dspi: Fix continuous selection format" to the spi tree

The patch

spi: spi-fsl-dspi: Fix continuous selection format

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From ccf7d8ee3d6eb600338a184e9a192ec1d5aee924 Mon Sep 17 00:00:00 2001

From: Sanchayan Maity <[email protected]>
Date: Tue, 22 Nov 2016 12:31:31 +0530
Subject: [PATCH] spi: spi-fsl-dspi: Fix continuous selection format

Current DMA implementation was not handling the continuous selection format viz. SPI chip select would be deasserted even between sequential serial transfers.

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

Signed-off-by Mark Brown <[email protected]>

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

drivers/spi/spi-fsl-dspi.c +6 ~~-14~~

index cb41c327bd77..7ada112bfd85 100644
@@ -196,6 +196,8 @@ struct fsl_dspi {
 	struct fsl_dspi_dma	*dma;
 };
 
+static u32 dspi_data_to_pushr(struct fsl_dspi *dspi, int tx_word);
+
 static inline int is_double_byte_mode(struct fsl_dspi *dspi)
 {
 	unsigned int val;

@@ -242,24 +244,15 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	int time_left;
 	int tx_word;
 	int i;
-	u16 val;
 
 	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;
-		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;
+	for (i = 0; i < dma->curr_xfer_len; i++) {
+		dspi->dma->tx_dma_buf[i] = dspi_data_to_pushr(dspi, tx_word);
+		if ((dspi->cs_change) && (!dspi->len))
+			dspi->dma->tx_dma_buf[i] &= ~SPI_PUSHR_CONT;
 	}
 
-	val = tx_word ? *(u16 *) dspi->tx : *(u8 *) dspi->tx;
-	dspi->dma->tx_dma_buf[i] = SPI_PUSHR_TXDATA(val) |
-					SPI_PUSHR_PCS(dspi->cs) |
-					SPI_PUSHR_CTAS(0);
-	dspi->tx += tx_word + 1;
-
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
 					dma->tx_dma_phys,
 					dma->curr_xfer_len *

@@ -351,7 +344,6 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 			curr_remaining_bytes -= dma->curr_xfer_len * word;
 			if (curr_remaining_bytes < 0)
 				curr_remaining_bytes = 0;
-			dspi->len = curr_remaining_bytes;
 		}
 	}

Signature · 1 line

2.10.2

Sanchayan Maity

author

2016-11-22 07:01 UTC #4 lore raw

PATCH v3 3/3 spi: spi-fsl-dspi: Fix incorrect freeing of DMA allocated buffers

In reply to Sanchayan Maity

Buffers allocated with a call to dma_alloc_coherent should be freed with dma_free_coherent instead of the currently used devm_kfree.

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

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

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

index 8af3151..7ada112 100644
@@ -420,9 +420,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:

Signature · 1 line

2.10.2

Mark Brown

maintainer

2016-11-22 17:26 UTC #5 lore raw

Applied "spi: spi-fsl-dspi: Fix incorrect freeing of DMA allocated buffers" to the spi tree

The patch

spi: spi-fsl-dspi: Fix incorrect freeing of DMA allocated buffers

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 27d21e9f988e527982a7516fcf411994f498787d Mon Sep 17 00:00:00 2001

From: Sanchayan Maity <[email protected]>
Date: Tue, 22 Nov 2016 12:31:32 +0530
Subject: [PATCH] spi: spi-fsl-dspi: Fix incorrect freeing of DMA allocated
buffers

Buffers allocated with a call to dma_alloc_coherent should be freed with dma_free_coherent instead of the currently used devm_kfree.

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

Signed-off-by Mark Brown <[email protected]>

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

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

index b1ee1f521ba0..22f7ce1279bd 100644
@@ -427,9 +427,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:

Signature · 1 line

2.10.2