Enable DMA for daVinci SPI controller

Enable DMA for daVinci SPI controller

[Frode Isaksen]

Enable DMA for the daVinci SPI using the SPI framework DMA mapping
function. 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.

In-Reply-To: 

[PATCH 1/8] 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.
Note that the dummy buffer used when doing one-way transfer, must
still be mapped in this module.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 drivers/spi/spi-davinci.c | 80 ++++++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 32 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 02fb967..84c0d76 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;
@@ -630,7 +644,6 @@ 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)
@@ -639,42 +652,40 @@ 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);
 
-		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;
+		if (!t->rx_buf) {
+			sg_init_table(&sg_rx, 1);
+			t->rx_dma = dma_map_single(&spi->dev, dummy_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_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;
+		if (!t->tx_buf) {
+			t->tx_dma = dma_map_single(&spi->dev, dummy_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;
 		}
-		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 ?: &sg_rx, t->rx_sg.nents ?: 1,
+				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 ?: &sg_tx, t->tx_sg.nents ?: 1,
+				DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (!txdesc)
 			goto err_desc;
@@ -713,10 +724,12 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 	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);
+		if (!t->rx_buf)
+			dma_unmap_single(&spi->dev, t->rx_dma,
+					t->len, DMA_FROM_DEVICE);
+		if (!t->tx_buf)
+			dma_unmap_single(&spi->dev, t->tx_dma,
+					t->len, DMA_TO_DEVICE);
 		kfree(dummy_buf);
 	}
 
@@ -742,9 +755,11 @@ 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);
+	if (!t->tx_buf)
+		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);
+	if (!t->rx_buf)
+		dma_unmap_single(&spi->dev, t->rx_dma, t->len, DMA_FROM_DEVICE);
 err_rx_map:
 	kfree(dummy_buf);
 err_alloc_dummy_buf:
@@ -990,6 +1005,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 16);
 	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

Re: [PATCH 1/8] spi: davinci: Use SPI framework to handle DMA mapping

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

On Fri, Feb 10, 2017 at 04:29:37PM +0100, Frode Isaksen wrote:

> Note that the dummy buffer used when doing one-way transfer, must
> still be mapped in this module.

No, the driver shouldn't be doing this - it should specify if it's
_MUST_RX and/or _MUST_TX and let the core handle it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frode Isaksen]

From: Fabien Parent <fparent-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 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 84c0d76..b7b2da1 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

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Fri, Feb 10, 2017 at 04:29:38PM +0100, Frode Isaksen wrote:

> if the channels are not defined in it.
> 
> Signed-off-by: Fabien Parent <fparent-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---

This patch doesn't have your signoff so I can't do anything with it,
please see SubmittingPatches for details of what the signoff means and
why it's important.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Frode Isaksen]

Limit the transfer size to 20 scatter/gather pages if
DMA is enabled.
The eDMA DMA engine is limited to 20 SG entries in one DMA
transaction. If this number is exceeded, DMA receive fails.
This error occurs with large vmalloc'ed buffers.

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

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index b7b2da1..f1b46f6 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
 	return __davinci_spi_can_dma(spi);
 }
 
+static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
+{
+	/*
+	 * The eDMA DMA engine is limited to 20 SG entries in one DMA
+	 * transaction. If this number is exceeded, DMA receive fails.
+	 * An extra SG entry is needed when the buffer is not page aligned.
+	 */
+	return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;
+}
+
 static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
 {
 	struct device *sdev = dspi->bitbang.master->dev.parent;
@@ -1010,6 +1020,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	master->setup = davinci_spi_setup;
 	master->cleanup = davinci_spi_cleanup;
 	master->can_dma = davinci_spi_can_dma;
+	master->max_transfer_size = davinci_spi_max_transfer_size;
 
 	dspi->bitbang.chipselect = davinci_spi_chipselect;
 	dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
-- 
2.7.4

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Kevin Hilman]

Frode Isaksen <fisaksen-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> writes:

> Limit the transfer size to 20 scatter/gather pages if
> DMA is enabled.
> The eDMA DMA engine is limited to 20 SG entries in one DMA
> transaction. If this number is exceeded, DMA receive fails.
> This error occurs with large vmalloc'ed buffers.
>
> Signed-off-by: Frode Isaksen <fisaksen-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/spi/spi-davinci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index b7b2da1..f1b46f6 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
>  	return __davinci_spi_can_dma(spi);
>  }
>  
> +static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
> +{
> +	/*
> +	 * The eDMA DMA engine is limited to 20 SG entries in one DMA
> +	 * transaction. If this number is exceeded, DMA receive fails.
> +	 * An extra SG entry is needed when the buffer is not page aligned.
> +	 */
> +	return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;

The number here should be a #define.

Also, comment says 20, code says 19.  This should be clarified.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Sekhar Nori]

+ Peter

On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
> Limit the transfer size to 20 scatter/gather pages if
> DMA is enabled.
> The eDMA DMA engine is limited to 20 SG entries in one DMA
> transaction. If this number is exceeded, DMA receive fails.
> This error occurs with large vmalloc'ed buffers.

This needs more explanation because there is support available in edma
driver for long SG lists by breaking them down into transfers using 20
PaRAM entries at a time. If thats not working for you, that needs
further debug.

> 
> Signed-off-by: Frode Isaksen <fisaksen-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Thanks,
Sekhar

> ---
>  drivers/spi/spi-davinci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index b7b2da1..f1b46f6 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
>  	return __davinci_spi_can_dma(spi);
>  }
>  
> +static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
> +{
> +	/*
> +	 * The eDMA DMA engine is limited to 20 SG entries in one DMA
> +	 * transaction. If this number is exceeded, DMA receive fails.
> +	 * An extra SG entry is needed when the buffer is not page aligned.
> +	 */
> +	return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;
> +}
> +
>  static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
>  {
>  	struct device *sdev = dspi->bitbang.master->dev.parent;
> @@ -1010,6 +1020,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
>  	master->setup = davinci_spi_setup;
>  	master->cleanup = davinci_spi_cleanup;
>  	master->can_dma = davinci_spi_can_dma;
> +	master->max_transfer_size = davinci_spi_max_transfer_size;
>  
>  	dspi->bitbang.chipselect = davinci_spi_chipselect;
>  	dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Frode Isaksen]

On 13/02/2017 06:59, Sekhar Nori wrote:
> + Peter
>
> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>> Limit the transfer size to 20 scatter/gather pages if
>> DMA is enabled.
>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>> transaction. If this number is exceeded, DMA receive fails.
>> This error occurs with large vmalloc'ed buffers.
> This needs more explanation because there is support available in edma
> driver for long SG lists by breaking them down into transfers using 20
> PaRAM entries at a time. If thats not working for you, that needs
> further debug.
The SPI controller has a FIFO of only 1 word, so at 1Mbps, filling the 
FIFO will take only 8us. Handling the DMA interrupt and re-programming 
the PaRAM entries takes much longer than that. At 1Mbps, about 50 bytes 
is lost on Rx, @ 2Mbps 100 bytes and @ 5Mbps about 260 bytes hinting 
towards a time setting up a new DMA transfer > 400us. If the Tx and Rx 
buffers are identically aligned there are no errors, because the 
re-programming of the Tx and Rx PaRAM entries happens at the same time.
I have also verified this with a scope. In the Tx direction, there is a 
pause in the transfer of 600us after the 20th SG entrey (when setting up 
new PaRAM entries). Since setting up Rx PaRAM is identical, this shows 
that breaking down the transfer is not working in the Rx direction for 
SPI caused by the relative high bit rate and small FIFO.

Thanks,
Frode
>> Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
> Thanks,
> Sekhar
>
>> ---
>>   drivers/spi/spi-davinci.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index b7b2da1..f1b46f6 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
>>   	return __davinci_spi_can_dma(spi);
>>   }
>>   
>> +static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +	/*
>> +	 * The eDMA DMA engine is limited to 20 SG entries in one DMA
>> +	 * transaction. If this number is exceeded, DMA receive fails.
>> +	 * An extra SG entry is needed when the buffer is not page aligned.
>> +	 */
>> +	return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;
>> +}
>> +
>>   static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
>>   {
>>   	struct device *sdev = dspi->bitbang.master->dev.parent;
>> @@ -1010,6 +1020,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
>>   	master->setup = davinci_spi_setup;
>>   	master->cleanup = davinci_spi_cleanup;
>>   	master->can_dma = davinci_spi_can_dma;
>> +	master->max_transfer_size = davinci_spi_max_transfer_size;
>>   
>>   	dspi->bitbang.chipselect = davinci_spi_chipselect;
>>   	dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
>>

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Sekhar Nori]

On Tuesday 14 February 2017 04:57 PM, Frode Isaksen wrote:
> 
> 
> On 13/02/2017 06:59, Sekhar Nori wrote:
>> + Peter
>>
>> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>>> Limit the transfer size to 20 scatter/gather pages if
>>> DMA is enabled.
>>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>>> transaction. If this number is exceeded, DMA receive fails.
>>> This error occurs with large vmalloc'ed buffers.
>> This needs more explanation because there is support available in edma
>> driver for long SG lists by breaking them down into transfers using 20
>> PaRAM entries at a time. If thats not working for you, that needs
>> further debug.

> The SPI controller has a FIFO of only 1 word, so at 1Mbps, filling the
> FIFO will take only 8us. Handling the DMA interrupt and re-programming
> the PaRAM entries takes much longer than that. At 1Mbps, about 50 bytes
> is lost on Rx, @ 2Mbps 100 bytes and @ 5Mbps about 260 bytes hinting
> towards a time setting up a new DMA transfer > 400us. If the Tx and Rx
> buffers are identically aligned there are no errors, because the
> re-programming of the Tx and Rx PaRAM entries happens at the same time.
> I have also verified this with a scope. In the Tx direction, there is a
> pause in the transfer of 600us after the 20th SG entrey (when setting up
> new PaRAM entries). Since setting up Rx PaRAM is identical, this shows
> that breaking down the transfer is not working in the Rx direction for
> SPI caused by the relative high bit rate and small FIFO.

SPI is synchronous transfer so it does not need a large FIFO. If DMA is
not able to replenish the TX shift register, the master should hold the
clock until the time it is ready with data. On the scope do you see
master continuing to pulse the clock while it is waiting for data to
arrive from DMA to its TX shift register ? That should not be happening.

Thanks,
Sekhar

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Frode Isaksen]

On 14/02/2017 16:29, Sekhar Nori wrote:
> On Tuesday 14 February 2017 04:57 PM, Frode Isaksen wrote:
>>
>> On 13/02/2017 06:59, Sekhar Nori wrote:
>>> + Peter
>>>
>>> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>>>> Limit the transfer size to 20 scatter/gather pages if
>>>> DMA is enabled.
>>>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>>>> transaction. If this number is exceeded, DMA receive fails.
>>>> This error occurs with large vmalloc'ed buffers.
>>> This needs more explanation because there is support available in edma
>>> driver for long SG lists by breaking them down into transfers using 20
>>> PaRAM entries at a time. If thats not working for you, that needs
>>> further debug.
>> The SPI controller has a FIFO of only 1 word, so at 1Mbps, filling the
>> FIFO will take only 8us. Handling the DMA interrupt and re-programming
>> the PaRAM entries takes much longer than that. At 1Mbps, about 50 bytes
>> is lost on Rx, @ 2Mbps 100 bytes and @ 5Mbps about 260 bytes hinting
>> towards a time setting up a new DMA transfer > 400us. If the Tx and Rx
>> buffers are identically aligned there are no errors, because the
>> re-programming of the Tx and Rx PaRAM entries happens at the same time.
>> I have also verified this with a scope. In the Tx direction, there is a
>> pause in the transfer of 600us after the 20th SG entrey (when setting up
>> new PaRAM entries). Since setting up Rx PaRAM is identical, this shows
>> that breaking down the transfer is not working in the Rx direction for
>> SPI caused by the relative high bit rate and small FIFO.
> SPI is synchronous transfer so it does not need a large FIFO. If DMA is
> not able to replenish the TX shift register, the master should hold the
> clock until the time it is ready with data. On the scope do you see
> master continuing to pulse the clock while it is waiting for data to
> arrive from DMA to its TX shift register ? That should not be happening.
Take the example of a long Rx-only transfer using vmalloc'ed buffer. The 
dummy Tx buffer will be a contiguous buffer (1 SG entry) and clock and 
dummy data will be continuous during the transfer with no pause. On the 
Rx side, the PaRAM entries needs to be reloaded after 20*4096 bytes 
Rx'ed. Handling the interrupt, reloading the PaRAM's and resuming DMA 
takes ~600 us. Since the slave is still transmitting, data will be lost, 
since the FIFO is only 1 word and 1 byte takes 8us @ 1MHz.
The 600us came from timing a transmit with vmalloc'ed buffer. The time 
between the last byte in the 20th SG entry and the next byte is 600us 
measured with a scope. The assumption is that handling interrupt and 
reloading PaRAM's is more or less the same on Tx and Rx side. This time 
seems to be confirmed by the number of bytes lost on the Rx side as well 
which is approximately 50 x XMHz.
Hope this clarifies..
Another solution to this would be to have the dummy Tx buffer and Rx 
buffer to be identical (same type of allocation, same length, same 
offset), but this requires changes in the SPI framework.

Thanks,
Frode

Thanks,
Frode
>
> Thanks,
> Sekhar

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Peter Ujfalusi]

On 02/13/2017 07:59 AM, Sekhar Nori wrote:
> + Peter
>
> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>> Limit the transfer size to 20 scatter/gather pages if
>> DMA is enabled.
>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>> transaction. If this number is exceeded, DMA receive fails.
>> This error occurs with large vmalloc'ed buffers.
>
> This needs more explanation because there is support available in edma
> driver for long SG lists by breaking them down into transfers using 20
> PaRAM entries at a time. If thats not working for you, that needs
> further debug.

While breaking down long SG list works in the eDMA driver, it is not w/o 
it's limitation. It might take (to) long to re-configure the paRAM slots 
for the next batch. We see missed events with MMC/SD in case of long SG 
list, so I would not be surprised if this is not causing issues in other 
places...

>
>>
>> Signed-off-by: Frode Isaksen <fisaksen@baylibre.com>
>
> Thanks,
> Sekhar
>
>> ---
>>  drivers/spi/spi-davinci.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index b7b2da1..f1b46f6 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
>>  	return __davinci_spi_can_dma(spi);
>>  }
>>
>> +static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +	/*
>> +	 * The eDMA DMA engine is limited to 20 SG entries in one DMA
>> +	 * transaction. If this number is exceeded, DMA receive fails.
>> +	 * An extra SG entry is needed when the buffer is not page aligned.
>> +	 */
>> +	return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;
>> +}
>> +
>>  static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
>>  {
>>  	struct device *sdev = dspi->bitbang.master->dev.parent;
>> @@ -1010,6 +1020,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
>>  	master->setup = davinci_spi_setup;
>>  	master->cleanup = davinci_spi_cleanup;
>>  	master->can_dma = davinci_spi_can_dma;
>> +	master->max_transfer_size = davinci_spi_max_transfer_size;
>>
>>  	dspi->bitbang.chipselect = davinci_spi_chipselect;
>>  	dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
>>
>

- Péter

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Frode Isaksen]

On 21/03/2017 23:02, Peter Ujfalusi wrote:
>
>
> On 02/13/2017 07:59 AM, Sekhar Nori wrote:
>> + Peter
>>
>> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>>> Limit the transfer size to 20 scatter/gather pages if
>>> DMA is enabled.
>>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>>> transaction. If this number is exceeded, DMA receive fails.
>>> This error occurs with large vmalloc'ed buffers.
>>
>> This needs more explanation because there is support available in edma
>> driver for long SG lists by breaking them down into transfers using 20
>> PaRAM entries at a time. If thats not working for you, that needs
>> further debug.
>
> While breaking down long SG list works in the eDMA driver, it is not w/o it's
> limitation. It might take (to) long to re-configure the paRAM slots for the
> next batch. We see missed events with MMC/SD in case of long SG list, so I
> would not be surprised if this is not causing issues in other places...
>
I checked the time to re-configure the paRAM slots with an oscillo and it takes
about 160uS. Since the slave is still transmitting, the number of bytes lost is
about 160/8=20. With the spi-loopback-test I saw about 30bytes lost when
re-configuring.

Thanks,
Frode
>>
>>>
>>> Signed-off-by: Frode Isaksen <fisaksen-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Thanks,
>> Sekhar
>>
>>> ---
>>>  drivers/spi/spi-davinci.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>>> index b7b2da1..f1b46f6 100644
>>> --- a/drivers/spi/spi-davinci.c
>>> +++ b/drivers/spi/spi-davinci.c
>>> @@ -485,6 +485,16 @@ static bool davinci_spi_can_dma(struct spi_master *master,
>>>      return __davinci_spi_can_dma(spi);
>>>  }
>>>
>>> +static size_t davinci_spi_max_transfer_size(struct spi_device *spi)
>>> +{
>>> +    /*
>>> +     * The eDMA DMA engine is limited to 20 SG entries in one DMA
>>> +     * transaction. If this number is exceeded, DMA receive fails.
>>> +     * An extra SG entry is needed when the buffer is not page aligned.
>>> +     */
>>> +    return (__davinci_spi_can_dma(spi)) ? 19 * PAGE_SIZE : SIZE_MAX;
>>> +}
>>> +
>>>  static int davinci_spi_check_error(struct davinci_spi *dspi, int int_status)
>>>  {
>>>      struct device *sdev = dspi->bitbang.master->dev.parent;
>>> @@ -1010,6 +1020,7 @@ static int davinci_spi_probe(struct platform_device
>>> *pdev)
>>>      master->setup = davinci_spi_setup;
>>>      master->cleanup = davinci_spi_cleanup;
>>>      master->can_dma = davinci_spi_can_dma;
>>> +    master->max_transfer_size = davinci_spi_max_transfer_size;
>>>
>>>      dspi->bitbang.chipselect = davinci_spi_chipselect;
>>>      dspi->bitbang.setup_transfer = davinci_spi_setup_transfer;
>>>
>>
>
> - Péter

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Sekhar Nori]

On Wednesday 22 March 2017 04:41 PM, Frode Isaksen wrote:
> 
> 
> On 21/03/2017 23:02, Peter Ujfalusi wrote:
>>
>>
>> On 02/13/2017 07:59 AM, Sekhar Nori wrote:
>>> + Peter
>>>
>>> On Friday 10 February 2017 08:59 PM, Frode Isaksen wrote:
>>>> Limit the transfer size to 20 scatter/gather pages if
>>>> DMA is enabled.
>>>> The eDMA DMA engine is limited to 20 SG entries in one DMA
>>>> transaction. If this number is exceeded, DMA receive fails.
>>>> This error occurs with large vmalloc'ed buffers.
>>>
>>> This needs more explanation because there is support available in edma
>>> driver for long SG lists by breaking them down into transfers using 20
>>> PaRAM entries at a time. If thats not working for you, that needs
>>> further debug.
>>
>> While breaking down long SG list works in the eDMA driver, it is not w/o it's
>> limitation. It might take (to) long to re-configure the paRAM slots for the
>> next batch. We see missed events with MMC/SD in case of long SG list, so I
>> would not be surprised if this is not causing issues in other places...
>>
> I checked the time to re-configure the paRAM slots with an oscillo and it takes
> about 160uS. Since the slave is still transmitting, the number of bytes lost is
> about 160/8=20. With the spi-loopback-test I saw about 30bytes lost when
> re-configuring.

I guess this means that most IPs continue to get more missed sync events
even after the first sync event is generated at end of MAX_NR_SG PaRAMs.
And may be even that first sync event miss is a problem because the IP
does not pause and ends up shifting data in the shift register anyway.

If this is so broken, should edma driver not refuse to handle more than
MAX_NR_SG for all transfers but mem-to-mem transfers?

Thanks,
Sekhar

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Peter Ujfalusi]

On 03/22/2017 01:35 PM, Sekhar Nori wrote:
> On Wednesday 22 March 2017 04:41 PM, Frode Isaksen wrote:
> I guess this means that most IPs continue to get more missed sync events
> even after the first sync event is generated at end of MAX_NR_SG PaRAMs.
> And may be even that first sync event miss is a problem because the IP
> does not pause and ends up shifting data in the shift register anyway.
>
> If this is so broken, should edma driver not refuse to handle more than
> MAX_NR_SG for all transfers but mem-to-mem transfers?

DM355, OMAP-L138, etc have only 128 paRAM slots. If we lift the restriction on 
the number of paRAM slots a channel can take, then DMA will most likely going 
to stop working: If any peripherals request 128 long SG transfer (and MMC 
easily does that alone) then we will have no available paRAM slot for other 
clients.

It might be possible to start the paRAM slot update for the next batch already 
when we have 1 or 2 slots to be processed, but that is going to be tricky and 
not even reliable.

Note: audio also sets limit on the number of periods to avoid this 
reconfiguration of paRAM slots.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] spi: davinci: limit the transfer size if DMA enabled

[Frode Isaksen]

On 22/03/2017 13:20, Peter Ujfalusi wrote:
> On 03/22/2017 01:35 PM, Sekhar Nori wrote:
>> On Wednesday 22 March 2017 04:41 PM, Frode Isaksen wrote:
>> I guess this means that most IPs continue to get more missed sync events
>> even after the first sync event is generated at end of MAX_NR_SG PaRAMs.
>> And may be even that first sync event miss is a problem because the IP
>> does not pause and ends up shifting data in the shift register anyway.
>>
>> If this is so broken, should edma driver not refuse to handle more than
>> MAX_NR_SG for all transfers but mem-to-mem transfers?
>
> DM355, OMAP-L138, etc have only 128 paRAM slots. If we lift the restriction on
> the number of paRAM slots a channel can take, then DMA will most likely going
> to stop working: If any peripherals request 128 long SG transfer (and MMC
> easily does that alone) then we will have no available paRAM slot for other
> clients.
>
> It might be possible to start the paRAM slot update for the next batch already
> when we have 1 or 2 slots to be processed, but that is going to be tricky and
> not even reliable.
Note that with SPI, a workaround for this issue has been found by using the rx
buffer as the dummy tx buffer when doing read-only transfers (this is the case
that fails). With this, the rx and tx paRAM slots are reconfigured at the same time.

Thanks,
Frode
>
> Note: audio also sets limit on the number of periods to avoid this
> reconfiguration of paRAM slots.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] 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 flash.

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

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index f1b46f6..c735425 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>
 
@@ -676,7 +677,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 			}
 			sg_dma_address(&sg_rx) = t->rx_dma;
 			sg_dma_len(&sg_rx) = t->len;
-		}
+		} else if (is_vmalloc_addr(t->rx_buf))
+			/* VIVT cache, flush since aliased address */
+			flush_kernel_vmap_range((void *)t->rx_buf, t->len);
 
 		sg_init_table(&sg_tx, 1);
 		if (!t->tx_buf) {
@@ -688,7 +691,9 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 			}
 			sg_dma_address(&sg_tx) = t->tx_dma;
 			sg_dma_len(&sg_tx) = t->len;
-		}
+		} else if (is_vmalloc_addr(t->tx_buf))
+			/* VIVT cache, flush since aliased address */
+			flush_kernel_vmap_range((void *)t->tx_buf, t->len);
 
 		rxdesc = dmaengine_prep_slave_sg(dspi->dma_rx,
 				t->rx_sg.sgl ?: &sg_rx, t->rx_sg.nents ?: 1,
@@ -741,6 +746,10 @@ static int davinci_spi_bufs(struct spi_device *spi, struct spi_transfer *t)
 		if (!t->rx_buf)
 			dma_unmap_single(&spi->dev, t->rx_dma,
 					t->len, DMA_FROM_DEVICE);
+		else if (is_vmalloc_addr(t->rx_buf))
+			/* VIVT cache, invalidate since aliased address */
+			invalidate_kernel_vmap_range((void *)t->rx_buf, t->len);
+
 		if (!t->tx_buf)
 			dma_unmap_single(&spi->dev, t->tx_dma,
 					t->len, DMA_TO_DEVICE);
-- 
2.7.4

[PATCH 5/8] 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index c735425..3c428f8 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 size_t davinci_spi_max_transfer_size(struct spi_device *spi)
@@ -634,10 +636,10 @@ 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);
@@ -740,7 +742,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 (!t->rx_buf)
-- 
2.7.4

[PATCH 6/8] spi: loopback-test: set HW loopback mode if loopback set

[Frode Isaksen]

Set the internal HW loopback mode if the loopback option
is set.

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

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 50e620f..ce552af 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -285,6 +285,11 @@ static int spi_loopback_test_probe(struct spi_device *spi)
 
 	dev_info(&spi->dev, "Executing spi-loopback-tests\n");
 
+	if (loopback) {
+		spi->mode |= SPI_LOOP;
+		ret = spi_setup(spi);
+		dev_info(&spi->dev, "configure loopback return: %i\n", ret);
+	}
 	ret = spi_test_run_tests(spi, spi_tests);
 
 	dev_info(&spi->dev, "Finished spi-loopback-tests with return: %i\n",
-- 
2.7.4

[PATCH 7/8] 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 ce552af..5618df3 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[] = {
 	{
@@ -970,13 +976,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;
@@ -1004,8 +1016,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

[PATCH 8/8] spi: loopback-test: limit length to spi_max_transfer_size()

[Frode Isaksen]

Limit length to spi_max_transfer_size() in the
spi loopback test driver.

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

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 5618df3..e1e698e 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -796,6 +796,9 @@ static int spi_test_run_iter(struct spi_device *spi,
 		return 0;
 	}
 
+	/* limit length to spi max transfer size */
+	len = min(spi_max_transfer_size(spi), len);
+
 	/* write out info */
 	if (!(len || tx_off || rx_off)) {
 		dev_info(&spi->dev, "Running test %s\n", test.description);
-- 
2.7.4