[U-Boot] [PATCH 1/2] MX28: SPI: Fix the DMA DCache race condition

[U-Boot] [PATCH 1/2] MX28: SPI: Fix the DMA DCache race condition

[Marek Vasut]

This patch fixes dcache-related problem. The problem manifested
when dcache was enabled and the following command issued twice:

mw 0x42000000 0 0x4000 ; sf probe ; sf read 0x42000000 0x0 0x10000 ; sha1sum 0x42000000 0x10000

The SHA1 checksum was correct during the first call. Yet with
every subsequent call of the above command, it differed and was
wrong.

It turns out this was because of a race condition. On the first
time the command was called, no cacheline contained any data from
the destination memory location. The DMA transfered data into the
location and the cache above the location was invalidated. Then the
checksum was computed, but that meant the data were loaded into data
cache.

On any subsequent call, the DMA again transfered data into the same
destination. Yet during the transfer, some of the DCache lines were
evicted and written back into the main memory. Once the DMA transfer
completed, the data cache was invalidated over the memory location as
usual. But the data that were to be loaded back into the data cache
by subsequent SHA1 checksuming were corrupted.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Otavio Salvador <otavio@ossystems.com.br>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/spi/mxs_spi.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
index 168dbe4..c399707 100644
--- a/drivers/spi/mxs_spi.c
+++ b/drivers/spi/mxs_spi.c
@@ -224,6 +224,7 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	struct mxs_dma_desc *dp;
 	uint32_t ctrl0;
 	uint32_t cache_data_count;
+	const uint32_t dstart = (uint32_t)data;
 	int dmach;
 	int tl;
 
@@ -246,10 +247,12 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	else
 		cache_data_count = length;
 
+	/* Flush data to DRAM so DMA can pick them up */
 	if (write)
-		/* Flush data to DRAM so DMA can pick them up */
-		flush_dcache_range((uint32_t)data,
-			(uint32_t)(data + cache_data_count));
+		flush_dcache_range(dstart, dstart + cache_data_count);
+
+	/* Invalidate the area, so no writeback into the RAM races with DMA */
+	invalidate_dcache_range(dstart, dstart + cache_data_count);
 
 	dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + slave->slave.bus;
 
@@ -310,10 +313,8 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 		return -EINVAL;
 
 	/* The data arrived into DRAM, invalidate cache over them */
-	if (!write) {
-		invalidate_dcache_range((uint32_t)data,
-			(uint32_t)(data + cache_data_count));
-	}
+	if (!write)
+		invalidate_dcache_range(dstart, dstart + cache_data_count);
 
 	return 0;
 }
-- 
1.7.10.4

[U-Boot] [PATCH 2/2] MX28: SPI: Fix the DMA chaining

[Marek Vasut]

It turns out that in order for the SPI DMA to properly support
continuous transfers longer than 65280 bytes, there are some very
important parts that were left out from the documentation.

Firstly, the XFER_SIZE register is not written with the whole length
of a transfer, but is written by each and every chained descriptor
with the length of the descriptors data buffer.

Next, unlike the demo code supplied by FSL, which only writes one PIO
word per descriptor, this does not apply if the descriptors are chained,
since the XFER_SIZE register must be written. Therefore, it is essential
to use four PIO words, CTRL0, CMD0, CMD1, XFER_SIZE. CMD0 and CMD1 are
written with zero, since they don't apply. The DMA programs the PIO words
in an incrementing order, so four PIO words.

Finally, unlike the demo code supplied by FSL, the SSP_CTRL0_IGNORE_CRC
must not be set during the whole transfer, but it must be set only on the
last descriptor in the chain.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Otavio Salvador <otavio@ossystems.com.br>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/spi/mxs_spi.c |   43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
index c399707..42e4c99 100644
--- a/drivers/spi/mxs_spi.c
+++ b/drivers/spi/mxs_spi.c
@@ -227,6 +227,7 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	const uint32_t dstart = (uint32_t)data;
 	int dmach;
 	int tl;
+	int ret = 0;
 
 	ALLOC_CACHE_ALIGN_BUFFER(struct mxs_dma_desc, desc, desc_count);
 
@@ -240,8 +241,6 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	if (!write)
 		ctrl0 |= SSP_CTRL0_READ;
 
-	writel(length, &ssp_regs->hw_ssp_xfer_size);
-
 	if (length % ARCH_DMA_MINALIGN)
 		cache_data_count = roundup(length, ARCH_DMA_MINALIGN);
 	else
@@ -284,39 +283,47 @@ static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 			tl = min(length, xfer_max_sz);
 
 		dp->cmd.data |=
-			(tl << MXS_DMA_DESC_BYTES_OFFSET) |
-			(1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+			((tl & 0xffff) << MXS_DMA_DESC_BYTES_OFFSET) |
+			(4 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
 			MXS_DMA_DESC_HALT_ON_TERMINATE |
 			MXS_DMA_DESC_TERMINATE_FLUSH;
-		dp->cmd.pio_words[0] = ctrl0;
 
 		data += tl;
 		length -= tl;
 
+		if (!length) {
+			dp->cmd.data |= MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM;
+
+			if (flags & SPI_XFER_END) {
+				ctrl0 &= ~SSP_CTRL0_LOCK_CS;
+				ctrl0 |= SSP_CTRL0_IGNORE_CRC;
+			}
+		}
+
+		/*
+		 * Write CTRL0, CMD0, CMD1, XFER_SIZE registers. It is
+		 * essential that the XFER_SIZE register is written on
+		 * a per-descriptor basis with the same size as is the
+		 * descriptor!
+		 */
+		dp->cmd.pio_words[0] = ctrl0;
+		dp->cmd.pio_words[1] = 0;
+		dp->cmd.pio_words[2] = 0;
+		dp->cmd.pio_words[3] = tl;
+
 		mxs_dma_desc_append(dmach, dp);
 
 		dp++;
 	}
 
-	dp->address = (dma_addr_t)dp;
-	dp->cmd.address = (dma_addr_t)0;
-	dp->cmd.data = MXS_DMA_DESC_COMMAND_NO_DMAXFER |
-			(1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
-			MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM;
-	if (flags & SPI_XFER_END) {
-		ctrl0 &= ~SSP_CTRL0_LOCK_CS;
-		dp->cmd.pio_words[0] = ctrl0 | SSP_CTRL0_IGNORE_CRC;
-	}
-	mxs_dma_desc_append(dmach, dp);
-
 	if (mxs_dma_go(dmach))
-		return -EINVAL;
+		ret = -EINVAL;
 
 	/* The data arrived into DRAM, invalidate cache over them */
 	if (!write)
 		invalidate_dcache_range(dstart, dstart + cache_data_count);
 
-	return 0;
+	return ret;
 }
 
 int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
-- 
1.7.10.4

[U-Boot] [PATCH 2/2] MX28: SPI: Fix the DMA chaining

[Stefano Babic]

On 01/09/2012 04:08, Marek Vasut wrote:
> It turns out that in order for the SPI DMA to properly support
> continuous transfers longer than 65280 bytes, there are some very
> important parts that were left out from the documentation.
> 
> Firstly, the XFER_SIZE register is not written with the whole length
> of a transfer, but is written by each and every chained descriptor
> with the length of the descriptors data buffer.
> 
> Next, unlike the demo code supplied by FSL, which only writes one PIO
> word per descriptor, this does not apply if the descriptors are chained,
> since the XFER_SIZE register must be written. Therefore, it is essential
> to use four PIO words, CTRL0, CMD0, CMD1, XFER_SIZE. CMD0 and CMD1 are
> written with zero, since they don't apply. The DMA programs the PIO words
> in an incrementing order, so four PIO words.
> 
> Finally, unlike the demo code supplied by FSL, the SSP_CTRL0_IGNORE_CRC
> must not be set during the whole transfer, but it must be set only on the
> last descriptor in the chain.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Otavio Salvador <otavio@ossystems.com.br>
> Cc: Stefano Babic <sbabic@denx.de>
> ---


Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/2] MX28: SPI: Fix the DMA DCache race condition

[Stefano Babic]

On 01/09/2012 04:07, Marek Vasut wrote:
> This patch fixes dcache-related problem. The problem manifested
> when dcache was enabled and the following command issued twice:
> 
> mw 0x42000000 0 0x4000 ; sf probe ; sf read 0x42000000 0x0 0x10000 ; sha1sum 0x42000000 0x10000
> 
> The SHA1 checksum was correct during the first call. Yet with
> every subsequent call of the above command, it differed and was
> wrong.
> 
> It turns out this was because of a race condition. On the first
> time the command was called, no cacheline contained any data from
> the destination memory location. The DMA transfered data into the
> location and the cache above the location was invalidated. Then the
> checksum was computed, but that meant the data were loaded into data
> cache.
> 
> On any subsequent call, the DMA again transfered data into the same
> destination. Yet during the transfer, some of the DCache lines were
> evicted and written back into the main memory. Once the DMA transfer
> completed, the data cache was invalidated over the memory location as
> usual. But the data that were to be loaded back into the data cache
> by subsequent SHA1 checksuming were corrupted.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Otavio Salvador <otavio@ossystems.com.br>
> Cc: Stefano Babic <sbabic@denx.de>
> ---

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================