[PATCH v2 0/6] Enable DMA for daVinci SPI controller

[PATCH v2 0/6] Enable DMA for daVinci SPI controller

[Frode Isaksen]

Enable DMA for the daVinci SPI using the SPI framework DMA mapping
function. Add fixes caused by large numer of SG entries
and VIVT cache.
Also, add some extra options to the spi-loopback-test
module to catch errors caused by many SG entries.
This has been tested using spidev_test, spi-loopback-test and
mounting and read/write UBIFS volume over SPI flash.

v2 changes:
- Use SPI_MASTER_MUST_ flag(s) for dummy buffer allocation (thanks
Mark Brown).
- For rx-only transfers, use rx buffer as dummy tx buffer to avoid
errors with more than 20 SG entries.
- Improved comments and use #define for contants.
- Abandon patches to set loopback and limit transfer size in
spi-loopback-test.

Thanks,
Frode

[PATCH v2 1/6] spi: davinci: Use SPI framework to handle DMA mapping

[Frode Isaksen]

From: Fabien Parent <fparent@baylibre.com>

Uppers layers like MTD can pass vmalloc'd buffers to the SPI driver,
and the current implementation will fail to map these kind of buffers.
The SPI framework is able to detect the best way to handle and map
buffers.
This commit updates the davinci SPI driver in order to use the SPI
framework to handle the DMA mapping of buffers coming from an upper
layer.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-davinci.c | 70 +++++++++++++----------------------------------
 1 file changed, 19 insertions(+), 51 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 02fb967..5b164e5 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -467,6 +467,20 @@ static void davinci_spi_cleanup(struct spi_device *spi)
 		kfree(spicfg);
 }
 
+static inline bool __davinci_spi_can_dma(struct spi_device *spi)
+{
+	struct davinci_spi_config *spicfg = spi->controller_data;
+
+	return spicfg ? spicfg->io_type == SPI_IO_TYPE_DMA : false;
+}
+
+static bool davinci_spi_can_dma(struct spi_master *master,
+				struct spi_device *spi,
+				struct spi_transfer *xfer)
+{
+	return __davinci_spi_can_dma(spi);
+}
+
 static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
 {
 	struct device *sdev = dspi->bitbang.master->dev.parent;
@@ -581,8 +595,6 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	struct davinci_spi_config *spicfg;
 	struct davinci_spi_platform_data *pdata;
 	unsigned uninitialized_var(rx_buf_count);
-	void *dummy_buf = NULL;
-	struct scatterlist sg_rx, sg_tx;
 
 	dspi = spi_master_get_devdata(spi->master);
 	pdata = &dspi->pdata;
@@ -630,51 +642,18 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 		};
 		struct dma_async_tx_descriptor *rxdesc;
 		struct dma_async_tx_descriptor *txdesc;
-		void *buf;
-
-		dummy_buf = kzalloc(t->len, GFP_KERNEL);
-		if (!dummy_buf)
-			goto err_alloc_dummy_buf;
 
 		dmaengine_slave_config(dspi->dma_rx, &dma_rx_conf);
 		dmaengine_slave_config(dspi->dma_tx, &dma_tx_conf);
 
-		sg_init_table(&sg_rx, 1);
-		if (!t->rx_buf)
-			buf = dummy_buf;
-		else
-			buf = t->rx_buf;
-		t->rx_dma = dma_map_single(&spi->dev, buf,
-				t->len, DMA_FROM_DEVICE);
-		if (dma_mapping_error(&spi->dev, !t->rx_dma)) {
-			ret = -EFAULT;
-			goto err_rx_map;
-		}
-		sg_dma_address(&sg_rx) = t->rx_dma;
-		sg_dma_len(&sg_rx) = t->len;
-
-		sg_init_table(&sg_tx, 1);
-		if (!t->tx_buf)
-			buf = dummy_buf;
-		else
-			buf = (void *)t->tx_buf;
-		t->tx_dma = dma_map_single(&spi->dev, buf,
-				t->len, DMA_TO_DEVICE);
-		if (dma_mapping_error(&spi->dev, t->tx_dma)) {
-			ret = -EFAULT;
-			goto err_tx_map;
-		}
-		sg_dma_address(&sg_tx) = t->tx_dma;
-		sg_dma_len(&sg_tx) = t->len;
-
 		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
-				&sg_rx, 1, DMA_DEV_TO_MEM,
+				t->rx_sg.sgl, t->rx_sg.nents, DMA_DEV_TO_MEM,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!rxdesc)
 			goto err_desc;
 
 		txdesc = dmaengine_prep_slave_sg(dspi->dma_tx,
-				&sg_tx, 1, DMA_MEM_TO_DEV,
+				t->tx_sg.sgl, t->tx_sg.nents, DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!txdesc)
 			goto err_desc;
@@ -710,16 +689,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	}
 
 	clear_io_bits(dspi->base + SPIINT, SPIINT_MASKALL);
-	if (spicfg->io_type == SPI_IO_TYPE_DMA) {
+	if (spicfg->io_type == SPI_IO_TYPE_DMA)
 		clear_io_bits(dspi->base + SPIINT, SPIINT_DMA_REQ_EN);
 
-		dma_unmap_single(&spi->dev, t->rx_dma,
-				t->len, DMA_FROM_DEVICE);
-		dma_unmap_single(&spi->dev, t->tx_dma,
-				t->len, DMA_TO_DEVICE);
-		kfree(dummy_buf);
-	}
-
 	clear_io_bits(dspi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
 	set_io_bits(dspi->base + SPIGCR1, SPIGCR1_POWERDOWN_MASK);
 
@@ -742,12 +714,6 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	return t->len;
 
 err_desc:
-	dma_unmap_single(&spi->dev, t->tx_dma, t->len, DMA_TO_DEVICE);
-err_tx_map:
-	dma_unmap_single(&spi->dev, t->rx_dma, t->len, DMA_FROM_DEVICE);
-err_rx_map:
-	kfree(dummy_buf);
-err_alloc_dummy_buf:
 	return ret;
 }
 
@@ -988,8 +954,10 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	master->bus_num = pdev->id;
 	master->num_chipselect = pdata->num_chipselect;
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 16);
+	master->flags = (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX);
 	master->setup = davinci_spi_setup;
 	master->cleanup = davinci_spi_cleanup;
+	master->can_dma = davinci_spi_can_dma;
 
 	dspi->bitbang.chipselect = davinci_spi_chipselect;
 	dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
-- 
2.7.4

[PATCH v2 1/6] spi: davinci: Use SPI framework to handle DMA mapping

[Mark Brown]

On Fri, Feb 17, 2017 at 11:38:19AM +0100, Frode Isaksen wrote:

> +static inline bool __davinci_spi_can_dma(struct spi_device *spi)
> +{
> +	struct davinci_spi_config *spicfg = spi->controller_data;
> +
> +	return spicfg ? spicfg->io_type == SPI_IO_TYPE_DMA : false;
> +}
> +
> +static bool davinci_spi_can_dma(struct spi_master *master,
> +				struct spi_device *spi,
> +				struct spi_transfer *xfer)
> +{
> +	return __davinci_spi_can_dma(spi);
> +}

Why the wrapper function?  __davinci_spi_can_dma has exactly one user
immediately below it.  Also please write normal if statements, it really
helps with legibility.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170219/ac1c20df/attachment.sig>

[PATCH v2 2/6] spi: davinci: enable DMA when channels are defined in DT

[Frode Isaksen]

From: Fabien Parent <fparent@baylibre.com>

When booting with DT the SPI driver is always using
the SPI_IO_TYPE_INTR mode to transfer data even if DMA channels are
defined in the DT.

This commit changes the behaviour to select the SPI_IO_TYPE_DMA mode
if DMA channels are defined in the DT and will keep SPI_IO_TYPE_INTR
if the channels are not defined in it.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-davinci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 5b164e5..9823908 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -389,6 +389,7 @@ static int davinci_spi_of_setup(struct spi_device *spi)
 {
 	struct davinci_spi_config *spicfg = spi->controller_data;
 	struct device_node *np = spi->dev.of_node;
+	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
 	u32 prop;
 
 	if (spicfg == NULL && np) {
@@ -400,6 +401,9 @@ static int davinci_spi_of_setup(struct spi_device *spi)
 		if (!of_property_read_u32(np, "ti,spi-wdelay", &prop))
 			spicfg->wdelay = (u8)prop;
 		spi->controller_data = spicfg;
+
+		if (dspi->dma_rx && dspi->dma_tx)
+			spicfg->io_type = SPI_IO_TYPE_DMA;
 	}
 
 	return 0;
-- 
2.7.4

[PATCH v2 2/6] spi: davinci: enable DMA when channels are defined in DT

[Mark Brown]

On Fri, Feb 17, 2017 at 11:38:20AM +0100, Frode Isaksen wrote:
> From: Fabien Parent <fparent@baylibre.com>
> 
> When booting with DT the SPI driver is always using
> the SPI_IO_TYPE_INTR mode to transfer data even if DMA channels are
> defined in the DT.

This looks good but I'll hold off on applying it without patch 1 in case
it introduces regressions for existing systems due to switching to DMA
but not handling vmalloc() mappings.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170219/5355c65d/attachment.sig>

[PATCH v2 3/6] spi: davinci: use rx buffer as dummy tx buffer

[Frode Isaksen]

When doing rx-only transfer, the transfer will fail
if the number of SG entries exceeds 20.
This happens because the eDMA DMA engine is limited
to 20 SG entries in one transaction, and when the
DMA transcation is resumed (which takes > 150us),
rx errors occurs because the slave is still transmitting.
Fix this by using the rx buffer as the dummy tx buffer,
so that resuming the rx transcation happens at the same
time as resuming the tx transcation.

Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-davinci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 9823908..2632ae0 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -656,6 +656,12 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 		if (!rxdesc)
 			goto err_desc;
 
+		if (!t->tx_buf) {
+			/* use rx buffer as dummy tx buffer */
+			t->tx_sg.sgl = t->rx_sg.sgl;
+			t->tx_sg.nents = t->rx_sg.nents;
+		}
+
 		txdesc = dmaengine_prep_slave_sg(dspi->dma_tx,
 				t->tx_sg.sgl, t->tx_sg.nents, DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -958,7 +964,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	master->bus_num = pdev->id;
 	master->num_chipselect = pdata->num_chipselect;
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 16);
-	master->flags = (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX);
+	master->flags = SPI_MASTER_MUST_RX;
 	master->setup = davinci_spi_setup;
 	master->cleanup = davinci_spi_cleanup;
 	master->can_dma = davinci_spi_can_dma;
-- 
2.7.4

Applied "spi: davinci: use rx buffer as dummy tx buffer" to the spi tree

[Mark Brown]

The patch

   spi: davinci: use rx buffer as dummy tx buffer

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

>From 6b3a631e7f8eca75a987ed760898d28fb3628143 Mon Sep 17 00:00:00 2001
From: Frode Isaksen <fisaksen@baylibre.com>
Date: Thu, 23 Feb 2017 19:01:58 +0100
Subject: [PATCH] spi: davinci: use rx buffer as dummy tx buffer

When doing rx-only transfer, the transfer will fail
if the number of SG entries exceeds 20.
This happens because the eDMA DMA engine is limited
to 20 SG entries in one transaction, and when the
DMA transcation is resumed (which takes > 150us),
rx errors occurs because the slave is still transmitting.
Fix this by using the rx buffer as the dummy tx buffer,
so that resuming the rx transcation happens at the same
time as resuming the tx transcation.

Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-davinci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 1e24395a04f2..ca122165a3c6 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -655,6 +655,12 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 		if (!rxdesc)
 			goto err_desc;
 
+		if (!t->tx_buf) {
+			/* use rx buffer as dummy tx buffer */
+			t->tx_sg.sgl = t->rx_sg.sgl;
+			t->tx_sg.nents = t->rx_sg.nents;
+		}
+
 		txdesc = dmaengine_prep_slave_sg(dspi->dma_tx,
 				t->tx_sg.sgl, t->tx_sg.nents, DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -957,7 +963,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	master->bus_num = pdev->id;
 	master->num_chipselect = pdata->num_chipselect;
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 16);
-	master->flags = (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX);
+	master->flags = SPI_MASTER_MUST_RX;
 	master->setup = davinci_spi_setup;
 	master->cleanup = davinci_spi_cleanup;
 	master->can_dma = davinci_spi_can_dma;
-- 
2.11.0

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

This CPU has VIVT caches, so need to flush the cache
for vmalloc'ed buffers, since the address may be aliased
(same phyiscal address used by multiple virtual addresses).
This fixes errors when mounting and reading/writing UBIFS
volumes with SPI NOR flash.

Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-davinci.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 2632ae0..b69a370 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -29,6 +29,7 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
 #include <linux/slab.h>
+#include <asm/cacheflush.h>
 
 #include <linux/platform_data/spi-davinci.h>
 
@@ -650,6 +651,10 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 		dmaengine_slave_config(dspi->dma_rx, &dma_rx_conf);
 		dmaengine_slave_config(dspi->dma_tx, &dma_tx_conf);
 
+		if (is_vmalloc_addr(t->rx_buf))
+			/* VIVT cache: flush since addr. may be aliased */
+			flush_kernel_vmap_range((void *)t->rx_buf, t->len);
+
 		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
 				t->rx_sg.sgl, t->rx_sg.nents, DMA_DEV_TO_MEM,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -660,7 +665,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 			/* use rx buffer as dummy tx buffer */
 			t->tx_sg.sgl = t->rx_sg.sgl;
 			t->tx_sg.nents = t->rx_sg.nents;
-		}
+		} else if (is_vmalloc_addr(t->tx_buf))
+			/* VIVT cache: flush since addr. may be aliased */
+			flush_kernel_vmap_range((void *)t->tx_buf, t->len);
 
 		txdesc = dmaengine_prep_slave_sg(dspi->dma_tx,
 				t->tx_sg.sgl, t->tx_sg.nents, DMA_MEM_TO_DEV,
@@ -699,8 +706,12 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	}
 
 	clear_io_bits(dspi->base + SPIINT, SPIINT_MASKALL);
-	if (spicfg->io_type == SPI_IO_TYPE_DMA)
+	if (spicfg->io_type == SPI_IO_TYPE_DMA) {
 		clear_io_bits(dspi->base + SPIINT, SPIINT_DMA_REQ_EN);
+		if (is_vmalloc_addr(t->rx_buf))
+			/* VIVT cache: invalidate since addr. may be aliased */
+			invalidate_kernel_vmap_range((void *)t->rx_buf, t->len);
+	}
 
 	clear_io_bits(dspi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
 	set_io_bits(dspi->base + SPIGCR1, SPIGCR1_POWERDOWN_MASK);
-- 
2.7.4

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Vignesh R]

On Friday 17 February 2017 04:08 PM, Frode Isaksen wrote:
> This CPU has VIVT caches, so need to flush the cache
> for vmalloc'ed buffers, since the address may be aliased
> (same phyiscal address used by multiple virtual addresses).
> This fixes errors when mounting and reading/writing UBIFS
> volumes with SPI NOR flash.
> 
> Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
> ---
>  drivers/spi/spi-davinci.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index 2632ae0..b69a370 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -29,6 +29,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi_bitbang.h>
>  #include <linux/slab.h>
> +#include <asm/cacheflush.h>
>  
>  #include <linux/platform_data/spi-davinci.h>
>  
> @@ -650,6 +651,10 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>  		dmaengine_slave_config(dspi->dma_rx, &dma_rx_conf);
>  		dmaengine_slave_config(dspi->dma_tx, &dma_tx_conf);
>  
> +		if (is_vmalloc_addr(t->rx_buf))
> +			/* VIVT cache: flush since addr. may be aliased */
> +			flush_kernel_vmap_range((void *)t->rx_buf, t->len);
> +
>  		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
>  				t->rx_sg.sgl, t->rx_sg.nents, DMA_DEV_TO_MEM,
>  				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> @@ -660,7 +665,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>  			/* use rx buffer as dummy tx buffer */
>  			t->tx_sg.sgl = t->rx_sg.sgl;
>  			t->tx_sg.nents = t->rx_sg.nents;
> -		}
> +		} else if (is_vmalloc_addr(t->tx_buf))
> +			/* VIVT cache: flush since addr. may be aliased */
> +			flush_kernel_vmap_range((void *)t->tx_buf, t->len);
>  

SPI core calls dma_unmap_sg(), that is supposed to flush caches.
If flush_kernel_vmap_range() call is required here to flush actual cache
lines, then what does dma_unmap_* calls in SPI core end up flushing?
Will it flush a different alias of same physical address? If so, isn't
that undesired?

>  		txdesc = dmaengine_prep_slave_sg(dspi->dma_tx,
>  				t->tx_sg.sgl, t->tx_sg.nents, DMA_MEM_TO_DEV,
> @@ -699,8 +706,12 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>  	}
>  
>  	clear_io_bits(dspi->base + SPIINT, SPIINT_MASKALL);
> -	if (spicfg->io_type == SPI_IO_TYPE_DMA)
> +	if (spicfg->io_type == SPI_IO_TYPE_DMA) {
>  		clear_io_bits(dspi->base + SPIINT, SPIINT_DMA_REQ_EN);
> +		if (is_vmalloc_addr(t->rx_buf))
> +			/* VIVT cache: invalidate since addr. may be aliased */
> +			invalidate_kernel_vmap_range((void *)t->rx_buf, t->len);
> +	}
>  
>  	clear_io_bits(dspi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
>  	set_io_bits(dspi->base + SPIGCR1, SPIGCR1_POWERDOWN_MASK);
> 

-- 
Regards
Vignesh

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 04:27:28PM +0530, Vignesh R wrote:
> On Friday 17 February 2017 04:08 PM, Frode Isaksen wrote:
> > @@ -650,6 +651,10 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
> >  		dmaengine_slave_config(dspi->dma_rx, &dma_rx_conf);
> >  		dmaengine_slave_config(dspi->dma_tx, &dma_tx_conf);
> >  
> > +		if (is_vmalloc_addr(t->rx_buf))
> > +			/* VIVT cache: flush since addr. may be aliased */
> > +			flush_kernel_vmap_range((void *)t->rx_buf, t->len);
> > +
> >  		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
> >  				t->rx_sg.sgl, t->rx_sg.nents, DMA_DEV_TO_MEM,
> >  				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > @@ -660,7 +665,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
> >  			/* use rx buffer as dummy tx buffer */
> >  			t->tx_sg.sgl = t->rx_sg.sgl;
> >  			t->tx_sg.nents = t->rx_sg.nents;
> > -		}
> > +		} else if (is_vmalloc_addr(t->tx_buf))
> > +			/* VIVT cache: flush since addr. may be aliased */
> > +			flush_kernel_vmap_range((void *)t->tx_buf, t->len);
> >  
> 
> SPI core calls dma_unmap_sg(), that is supposed to flush caches.
> If flush_kernel_vmap_range() call is required here to flush actual cache
> lines, then what does dma_unmap_* calls in SPI core end up flushing?

The DMA API deals with the _kernel_ lowmem mapping.  It has no knowledge
of any other aliases in the system.  When you have a VIVT cache (as all
old ARM CPUs have) then if you access the memory through a different
alias from the kernel lowmem mapping (iow, vmalloc) then the DMA API
can't help you.

However, the correct place to use flush_kernel_vmap_range() etc is not
in drivers - it's supposed to be done in the callers that know that
the memory is aliased.

For full details on these flushing functions, see cachetlb.txt.  This
does not remove the requirement to also use the DMA API.

=== cachetlb.txt ===

The final category of APIs is for I/O to deliberately aliased address
ranges inside the kernel.  Such aliases are set up by use of the
vmap/vmalloc API.  Since kernel I/O goes via physical pages, the I/O
subsystem assumes that the user mapping and kernel offset mapping are
the only aliases.  This isn't true for vmap aliases, so anything in
the kernel trying to do I/O to vmap areas must manually manage
coherency.  It must do this by flushing the vmap range before doing
I/O and invalidating it after the I/O returns.

  void flush_kernel_vmap_range(void *vaddr, int size)
       flushes the kernel cache for a given virtual address range in
       the vmap area.  This is to make sure that any data the kernel
       modified in the vmap range is made visible to the physical
       page.  The design is to make this area safe to perform I/O on.
       Note that this API does *not* also flush the offset map alias
       of the area.

  void invalidate_kernel_vmap_range(void *vaddr, int size) invalidates
       the cache for a given virtual address range in the vmap area
       which prevents the processor from making the cache stale by
       speculatively reading data while the I/O was occurring to the
       physical pages.  This is only necessary for data reads into the
       vmap area.


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

On 17/02/2017 12:22, Russell King - ARM Linux wrote:
> On Fri, Feb 17, 2017 at 04:27:28PM +0530, Vignesh R wrote:
>> On Friday 17 February 2017 04:08 PM, Frode Isaksen wrote:
>>> @@ -650,6 +651,10 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>>>  		dmaengine_slave_config(dspi->dma_rx, &dma_rx_conf);
>>>  		dmaengine_slave_config(dspi->dma_tx, &dma_tx_conf);
>>>  
>>> +		if (is_vmalloc_addr(t->rx_buf))
>>> +			/* VIVT cache: flush since addr. may be aliased */
>>> +			flush_kernel_vmap_range((void *)t->rx_buf, t->len);
>>> +
>>>  		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
>>>  				t->rx_sg.sgl, t->rx_sg.nents, DMA_DEV_TO_MEM,
>>>  				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>> @@ -660,7 +665,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
>>>  			/* use rx buffer as dummy tx buffer */
>>>  			t->tx_sg.sgl = t->rx_sg.sgl;
>>>  			t->tx_sg.nents = t->rx_sg.nents;
>>> -		}
>>> +		} else if (is_vmalloc_addr(t->tx_buf))
>>> +			/* VIVT cache: flush since addr. may be aliased */
>>> +			flush_kernel_vmap_range((void *)t->tx_buf, t->len);
>>>  
>> SPI core calls dma_unmap_sg(), that is supposed to flush caches.
>> If flush_kernel_vmap_range() call is required here to flush actual cache
>> lines, then what does dma_unmap_* calls in SPI core end up flushing?
> The DMA API deals with the _kernel_ lowmem mapping.  It has no knowledge
> of any other aliases in the system.  When you have a VIVT cache (as all
> old ARM CPUs have) then if you access the memory through a different
> alias from the kernel lowmem mapping (iow, vmalloc) then the DMA API
> can't help you.
>
> However, the correct place to use flush_kernel_vmap_range() etc is not
> in drivers - it's supposed to be done in the callers that know that
> the memory is aliased.
OK, so this should be done in the ubifs layer instead ? xfs already does this, but no other fs.

Thanks,
Frode

>
> For full details on these flushing functions, see cachetlb.txt.  This
> does not remove the requirement to also use the DMA API.
>
> === cachetlb.txt ===
>
> The final category of APIs is for I/O to deliberately aliased address
> ranges inside the kernel.  Such aliases are set up by use of the
> vmap/vmalloc API.  Since kernel I/O goes via physical pages, the I/O
> subsystem assumes that the user mapping and kernel offset mapping are
> the only aliases.  This isn't true for vmap aliases, so anything in
> the kernel trying to do I/O to vmap areas must manually manage
> coherency.  It must do this by flushing the vmap range before doing
> I/O and invalidating it after the I/O returns.
>
>   void flush_kernel_vmap_range(void *vaddr, int size)
>        flushes the kernel cache for a given virtual address range in
>        the vmap area.  This is to make sure that any data the kernel
>        modified in the vmap range is made visible to the physical
>        page.  The design is to make this area safe to perform I/O on.
>        Note that this API does *not* also flush the offset map alias
>        of the area.
>
>   void invalidate_kernel_vmap_range(void *vaddr, int size) invalidates
>        the cache for a given virtual address range in the vmap area
>        which prevents the processor from making the cache stale by
>        speculatively reading data while the I/O was occurring to the
>        physical pages.  This is only necessary for data reads into the
>        vmap area.
>
>

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 12:36:17PM +0100, Frode Isaksen wrote:
> On 17/02/2017 12:22, Russell King - ARM Linux wrote:
> > The DMA API deals with the _kernel_ lowmem mapping.  It has no knowledge
> > of any other aliases in the system.  When you have a VIVT cache (as all
> > old ARM CPUs have) then if you access the memory through a different
> > alias from the kernel lowmem mapping (iow, vmalloc) then the DMA API
> > can't help you.
> >
> > However, the correct place to use flush_kernel_vmap_range() etc is not
> > in drivers - it's supposed to be done in the callers that know that
> > the memory is aliased.
> 
> OK, so this should be done in the ubifs layer instead ? xfs already does
> this, but no other fs.

These APIs were created when XFS was being used on older ARMs and people
experienced corruption.  XFS was the only filesystem driver which wanted
to do this (horrid, imho) DMA to memory that it accessed via a vmalloc
area mapping.

If ubifs is also doing this, it's followed XFS down the same route, but
ignored the need for additional flushing.

The down-side to adding this at the filesystem layer is that you get the
impact whether or not the driver does DMA.  However, for XFS that's
absolutely necessary, as block devices will transfer to the kernel lowmem
mapping, which itself will alias with the vmalloc area mapping.

SPI is rather another special case - rather than SPI following the
established mechanism of passing data references via scatterlists or
similar, it also passes them via virtual addresses, which means SPI
can directly access the vmalloc area when performing PIO.  This
really makes the problem more complex, because it means that if you
do have a SPI driver that does that, it's going to be reading/writing
direct from vmalloc space.

That's not a problem as long as the data is only accessed via vmalloc
space, but it will definitely go totally wrong if the data is
subsequently mapped into userspace.

The other important thing to realise is that the interfaces in
cachetlb.txt assume that it's the lowmem mapping that will be accessed,
and the IO device will push that data out to physical memory (either via
the DMA API, or flush_kernel_dcache_page()).  That's not true of SPI,
as it passes virtual addresses around.

So... overall, I'm not sure that this problem is properly solvable given
SPIs insistance on passing virtual addresses and the differences in this
area between SPI and block.

What I'm quite sure about is that adding yet more cache flushing
interfaces for legacy cache types really isn't the way forward.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

On 17/02/2017 13:07, Russell King - ARM Linux wrote:
> On Fri, Feb 17, 2017 at 12:36:17PM +0100, Frode Isaksen wrote:
>> On 17/02/2017 12:22, Russell King - ARM Linux wrote:
>>> The DMA API deals with the _kernel_ lowmem mapping.  It has no knowledge
>>> of any other aliases in the system.  When you have a VIVT cache (as all
>>> old ARM CPUs have) then if you access the memory through a different
>>> alias from the kernel lowmem mapping (iow, vmalloc) then the DMA API
>>> can't help you.
>>>
>>> However, the correct place to use flush_kernel_vmap_range() etc is not
>>> in drivers - it's supposed to be done in the callers that know that
>>> the memory is aliased.
>> OK, so this should be done in the ubifs layer instead ? xfs already does
>> this, but no other fs.
> These APIs were created when XFS was being used on older ARMs and people
> experienced corruption.  XFS was the only filesystem driver which wanted
> to do this (horrid, imho) DMA to memory that it accessed via a vmalloc
> area mapping.
>
> If ubifs is also doing this, it's followed XFS down the same route, but
> ignored the need for additional flushing.
>
> The down-side to adding this at the filesystem layer is that you get the
> impact whether or not the driver does DMA.  However, for XFS that's
> absolutely necessary, as block devices will transfer to the kernel lowmem
> mapping, which itself will alias with the vmalloc area mapping.
>
> SPI is rather another special case - rather than SPI following the
> established mechanism of passing data references via scatterlists or
> similar, it also passes them via virtual addresses, which means SPI
> can directly access the vmalloc area when performing PIO.  This
> really makes the problem more complex, because it means that if you
> do have a SPI driver that does that, it's going to be reading/writing
> direct from vmalloc space.
>
> That's not a problem as long as the data is only accessed via vmalloc
> space, but it will definitely go totally wrong if the data is
> subsequently mapped into userspace.
Thanks a lot for explaining...
It often(2/5) goes wrong within this simple function called when mounting the UBIFS filesystem w/o a priori any userspace interaction:
fs/ubifs/lpt.c:
static int read_ltab(struct ubifs_info *c)
{
    int err;
    void *buf;
    int retry=1;

    buf = vmalloc(c->ltab_sz);
    if (!buf)
        return -ENOMEM;
    err = ubifs_leb_read(c, c->ltab_lnum, buf, c->ltab_offs, c->ltab_sz, 1);
    if (err)
        goto out;
retry:
    err = unpack_ltab(c, buf);
    if (err && retry--) {
        pr_err("%s: flush cache %p[%d] and retry\n", __func__, buf, c->ltab_sz);
        invalidate_kernel_vmap_range(buf, c->ltab_sz);
        goto retry;
    }
out:
    vfree(buf);
    return err;
}
The retry code is added by me and with this code there is no error when retrying after flushing the cache. As you can see, the vmalloc'es buffer is allocated and freed within this function.
read_ltab: flush cache c9543000[11] and retry
Ok after this !!

Thanks again,
Frode
>
> The other important thing to realise is that the interfaces in
> cachetlb.txt assume that it's the lowmem mapping that will be accessed,
> and the IO device will push that data out to physical memory (either via
> the DMA API, or flush_kernel_dcache_page()).  That's not true of SPI,
> as it passes virtual addresses around.
>
> So... overall, I'm not sure that this problem is properly solvable given
> SPIs insistance on passing virtual addresses and the differences in this
> area between SPI and block.
>
> What I'm quite sure about is that adding yet more cache flushing
> interfaces for legacy cache types really isn't the way forward.
>

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Vignesh R]

On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
[...]
> SPI is rather another special case - rather than SPI following the 
> established mechanism of passing data references via scatterlists or 
> similar, it also passes them via virtual addresses, which means SPI 
> can directly access the vmalloc area when performing PIO.  This 
> really makes the problem more complex, because it means that if you 
> do have a SPI driver that does that, it's going to be
> reading/writing direct from vmalloc space.
> 
> That's not a problem as long as the data is only accessed via
> vmalloc space, but it will definitely go totally wrong if the data
> is subsequently mapped into userspace.
> 
> The other important thing to realise is that the interfaces in 
> cachetlb.txt assume that it's the lowmem mapping that will be
> accessed, and the IO device will push that data out to physical
> memory (either via the DMA API, or flush_kernel_dcache_page()).
> That's not true of SPI, as it passes virtual addresses around.
> 
> So... overall, I'm not sure that this problem is properly solvable
> given SPIs insistance on passing virtual addresses and the
> differences in this area between SPI and block.
> 

I am debugging another issue with UBIFS wherein pages allocated by
vmalloc are in highmem region that are not addressable using 32 bit
addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
buffers at all.
When dma_map_sg() is called to map these pages by spi_map_buf() the
physical address is just truncated to 32 bit in pfn_to_dma() (as part of
dma_map_sg() call). This results in random crashes as DMA starts
accessing random memory during SPI read.

Given, the above problem and also issue surrounding VIVT caches, I am
thinking of may be using pre-allocated fixed size bounce buffer to
handle buffers not in lowmem mapping.
I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
running at 76.8MHz and do not see any significant degradation in
performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
buffers only during initial preparing and mounting phase and not during
file read/write.

So, is it an acceptable solution to modify spi_map_buf() to use bounce
buffer when buffer does not belong to lowmem region?

-- 
Regards
Vignesh

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

On 20/02/2017 07:55, Vignesh R wrote:
>
> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
> [...]
>> SPI is rather another special case - rather than SPI following the 
>> established mechanism of passing data references via scatterlists or 
>> similar, it also passes them via virtual addresses, which means SPI 
>> can directly access the vmalloc area when performing PIO.  This 
>> really makes the problem more complex, because it means that if you 
>> do have a SPI driver that does that, it's going to be
>> reading/writing direct from vmalloc space.
>>
>> That's not a problem as long as the data is only accessed via
>> vmalloc space, but it will definitely go totally wrong if the data
>> is subsequently mapped into userspace.
>>
>> The other important thing to realise is that the interfaces in 
>> cachetlb.txt assume that it's the lowmem mapping that will be
>> accessed, and the IO device will push that data out to physical
>> memory (either via the DMA API, or flush_kernel_dcache_page()).
>> That's not true of SPI, as it passes virtual addresses around.
>>
>> So... overall, I'm not sure that this problem is properly solvable
>> given SPIs insistance on passing virtual addresses and the
>> differences in this area between SPI and block.
>>
> I am debugging another issue with UBIFS wherein pages allocated by
> vmalloc are in highmem region that are not addressable using 32 bit
> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
> buffers at all.
> When dma_map_sg() is called to map these pages by spi_map_buf() the
> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
> dma_map_sg() call). This results in random crashes as DMA starts
> accessing random memory during SPI read.
>
> Given, the above problem and also issue surrounding VIVT caches, I am
> thinking of may be using pre-allocated fixed size bounce buffer to
> handle buffers not in lowmem mapping.
> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
> running at 76.8MHz and do not see any significant degradation in
> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
> buffers only during initial preparing and mounting phase and not during
> file read/write.
I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?

Thanks,
Frode
>
> So, is it an acceptable solution to modify spi_map_buf() to use bounce
> buffer when buffer does not belong to lowmem region?
>

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Vignesh R]

On Monday 20 February 2017 02:56 PM, Frode Isaksen wrote:
> 
> 
> On 20/02/2017 07:55, Vignesh R wrote:
>>
>> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
>> [...]
>>> SPI is rather another special case - rather than SPI following the 
>>> established mechanism of passing data references via scatterlists or 
>>> similar, it also passes them via virtual addresses, which means SPI 
>>> can directly access the vmalloc area when performing PIO.  This 
>>> really makes the problem more complex, because it means that if you 
>>> do have a SPI driver that does that, it's going to be
>>> reading/writing direct from vmalloc space.
>>>
>>> That's not a problem as long as the data is only accessed via
>>> vmalloc space, but it will definitely go totally wrong if the data
>>> is subsequently mapped into userspace.
>>>
>>> The other important thing to realise is that the interfaces in 
>>> cachetlb.txt assume that it's the lowmem mapping that will be
>>> accessed, and the IO device will push that data out to physical
>>> memory (either via the DMA API, or flush_kernel_dcache_page()).
>>> That's not true of SPI, as it passes virtual addresses around.
>>>
>>> So... overall, I'm not sure that this problem is properly solvable
>>> given SPIs insistance on passing virtual addresses and the
>>> differences in this area between SPI and block.
>>>
>> I am debugging another issue with UBIFS wherein pages allocated by
>> vmalloc are in highmem region that are not addressable using 32 bit
>> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
>> buffers at all.
>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>> dma_map_sg() call). This results in random crashes as DMA starts
>> accessing random memory during SPI read.
>>
>> Given, the above problem and also issue surrounding VIVT caches, I am
>> thinking of may be using pre-allocated fixed size bounce buffer to
>> handle buffers not in lowmem mapping.
>> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
>> running at 76.8MHz and do not see any significant degradation in
>> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
>> buffers only during initial preparing and mounting phase and not during
>> file read/write.
> I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?

read_ltab() isn't the only place where vmalloc() is used. A quick grep
for vmalloc on fs/ubifs/ shows about ~19 occurrence. I guess every
vmalloc() call can potentially allocate memory from highmem and might
potentially cause issue for VIVT and such aliasing caches.
Fixing just one such case isn't going to help IMHO.


-- 
Regards
Vignesh

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

On 20/02/2017 10:47, Vignesh R wrote:
>
> On Monday 20 February 2017 02:56 PM, Frode Isaksen wrote:
>>
>> On 20/02/2017 07:55, Vignesh R wrote:
>>> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
>>> [...]
>>>> SPI is rather another special case - rather than SPI following the 
>>>> established mechanism of passing data references via scatterlists or 
>>>> similar, it also passes them via virtual addresses, which means SPI 
>>>> can directly access the vmalloc area when performing PIO.  This 
>>>> really makes the problem more complex, because it means that if you 
>>>> do have a SPI driver that does that, it's going to be
>>>> reading/writing direct from vmalloc space.
>>>>
>>>> That's not a problem as long as the data is only accessed via
>>>> vmalloc space, but it will definitely go totally wrong if the data
>>>> is subsequently mapped into userspace.
>>>>
>>>> The other important thing to realise is that the interfaces in 
>>>> cachetlb.txt assume that it's the lowmem mapping that will be
>>>> accessed, and the IO device will push that data out to physical
>>>> memory (either via the DMA API, or flush_kernel_dcache_page()).
>>>> That's not true of SPI, as it passes virtual addresses around.
>>>>
>>>> So... overall, I'm not sure that this problem is properly solvable
>>>> given SPIs insistance on passing virtual addresses and the
>>>> differences in this area between SPI and block.
>>>>
>>> I am debugging another issue with UBIFS wherein pages allocated by
>>> vmalloc are in highmem region that are not addressable using 32 bit
>>> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
>>> buffers at all.
>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>> dma_map_sg() call). This results in random crashes as DMA starts
>>> accessing random memory during SPI read.
>>>
>>> Given, the above problem and also issue surrounding VIVT caches, I am
>>> thinking of may be using pre-allocated fixed size bounce buffer to
>>> handle buffers not in lowmem mapping.
>>> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
>>> running at 76.8MHz and do not see any significant degradation in
>>> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
>>> buffers only during initial preparing and mounting phase and not during
>>> file read/write.
>> I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?
> read_ltab() isn't the only place where vmalloc() is used. A quick grep
> for vmalloc on fs/ubifs/ shows about ~19 occurrence. I guess every
> vmalloc() call can potentially allocate memory from highmem and might
> potentially cause issue for VIVT and such aliasing caches.
> Fixing just one such case isn't going to help IMHO.
Of course fixing it only in one place is of course not going to help..
For the moment there are 3 solutions to the UBIFS DMA problem:
1) Always use bounce buffer for vmalloc'ed buffers - impacts everyone.
2) Remove use of vmalloc'ed buffers in UBIFS - is it possible ?
3) Flush cache in UBIFS when reading/wring vmalloc'ed buffers - does not fix the higmem case.
I will try to see if option 2) is possible.

Thanks,
Frode
>
>

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Vignesh R]

On Monday 20 February 2017 04:04 PM, Frode Isaksen wrote:
> 
> 
> On 20/02/2017 10:47, Vignesh R wrote:
>>
>> On Monday 20 February 2017 02:56 PM, Frode Isaksen wrote:
>>>
>>> On 20/02/2017 07:55, Vignesh R wrote:
>>>> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
[...]
>>>> I am debugging another issue with UBIFS wherein pages allocated by
>>>> vmalloc are in highmem region that are not addressable using 32 bit
>>>> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
>>>> buffers at all.
>>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>>> dma_map_sg() call). This results in random crashes as DMA starts
>>>> accessing random memory during SPI read.
>>>>
>>>> Given, the above problem and also issue surrounding VIVT caches, I am
>>>> thinking of may be using pre-allocated fixed size bounce buffer to
>>>> handle buffers not in lowmem mapping.
>>>> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
>>>> running at 76.8MHz and do not see any significant degradation in
>>>> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
>>>> buffers only during initial preparing and mounting phase and not during
>>>> file read/write.
>>> I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?
>> read_ltab() isn't the only place where vmalloc() is used. A quick grep
>> for vmalloc on fs/ubifs/ shows about ~19 occurrence. I guess every
>> vmalloc() call can potentially allocate memory from highmem and might
>> potentially cause issue for VIVT and such aliasing caches.
>> Fixing just one such case isn't going to help IMHO.
> Of course fixing it only in one place is of course not going to help..
> For the moment there are 3 solutions to the UBIFS DMA problem:
> 1) Always use bounce buffer for vmalloc'ed buffers - impacts everyone.
> 2) Remove use of vmalloc'ed buffers in UBIFS - is it possible ?

Maybe. But what about mtdblock with JFFS2 on top of it, mtdblock still
uses vmalloc'd buffers?


-- 
Regards
Vignesh

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Frode Isaksen]

On 20/02/2017 12:27, Vignesh R wrote:
>
> On Monday 20 February 2017 04:04 PM, Frode Isaksen wrote:
>>
>> On 20/02/2017 10:47, Vignesh R wrote:
>>> On Monday 20 February 2017 02:56 PM, Frode Isaksen wrote:
>>>> On 20/02/2017 07:55, Vignesh R wrote:
>>>>> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
> [...]
>>>>> I am debugging another issue with UBIFS wherein pages allocated by
>>>>> vmalloc are in highmem region that are not addressable using 32 bit
>>>>> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
>>>>> buffers at all.
>>>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>>>> dma_map_sg() call). This results in random crashes as DMA starts
>>>>> accessing random memory during SPI read.
>>>>>
>>>>> Given, the above problem and also issue surrounding VIVT caches, I am
>>>>> thinking of may be using pre-allocated fixed size bounce buffer to
>>>>> handle buffers not in lowmem mapping.
>>>>> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
>>>>> running at 76.8MHz and do not see any significant degradation in
>>>>> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
>>>>> buffers only during initial preparing and mounting phase and not during
>>>>> file read/write.
>>>> I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?
>>> read_ltab() isn't the only place where vmalloc() is used. A quick grep
>>> for vmalloc on fs/ubifs/ shows about ~19 occurrence. I guess every
>>> vmalloc() call can potentially allocate memory from highmem and might
>>> potentially cause issue for VIVT and such aliasing caches.
>>> Fixing just one such case isn't going to help IMHO.
>> Of course fixing it only in one place is of course not going to help..
>> For the moment there are 3 solutions to the UBIFS DMA problem:
>> 1) Always use bounce buffer for vmalloc'ed buffers - impacts everyone.
>> 2) Remove use of vmalloc'ed buffers in UBIFS - is it possible ?
> Maybe. But what about mtdblock with JFFS2 on top of it, mtdblock still
> uses vmalloc'd buffers?
That's why I put the cache flush in the SPI driver in the first try..
FYI, I replaced the use of vmalloc with kmalloc in a few places in the UBIFS layer and it seems to work. The size of the buffers allocated varies from 11 (?) to 65408 bytes.
Maybe the best place to handle this is in the SPI flash drivers(s). These drivers knows that they may handle vmalloc'ed buffers and the should assume that the underlying SPI driver may use DMA.

Thanks,
Frode
>
>

[PATCH v2 4/6] spi: davinci: flush caches when performing DMA

[Vignesh R]

On Monday 20 February 2017 09:19 PM, Frode Isaksen wrote:
> 
> 
> On 20/02/2017 12:27, Vignesh R wrote:
>>
>> On Monday 20 February 2017 04:04 PM, Frode Isaksen wrote:
>>>
>>> On 20/02/2017 10:47, Vignesh R wrote:
>>>> On Monday 20 February 2017 02:56 PM, Frode Isaksen wrote:
>>>>> On 20/02/2017 07:55, Vignesh R wrote:
>>>>>> On Friday 17 February 2017 05:37 PM, Russell King - ARM Linux wrote:
>> [...]
>>>>>> I am debugging another issue with UBIFS wherein pages allocated by
>>>>>> vmalloc are in highmem region that are not addressable using 32 bit
>>>>>> addresses and is backed by LPAE. So, a 32 bit DMA cannot access these
>>>>>> buffers at all.
>>>>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>>>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>>>>> dma_map_sg() call). This results in random crashes as DMA starts
>>>>>> accessing random memory during SPI read.
>>>>>>
>>>>>> Given, the above problem and also issue surrounding VIVT caches, I am
>>>>>> thinking of may be using pre-allocated fixed size bounce buffer to
>>>>>> handle buffers not in lowmem mapping.
>>>>>> I have tried using 64KB pre-allocated buffer on TI DRA74 EVM with QSPI
>>>>>> running at 76.8MHz and do not see any significant degradation in
>>>>>> performance with UBIFS. Mainly because UBIFS seems to use vmalloc'd
>>>>>> buffers only during initial preparing and mounting phase and not during
>>>>>> file read/write.
>>>>> I am seeing a bug caused by VIVT cache in 'read_ltab()' function. In this function, the vmalloc'ed buffer is of size 11. Isn't it better to use kmalloc in this case ?
>>>> read_ltab() isn't the only place where vmalloc() is used. A quick grep
>>>> for vmalloc on fs/ubifs/ shows about ~19 occurrence. I guess every
>>>> vmalloc() call can potentially allocate memory from highmem and might
>>>> potentially cause issue for VIVT and such aliasing caches.
>>>> Fixing just one such case isn't going to help IMHO.
>>> Of course fixing it only in one place is of course not going to help..
>>> For the moment there are 3 solutions to the UBIFS DMA problem:
>>> 1) Always use bounce buffer for vmalloc'ed buffers - impacts everyone.
>>> 2) Remove use of vmalloc'ed buffers in UBIFS - is it possible ?
>> Maybe. But what about mtdblock with JFFS2 on top of it, mtdblock still
>> uses vmalloc'd buffers?
> That's why I put the cache flush in the SPI driver in the first try..
> FYI, I replaced the use of vmalloc with kmalloc in a few places in the UBIFS layer and it seems to work. The size of the buffers allocated varies from 11 (?) to 65408 bytes.

FYI, here is the discussion on DMA + UBIFS:
http://lists.infradead.org/pipermail/linux-mtd/2016-October/069856.html

> Maybe the best place to handle this is in the SPI flash drivers(s). These drivers knows that they may handle vmalloc'ed buffers and the should assume that the underlying SPI driver may use DMA.
> 

I guess so, NAND core already seems to use bounce buffer for vmalloc'd
addresses.


-- 
Regards
Vignesh

[PATCH v2 5/6] spi: davinci: do not use DMA if transfer length is less than 16

[Frode Isaksen]

Higher bitrate and lower CPU load if using PIO in this case.

Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-davinci.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index b69a370..826bff1 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -110,6 +110,8 @@
 #define SPIDEF		0x4c
 #define SPIFMT0		0x50
 
+#define DMA_MIN_BYTES	16
+
 /* SPI Controller driver's private data. */
 struct davinci_spi {
 	struct spi_bitbang	bitbang;
@@ -483,7 +485,7 @@ static bool davinci_spi_can_dma(struct spi_master *master,
 				struct spi_device *spi,
 				struct spi_transfer *xfer)
 {
-	return __davinci_spi_can_dma(spi);
+	return __davinci_spi_can_dma(spi) && (xfer->len >= DMA_MIN_BYTES);
 }
 
 static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
@@ -622,10 +624,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 	reinit_completion(&dspi->done);
 
-	if (spicfg->io_type == SPI_IO_TYPE_INTR)
-		set_io_bits(dspi->base + SPIINT, SPIINT_MASKINT);
-
-	if (spicfg->io_type != SPI_IO_TYPE_DMA) {
+	if (!davinci_spi_can_dma(spi->master, spi, t)) {
+		if (spicfg->io_type != SPI_IO_TYPE_POLL)
+			set_io_bits(dspi->base + SPIINT, SPIINT_MASKINT);
 		/* start the transfer */
 		dspi->wcount--;
 		tx_data = dspi->get_tx(dspi);
@@ -706,7 +707,7 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	}
 
 	clear_io_bits(dspi->base + SPIINT, SPIINT_MASKALL);
-	if (spicfg->io_type == SPI_IO_TYPE_DMA) {
+	if (davinci_spi_can_dma(spi->master, spi, t)) {
 		clear_io_bits(dspi->base + SPIINT, SPIINT_DMA_REQ_EN);
 		if (is_vmalloc_addr(t->rx_buf))
 			/* VIVT cache: invalidate since addr. may be aliased */
-- 
2.7.4

[PATCH v2 5/6] spi: davinci: do not use DMA if transfer length is less than 16

[Arnd Bergmann]

On Fri, Feb 17, 2017 at 11:38 AM, Frode Isaksen <fisaksen@baylibre.com> wrote:
> Higher bitrate and lower CPU load if using PIO in this case.
>
> Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
> ---
>  drivers/spi/spi-davinci.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index b69a370..826bff1 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -110,6 +110,8 @@
>  #define SPIDEF         0x4c
>  #define SPIFMT0                0x50
>
> +#define DMA_MIN_BYTES  16

16 seems low as the cutoff. Have you found this experimentally and
tested that for e.g. 32
bytes there is actually an advantage in using DMA?

    Arnd

[PATCH v2 5/6] spi: davinci: do not use DMA if transfer length is less than 16

[Frode Isaksen]

On 17/02/2017 17:37, Arnd Bergmann wrote:
> On Fri, Feb 17, 2017 at 11:38 AM, Frode Isaksen <fisaksen@baylibre.com> wrote:
>> Higher bitrate and lower CPU load if using PIO in this case.
>>
>> Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
>> ---
>>  drivers/spi/spi-davinci.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index b69a370..826bff1 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -110,6 +110,8 @@
>>  #define SPIDEF         0x4c
>>  #define SPIFMT0                0x50
>>
>> +#define DMA_MIN_BYTES  16
> 16 seems low as the cutoff. Have you found this experimentally and
> tested that for e.g. 32
> bytes there is actually an advantage in using DMA?
Yes, I have tested for different sizes. Actually 32 bytes is better @30MHz, but if the bit rate goes down, DMA wins over PIO. Do you prefer 32 bytes ?

Frode
>
>     Arnd

[PATCH v2 5/6] spi: davinci: do not use DMA if transfer length is less than 16

[Arnd Bergmann]

On Fri, Feb 17, 2017 at 5:43 PM, Frode Isaksen <fisaksen@baylibre.com> wrote:
> On 17/02/2017 17:37, Arnd Bergmann wrote:
>> On Fri, Feb 17, 2017 at 11:38 AM, Frode Isaksen <fisaksen@baylibre.com> wrote:
>> 16 seems low as the cutoff. Have you found this experimentally and
>> tested that for e.g. 32
>> bytes there is actually an advantage in using DMA?
> Yes, I have tested for different sizes. Actually 32 bytes is better @30MHz, but if the bit rate goes down, DMA wins over PIO. Do you prefer 32 bytes ?

No, I don't care about the specific value, just making sure that you had
actually tested this, as I had expected PIO to be faster here.

     Arnd

[PATCH v2 6/6] spi: loopback-test: add option to use vmalloc'ed buffers

[Frode Isaksen]

Using vmalloc'ed buffers will use one SG entry for each page,
that may provoke DMA errors for large transfers.
Also vmalloc'ed buffers may cause errors on CPU's with VIVT cache.
Add this option to catch these errors when testing.

Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
---
 drivers/spi/spi-loopback-test.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 50e620f..86686dd 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -55,6 +55,12 @@ module_param(run_only_test, int, 0);
 MODULE_PARM_DESC(run_only_test,
 		 "only run the test with this number (0-based !)");
 
+/* use vmalloc'ed buffers */
+int use_vmalloc;
+module_param(use_vmalloc, int, 0644);
+MODULE_PARM_DESC(use_vmalloc,
+		 "use vmalloc'ed buffers instead of kmalloc'ed");
+
 /* the actual tests to execute */
 static struct spi_test spi_tests[] = {
 	{
@@ -965,13 +971,19 @@ int spi_test_run_tests(struct spi_device *spi,
 	/* allocate rx/tx buffers of 128kB size without devm
 	 * in the hope that is on a page boundary
 	 */
-	rx = kzalloc(SPI_TEST_MAX_SIZE_PLUS, GFP_KERNEL);
+	if (use_vmalloc)
+		rx = vmalloc(SPI_TEST_MAX_SIZE_PLUS);
+	else
+		rx = kzalloc(SPI_TEST_MAX_SIZE_PLUS, GFP_KERNEL);
 	if (!rx) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	tx = kzalloc(SPI_TEST_MAX_SIZE_PLUS, GFP_KERNEL);
+	if (use_vmalloc)
+		tx = vmalloc(SPI_TEST_MAX_SIZE_PLUS);
+	else
+		tx = kzalloc(SPI_TEST_MAX_SIZE_PLUS, GFP_KERNEL);
 	if (!tx) {
 		ret = -ENOMEM;
 		goto out;
@@ -999,8 +1011,13 @@ int spi_test_run_tests(struct spi_device *spi,
 	}
 
 out:
-	kfree(rx);
-	kfree(tx);
+	if (use_vmalloc) {
+		vfree(rx);
+		vfree(tx);
+	} else {
+		kfree(rx);
+		kfree(tx);
+	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(spi_test_run_tests);
-- 
2.7.4