[PATCH RESEND 0/2] spi: sun4i: add DMA support

[PATCH RESEND 0/2] spi: sun4i: add DMA support

[Priit Laes]

While trying to get SPI TFT screen working with Cubietruck, I noticed that
at current state driver didn't actually support SPI data bursts bigger than
n bytes (n is probably ~64 bytes).

Patches below were sent to mailinglists a while ago (May 2015) but apparently
were not addressed to correct maintainers and therefore were never applied.

With those patches, it's now possible to use TFT screens over SPI (fbtft
driver in staging), although there seems to be a possible issue where bigger
data bursts cause the driver to temporarily fall back to PIO mode:

spi_master spi32766: Using DMA mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using DMA mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer


Emilio López (1):
  spi: sun4i: add DMA support

Michal Suchanek (1):
  ARM: sunxi: spi: add notice about SPI FIFO limit.

 drivers/spi/spi-sun4i.c | 142 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 130 insertions(+), 12 deletions(-)

-- 
2.7.2

[PATCH 1/2] spi: sun4i: add DMA support

[Priit Laes]

From: Emilio López <emilio@elopez.com.ar>

This patch adds support for 64 byte or bigger transfers on the
sun4i SPI controller. Said transfers will be performed via DMA.

Signed-off-by: Emilio López <emilio@elopez.com.ar>
Tested-by: Michal Suchanek <hramrach@gmail.com>
Tested-by: Priit Laes <plaes@plaes.org>
---
 drivers/spi/spi-sun4i.c | 140 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 128 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 1ddd9e2..78141a6 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -14,6 +14,8 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -34,6 +36,7 @@
 #define SUN4I_CTL_CPHA				BIT(2)
 #define SUN4I_CTL_CPOL				BIT(3)
 #define SUN4I_CTL_CS_ACTIVE_LOW			BIT(4)
+#define SUN4I_CTL_DMAMC_DEDICATED		BIT(5)
 #define SUN4I_CTL_LMTF				BIT(6)
 #define SUN4I_CTL_TF_RST			BIT(8)
 #define SUN4I_CTL_RF_RST			BIT(9)
@@ -51,6 +54,8 @@
 #define SUN4I_INT_STA_REG		0x10
 
 #define SUN4I_DMA_CTL_REG		0x14
+#define SUN4I_DMA_CTL_RF_READY			BIT(0)
+#define SUN4I_DMA_CTL_TF_NOT_FULL		BIT(10)
 
 #define SUN4I_WAIT_REG			0x18
 
@@ -130,6 +135,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
 	}
 }
 
+static bool sun4i_spi_can_dma(struct spi_master *master,
+			      struct spi_device *spi,
+			      struct spi_transfer *tfr)
+{
+	return tfr->len >= SUN4I_FIFO_DEPTH;
+}
+
 static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -172,14 +184,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 				  struct spi_transfer *tfr)
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
 	unsigned int mclk_rate, div, timeout;
 	unsigned int tx_len = 0;
+	u32 reg, trigger = 0;
 	int ret = 0;
-	u32 reg;
-
-	/* We don't support transfer larger than the FIFO */
-	if (tfr->len > SUN4I_FIFO_DEPTH)
-		return -EINVAL;
 
 	reinit_completion(&sspi->done);
 	sspi->tx_buf = tfr->tx_buf;
@@ -189,7 +198,6 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	/* Clear pending interrupts */
 	sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);
 
-
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 
 	/* Reset FIFOs */
@@ -269,12 +277,65 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
 	sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
 
-	/* Fill the TX FIFO */
-	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
-
 	/* Enable the interrupts */
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
 
+	if (sun4i_spi_can_dma(master, spi, tfr)) {
+		dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+		if (sspi->tx_buf) {
+			desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+					tfr->tx_sg.sgl, tfr->tx_sg.nents,
+					DMA_TO_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_tx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			trigger |= SUN4I_DMA_CTL_TF_NOT_FULL;
+
+			dmaengine_submit(desc_tx);
+			dma_async_issue_pending(master->dma_tx);
+
+		}
+
+		if (sspi->rx_buf) {
+			desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+					tfr->rx_sg.sgl, tfr->rx_sg.nents,
+					DMA_FROM_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_rx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			trigger |= SUN4I_DMA_CTL_RF_READY;
+
+			dmaengine_submit(desc_rx);
+			dma_async_issue_pending(master->dma_rx);
+		}
+
+		/* Enable Dedicated DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		reg |= SUN4I_CTL_DMAMC_DEDICATED;
+		sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger);
+	} else {
+		dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+		/* Disable DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		sun4i_spi_write(sspi, SUN4I_CTL_REG,
+				reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+		/* Fill the TX FIFO */
+		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
+
 	/* Start the transfer */
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
@@ -286,7 +347,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 		goto out;
 	}
 
-	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+		/* The receive transfer should be the last one to finish */
+		dma_wait_for_async_tx(desc_rx);
+	} else {
+		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
 
 out:
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
@@ -351,6 +417,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
 
 static int sun4i_spi_probe(struct platform_device *pdev)
 {
+	struct dma_slave_config dma_sconfig;
 	struct spi_master *master;
 	struct sun4i_spi *sspi;
 	struct resource	*res;
@@ -386,7 +453,9 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
+	init_completion(&sspi->done);
 	sspi->master = master;
+	master->can_dma = sun4i_spi_can_dma;
 	master->set_cs = sun4i_spi_set_cs;
 	master->transfer_one = sun4i_spi_transfer_one;
 	master->num_chipselect = 4;
@@ -409,7 +478,45 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
-	init_completion(&sspi->done);
+	master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+	if (IS_ERR(master->dma_tx)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+		ret = PTR_ERR(master->dma_tx);
+		goto err_free_master;
+	}
+
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+		goto err_tx_dma_release;
+	}
+
+	master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+	if (IS_ERR(master->dma_rx)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+		ret = PTR_ERR(master->dma_rx);
+		goto err_tx_dma_release;
+	}
+
+	dma_sconfig.direction = DMA_DEV_TO_MEM;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(master->dma_rx, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure RX DMA slave\n");
+		goto err_rx_dma_release;
+	}
 
 	/*
 	 * This wake-up/shutdown pattern is to be able to have the
@@ -418,7 +525,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	ret = sun4i_spi_runtime_resume(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't resume the device\n");
-		goto err_free_master;
+		goto err_rx_dma_release;
 	}
 
 	pm_runtime_set_active(&pdev->dev);
@@ -436,6 +543,10 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	sun4i_spi_runtime_suspend(&pdev->dev);
+err_rx_dma_release:
+	dma_release_channel(master->dma_rx);
+err_tx_dma_release:
+	dma_release_channel(master->dma_tx);
 err_free_master:
 	spi_master_put(master);
 	return ret;
@@ -443,8 +554,13 @@ err_free_master:
 
 static int sun4i_spi_remove(struct platform_device *pdev)
 {
+	struct spi_master *master = platform_get_drvdata(pdev);
+
 	pm_runtime_disable(&pdev->dev);
 
+	dma_release_channel(master->dma_rx);
+	dma_release_channel(master->dma_tx);
+
 	return 0;
 }
 
-- 
2.7.2

[PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit.

[Priit Laes]

From: Michal Suchanek <hramrach@gmail.com>

When testing SPI without DMA I noticed that filling the FIFO on the
spi controller causes timeout. This should never happen with DMA support
so just adding a comment.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/spi/spi-sun4i.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 78141a6..b750664 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -333,7 +333,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
 
 		/* Fill the TX FIFO */
-		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+		/* Filling the fifo fully causes timeout for some reason - at least on spi2 on a10s */
+		/* The can_dma check is txlen >= SUN4I_FIFO_DEPTH so with DMA this should never happen anyway. */
+		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
 	}
 
 	/* Start the transfer */
-- 
2.7.2

Re: [PATCH RESEND 0/2] spi: sun4i: add DMA support

[Mark Brown]

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

On Fri, Feb 26, 2016 at 07:56:55AM +0200, Priit Laes wrote:
> While trying to get SPI TFT screen working with Cubietruck, I noticed that
> at current state driver didn't actually support SPI data bursts bigger than
> n bytes (n is probably ~64 bytes).

If you're going to add noise like "RESEND" to your subject lines (which
makes the visible space for the actual subject smaller so makes handling
incoming mail harder) please at least make sure it's true, you've not
sent these before.

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

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Mark Brown]

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

On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote:
> From: Emilio López <emilio@elopez.com.ar>
> 
> This patch adds support for 64 byte or bigger transfers on the
> sun4i SPI controller. Said transfers will be performed via DMA.
> 
> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> Tested-by: Michal Suchanek <hramrach@gmail.com>
> Tested-by: Priit Laes <plaes@plaes.org>
> ---

You *must* sign off any patches you are sending, please see
SubmittingPatches.

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

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Michal Suchanek]

Hello,

On 26 February 2016 at 13:25, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote:
>> From: Emilio López <emilio@elopez.com.ar>
>>
>> This patch adds support for 64 byte or bigger transfers on the
>> sun4i SPI controller. Said transfers will be performed via DMA.
>>
>> Signed-off-by: Emilio López <emilio@elopez.com.ar>
>> Tested-by: Michal Suchanek <hramrach@gmail.com>
>> Tested-by: Priit Laes <plaes@plaes.org>
>> ---
>
> You *must* sign off any patches you are sending, please see
> SubmittingPatches.

I have sent these patches in the past. Besides this non-technical
objection there were multiple technical objections.

IIRC one was that the driver does not handle the case when the DMA
channels are not available. As I understand it the channels are
exclusively reserved for a particular peripherial on sunxi platform so
this ShoulNotHappen(tm). So it's probably fine for the driver to fail
probe  when you have broken DT or no DMA engine support for sunxi
platform.

The driver could also work fully without DMA and there is a patch for
that also but since nobody will be testing that codepath it's probably
better to not include it.

I have not addressed the other objections so far.

Thanks

Michal

Re: [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit.

[Maxime Ripard]

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

Hi,

On Fri, Feb 26, 2016 at 07:56:57AM +0200, Priit Laes wrote:
> From: Michal Suchanek <hramrach@gmail.com>
> 
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout. This should never happen with DMA support
> so just adding a comment.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/spi/spi-sun4i.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 78141a6..b750664 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -333,7 +333,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
>  
>  		/* Fill the TX FIFO */
> -		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
> +		/* Filling the fifo fully causes timeout for some reason - at least on spi2 on a10s */
> +		/* The can_dma check is txlen >= SUN4I_FIFO_DEPTH so with DMA this should never happen anyway. */
> +		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);

Please wrap the lines at 80 chars and use the proper multiline comment
style.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Maxime Ripard]

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

On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
> Hello,
> 
> On 26 February 2016 at 13:25, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote:
> >> From: Emilio López <emilio@elopez.com.ar>
> >>
> >> This patch adds support for 64 byte or bigger transfers on the
> >> sun4i SPI controller. Said transfers will be performed via DMA.
> >>
> >> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> >> Tested-by: Michal Suchanek <hramrach@gmail.com>
> >> Tested-by: Priit Laes <plaes@plaes.org>
> >> ---
> >
> > You *must* sign off any patches you are sending, please see
> > SubmittingPatches.
> 
> I have sent these patches in the past.

Mike's point was that since it's Priit that he's submitting the
patches, he must add his SoB.

> Besides this non-technical objection there were multiple technical
> objections.
> 
> IIRC one was that the driver does not handle the case when the DMA
> channels are not available. As I understand it the channels are
> exclusively reserved for a particular peripherial on sunxi platform so
> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
> probe  when you have broken DT or no DMA engine support for sunxi
> platform.

There's a quite trivial scenario that would trigger this: if the dma
driver or dmaengine is not enabled / loaded.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Michal Suchanek]

Hello,

On 6 March 2016 at 22:42, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:

>> Besides this non-technical objection there were multiple technical
>> objections.
>>
>> IIRC one was that the driver does not handle the case when the DMA
>> channels are not available. As I understand it the channels are
>> exclusively reserved for a particular peripherial on sunxi platform so
>> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
>> probe  when you have broken DT or no DMA engine support for sunxi
>> platform.
>
> There's a quite trivial scenario that would trigger this: if the dma
> driver or dmaengine is not enabled / loaded.

There are other trivial scenarios under which the driver will fail
like loading wrong DT, not compiling or loading the sunxi pinmux
driver, and whatnot.

When you misconfigure your kernel it does not work. So long as the
driver just fails and does not crash and burn it's normal. Since the
driver is pretty much useless without DMA as it is now (63 byte
transfer limit) losing the non-DMA functionality when DMA engine is
not compiled-in does not seem that terrible.

Thanks

Michal

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Maxime Ripard]

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

On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
> Hello,
> 
> On 6 March 2016 at 22:42, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
> 
> >> Besides this non-technical objection there were multiple technical
> >> objections.
> >>
> >> IIRC one was that the driver does not handle the case when the DMA
> >> channels are not available. As I understand it the channels are
> >> exclusively reserved for a particular peripherial on sunxi platform so
> >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
> >> probe  when you have broken DT or no DMA engine support for sunxi
> >> platform.
> >
> > There's a quite trivial scenario that would trigger this: if the dma
> > driver or dmaengine is not enabled / loaded.
> 
> There are other trivial scenarios under which the driver will fail
> like loading wrong DT, not compiling or loading the sunxi pinmux
> driver, and whatnot.

I don't see what the pinmux has to do with SPI and DMA, and you're
wrong, the pinmux driver is always compiled in if you enable the sunxi
support.

And you're missing the whole point. DMA accesses are optional, pinmux
and proper machine support are not.

> When you misconfigure your kernel it does not work. So long as the
> driver just fails and does not crash and burn it's normal. Since the
> driver is pretty much useless without DMA as it is now (63 byte
> transfer limit) losing the non-DMA functionality when DMA engine is
> not compiled-in does not seem that terrible.

You're mixing two things up: the fact that we can't do more than the
FIFO length in PIO and that we're missing DMA support. We have patches
to address both, and there's no depedency between the two.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Michal Suchanek]

On 17 March 2016 at 08:27, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
>> Hello,
>>
>> On 6 March 2016 at 22:42, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
>>
>> >> Besides this non-technical objection there were multiple technical
>> >> objections.
>> >>
>> >> IIRC one was that the driver does not handle the case when the DMA
>> >> channels are not available. As I understand it the channels are
>> >> exclusively reserved for a particular peripherial on sunxi platform so
>> >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
>> >> probe  when you have broken DT or no DMA engine support for sunxi
>> >> platform.
>> >
>> > There's a quite trivial scenario that would trigger this: if the dma
>> > driver or dmaengine is not enabled / loaded.
>>
>> There are other trivial scenarios under which the driver will fail
>> like loading wrong DT, not compiling or loading the sunxi pinmux
>> driver, and whatnot.
>
> I don't see what the pinmux has to do with SPI and DMA, and you're
> wrong, the pinmux driver is always compiled in if you enable the sunxi
> support.
>
> And you're missing the whole point. DMA accesses are optional, pinmux
> and proper machine support are not.
>
>> When you misconfigure your kernel it does not work. So long as the
>> driver just fails and does not crash and burn it's normal. Since the
>> driver is pretty much useless without DMA as it is now (63 byte
>> transfer limit) losing the non-DMA functionality when DMA engine is
>> not compiled-in does not seem that terrible.
>
> You're mixing two things up: the fact that we can't do more than the
> FIFO length in PIO and that we're missing DMA support. We have patches
> to address both, and there's no depedency between the two.

The only thing is that although DMA is optional on pretty much any
system you will have DMA available unless you broke your config. You
really do want the DMA support when it is available. So there will be
nobody testing the non-DMA part and it will be prone to bitrot.

Thanks

Michal

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Mark Brown]

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

On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote:
> On 17 March 2016 at 08:27, Maxime Ripard

> > You're mixing two things up: the fact that we can't do more than the
> > FIFO length in PIO and that we're missing DMA support. We have patches
> > to address both, and there's no depedency between the two.

> The only thing is that although DMA is optional on pretty much any
> system you will have DMA available unless you broke your config. You
> really do want the DMA support when it is available. So there will be
> nobody testing the non-DMA part and it will be prone to bitrot.

Well, it might be worth doing PIO for very short transfers even if you
have DMA - it's quite common for this to perform better.

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

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Michal Suchanek]

On 17 March 2016 at 12:43, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote:
>> On 17 March 2016 at 08:27, Maxime Ripard
>
>> > You're mixing two things up: the fact that we can't do more than the
>> > FIFO length in PIO and that we're missing DMA support. We have patches
>> > to address both, and there's no depedency between the two.
>
>> The only thing is that although DMA is optional on pretty much any
>> system you will have DMA available unless you broke your config. You
>> really do want the DMA support when it is available. So there will be
>> nobody testing the non-DMA part and it will be prone to bitrot.
>
> Well, it might be worth doing PIO for very short transfers even if you
> have DMA - it's quite common for this to perform better.

That's what the driver does. The discussion revolves around the fact
that the driver does not attempt to work (even for very short
transfers) when the DMA channels are not configured and just bails
out. AFAICT the channels are always available when the system is
properly configured and the dmaengine driver loaded.

Very few device drivers would work with 63byte transfers only and the
code for manually driving the CS line in case the DMA engine fails to
configure will necessarily go untested most of the time since most
systems will have DMA configured properly.

Thanks

Michal

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Mark Brown]

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

On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:

> That's what the driver does. The discussion revolves around the fact
> that the driver does not attempt to work (even for very short
> transfers) when the DMA channels are not configured and just bails
> out. AFAICT the channels are always available when the system is
> properly configured and the dmaengine driver loaded.

The driver should tell the core about this constraint so the core can
split the transfers for it.

> Very few device drivers would work with 63byte transfers only and the
> code for manually driving the CS line in case the DMA engine fails to
> configure will necessarily go untested most of the time since most
> systems will have DMA configured properly.

A lot of devices will be perfectly happy with 63 byte transfers,
register accesses for example tend to be much smaller than that.  The
manual /CS might be an issue but for most SoCs that is easily addressed
by driving the pin as a GPIO if there's an issue.

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

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Maxime Ripard]

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

On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:
> On 17 March 2016 at 12:43, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote:
> >> On 17 March 2016 at 08:27, Maxime Ripard
> >
> >> > You're mixing two things up: the fact that we can't do more than the
> >> > FIFO length in PIO and that we're missing DMA support. We have patches
> >> > to address both, and there's no depedency between the two.
> >
> >> The only thing is that although DMA is optional on pretty much any
> >> system you will have DMA available unless you broke your config. You
> >> really do want the DMA support when it is available. So there will be
> >> nobody testing the non-DMA part and it will be prone to bitrot.
> >
> > Well, it might be worth doing PIO for very short transfers even if you
> > have DMA - it's quite common for this to perform better.
> 
> That's what the driver does. The discussion revolves around the fact
> that the driver does not attempt to work (even for very short
> transfers) when the DMA channels are not configured and just bails
> out. AFAICT the channels are always available when the system is
> properly configured and the dmaengine driver loaded.

I guess we just don't have the same defininition of "proper".

Let's take an opposite view. Can you have SPI working now if the
DMAEngine framework is not there? Yes. Why should it change? And even
more so, why should it change silently?

> Very few device drivers would work with 63byte transfers only and the
> code for manually driving the CS line in case the DMA engine fails to
> configure will necessarily go untested most of the time since most
> systems will have DMA configured properly.

That's not true. A significant portion would work. I don't like to
make up numbers, but I guess only the EEPROMs and rather big screens
wouldn't work.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] spi: sun4i: add DMA support

[Mark Brown]

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

On Fri, Mar 18, 2016 at 09:23:10PM +0100, Maxime Ripard wrote:
> On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:

> > Very few device drivers would work with 63byte transfers only and the
> > code for manually driving the CS line in case the DMA engine fails to
> > configure will necessarily go untested most of the time since most
> > systems will have DMA configured properly.

> That's not true. A significant portion would work. I don't like to
> make up numbers, but I guess only the EEPROMs and rather big screens
> wouldn't work.

You'll probably also see devices doing things like firmware downloads,
there's a bunch of applications for larger transfers.  Like you say it's
definitely not going to be a universal problem though, there's many
devices that just use SPI for fast register access.

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