Re: [PATCH 1/9] spi: sunxi: fix transfer timeout

Re: [PATCH 1/9] spi: sunxi: fix transfer timeout

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:45PM -0000, Michal Suchanek wrote:
> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
> actual time the transfer is supposed to take and multiply by 2 for good
> measure.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/spi/spi-sun4i.c | 10 +++++++++-
>  drivers/spi/spi-sun6i.c | 10 +++++++++-
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index fbb0a4d..48532ec 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -170,6 +170,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  {
>  	struct sun4i_spi *sspi = spi_master_get_devdata(master);
>  	unsigned int mclk_rate, div, timeout;
> +	unsigned int start, end, tx_time;
>  	unsigned int tx_len = 0;
>  	int ret = 0;
>  	u32 reg;
> @@ -279,9 +280,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>  	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>  
> +	tx_time = max_t(int, tfr->len * 8 * 2 / (speed / 1000), 100);
> +	start = jiffies;
>  	timeout = wait_for_completion_timeout(&sspi->done,
> -					      msecs_to_jiffies(1000));
> +					      msecs_to_jiffies(tx_time));
> +	end = jiffies;
>  	if (!timeout) {
> +		dev_warn(&master->dev,
> +			 "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
> +			 dev_name(&spi->dev), tfr->len, speed,
> +			 jiffies_to_msecs(end - start), tx_time);

Timeout already contains the number of jiffies elapsed (and I'm not
sure anyone reading that message would get that the last number is
actually the maximum timeout).

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/9] spi: sunxi: fix transfer timeout

[Mark Brown]

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

On Thu, Aug 20, 2015 at 02:19:45PM -0000, Michal Suchanek wrote:

>  drivers/spi/spi-sun4i.c | 10 +++++++++-
>  drivers/spi/spi-sun6i.c | 10 +++++++++-

Are we *sure* we can't work on merging these drivers :(

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

Re: [PATCH 1/9] spi: sunxi: fix transfer timeout

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 11:41:32AM -0700, Mark Brown wrote:
> On Thu, Aug 20, 2015 at 02:19:45PM -0000, Michal Suchanek wrote:
> 
> >  drivers/spi/spi-sun4i.c | 10 +++++++++-
> >  drivers/spi/spi-sun6i.c | 10 +++++++++-
> 
> Are we *sure* we can't work on merging these drivers :(

Those are two different IPs, that don't really share anything but
their author...

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/9] spi: sunxi: fix transfer timeout

[Mark Brown]

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

On Thu, Aug 20, 2015 at 09:34:33PM +0200, Maxime Ripard wrote:
> On Thu, Aug 20, 2015 at 11:41:32AM -0700, Mark Brown wrote:
> > On Thu, Aug 20, 2015 at 02:19:45PM -0000, Michal Suchanek wrote:

> > >  drivers/spi/spi-sun4i.c | 10 +++++++++-
> > >  drivers/spi/spi-sun6i.c | 10 +++++++++-

> > Are we *sure* we can't work on merging these drivers :(

> Those are two different IPs, that don't really share anything but
> their author...

I seem to be seeing a number of changes like this one which make
apparently very similar modifications to both.  Perhaps there is more
core usage that should be happening instead?

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

Re: [PATCH 1/9] spi: sunxi: fix transfer timeout

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:08:30PM -0700, Mark Brown wrote:
> On Thu, Aug 20, 2015 at 09:34:33PM +0200, Maxime Ripard wrote:
> > On Thu, Aug 20, 2015 at 11:41:32AM -0700, Mark Brown wrote:
> > > On Thu, Aug 20, 2015 at 02:19:45PM -0000, Michal Suchanek wrote:
> 
> > > >  drivers/spi/spi-sun4i.c | 10 +++++++++-
> > > >  drivers/spi/spi-sun6i.c | 10 +++++++++-
> 
> > > Are we *sure* we can't work on merging these drivers :(
> 
> > Those are two different IPs, that don't really share anything but
> > their author...
> 
> I seem to be seeing a number of changes like this one which make
> apparently very similar modifications to both.  Perhaps there is more
> core usage that should be happening instead?

Yeah, because I wrote the two at the same time, and they share the
same flaws. But that doesn't really mean that you can share anything
at the driver level. And I'm not really sure that we can do much more
at the framework level either, except maybe handling the timeout
directly (but then the drivers would have to handle the recovering
after a timeout too).

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/9] spi: sunxi: fix transfer timeout

[Mark Brown]

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

On Thu, Aug 20, 2015 at 11:18:50PM +0200, Maxime Ripard wrote:

> at the driver level. And I'm not really sure that we can do much more
> at the framework level either, except maybe handling the timeout
> directly (but then the drivers would have to handle the recovering
> after a timeout too).

We have handle_err() for that sort of cleanup.

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

Re: [PATCH 2/9] spi: sun4i: fix FIFO limit

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout.
> 
> Always leave room for one byte in the FIFO.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/spi/spi-sun4i.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 48532ec..707f61b 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -178,6 +178,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  	/* We don't support transfer larger than the FIFO */
>  	if (tfr->len > SUN4I_FIFO_DEPTH)
>  		return -EINVAL;
> +	if (tfr->tx_buf && tfr->len => SUN4I_FIFO_DEPTH)
> +		return -EINVAL;

Shouldn't you fix the condition just above instead?

>  
>  	reinit_completion(&sspi->done);
>  	sspi->tx_buf = tfr->tx_buf;
> @@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  	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);
> +	/* Filling the fifo fully causes timeout for some reason
> +	 * at least on spi2 on a10s */

Please use the proper comment style. And it only happens on that
particular SPI controller and SoC combination?

> +	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
>  
>  	/* Enable the interrupts */
>  	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
> -- 
> 2.1.4
> 

-- 
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: [linux-sunxi] [PATCH 4/9] spi: sun4i: add DMA support

[Michal Suchanek]

On 20 August 2015 at 16:19, Emilio López <emilio@elopez.com.ar> wrote:
> From: Emilio López <emilio@elopez.com.ar>

Something went wrong with overriding the headers

Sorry

Michal

Re: [PATCH 4/9] spi: sun4i: add DMA support

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Emilio López 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>

This should have your SoB.

> ---
>  drivers/spi/spi-sun4i.c | 145 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 130 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 4dda366..63242a7 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);
> @@ -169,17 +181,12 @@ 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 speed, mclk_rate, div, timeout;
>  	unsigned int start, end, tx_time;
>  	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;
> -	if (tfr->tx_buf && tfr->len => SUN4I_FIFO_DEPTH)
> -		return -EINVAL;
>  
>  	reinit_completion(&sspi->done);
>  	sspi->tx_buf = tfr->tx_buf;
> @@ -277,14 +284,67 @@ 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 */
> -	/* Filling the fifo fully causes timeout for some reason
> -	 * at least on spi2 on a10s */
> -	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
> -
>  	/* 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);
> +		}

What happens if the dma driver controller isn't present in your
system? Or that it doesn't have any channels available anymore?

> +		/* 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 */
> +		/* Filling the fifo fully causes timeout for some reason
> +		 * at least on spi2 on a10s */
> +		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
> +	}
> +
>  	/* Start the transfer */
>  	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>  	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
> @@ -303,7 +363,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);

Nope, this is only meant for async_tx. You should register a callback
in your transfer that will mark the completion structure as completed,
and then drain the FIFO only if not using DMA.

> +	} else {
> +		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
> +	}
>  
>  out:
>  	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
> @@ -368,6 +433,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;
> @@ -403,7 +469,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;
> @@ -426,7 +494,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;

We can't use a higher bust size?

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 4/9] spi: sun4i: add DMA support

[Mark Brown]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Emilio López wrote:

> -	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);

What if it's a transmit only transfer?  We'll fall over to this...

> +	} else {
> +		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
> +	}

...which manually reads data from the FIFO which doesn't seem like what
we want, won't it conflict with the DMA?

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

Re: [linux-sunxi] Re: [PATCH 4/9] spi: sun4i: add DMA support

[Michal Suchanek]

On 20 August 2015 at 16:56, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

>> +             /* 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 */
>> +             /* Filling the fifo fully causes timeout for some reason
>> +              * at least on spi2 on a10s */
>> +             sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
>> +     }
>> +
>>       /* Start the transfer */
>>       reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>       sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>> @@ -303,7 +363,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);
>
> Nope, this is only meant for async_tx. You should register a callback
> in your transfer that will mark the completion structure as completed,
> and then drain the FIFO only if not using DMA.

What exactly is wrong with this?

I did not observe data corruption. Passing desc_rx to
dma_wait_for_async_tx looks odd on closer inspection, though. Will
look through some other spi driver code.

>> -     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;
>
> We can't use a higher bust size?

Who actually does?

It accomplishes the transfer with burst size of 1 so that's good enough.

Researching alignment requirements and other oddities of Chinese
controllers when larger burst size is used can be topic for another
patch.


On 20 August 2015 at 20:58, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Aug 20, 2015 at 02:19:46PM -0000, Emilio López wrote:
>
>> -     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);
>
> What if it's a transmit only transfer?  We'll fall over to this...
>
>> +     } else {
>> +             sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
>> +     }
>
> ...which manually reads data from the FIFO which doesn't seem like what
... which should be empty since RX is not enabled.
> we want, won't it conflict with the DMA?
It does not seem to conflict in practice.


Thanks

Michal

Re: [PATCH 4/9] spi: sun4i: add DMA support

[Mark Brown]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Emilio López wrote:

> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> Tested-by: Michal Suchanek <hramrach@gmail.com>

Also, if you're sending on a patch from someone else you must add a
Signed-off-by, see SubmittingPatches.

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

Re: [PATCH 5/9] ARM: dts: sun7i: cubieboard2: Enable SPI0

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
> Only SPI0 is enabled. The schematic denotes it as the only SPI bus.
> Other SPI pins are reserved for different peripherals.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

What device is connected to the other end?

> ---
>  arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> index 39a51d5..9861304 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> @@ -198,6 +198,13 @@
>  	status = "okay";
>  };
>  
> +&spi0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi0_pins_a>,
> +		    <&spi0_cs0_pins_a>;
> +	status = "okay";
> +};
> +

You should probably add an alias to that bus too.

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 7/9] ARM: dts: sun5i: add SPI pins on A13 and A10s

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:47PM -0000, Michal Suchanek wrote:
> According to datasheet some pins are available on A10s only while others
> are shared with A13.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
> This time add all spi pins and make the CS pins separate as is seen with
> current sun4i DTs
> ---
>  arch/arm/boot/dts/sun5i-a10s.dtsi | 21 +++++++++++++++++
>  arch/arm/boot/dts/sun5i.dtsi      | 49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
> index f11efb7..d9610fa 100644
> --- a/arch/arm/boot/dts/sun5i-a10s.dtsi
> +++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
> @@ -201,6 +201,27 @@
>  		allwinner,drive = <SUN4I_PINCTRL_30_MA>;
>  		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>  	};
> +
> +	spi1_cs1_pins_a: spi1_cs1@0 {
> +		allwinner,pins = "PG13";
> +		allwinner,function = "spi1";
> +		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +	};
> +
> +	spi2_pins_a: spi2@0 {
> +		allwinner,pins = "PB12", "PB13", "PB14";
> +		allwinner,function = "spi1";
> +		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +	};
> +
> +	spi2_cs0_pins_a: spi2_cs0@0 {
> +		allwinner,pins = "PB11";
> +		allwinner,function = "spi1";
> +		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +	};

spi2 nodes with spi1 function ??? How can that even work?

>  };
>  
>  &sram_a {
> diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
> index 54b0978..7d32d49 100644
> --- a/arch/arm/boot/dts/sun5i.dtsi
> +++ b/arch/arm/boot/dts/sun5i.dtsi
> @@ -516,6 +516,55 @@
>  				allwinner,drive = <SUN4I_PINCTRL_30_MA>;
>  				allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
>  			};
> +
> +			spi0_pins_a: spi0@0 {
> +				allwinner,pins = "PC00", "PC01", "PC02";
> +				allwinner,function = "spi0";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi0_cs0_pins_a: spi0_cs0@0 {
> +				allwinner,pins = "PC03";
> +				allwinner,function = "spi0";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi1_pins_a: spi1@0 {
> +				allwinner,pins = "PG10", "PG11", "PG12";
> +				allwinner,function = "spi1";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi1_cs0_pins_a: spi1_cs0@0 {
> +				allwinner,pins = "PG09";
> +				allwinner,function = "spi1";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi2_pins_b: spi2@1 {
> +				allwinner,pins = "PE01", "PE02", "PE03";
> +				allwinner,function = "spi2";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi2_cs0_pins_b: spi2_cs1@1 {
> +				allwinner,pins = "PE00";
> +				allwinner,function = "spi2";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +
> +			spi2_cs1_pins_a: spi2_cs1@0 {
> +				allwinner,pins = "PB10";
> +				allwinner,function = "spi2";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};

Please only add the pins you intend to use.

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 3/9] spi: sunxi: check that transfer speed is non-zero

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
> When the maximum transfer speed is not set for a SPI slave the value
> remains 0 and the code in sunxi SPI divides by it. Use an arbitrary
> speed instead in that case.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

spi->max_speed_hz is set using the spi-max-frequency property, and
spi->will error out if that property isn't set. What am I missing?

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 3/9] spi: sunxi: check that transfer speed is non-zero

[Michal Suchanek]

On 20 August 2015 at 16:48, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
>> When the maximum transfer speed is not set for a SPI slave the value
>> remains 0 and the code in sunxi SPI divides by it. Use an arbitrary
>> speed instead in that case.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> spi->max_speed_hz is set using the spi-max-frequency property, and
> spi->will error out if that property isn't set. What am I missing?
>

Spidev I guess.

You can set the speed to arbitrary value from userspace at any time
using the spidev ioctls.

Thanks

Michal

Re: [PATCH 3/9] spi: sunxi: check that transfer speed is non-zero

[Maxime Ripard]

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

On Thu, Aug 20, 2015 at 09:45:07PM +0200, Michal Suchanek wrote:
> On 20 August 2015 at 16:48, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
> >> When the maximum transfer speed is not set for a SPI slave the value
> >> remains 0 and the code in sunxi SPI divides by it. Use an arbitrary
> >> speed instead in that case.
> >>
> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> >
> > spi->max_speed_hz is set using the spi-max-frequency property, and
> > spi->will error out if that property isn't set. What am I missing?
> >
> 
> Spidev I guess.
> 
> You can set the speed to arbitrary value from userspace at any time
> using the spidev ioctls.

Then why should it be fixed at the controller driver level? It looks
more like an issue that impacts everyone and should be fixed either in
spidev directly or in the framework.

-- 
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 3/9] spi: sunxi: check that transfer speed is non-zero

[Mark Brown]

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

On Thu, Aug 20, 2015 at 02:19:46PM -0000, Michal Suchanek wrote:
> When the maximum transfer speed is not set for a SPI slave the value
> remains 0 and the code in sunxi SPI divides by it. Use an arbitrary
> speed instead in that case.

You should always be using the transfer speed that is defined per
transfer, the core will ensure that this is always set.

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