[PATCH v3 0/6] spi: Add support for DMA transfers in sun6i SPI driver

[PATCH v3 0/6] spi: Add support for DMA transfers in sun6i SPI driver

[Sergey Suloev]

The following patchset provides corrections for PIO-mode
and support for DMA transfers in sun6i SPI driver.

Changes in v2:
1) Fixed issue with misplacing a piece of code that requires access
to the transfer structure into sun6i_spi_prepare_message() function
where the transfer structure is not available.

2) Fixed issue with passing an invalid argument into devm_request_irq()
function.

Changes in v3:
1) Restored processing of 3/4 FIFO full interrupt.

2) Debug log enhancements.

Sergey Suloev (6):
  spi: sun6i: coding style/readability improvements
  spi: sun6i: handle chip select polarity flag
  spi: sun6i: restrict transfer length in PIO-mode
  spi: sun6i: use completion provided by SPI core
  spi: sun6i: introduce register set/unset helpers
  spi: sun6i: add DMA transfers support

 drivers/spi/spi-sun6i.c | 526 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 402 insertions(+), 124 deletions(-)

-- 
2.16.2

[PATCH v3 1/6] spi: sun6i: coding style/readability improvements

[Sergey Suloev]

Minor changes to fulfill the coding style and improve
the readability of the code.

Changes in v2:
1) Fixed issue with misplacing a piece of code that requires access
to the transfer structure into sun6i_spi_prepare_message() function
where the transfer structure is not available.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 97 +++++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 39 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 8533f4e..88ad45e 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -88,8 +88,12 @@
 #define SUN6I_TXDATA_REG		0x200
 #define SUN6I_RXDATA_REG		0x300
 
+#define SUN6I_SPI_MODE_BITS		(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
+
+#define SUN6I_SPI_MAX_SPEED_HZ		100000000
+#define SUN6I_SPI_MIN_SPEED_HZ		3000
+
 struct sun6i_spi {
-	struct spi_master	*master;
 	void __iomem		*base_addr;
 	struct clk		*hclk;
 	struct clk		*mclk;
@@ -189,6 +193,9 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
 	else
 		reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
 
+	/* We want to control the chip select manually */
+	reg |= SUN6I_TFR_CTL_CS_MANUAL;
+
 	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
 }
 
@@ -197,6 +204,39 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
 	return SUN6I_MAX_XFER_SIZE - 1;
 }
 
+static int sun6i_spi_prepare_message(struct spi_master *master,
+				     struct spi_message *msg)
+{
+	struct spi_device *spi = msg->spi;
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+	u32 reg;
+
+	/*
+	 * Setup the transfer control register: Chip Select,
+	 * polarities, etc.
+	 */
+	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
+
+	if (spi->mode & SPI_CPOL)
+		reg |= SUN6I_TFR_CTL_CPOL;
+	else
+		reg &= ~SUN6I_TFR_CTL_CPOL;
+
+	if (spi->mode & SPI_CPHA)
+		reg |= SUN6I_TFR_CTL_CPHA;
+	else
+		reg &= ~SUN6I_TFR_CTL_CPHA;
+
+	if (spi->mode & SPI_LSB_FIRST)
+		reg |= SUN6I_TFR_CTL_FBS;
+	else
+		reg &= ~SUN6I_TFR_CTL_FBS;
+
+	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+
+	return 0;
+}
+
 static int sun6i_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *tfr)
@@ -235,27 +275,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
 			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
 
-	/*
-	 * Setup the transfer control register: Chip Select,
-	 * polarities, etc.
-	 */
-	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
-
-	if (spi->mode & SPI_CPOL)
-		reg |= SUN6I_TFR_CTL_CPOL;
-	else
-		reg &= ~SUN6I_TFR_CTL_CPOL;
-
-	if (spi->mode & SPI_CPHA)
-		reg |= SUN6I_TFR_CTL_CPHA;
-	else
-		reg &= ~SUN6I_TFR_CTL_CPHA;
-
-	if (spi->mode & SPI_LSB_FIRST)
-		reg |= SUN6I_TFR_CTL_FBS;
-	else
-		reg &= ~SUN6I_TFR_CTL_FBS;
 
+	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
 	/*
 	 * If it's a TX only transfer, we don't want to fill the RX
 	 * FIFO with bogus data
@@ -265,11 +286,9 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	else
 		reg |= SUN6I_TFR_CTL_DHB;
 
-	/* We want to control the chip select manually */
-	reg |= SUN6I_TFR_CTL_CS_MANUAL;
-
 	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
 
+
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
 	if (mclk_rate < (2 * tfr->speed_hz)) {
@@ -442,12 +461,24 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 	struct resource	*res;
 	int ret = 0, irq;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct sun6i_spi));
+	master = spi_alloc_master(&pdev->dev, sizeof(*sspi));
 	if (!master) {
 		dev_err(&pdev->dev, "Unable to allocate SPI Master\n");
 		return -ENOMEM;
 	}
 
+	master->max_speed_hz = SUN6I_SPI_MAX_SPEED_HZ;
+	master->min_speed_hz = SUN6I_SPI_MIN_SPEED_HZ;
+	master->num_chipselect = 4;
+	master->mode_bits = SUN6I_SPI_MODE_BITS;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->set_cs = sun6i_spi_set_cs;
+	master->prepare_message = sun6i_spi_prepare_message;
+	master->transfer_one = sun6i_spi_transfer_one;
+	master->max_transfer_size = sun6i_spi_max_transfer_size;
+	master->dev.of_node = pdev->dev.of_node;
+	master->auto_runtime_pm = true;
+
 	platform_set_drvdata(pdev, master);
 	sspi = spi_master_get_devdata(master);
 
@@ -466,26 +497,14 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun6i_spi_handler,
-			       0, "sun6i-spi", sspi);
+			       0, dev_name(&pdev->dev), sspi);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request IRQ\n");
 		goto err_free_master;
 	}
 
-	sspi->master = master;
 	sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev);
 
-	master->max_speed_hz = 100 * 1000 * 1000;
-	master->min_speed_hz = 3 * 1000;
-	master->set_cs = sun6i_spi_set_cs;
-	master->transfer_one = sun6i_spi_transfer_one;
-	master->num_chipselect = 4;
-	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
-	master->bits_per_word_mask = SPI_BPW_MASK(8);
-	master->dev.of_node = pdev->dev.of_node;
-	master->auto_runtime_pm = true;
-	master->max_transfer_size = sun6i_spi_max_transfer_size;
-
 	sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
 	if (IS_ERR(sspi->hclk)) {
 		dev_err(&pdev->dev, "Unable to acquire AHB clock\n");
@@ -525,7 +544,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
-		dev_err(&pdev->dev, "cannot register SPI master\n");
+		dev_err(&pdev->dev, "Couldn't register SPI master\n");
 		goto err_pm_disable;
 	}
 
-- 
2.16.2

Re: [PATCH v3 1/6] spi: sun6i: coding style/readability improvements

[Maxime Ripard]

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

On Tue, Apr 03, 2018 at 06:44:44PM +0300, Sergey Suloev wrote:
> Minor changes to fulfill the coding style and improve
> the readability of the code.
> 
> Changes in v2:
> 1) Fixed issue with misplacing a piece of code that requires access
> to the transfer structure into sun6i_spi_prepare_message() function
> where the transfer structure is not available.

This shouldn't be in your commit log.

> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
> ---
>  drivers/spi/spi-sun6i.c | 97 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 58 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 8533f4e..88ad45e 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -88,8 +88,12 @@
>  #define SUN6I_TXDATA_REG		0x200
>  #define SUN6I_RXDATA_REG		0x300
>  
> +#define SUN6I_SPI_MODE_BITS		(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
> +
> +#define SUN6I_SPI_MAX_SPEED_HZ		100000000
> +#define SUN6I_SPI_MIN_SPEED_HZ		3000
> +
>  struct sun6i_spi {
> -	struct spi_master	*master;
>  	void __iomem		*base_addr;
>  	struct clk		*hclk;
>  	struct clk		*mclk;
> @@ -189,6 +193,9 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>  	else
>  		reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
>  
> +	/* We want to control the chip select manually */
> +	reg |= SUN6I_TFR_CTL_CS_MANUAL;
> +
>  	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
>  }
>  
> @@ -197,6 +204,39 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>  	return SUN6I_MAX_XFER_SIZE - 1;
>  }
>  
> +static int sun6i_spi_prepare_message(struct spi_master *master,
> +				     struct spi_message *msg)
> +{
> +	struct spi_device *spi = msg->spi;
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +	u32 reg;
> +
> +	/*
> +	 * Setup the transfer control register: Chip Select,
> +	 * polarities, etc.
> +	 */
> +	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> +
> +	if (spi->mode & SPI_CPOL)
> +		reg |= SUN6I_TFR_CTL_CPOL;
> +	else
> +		reg &= ~SUN6I_TFR_CTL_CPOL;
> +
> +	if (spi->mode & SPI_CPHA)
> +		reg |= SUN6I_TFR_CTL_CPHA;
> +	else
> +		reg &= ~SUN6I_TFR_CTL_CPHA;
> +
> +	if (spi->mode & SPI_LSB_FIRST)
> +		reg |= SUN6I_TFR_CTL_FBS;
> +	else
> +		reg &= ~SUN6I_TFR_CTL_FBS;
> +
> +	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
> +
> +	return 0;
> +}
> +

You're doing way more than just "coding style improvements", please
split the patches accordingly.

And slow down between versions, you had 0 review on the v1 and the v2,
and you're at v3 already in just a couple of days.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH v3 2/6] spi: sun6i: handle chip select polarity flag

[Sergey Suloev]

The chip select polarity flag is declared as supported
but is not handled in the code.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 88ad45e..78acc1f 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -193,6 +193,12 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
 	else
 		reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
 
+	/* Handle chip select "reverse" polarity */
+	if (spi->mode & SPI_CS_HIGH)
+		reg &= ~SUN6I_TFR_CTL_SPOL;
+	else
+		reg |= SUN6I_TFR_CTL_SPOL;
+
 	/* We want to control the chip select manually */
 	reg |= SUN6I_TFR_CTL_CS_MANUAL;
 
-- 
2.16.2

[PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

There is no need to handle 3/4 empty interrupt as the maximum
supported transfer length in PIO mode is equal to FIFO depth,
i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.

Changes in v3:
1) Restored processing of 3/4 FIFO full interrupt.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 78acc1f..c09ad10 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
 
 static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
 {
-	return SUN6I_MAX_XFER_SIZE - 1;
+	struct spi_master *master = spi->master;
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+	return sspi->fifo_depth;
 }
 
 static int sun6i_spi_prepare_message(struct spi_master *master,
@@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	int ret = 0;
 	u32 reg;
 
-	if (tfr->len > SUN6I_MAX_XFER_SIZE)
-		return -EINVAL;
+	/* A zero length transfer never finishes if programmed
+	   in the hardware */
+	if (!tfr->len)
+		return 0;
+
+	/* Don't support transfer larger than the FIFO */
+	if (tfr->len > sspi->fifo_depth)
+		return -EMSGSIZE;
 
 	reinit_completion(&sspi->done);
 	sspi->tx_buf = tfr->tx_buf;
@@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	 */
 	trig_level = sspi->fifo_depth / 4 * 3;
 	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
-			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
-			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
+			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
 
 
 	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
 
 	/* Enable the interrupts */
-	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
 	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
 					 SUN6I_INT_CTL_RF_RDY);
-	if (tx_len > sspi->fifo_depth)
-		sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
 
 	/* Start the transfer */
 	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -376,7 +381,9 @@ out:
 static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
 {
 	struct sun6i_spi *sspi = dev_id;
-	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
+	u32 status;
+
+	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
 
 	/* Transfer complete */
 	if (status & SUN6I_INT_CTL_TC) {
@@ -388,26 +395,12 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
 
 	/* Receive FIFO 3/4 full */
 	if (status & SUN6I_INT_CTL_RF_RDY) {
-		sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
+		sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
 		/* Only clear the interrupt _after_ draining the FIFO */
 		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_RDY);
 		return IRQ_HANDLED;
 	}
 
-	/* Transmit FIFO 3/4 empty */
-	if (status & SUN6I_INT_CTL_TF_ERQ) {
-		sun6i_spi_fill_fifo(sspi, SUN6I_FIFO_DEPTH);
-
-		if (!sspi->len)
-			/* nothing left to transmit */
-			sun6i_spi_disable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
-
-		/* Only clear the interrupt _after_ re-seeding the FIFO */
-		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TF_ERQ);
-
-		return IRQ_HANDLED;
-	}
-
 	return IRQ_NONE;
 }
 
-- 
2.16.2

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

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

On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
> There is no need to handle 3/4 empty interrupt as the maximum
> supported transfer length in PIO mode is equal to FIFO depth,
> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>
> Changes in v3:
> 1) Restored processing of 3/4 FIFO full interrupt.
> 
> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
> ---
>  drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
>  1 file changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 78acc1f..c09ad10 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>  
>  static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>  {
> -	return SUN6I_MAX_XFER_SIZE - 1;
> +	struct spi_master *master = spi->master;
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +
> +	return sspi->fifo_depth;

Doesn't that effectively revert 3288d5cb40c0 ?

Why do you need to do so?

>  }
>  
>  static int sun6i_spi_prepare_message(struct spi_master *master,
> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	int ret = 0;
>  	u32 reg;
>  
> -	if (tfr->len > SUN6I_MAX_XFER_SIZE)
> -		return -EINVAL;
> +	/* A zero length transfer never finishes if programmed
> +	   in the hardware */

Improper comment style here. Please make sure to run checkpatch before
sending your patches.

> +	if (!tfr->len)
> +		return 0;

Can that case even happen?

> +	/* Don't support transfer larger than the FIFO */
> +	if (tfr->len > sspi->fifo_depth)
> +		return -EMSGSIZE;

You're changing the return type, why?

>  	reinit_completion(&sspi->done);
>  	sspi->tx_buf = tfr->tx_buf;
> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	 */
>  	trig_level = sspi->fifo_depth / 4 * 3;
>  	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> -			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
> -			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
> +			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>  
>  
>  	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>  
>  	/* Enable the interrupts */
> -	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
>  	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
>  					 SUN6I_INT_CTL_RF_RDY);
> -	if (tx_len > sspi->fifo_depth)
> -		sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);

This would also need to be explained in your commit log.

>  
>  	/* Start the transfer */
>  	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> @@ -376,7 +381,9 @@ out:
>  static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>  {
>  	struct sun6i_spi *sspi = dev_id;
> -	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> +	u32 status;
> +
> +	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);

Why is this change needed?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/04/2018 09:50 AM, Maxime Ripard wrote:
> On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
>> There is no need to handle 3/4 empty interrupt as the maximum
>> supported transfer length in PIO mode is equal to FIFO depth,
>> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>>
>> Changes in v3:
>> 1) Restored processing of 3/4 FIFO full interrupt.
>>
>> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
>> ---
>>   drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
>>   1 file changed, 17 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 78acc1f..c09ad10 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>>   
>>   static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>>   {
>> -	return SUN6I_MAX_XFER_SIZE - 1;
>> +	struct spi_master *master = spi->master;
>> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
>> +
>> +	return sspi->fifo_depth;
> Doesn't that effectively revert 3288d5cb40c0 ?
>
> Why do you need to do so?
Need what?

Change is supposed to restrict max transfer size for PIO mode otherwise 
it will fail.
The maximum transfer length = FIFO depth for PIO mode.

>
>>   }
>>   
>>   static int sun6i_spi_prepare_message(struct spi_master *master,
>> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>   	int ret = 0;
>>   	u32 reg;
>>   
>> -	if (tfr->len > SUN6I_MAX_XFER_SIZE)
>> -		return -EINVAL;
>> +	/* A zero length transfer never finishes if programmed
>> +	   in the hardware */
> Improper comment style here. Please make sure to run checkpatch before
> sending your patches.
ok
>
>> +	if (!tfr->len)
>> +		return 0;
> Can that case even happen?
Not sure, just for safety.
>
>> +	/* Don't support transfer larger than the FIFO */
>> +	if (tfr->len > sspi->fifo_depth)
>> +		return -EMSGSIZE;
> You're changing the return type, why?
As  a more appropriate one
>
>>   	reinit_completion(&sspi->done);
>>   	sspi->tx_buf = tfr->tx_buf;
>> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>   	 */
>>   	trig_level = sspi->fifo_depth / 4 * 3;
>>   	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
>> -			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
>> -			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
>> +			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>>   
>>   
>>   	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>   	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>>   
>>   	/* Enable the interrupts */
>> -	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
>>   	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
>>   					 SUN6I_INT_CTL_RF_RDY);
>> -	if (tx_len > sspi->fifo_depth)
>> -		sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
> This would also need to be explained in your commit log.
What exactly and in what way ?
>
>>   
>>   	/* Start the transfer */
>>   	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> @@ -376,7 +381,9 @@ out:
>>   static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>>   {
>>   	struct sun6i_spi *sspi = dev_id;
>> -	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>> +	u32 status;
>> +
>> +	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> Why is this change needed?
A minor one, for readability.
>
> Maxime
>

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

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

On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
> On 04/04/2018 09:50 AM, Maxime Ripard wrote:
> > On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
> > > There is no need to handle 3/4 empty interrupt as the maximum
> > > supported transfer length in PIO mode is equal to FIFO depth,
> > > i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
> > > 
> > > Changes in v3:
> > > 1) Restored processing of 3/4 FIFO full interrupt.
> > > 
> > > Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
> > > ---
> > >   drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
> > >   1 file changed, 17 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> > > index 78acc1f..c09ad10 100644
> > > --- a/drivers/spi/spi-sun6i.c
> > > +++ b/drivers/spi/spi-sun6i.c
> > > @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
> > >   static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
> > >   {
> > > -	return SUN6I_MAX_XFER_SIZE - 1;
> > > +	struct spi_master *master = spi->master;
> > > +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> > > +
> > > +	return sspi->fifo_depth;
> > Doesn't that effectively revert 3288d5cb40c0 ?
> > 
> > Why do you need to do so?
>
> Need what?

Why do you need to revert that change.

> Change is supposed to restrict max transfer size for PIO mode otherwise it
> will fail.
> The maximum transfer length = FIFO depth for PIO mode.

The point of that patch was precisely to allow to send more data than
the FIFO. You're breaking that behaviour without any justification,
and this is not ok.

> > 
> > >   }
> > >   static int sun6i_spi_prepare_message(struct spi_master *master,
> > > @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > >   	int ret = 0;
> > >   	u32 reg;
> > > -	if (tfr->len > SUN6I_MAX_XFER_SIZE)
> > > -		return -EINVAL;
> > > +	/* A zero length transfer never finishes if programmed
> > > +	   in the hardware */
> > Improper comment style here. Please make sure to run checkpatch before
> > sending your patches.
> ok
> > 
> > > +	if (!tfr->len)
> > > +		return 0;
> > Can that case even happen?
> Not sure, just for safety.
> > 
> > > +	/* Don't support transfer larger than the FIFO */
> > > +	if (tfr->len > sspi->fifo_depth)
> > > +		return -EMSGSIZE;
> > You're changing the return type, why?
> As  a more appropriate one

The fact that it's more appropriate should be justified.

> > 
> > >   	reinit_completion(&sspi->done);
> > >   	sspi->tx_buf = tfr->tx_buf;
> > > @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > >   	 */
> > >   	trig_level = sspi->fifo_depth / 4 * 3;
> > >   	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> > > -			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
> > > -			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
> > > +			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
> > >   	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> > > @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > >   	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> > >   	/* Enable the interrupts */
> > > -	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
> > >   	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
> > >   					 SUN6I_INT_CTL_RF_RDY);
> > > -	if (tx_len > sspi->fifo_depth)
> > > -		sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
> > This would also need to be explained in your commit log.
> What exactly and in what way ?

You should explain, at least:
A) What is the current behaviour
B) Why that is a problem, or what problem does it cause
C) What solution you implement and why you think it's justified

> > 
> > >   	/* Start the transfer */
> > >   	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> > > @@ -376,7 +381,9 @@ out:
> > >   static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> > >   {
> > >   	struct sun6i_spi *sspi = dev_id;
> > > -	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> > > +	u32 status;
> > > +
> > > +	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> > Why is this change needed?
> A minor one, for readability.

That's arguable, and you should have a single logical change per
patch. So this doesn't belong in this one.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
>> On 04/04/2018 09:50 AM, Maxime Ripard wrote:
>>> On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
>>>> There is no need to handle 3/4 empty interrupt as the maximum
>>>> supported transfer length in PIO mode is equal to FIFO depth,
>>>> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>>>>
>>>> Changes in v3:
>>>> 1) Restored processing of 3/4 FIFO full interrupt.
>>>>
>>>> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
>>>> ---
>>>>    drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
>>>>    1 file changed, 17 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>>> index 78acc1f..c09ad10 100644
>>>> --- a/drivers/spi/spi-sun6i.c
>>>> +++ b/drivers/spi/spi-sun6i.c
>>>> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>>>>    static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>>>>    {
>>>> -	return SUN6I_MAX_XFER_SIZE - 1;
>>>> +	struct spi_master *master = spi->master;
>>>> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
>>>> +
>>>> +	return sspi->fifo_depth;
>>> Doesn't that effectively revert 3288d5cb40c0 ?
>>>
>>> Why do you need to do so?
>> Need what?
> Why do you need to revert that change.
>
>> Change is supposed to restrict max transfer size for PIO mode otherwise it
>> will fail.
>> The maximum transfer length = FIFO depth for PIO mode.
> The point of that patch was precisely to allow to send more data than
> the FIFO. You're breaking that behaviour without any justification,
> and this is not ok.

I am sorry, but you can't. That's a hardware limitation.
>>>>    }
>>>>    static int sun6i_spi_prepare_message(struct spi_master *master,
>>>> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>>    	int ret = 0;
>>>>    	u32 reg;
>>>> -	if (tfr->len > SUN6I_MAX_XFER_SIZE)
>>>> -		return -EINVAL;
>>>> +	/* A zero length transfer never finishes if programmed
>>>> +	   in the hardware */
>>> Improper comment style here. Please make sure to run checkpatch before
>>> sending your patches.
>> ok
>>>> +	if (!tfr->len)
>>>> +		return 0;
>>> Can that case even happen?
>> Not sure, just for safety.
>>>> +	/* Don't support transfer larger than the FIFO */
>>>> +	if (tfr->len > sspi->fifo_depth)
>>>> +		return -EMSGSIZE;
>>> You're changing the return type, why?
>> As  a more appropriate one
> The fact that it's more appropriate should be justified.
>
>>>>    	reinit_completion(&sspi->done);
>>>>    	sspi->tx_buf = tfr->tx_buf;
>>>> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>>    	 */
>>>>    	trig_level = sspi->fifo_depth / 4 * 3;
>>>>    	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
>>>> -			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
>>>> -			(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
>>>> +			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>>>>    	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>>> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>>    	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>>>>    	/* Enable the interrupts */
>>>> -	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
>>>>    	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
>>>>    					 SUN6I_INT_CTL_RF_RDY);
>>>> -	if (tx_len > sspi->fifo_depth)
>>>> -		sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
>>> This would also need to be explained in your commit log.
>> What exactly and in what way ?
> You should explain, at least:
> A) What is the current behaviour
> B) Why that is a problem, or what problem does it cause
> C) What solution you implement and why you think it's justified
>
>>>>    	/* Start the transfer */
>>>>    	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>>> @@ -376,7 +381,9 @@ out:
>>>>    static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>>>>    {
>>>>    	struct sun6i_spi *sspi = dev_id;
>>>> -	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>>>> +	u32 status;
>>>> +
>>>> +	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>>> Why is this change needed?
>> A minor one, for readability.
> That's arguable, and you should have a single logical change per
> patch. So this doesn't belong in this one.
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Mark Brown]

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

On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> On 04/05/2018 12:19 PM, Maxime Ripard wrote:

> > The point of that patch was precisely to allow to send more data than
> > the FIFO. You're breaking that behaviour without any justification,
> > and this is not ok.

> I am sorry, but you can't. That's a hardware limitation.

Are you positive about that?  Normally you can add things to hardware
FIFOs while they're being drained so so long as you can keep data
flowing in at least as fast as it's being consumed.

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/05/2018 04:17 PM, Mark Brown wrote:
> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>> The point of that patch was precisely to allow to send more data than
>>> the FIFO. You're breaking that behaviour without any justification,
>>> and this is not ok.
>> I am sorry, but you can't. That's a hardware limitation.
> Are you positive about that?  Normally you can add things to hardware
> FIFOs while they're being drained so so long as you can keep data
> flowing in at least as fast as it's being consumed.

Well, normally yes, but this is not the case with the hardware that I 
own. My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a 
transfer larger than FIFO then TC interrupt never happens.

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
> On 04/05/2018 04:17 PM, Mark Brown wrote:
> > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> > > On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> > > > The point of that patch was precisely to allow to send more data than
> > > > the FIFO. You're breaking that behaviour without any justification,
> > > > and this is not ok.
> > > I am sorry, but you can't. That's a hardware limitation.
> > Are you positive about that?  Normally you can add things to hardware
> > FIFOs while they're being drained so so long as you can keep data
> > flowing in at least as fast as it's being consumed.
> 
> Well, normally yes, but this is not the case with the hardware that I own.
> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
> larger than FIFO then TC interrupt never happens.

Because you're not supposed to have a transfer larger than the FIFO,
but to have to setup at first a transfer the size of the FIFO, and
then when it's (or starts to be) depleted, fill it up again.

That's the point of the patch you're reverting, and if it doesn't
work, you should make it work and not simply revert it.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>> The point of that patch was precisely to allow to send more data than
>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>> and this is not ok.
>>>> I am sorry, but you can't. That's a hardware limitation.
>>> Are you positive about that?  Normally you can add things to hardware
>>> FIFOs while they're being drained so so long as you can keep data
>>> flowing in at least as fast as it's being consumed.
>> Well, normally yes, but this is not the case with the hardware that I own.
>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>> larger than FIFO then TC interrupt never happens.
> Because you're not supposed to have a transfer larger than the FIFO,
> but to have to setup at first a transfer the size of the FIFO, and
> then when it's (or starts to be) depleted, fill it up again.

According to what you said the driver must implement 
"transfer_one_message" instead of "transfer_one"

>
> That's the point of the patch you're reverting, and if it doesn't
> work, you should make it work and not simply revert it.
>
> Maxime
>

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

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

On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> > On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
> > > On 04/05/2018 04:17 PM, Mark Brown wrote:
> > > > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> > > > > On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> > > > > > The point of that patch was precisely to allow to send more data than
> > > > > > the FIFO. You're breaking that behaviour without any justification,
> > > > > > and this is not ok.
> > > > > I am sorry, but you can't. That's a hardware limitation.
> > > > Are you positive about that?  Normally you can add things to hardware
> > > > FIFOs while they're being drained so so long as you can keep data
> > > > flowing in at least as fast as it's being consumed.
> > > Well, normally yes, but this is not the case with the hardware that I own.
> > > My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
> > > larger than FIFO then TC interrupt never happens.
> > Because you're not supposed to have a transfer larger than the FIFO,
> > but to have to setup at first a transfer the size of the FIFO, and
> > then when it's (or starts to be) depleted, fill it up again.
> 
> According to what you said the driver must implement
> "transfer_one_message" instead of "transfer_one"

I'm not sure what makes you think that I said that.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>>>> The point of that patch was precisely to allow to send more data than
>>>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>>>> and this is not ok.
>>>>>> I am sorry, but you can't. That's a hardware limitation.
>>>>> Are you positive about that?  Normally you can add things to hardware
>>>>> FIFOs while they're being drained so so long as you can keep data
>>>>> flowing in at least as fast as it's being consumed.
>>>> Well, normally yes, but this is not the case with the hardware that I own.
>>>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>>>> larger than FIFO then TC interrupt never happens.
>>> Because you're not supposed to have a transfer larger than the FIFO,
>>> but to have to setup at first a transfer the size of the FIFO, and
>>> then when it's (or starts to be) depleted, fill it up again.
>> According to what you said the driver must implement
>> "transfer_one_message" instead of "transfer_one"
> I'm not sure what makes you think that I said that.
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Because current implementation tries to send more than FIFO-depth of 
data in a single call to "transfer_one" which is wrong.

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Mark Brown]

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

On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> > > On 04/06/2018 10:34 AM, Maxime Ripard wrote:

> > > According to what you said the driver must implement
> > > "transfer_one_message" instead of "transfer_one"

> > I'm not sure what makes you think that I said that.

> Because current implementation tries to send more than FIFO-depth of data in
> a single call to "transfer_one" which is wrong.

No, that's absolutely not the case.  All any of these functions has to
do is transfer whatever they were asked to, how they do it is not at all
important to the framework.

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/09/2018 01:50 PM, Mark Brown wrote:
> On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
>> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
>>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>>> According to what you said the driver must implement
>>>> "transfer_one_message" instead of "transfer_one"
>>> I'm not sure what makes you think that I said that.
>> Because current implementation tries to send more than FIFO-depth of data in
>> a single call to "transfer_one" which is wrong.
> No, that's absolutely not the case.  All any of these functions has to
> do is transfer whatever they were asked to, how they do it is not at all
> important to the framework.

I think you don't fully understand the issue. Let's talk about sun4i 
and  sun6i SPI  drivers separately.

sun4i

1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode  
but transfer_one() function doesn't follow the declaration allowing PIO 
transfers longer than FIFO depth  by just refilling FIFO using 3/4 FIFO 
empty interrupt. I can definitely state here that long transfers WON'T 
WORK on real hardware. I tested it and that's why I can say that. But as 
soon as sun4i SPI driver  is correctly declaring max_transfer_size then 
"smart" clients will work well by limiting a single transfer size to 
FIFO depth. I tested it with real hardware, again.


sun6i

2) it allows PIO transfers of any length by declaring max_transfer_size 
to a huge number, i.e. you can ONLY make this driver work in PIO mode  
by limiting a single transfer size to FIFO depth (64 or 128 bytes) on 
client side  and ignore max_transfer_size  exposed by the driver. Again, 
tested with real hardware.

All above doesn't work for DMA mode as there is no such limitation.

I can't clearly explain what is happening in the hardware in PIO mode 
but it seems that TC interrupt doesn't arrive in time when refilling 
FIFO multiple times takes place and every long transfer will end up with 
a timeout error.

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Mark Brown]

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

On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
> On 04/09/2018 01:50 PM, Mark Brown wrote:
> > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:

> > > Because current implementation tries to send more than FIFO-depth of data in
> > > a single call to "transfer_one" which is wrong.

> > No, that's absolutely not the case.  All any of these functions has to
> > do is transfer whatever they were asked to, how they do it is not at all
> > important to the framework.

> I think you don't fully understand the issue. Let's talk about sun4i and 
> sun6i SPI  drivers separately.

> sun4i

> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode  but
> transfer_one() function doesn't follow the declaration allowing PIO
> transfers longer than FIFO depth  by just refilling FIFO using 3/4 FIFO
> empty interrupt. I can definitely state here that long transfers WON'T WORK
> on real hardware. I tested it and that's why I can say that. But as soon as
> sun4i SPI driver  is correctly declaring max_transfer_size then "smart"
> clients will work well by limiting a single transfer size to FIFO depth. I
> tested it with real hardware, again.

None of this is in the slightest bit related to the use of transfer_one() 
vs transfer_one_message().  These are all implementation details and
will so far as I can tell apply equally to both APIs.

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

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

On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
> On 04/09/2018 01:50 PM, Mark Brown wrote:
> > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
> > > On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> > > > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> > > > > On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> > > > > According to what you said the driver must implement
> > > > > "transfer_one_message" instead of "transfer_one"
> > > > I'm not sure what makes you think that I said that.
> > > Because current implementation tries to send more than FIFO-depth of data in
> > > a single call to "transfer_one" which is wrong.
> > No, that's absolutely not the case.  All any of these functions has to
> > do is transfer whatever they were asked to, how they do it is not at all
> > important to the framework.
> 
> I think you don't fully understand the issue. Let's talk about sun4i and 
> sun6i SPI  drivers separately.
> 
> sun4i
> 
> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode  but
> transfer_one() function doesn't follow the declaration allowing PIO
> transfers longer than FIFO depth  by just refilling FIFO using 3/4 FIFO
> empty interrupt. I can definitely state here that long transfers WON'T WORK
> on real hardware.

Surely the original author of the patch allowing to do just that
disagrees with you. And it's not about the hardware itself, it's about
how the driver operates as well.

> I tested it and that's why I can say that.

Then it must be fixed, and not silently reverted.

> But as soon as sun4i SPI driver  is correctly declaring
> max_transfer_size then "smart" clients will work well by limiting a
> single transfer size to FIFO depth. I tested it with real hardware,
> again.

This is really not my point. What would prevent you from doing
multiple transfers in that case, and filling the FIFO entirely,
waiting for it to be done, then resuming until you have sent the right
number of bytes?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/09/2018 02:36 PM, Maxime Ripard wrote:
> On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
>> On 04/09/2018 01:50 PM, Mark Brown wrote:
>>> On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
>>>> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
>>>>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>>>>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>>>>> According to what you said the driver must implement
>>>>>> "transfer_one_message" instead of "transfer_one"
>>>>> I'm not sure what makes you think that I said that.
>>>> Because current implementation tries to send more than FIFO-depth of data in
>>>> a single call to "transfer_one" which is wrong.
>>> No, that's absolutely not the case.  All any of these functions has to
>>> do is transfer whatever they were asked to, how they do it is not at all
>>> important to the framework.
>> I think you don't fully understand the issue. Let's talk about sun4i and
>> sun6i SPI  drivers separately.
>>
>> sun4i
>>
>> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode  but
>> transfer_one() function doesn't follow the declaration allowing PIO
>> transfers longer than FIFO depth  by just refilling FIFO using 3/4 FIFO
>> empty interrupt. I can definitely state here that long transfers WON'T WORK
>> on real hardware.
> Surely the original author of the patch allowing to do just that
> disagrees with you.
I am not getting the point why the driver is declaring the max transfer 
length value and not following the rule.
> And it's not about the hardware itself, it's about
> how the driver operates as well.
>
>> I tested it and that's why I can say that.
> Then it must be fixed, and not silently reverted.
>
>> But as soon as sun4i SPI driver  is correctly declaring
>> max_transfer_size then "smart" clients will work well by limiting a
>> single transfer size to FIFO depth. I tested it with real hardware,
>> again.
> This is really not my point. What would prevent you from doing
> multiple transfers in that case, and filling the FIFO entirely,
> waiting for it to be done, then resuming until you have sent the right
> number of bytes?
Because it makes no sense IMHO. I can't see any single point in allowing 
long PIO transfers. Can you find at least one ?

I think we should reuse as much SPI core code as possible. The SPI core 
can handle an SPI message with multiple transfers,
all we need is to have max_transfer_size = FIFO depth and restrict it in 
transfer_one().

>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Maxime Ripard]

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

On Mon, Apr 09, 2018 at 02:59:57PM +0300, Sergey Suloev wrote:
> > > But as soon as sun4i SPI driver  is correctly declaring
> > > max_transfer_size then "smart" clients will work well by limiting a
> > > single transfer size to FIFO depth. I tested it with real hardware,
> > > again.
> > This is really not my point. What would prevent you from doing
> > multiple transfers in that case, and filling the FIFO entirely,
> > waiting for it to be done, then resuming until you have sent the right
> > number of bytes?
>
> Because it makes no sense IMHO. I can't see any single point in allowing
> long PIO transfers. Can you find at least one ?

I'm probably going to state the obvious here, but to allow long transfers?

> I think we should reuse as much SPI core code as possible. The SPI
> core can handle an SPI message with multiple transfers, all we need
> is to have max_transfer_size = FIFO depth and restrict it in
> transfer_one().

There's not a single call to the max_transfer_size hook in the SPI
core in 4.16, so that seems a bit too optimistic.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Sergey Suloev]

On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>> The point of that patch was precisely to allow to send more data than
>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>> and this is not ok.
>>>> I am sorry, but you can't. That's a hardware limitation.
>>> Are you positive about that?  Normally you can add things to hardware
>>> FIFOs while they're being drained so so long as you can keep data
>>> flowing in at least as fast as it's being consumed.
>> Well, normally yes, but this is not the case with the hardware that I own.
>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>> larger than FIFO then TC interrupt never happens.
> Because you're not supposed to have a transfer larger than the FIFO,
> but to have to setup at first a transfer the size of the FIFO, and
> then when it's (or starts to be) depleted, fill it up again.
According to what you said the driver must implement 
"transfer_one_message" instead of "transfer_one",
because the maximum transfer length is 64 bytes (for sun4i) and we 
shouldn't allow "transfer_one" handle
more than 64 bytes. Otherwise it breaks the concept.
>
> That's the point of the patch you're reverting, and if it doesn't
> work, you should make it work and not simply revert it.
>
> Maxime
>

Re: [PATCH v3 3/6] spi: sun6i: restrict transfer length in PIO-mode

[Mark Brown]

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

On Thu, Apr 05, 2018 at 11:19:13AM +0200, Maxime Ripard wrote:
> On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:

> > What exactly and in what way ?

> You should explain, at least:
> A) What is the current behaviour
> B) Why that is a problem, or what problem does it cause
> C) What solution you implement and why you think it's justified

Right, this is key - the top level problem with most of this patch set
is that it's hard to understand what the changes are intended to do or
why.  It's really important that people reading the changes be able to
understand what's going on, especially if technical problems have been
found since that tends to make people look more closely.  Part of this
is about splitting the changes out so that each patch does one thing
(which makes it easier to understand them) and part of it is about
explaining those changes clearly.

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

[PATCH v3 4/6] spi: sun6i: use completion provided by SPI core

[Sergey Suloev]

As long as the completion is already provided by the SPI core
then there is no need to waste extra-memory on this.
Also a waiting function was added to avoid code duplication.

Changes in v2:
1) Fixed issue with passing an invalid argument into devm_request_irq()
function.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 52 ++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index c09ad10..0912404 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -99,8 +99,6 @@ struct sun6i_spi {
 	struct clk		*mclk;
 	struct reset_control	*rstc;
 
-	struct completion	done;
-
 	const u8		*tx_buf;
 	u8			*rx_buf;
 	int			len;
@@ -246,6 +244,30 @@ static int sun6i_spi_prepare_message(struct spi_master *master,
 	return 0;
 }
 
+static int sun6i_spi_wait_for_transfer(struct spi_device *spi,
+				       struct spi_transfer *tfr)
+{
+	struct spi_master *master = spi->master;
+	unsigned int start, end, tx_time;
+	unsigned int timeout;
+
+	/* smart wait for completion */
+	tx_time = max(tfr->len * 8 * 2 / (tfr->speed_hz / 1000), 100U);
+	start = jiffies;
+	timeout = wait_for_completion_timeout(&master->xfer_completion,
+					      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, tfr->speed_hz,
+			 jiffies_to_msecs(end - start), tx_time);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static int sun6i_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *tfr)
@@ -267,7 +289,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	if (tfr->len > sspi->fifo_depth)
 		return -EMSGSIZE;
 
-	reinit_completion(&sspi->done);
 	sspi->tx_buf = tfr->tx_buf;
 	sspi->rx_buf = tfr->rx_buf;
 	sspi->len = tfr->len;
@@ -358,21 +379,9 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
 	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
 
-	tx_time = max(tfr->len * 8 * 2 / (tfr->speed_hz / 1000), 100U);
-	start = jiffies;
-	timeout = wait_for_completion_timeout(&sspi->done,
-					      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, tfr->speed_hz,
-			 jiffies_to_msecs(end - start), tx_time);
-		ret = -ETIMEDOUT;
-		goto out;
-	}
+	/* Wait for completion */
+	ret = sun6i_spi_wait_for_transfer(spi, tfr);
 
-out:
 	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
 
 	return ret;
@@ -380,7 +389,8 @@ out:
 
 static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
 {
-	struct sun6i_spi *sspi = dev_id;
+	struct spi_master *master = dev_id;
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
 	u32 status;
 
 	status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
@@ -389,7 +399,7 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
 	if (status & SUN6I_INT_CTL_TC) {
 		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
 		sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
-		complete(&sspi->done);
+		spi_finalize_current_transfer(master);
 		return IRQ_HANDLED;
 	}
 
@@ -496,7 +506,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun6i_spi_handler,
-			       0, dev_name(&pdev->dev), sspi);
+			       0, dev_name(&pdev->dev), master);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request IRQ\n");
 		goto err_free_master;
@@ -518,8 +528,6 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
-	init_completion(&sspi->done);
-
 	sspi->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(sspi->rstc)) {
 		dev_err(&pdev->dev, "Couldn't get reset controller\n");
-- 
2.16.2

Re: [PATCH v3 4/6] spi: sun6i: use completion provided by SPI core

[Maxime Ripard]

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

On Tue, Apr 03, 2018 at 06:44:47PM +0300, Sergey Suloev wrote:
> As long as the completion is already provided by the SPI core
> then there is no need to waste extra-memory on this.
> Also a waiting function was added to avoid code duplication.

This would need to be split in two patches, one to add the waiting
function, the other one to convert to the core struct completion

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH v3 5/6] spi: sun6i: introduce register set/unset helpers

[Sergey Suloev]

Two helper functions were added in order to set/unset
specified flags in registers.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 0912404..2fa9d6e 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -115,29 +115,29 @@ static inline void sun6i_spi_write(struct sun6i_spi *sspi, u32 reg, u32 value)
 	writel(value, sspi->base_addr + reg);
 }
 
-static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
+static inline void sun6i_spi_set(struct sun6i_spi *sspi, u32 addr, u32 val)
 {
-	u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
-
-	reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+	u32 reg = sun6i_spi_read(sspi, addr);
 
-	return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
+	reg |= val;
+	sun6i_spi_write(sspi, addr, reg);
 }
 
-static inline void sun6i_spi_enable_interrupt(struct sun6i_spi *sspi, u32 mask)
+static inline void sun6i_spi_unset(struct sun6i_spi *sspi, u32 addr, u32 val)
 {
-	u32 reg = sun6i_spi_read(sspi, SUN6I_INT_CTL_REG);
+	u32 reg = sun6i_spi_read(sspi, addr);
 
-	reg |= mask;
-	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg);
+	reg &= ~val;
+	sun6i_spi_write(sspi, addr, reg);
 }
 
-static inline void sun6i_spi_disable_interrupt(struct sun6i_spi *sspi, u32 mask)
+static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
 {
-	u32 reg = sun6i_spi_read(sspi, SUN6I_INT_CTL_REG);
+	u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
 
-	reg &= ~mask;
-	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg);
+	reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+
+	return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
 }
 
 static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
@@ -310,18 +310,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
 			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
 
-
-	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
 	/*
 	 * If it's a TX only transfer, we don't want to fill the RX
 	 * FIFO with bogus data
 	 */
 	if (sspi->rx_buf)
-		reg &= ~SUN6I_TFR_CTL_DHB;
+		sun6i_spi_unset(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
 	else
-		reg |= SUN6I_TFR_CTL_DHB;
-
-	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+		sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
 
 
 	/* Ensure that we have a parent clock fast enough */
@@ -376,8 +372,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 					 SUN6I_INT_CTL_RF_RDY);
 
 	/* Start the transfer */
-	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
-	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
+	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
 
 	/* Wait for completion */
 	ret = sun6i_spi_wait_for_transfer(spi, tfr);
-- 
2.16.2

Re: [PATCH v3 5/6] spi: sun6i: introduce register set/unset helpers

[kbuild test robot]

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

Hi Sergey,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.16 next-20180403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sergey-Suloev/spi-Add-support-for-DMA-transfers-in-sun6i-SPI-driver/20180404-053231
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

Note: the linux-review/Sergey-Suloev/spi-Add-support-for-DMA-transfers-in-sun6i-SPI-driver/20180404-053231 HEAD a0c010a285d830f07bb81ea59eaea8773d78b74c builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers//spi/spi-sun6i.c: In function 'sun6i_spi_transfer_one':
>> drivers//spi/spi-sun6i.c:371:2: error: implicit declaration of function 'sun6i_spi_enable_interrupt'; did you mean 'sun6i_spi_transfer_one'? [-Werror=implicit-function-declaration]
     sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
     ^~~~~~~~~~~~~~~~~~~~~~~~~~
     sun6i_spi_transfer_one
   drivers//spi/spi-sun6i.c:277:27: warning: unused variable 'tx_time' [-Wunused-variable]
     unsigned int start, end, tx_time;
                              ^~~~~~~
   drivers//spi/spi-sun6i.c:277:22: warning: unused variable 'end' [-Wunused-variable]
     unsigned int start, end, tx_time;
                         ^~~
   drivers//spi/spi-sun6i.c:277:15: warning: unused variable 'start' [-Wunused-variable]
     unsigned int start, end, tx_time;
                  ^~~~~
   drivers//spi/spi-sun6i.c:276:31: warning: unused variable 'timeout' [-Wunused-variable]
     unsigned int mclk_rate, div, timeout;
                                  ^~~~~~~
   cc1: some warnings being treated as errors

vim +371 drivers//spi/spi-sun6i.c

43836daab Sergey Suloev   2018-04-03  270  
3558fe900 Maxime Ripard   2014-02-05  271  static int sun6i_spi_transfer_one(struct spi_master *master,
3558fe900 Maxime Ripard   2014-02-05  272  				  struct spi_device *spi,
3558fe900 Maxime Ripard   2014-02-05  273  				  struct spi_transfer *tfr)
3558fe900 Maxime Ripard   2014-02-05  274  {
3558fe900 Maxime Ripard   2014-02-05  275  	struct sun6i_spi *sspi = spi_master_get_devdata(master);
3558fe900 Maxime Ripard   2014-02-05  276  	unsigned int mclk_rate, div, timeout;
719bd6542 Michal Suchanek 2016-06-13  277  	unsigned int start, end, tx_time;
913f536c6 Icenowy Zheng   2017-03-06  278  	unsigned int trig_level;
3558fe900 Maxime Ripard   2014-02-05  279  	unsigned int tx_len = 0;
3558fe900 Maxime Ripard   2014-02-05  280  	int ret = 0;
3558fe900 Maxime Ripard   2014-02-05  281  	u32 reg;
3558fe900 Maxime Ripard   2014-02-05  282  
e31cf0250 Sergey Suloev   2018-04-03  283  	/* A zero length transfer never finishes if programmed
e31cf0250 Sergey Suloev   2018-04-03  284  	   in the hardware */
e31cf0250 Sergey Suloev   2018-04-03  285  	if (!tfr->len)
e31cf0250 Sergey Suloev   2018-04-03  286  		return 0;
e31cf0250 Sergey Suloev   2018-04-03  287  
e31cf0250 Sergey Suloev   2018-04-03  288  	/* Don't support transfer larger than the FIFO */
e31cf0250 Sergey Suloev   2018-04-03  289  	if (tfr->len > sspi->fifo_depth)
e31cf0250 Sergey Suloev   2018-04-03  290  		return -EMSGSIZE;
3558fe900 Maxime Ripard   2014-02-05  291  
3558fe900 Maxime Ripard   2014-02-05  292  	sspi->tx_buf = tfr->tx_buf;
3558fe900 Maxime Ripard   2014-02-05  293  	sspi->rx_buf = tfr->rx_buf;
3558fe900 Maxime Ripard   2014-02-05  294  	sspi->len = tfr->len;
3558fe900 Maxime Ripard   2014-02-05  295  
3558fe900 Maxime Ripard   2014-02-05  296  	/* Clear pending interrupts */
3558fe900 Maxime Ripard   2014-02-05  297  	sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0);
3558fe900 Maxime Ripard   2014-02-05  298  
3558fe900 Maxime Ripard   2014-02-05  299  	/* Reset FIFO */
3558fe900 Maxime Ripard   2014-02-05  300  	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
3558fe900 Maxime Ripard   2014-02-05  301  			SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
3558fe900 Maxime Ripard   2014-02-05  302  
3558fe900 Maxime Ripard   2014-02-05  303  	/*
913f536c6 Icenowy Zheng   2017-03-06  304  	 * Setup FIFO interrupt trigger level
913f536c6 Icenowy Zheng   2017-03-06  305  	 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
913f536c6 Icenowy Zheng   2017-03-06  306  	 * value used in old generation of Allwinner SPI controller.
913f536c6 Icenowy Zheng   2017-03-06  307  	 * (See spi-sun4i.c)
913f536c6 Icenowy Zheng   2017-03-06  308  	 */
913f536c6 Icenowy Zheng   2017-03-06  309  	trig_level = sspi->fifo_depth / 4 * 3;
913f536c6 Icenowy Zheng   2017-03-06  310  	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
e31cf0250 Sergey Suloev   2018-04-03  311  			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
913f536c6 Icenowy Zheng   2017-03-06  312  
3558fe900 Maxime Ripard   2014-02-05  313  	/*
3558fe900 Maxime Ripard   2014-02-05  314  	 * If it's a TX only transfer, we don't want to fill the RX
3558fe900 Maxime Ripard   2014-02-05  315  	 * FIFO with bogus data
3558fe900 Maxime Ripard   2014-02-05  316  	 */
3558fe900 Maxime Ripard   2014-02-05  317  	if (sspi->rx_buf)
2c98d976c Sergey Suloev   2018-04-03  318  		sun6i_spi_unset(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
3558fe900 Maxime Ripard   2014-02-05  319  	else
2c98d976c Sergey Suloev   2018-04-03  320  		sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
3558fe900 Maxime Ripard   2014-02-05  321  
09d186f3c Sergey Suloev   2018-04-03  322  
3558fe900 Maxime Ripard   2014-02-05  323  	/* Ensure that we have a parent clock fast enough */
3558fe900 Maxime Ripard   2014-02-05  324  	mclk_rate = clk_get_rate(sspi->mclk);
47284e3e0 Marcus Weseloh  2015-11-08  325  	if (mclk_rate < (2 * tfr->speed_hz)) {
47284e3e0 Marcus Weseloh  2015-11-08  326  		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
3558fe900 Maxime Ripard   2014-02-05  327  		mclk_rate = clk_get_rate(sspi->mclk);
3558fe900 Maxime Ripard   2014-02-05  328  	}
3558fe900 Maxime Ripard   2014-02-05  329  
3558fe900 Maxime Ripard   2014-02-05  330  	/*
3558fe900 Maxime Ripard   2014-02-05  331  	 * Setup clock divider.
3558fe900 Maxime Ripard   2014-02-05  332  	 *
3558fe900 Maxime Ripard   2014-02-05  333  	 * We have two choices there. Either we can use the clock
3558fe900 Maxime Ripard   2014-02-05  334  	 * divide rate 1, which is calculated thanks to this formula:
3558fe900 Maxime Ripard   2014-02-05  335  	 * SPI_CLK = MOD_CLK / (2 ^ cdr)
3558fe900 Maxime Ripard   2014-02-05  336  	 * Or we can use CDR2, which is calculated with the formula:
3558fe900 Maxime Ripard   2014-02-05  337  	 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
3558fe900 Maxime Ripard   2014-02-05  338  	 * Wether we use the former or the latter is set through the
3558fe900 Maxime Ripard   2014-02-05  339  	 * DRS bit.
3558fe900 Maxime Ripard   2014-02-05  340  	 *
3558fe900 Maxime Ripard   2014-02-05  341  	 * First try CDR2, and if we can't reach the expected
3558fe900 Maxime Ripard   2014-02-05  342  	 * frequency, fall back to CDR1.
3558fe900 Maxime Ripard   2014-02-05  343  	 */
47284e3e0 Marcus Weseloh  2015-11-08  344  	div = mclk_rate / (2 * tfr->speed_hz);
3558fe900 Maxime Ripard   2014-02-05  345  	if (div <= (SUN6I_CLK_CTL_CDR2_MASK + 1)) {
3558fe900 Maxime Ripard   2014-02-05  346  		if (div > 0)
3558fe900 Maxime Ripard   2014-02-05  347  			div--;
3558fe900 Maxime Ripard   2014-02-05  348  
3558fe900 Maxime Ripard   2014-02-05  349  		reg = SUN6I_CLK_CTL_CDR2(div) | SUN6I_CLK_CTL_DRS;
3558fe900 Maxime Ripard   2014-02-05  350  	} else {
47284e3e0 Marcus Weseloh  2015-11-08  351  		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
3558fe900 Maxime Ripard   2014-02-05  352  		reg = SUN6I_CLK_CTL_CDR1(div);
3558fe900 Maxime Ripard   2014-02-05  353  	}
3558fe900 Maxime Ripard   2014-02-05  354  
3558fe900 Maxime Ripard   2014-02-05  355  	sun6i_spi_write(sspi, SUN6I_CLK_CTL_REG, reg);
3558fe900 Maxime Ripard   2014-02-05  356  
3558fe900 Maxime Ripard   2014-02-05  357  	/* Setup the transfer now... */
3558fe900 Maxime Ripard   2014-02-05  358  	if (sspi->tx_buf)
3558fe900 Maxime Ripard   2014-02-05  359  		tx_len = tfr->len;
3558fe900 Maxime Ripard   2014-02-05  360  
3558fe900 Maxime Ripard   2014-02-05  361  	/* Setup the counters */
3558fe900 Maxime Ripard   2014-02-05  362  	sun6i_spi_write(sspi, SUN6I_BURST_CNT_REG, SUN6I_BURST_CNT(tfr->len));
3558fe900 Maxime Ripard   2014-02-05  363  	sun6i_spi_write(sspi, SUN6I_XMIT_CNT_REG, SUN6I_XMIT_CNT(tx_len));
3558fe900 Maxime Ripard   2014-02-05  364  	sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
3558fe900 Maxime Ripard   2014-02-05  365  			SUN6I_BURST_CTL_CNT_STC(tx_len));
3558fe900 Maxime Ripard   2014-02-05  366  
3558fe900 Maxime Ripard   2014-02-05  367  	/* Fill the TX FIFO */
10565dfd3 Milo Kim        2016-10-28  368  	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
3558fe900 Maxime Ripard   2014-02-05  369  
3558fe900 Maxime Ripard   2014-02-05  370  	/* Enable the interrupts */
913f536c6 Icenowy Zheng   2017-03-06 @371  	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
913f536c6 Icenowy Zheng   2017-03-06  372  					 SUN6I_INT_CTL_RF_RDY);
3558fe900 Maxime Ripard   2014-02-05  373  
3558fe900 Maxime Ripard   2014-02-05  374  	/* Start the transfer */
2c98d976c Sergey Suloev   2018-04-03  375  	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
3558fe900 Maxime Ripard   2014-02-05  376  
43836daab Sergey Suloev   2018-04-03  377  	/* Wait for completion */
43836daab Sergey Suloev   2018-04-03  378  	ret = sun6i_spi_wait_for_transfer(spi, tfr);
3558fe900 Maxime Ripard   2014-02-05  379  
3558fe900 Maxime Ripard   2014-02-05  380  	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
3558fe900 Maxime Ripard   2014-02-05  381  
3558fe900 Maxime Ripard   2014-02-05  382  	return ret;
3558fe900 Maxime Ripard   2014-02-05  383  }
3558fe900 Maxime Ripard   2014-02-05  384  

:::::: The code at line 371 was first introduced by commit
:::::: 913f536c6c18a2e19e32f06971101c1d0ae3739c spi: sun6i: Allow transfers larger than FIFO size

:::::: TO: Icenowy Zheng <icenowy@aosc.xyz>
:::::: CC: Mark Brown <broonie@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52801 bytes --]

Re: [PATCH v3 5/6] spi: sun6i: introduce register set/unset helpers

[Maxime Ripard]

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

On Tue, Apr 03, 2018 at 06:44:48PM +0300, Sergey Suloev wrote:
> Two helper functions were added in order to set/unset
> specified flags in registers.
> 
> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>

Again, I'm not really sure what the benefit of that is. Your diffstat
is pretty much identical, so it's basically just moving code around.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH v3 6/6] spi: sun6i: add DMA transfers support

[Sergey Suloev]

DMA transfers are now available for sun6i and sun8i SoCs.
The DMA mode is used automatically as soon as requested
transfer length is more than FIFO length.

Changes in v3:
1) Debug log enhancements.

Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
---
 drivers/spi/spi-sun6i.c | 331 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 294 insertions(+), 37 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 2fa9d6e..7f41871 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.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>
@@ -55,17 +57,20 @@
 
 #define SUN6I_FIFO_CTL_REG		0x18
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK	0xff
-#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS	0
+#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS	0
+#define SUN6I_FIFO_CTL_RF_DRQ_EN		BIT(8)
 #define SUN6I_FIFO_CTL_RF_RST			BIT(15)
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK	0xff
-#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS	16
+#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS	16
+#define SUN6I_FIFO_CTL_TF_DRQ_EN		BIT(24)
 #define SUN6I_FIFO_CTL_TF_RST			BIT(31)
+#define SUN6I_FIFO_CTL_DMA_DEDICATE		BIT(9)|BIT(25)
 
 #define SUN6I_FIFO_STA_REG		0x1c
 #define SUN6I_FIFO_STA_RF_CNT_MASK		0x7f
-#define SUN6I_FIFO_STA_RF_CNT_BITS		0
+#define SUN6I_FIFO_STA_RF_CNT_POS		0
 #define SUN6I_FIFO_STA_TF_CNT_MASK		0x7f
-#define SUN6I_FIFO_STA_TF_CNT_BITS		16
+#define SUN6I_FIFO_STA_TF_CNT_POS		16
 
 #define SUN6I_CLK_CTL_REG		0x24
 #define SUN6I_CLK_CTL_CDR2_MASK			0xff
@@ -135,7 +140,7 @@ static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
 {
 	u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
 
-	reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+	reg >>= SUN6I_FIFO_STA_TF_CNT_POS;
 
 	return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
 }
@@ -148,7 +153,7 @@ static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
 	/* See how much data is available */
 	reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
 	reg &= SUN6I_FIFO_STA_RF_CNT_MASK;
-	cnt = reg >> SUN6I_FIFO_STA_RF_CNT_BITS;
+	cnt = reg >> SUN6I_FIFO_STA_RF_CNT_POS;
 
 	if (len > cnt)
 		len = cnt;
@@ -177,6 +182,15 @@ static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi, int len)
 	}
 }
 
+static bool sun6i_spi_can_dma(struct spi_master *master,
+			      struct spi_device *spi,
+			      struct spi_transfer *tfr)
+{
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+	return tfr->len > sspi->fifo_depth;
+}
+
 static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -208,6 +222,9 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
 	struct spi_master *master = spi->master;
 	struct sun6i_spi *sspi = spi_master_get_devdata(master);
 
+	if (master->can_dma)
+		return SUN6I_MAX_XFER_SIZE;
+
 	return sspi->fifo_depth;
 }
 
@@ -268,16 +285,187 @@ static int sun6i_spi_wait_for_transfer(struct spi_device *spi,
 	return 0;
 }
 
+static void sun6i_spi_dma_callback(void *param)
+{
+	struct spi_master *master = param;
+
+	dev_dbg(&master->dev, "DMA transfer complete\n");
+	spi_finalize_current_transfer(master);
+}
+
+static int sun6i_spi_dmap_prep_tx(struct spi_master *master,
+				  struct spi_transfer *tfr,
+				  dma_cookie_t *cookie)
+{
+	struct dma_async_tx_descriptor *chan_desc = NULL;
+
+	chan_desc = 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 (!chan_desc) {
+		dev_err(&master->dev,
+			"Couldn't prepare TX DMA slave\n");
+		return -EIO;
+	}
+
+	chan_desc->callback = sun6i_spi_dma_callback;
+	chan_desc->callback_param = master;
+
+	*cookie = dmaengine_submit(chan_desc);
+	dma_async_issue_pending(master->dma_tx);
+
+	return 0;
+}
+
+static int sun6i_spi_dmap_prep_rx(struct spi_master *master,
+				  struct spi_transfer *tfr,
+				  dma_cookie_t *cookie)
+{
+	struct dma_async_tx_descriptor *chan_desc = NULL;
+
+	chan_desc = 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 (!chan_desc) {
+		dev_err(&master->dev,
+			"Couldn't prepare RX DMA slave\n");
+		return -EIO;
+	}
+
+	chan_desc->callback = sun6i_spi_dma_callback;
+	chan_desc->callback_param = master;
+
+	*cookie = dmaengine_submit(chan_desc);
+	dma_async_issue_pending(master->dma_rx);
+
+	return 0;
+}
+
+static int sun6i_spi_transfer_one_dma(struct spi_device *spi,
+				      struct spi_transfer *tfr)
+{
+	struct spi_master *master = spi->master;
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+	dma_cookie_t tx_cookie = 0,rx_cookie = 0;
+	enum dma_status status;
+	int ret;
+	u32 reg, trig_level = 0;
+
+	dev_dbg(&master->dev, "Using DMA mode for transfer\n");
+
+	/* Disable interrupts */
+	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+
+	reg = sun6i_spi_read(sspi, SUN6I_FIFO_CTL_REG);
+
+	if (sspi->tx_buf) {
+		ret = sun6i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
+		if (ret)
+			goto out;
+
+		reg |= SUN6I_FIFO_CTL_TF_DRQ_EN;
+
+		trig_level = sspi->fifo_depth;
+		reg &= ~SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK;
+		reg |= (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS);
+	}
+
+	if (sspi->rx_buf) {
+		ret = sun6i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
+		if (ret)
+			goto out;
+
+		reg |= SUN6I_FIFO_CTL_RF_DRQ_EN;
+
+		trig_level = 1;
+		reg &= ~SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK;
+		reg |= (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS);
+	}
+
+	/* Enable Dedicated DMA requests */
+	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
+			reg | SUN6I_FIFO_CTL_DMA_DEDICATE);
+
+	/* Start transfer */
+	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
+
+	ret = sun6i_spi_wait_for_transfer(spi, tfr);
+	if (ret)
+		goto out;
+
+	if (sspi->tx_buf && (status = dma_async_is_tx_complete(master->dma_tx,
+			tx_cookie, NULL, NULL))) {
+		dev_warn(&master->dev,
+			"DMA returned completion status of: %s\n",
+			status == DMA_ERROR ? "error" : "in progress");
+	}
+	if (sspi->rx_buf && (status = dma_async_is_tx_complete(master->dma_rx,
+			rx_cookie, NULL, NULL))) {
+		dev_warn(&master->dev,
+			"DMA returned completion status of: %s\n",
+			status == DMA_ERROR ? "error" : "in progress");
+	}
+
+out:
+	if (ret) {
+		dev_dbg(&master->dev, "DMA channel teardown\n");
+		if (sspi->tx_buf)
+			dmaengine_terminate_sync(master->dma_tx);
+		if (sspi->rx_buf)
+			dmaengine_terminate_sync(master->dma_rx);
+	}
+
+	sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
+
+	return ret;
+}
+
+static int sun6i_spi_transfer_one_pio(struct spi_device *spi,
+				      struct spi_transfer *tfr)
+{
+	struct spi_master *master = spi->master;
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+	unsigned int trig_level;
+	int ret;
+
+	/* Disable DMA requests */
+	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, 0);
+
+	/*
+	 * Setup FIFO interrupt trigger level
+	 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
+	 * value used in old generation of Allwinner SPI controller.
+	 * (See spi-sun4i.c)
+	 */
+	trig_level = sspi->fifo_depth / 4 * 3;
+	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
+			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS));
+
+	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
+
+	/* Enable the interrupts */
+	sun6i_spi_set(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC |
+					       SUN6I_INT_CTL_RF_RDY);
+
+	/* Start transfer */
+	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
+
+	ret = sun6i_spi_wait_for_transfer(spi, tfr);
+
+	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+
+	return ret;
+}
+
 static int sun6i_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *tfr)
 {
 	struct sun6i_spi *sspi = spi_master_get_devdata(master);
-	unsigned int mclk_rate, div, timeout;
-	unsigned int start, end, tx_time;
-	unsigned int trig_level;
+	unsigned int mclk_rate, div;
 	unsigned int tx_len = 0;
-	int ret = 0;
 	u32 reg;
 
 	/* A zero length transfer never finishes if programmed
@@ -285,10 +473,15 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	if (!tfr->len)
 		return 0;
 
-	/* Don't support transfer larger than the FIFO */
-	if (tfr->len > sspi->fifo_depth)
+	if (tfr->len > SUN6I_MAX_XFER_SIZE)
 		return -EMSGSIZE;
 
+	if (!master->can_dma) {
+		/* Don't support transfer larger than the FIFO */
+		if (tfr->len > sspi->fifo_depth)
+			return -EMSGSIZE;
+	}
+
 	sspi->tx_buf = tfr->tx_buf;
 	sspi->rx_buf = tfr->rx_buf;
 	sspi->len = tfr->len;
@@ -300,16 +493,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
 			SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
 
-	/*
-	 * Setup FIFO interrupt trigger level
-	 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
-	 * value used in old generation of Allwinner SPI controller.
-	 * (See spi-sun4i.c)
-	 */
-	trig_level = sspi->fifo_depth / 4 * 3;
-	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
-			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
-
 	/*
 	 * If it's a TX only transfer, we don't want to fill the RX
 	 * FIFO with bogus data
@@ -364,22 +547,10 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 	sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
 			SUN6I_BURST_CTL_CNT_STC(tx_len));
 
-	/* Fill the TX FIFO */
-	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
-
-	/* Enable the interrupts */
-	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
-					 SUN6I_INT_CTL_RF_RDY);
-
-	/* Start the transfer */
-	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
-
-	/* Wait for completion */
-	ret = sun6i_spi_wait_for_transfer(spi, tfr);
-
-	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+	if (sun6i_spi_can_dma(master, spi, tfr))
+		return sun6i_spi_transfer_one_dma(spi, tfr);
 
-	return ret;
+	return sun6i_spi_transfer_one_pio(spi, tfr);
 }
 
 static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
@@ -409,6 +580,77 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
+static int sun6i_spi_dma_setup(struct platform_device *pdev,
+			       struct resource *res)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+	struct dma_slave_config dma_sconf;
+	int ret;
+
+	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 TX channel\n");
+		ret = PTR_ERR(master->dma_tx);
+		goto out;
+	}
+
+	dma_sconf.direction = DMA_MEM_TO_DEV;
+	dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconf.dst_addr = res->start + SUN6I_TXDATA_REG;
+	dma_sconf.dst_maxburst = 1;
+	dma_sconf.src_maxburst = 1;
+
+	ret = dmaengine_slave_config(master->dma_tx, &dma_sconf);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure DMA TX slave\n");
+		goto err_rel_tx;
+	}
+
+	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 RX channel\n");
+		ret = PTR_ERR(master->dma_rx);
+		goto err_rel_tx;
+	}
+
+	dma_sconf.direction = DMA_DEV_TO_MEM;
+	dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconf.src_addr = res->start + SUN6I_RXDATA_REG;
+	dma_sconf.src_maxburst = 1;
+	dma_sconf.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(master->dma_rx, &dma_sconf);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure DMA RX slave\n");
+		goto err_rel_rx;
+	}
+
+	/* don't set can_dma unless both channels are valid*/
+	master->can_dma = sun6i_spi_can_dma;
+
+	return 0;
+
+err_rel_rx:
+	dma_release_channel(master->dma_rx);
+err_rel_tx:
+	dma_release_channel(master->dma_tx);
+out:
+	master->dma_tx = NULL;
+	master->dma_rx = NULL;
+	return ret;
+}
+
+static void sun6i_spi_dma_release(struct spi_master *master)
+{
+	if (master->can_dma) {
+		dma_release_channel(master->dma_rx);
+		dma_release_channel(master->dma_tx);
+	}
+}
+
 static int sun6i_spi_runtime_resume(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
@@ -530,6 +772,16 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
+	ret = sun6i_spi_dma_setup(pdev, res);
+	if (ret) {
+		if (ret == -EPROBE_DEFER) {
+			/* wait for the dma driver to load */
+			goto err_free_master;
+		}
+		dev_warn(&pdev->dev,
+			"Unable to setup DMA channels: DMA transfers disabled\n");
+	}
+
 	/*
 	 * This wake-up/shutdown pattern is to be able to have the
 	 * device woken up, even if runtime_pm is disabled
@@ -556,14 +808,19 @@ err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	sun6i_spi_runtime_suspend(&pdev->dev);
 err_free_master:
+	sun6i_spi_dma_release(master);
 	spi_master_put(master);
 	return ret;
 }
 
 static int sun6i_spi_remove(struct platform_device *pdev)
 {
+	struct spi_master *master = platform_get_drvdata(pdev);
+
 	pm_runtime_force_suspend(&pdev->dev);
 
+	sun6i_spi_dma_release(master);
+
 	return 0;
 }
 
-- 
2.16.2

Re: [PATCH v3 6/6] spi: sun6i: add DMA transfers support

[Maxime Ripard]

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

On Tue, Apr 03, 2018 at 06:44:49PM +0300, Sergey Suloev wrote:
> DMA transfers are now available for sun6i and sun8i SoCs.
> The DMA mode is used automatically as soon as requested
> transfer length is more than FIFO length.
> 
> Changes in v3:
> 1) Debug log enhancements.
> 
> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com>
> ---
>  drivers/spi/spi-sun6i.c | 331 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 294 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 2fa9d6e..7f41871 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.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>
> @@ -55,17 +57,20 @@
>  
>  #define SUN6I_FIFO_CTL_REG		0x18
>  #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK	0xff
> -#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS	0
> +#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS	0

Why are you changing that name

> +#define SUN6I_FIFO_CTL_RF_DRQ_EN		BIT(8)
>  #define SUN6I_FIFO_CTL_RF_RST			BIT(15)
>  #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK	0xff
> -#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS	16
> +#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS	16

Ditto.

> +#define SUN6I_FIFO_CTL_TF_DRQ_EN		BIT(24)
>  #define SUN6I_FIFO_CTL_TF_RST			BIT(31)
> +#define SUN6I_FIFO_CTL_DMA_DEDICATE		BIT(9)|BIT(25)
>  
>  #define SUN6I_FIFO_STA_REG		0x1c
>  #define SUN6I_FIFO_STA_RF_CNT_MASK		0x7f
> -#define SUN6I_FIFO_STA_RF_CNT_BITS		0
> +#define SUN6I_FIFO_STA_RF_CNT_POS		0

Ditto.

>  #define SUN6I_FIFO_STA_TF_CNT_MASK		0x7f
> -#define SUN6I_FIFO_STA_TF_CNT_BITS		16
> +#define SUN6I_FIFO_STA_TF_CNT_POS		16

Ditto.

>  
>  #define SUN6I_CLK_CTL_REG		0x24
>  #define SUN6I_CLK_CTL_CDR2_MASK			0xff
> @@ -135,7 +140,7 @@ static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
>  {
>  	u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
>  
> -	reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
> +	reg >>= SUN6I_FIFO_STA_TF_CNT_POS;
>  
>  	return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
>  }
> @@ -148,7 +153,7 @@ static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
>  	/* See how much data is available */
>  	reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
>  	reg &= SUN6I_FIFO_STA_RF_CNT_MASK;
> -	cnt = reg >> SUN6I_FIFO_STA_RF_CNT_BITS;
> +	cnt = reg >> SUN6I_FIFO_STA_RF_CNT_POS;
>  
>  	if (len > cnt)
>  		len = cnt;
> @@ -177,6 +182,15 @@ static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi, int len)
>  	}
>  }
>  
> +static bool sun6i_spi_can_dma(struct spi_master *master,
> +			      struct spi_device *spi,
> +			      struct spi_transfer *tfr)
> +{
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +
> +	return tfr->len > sspi->fifo_depth;
> +}
> +
>  static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>  {
>  	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
> @@ -208,6 +222,9 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>  	struct spi_master *master = spi->master;
>  	struct sun6i_spi *sspi = spi_master_get_devdata(master);
>  
> +	if (master->can_dma)
> +		return SUN6I_MAX_XFER_SIZE;
> +
>  	return sspi->fifo_depth;
>  }
>  
> @@ -268,16 +285,187 @@ static int sun6i_spi_wait_for_transfer(struct spi_device *spi,
>  	return 0;
>  }
>  
> +static void sun6i_spi_dma_callback(void *param)
> +{
> +	struct spi_master *master = param;
> +
> +	dev_dbg(&master->dev, "DMA transfer complete\n");
> +	spi_finalize_current_transfer(master);
> +}
> +
> +static int sun6i_spi_dmap_prep_tx(struct spi_master *master,
> +				  struct spi_transfer *tfr,
> +				  dma_cookie_t *cookie)
> +{
> +	struct dma_async_tx_descriptor *chan_desc = NULL;
> +
> +	chan_desc = 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 (!chan_desc) {
> +		dev_err(&master->dev,
> +			"Couldn't prepare TX DMA slave\n");
> +		return -EIO;
> +	}
> +
> +	chan_desc->callback = sun6i_spi_dma_callback;
> +	chan_desc->callback_param = master;
> +
> +	*cookie = dmaengine_submit(chan_desc);
> +	dma_async_issue_pending(master->dma_tx);
> +
> +	return 0;
> +}
> +
> +static int sun6i_spi_dmap_prep_rx(struct spi_master *master,
> +				  struct spi_transfer *tfr,
> +				  dma_cookie_t *cookie)
> +{
> +	struct dma_async_tx_descriptor *chan_desc = NULL;
> +
> +	chan_desc = 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 (!chan_desc) {
> +		dev_err(&master->dev,
> +			"Couldn't prepare RX DMA slave\n");
> +		return -EIO;
> +	}
> +
> +	chan_desc->callback = sun6i_spi_dma_callback;
> +	chan_desc->callback_param = master;
> +
> +	*cookie = dmaengine_submit(chan_desc);
> +	dma_async_issue_pending(master->dma_rx);
> +
> +	return 0;
> +}
> +
> +static int sun6i_spi_transfer_one_dma(struct spi_device *spi,
> +				      struct spi_transfer *tfr)
> +{
> +	struct spi_master *master = spi->master;
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +	dma_cookie_t tx_cookie = 0,rx_cookie = 0;
> +	enum dma_status status;
> +	int ret;
> +	u32 reg, trig_level = 0;
> +
> +	dev_dbg(&master->dev, "Using DMA mode for transfer\n");
> +
> +	/* Disable interrupts */
> +	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> +
> +	reg = sun6i_spi_read(sspi, SUN6I_FIFO_CTL_REG);
> +
> +	if (sspi->tx_buf) {
> +		ret = sun6i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
> +		if (ret)
> +			goto out;
> +
> +		reg |= SUN6I_FIFO_CTL_TF_DRQ_EN;
> +
> +		trig_level = sspi->fifo_depth;
> +		reg &= ~SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK;
> +		reg |= (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS);
> +	}
> +
> +	if (sspi->rx_buf) {
> +		ret = sun6i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
> +		if (ret)
> +			goto out;
> +
> +		reg |= SUN6I_FIFO_CTL_RF_DRQ_EN;
> +
> +		trig_level = 1;
> +		reg &= ~SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK;
> +		reg |= (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS);
> +	}
> +
> +	/* Enable Dedicated DMA requests */
> +	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> +			reg | SUN6I_FIFO_CTL_DMA_DEDICATE);
> +
> +	/* Start transfer */
> +	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> +
> +	ret = sun6i_spi_wait_for_transfer(spi, tfr);
> +	if (ret)
> +		goto out;
> +
> +	if (sspi->tx_buf && (status = dma_async_is_tx_complete(master->dma_tx,
> +			tx_cookie, NULL, NULL))) {
> +		dev_warn(&master->dev,
> +			"DMA returned completion status of: %s\n",
> +			status == DMA_ERROR ? "error" : "in progress");
> +	}
> +	if (sspi->rx_buf && (status = dma_async_is_tx_complete(master->dma_rx,
> +			rx_cookie, NULL, NULL))) {
> +		dev_warn(&master->dev,
> +			"DMA returned completion status of: %s\n",
> +			status == DMA_ERROR ? "error" : "in progress");
> +	}

I'm not sure why that would be needed.

> +out:
> +	if (ret) {
> +		dev_dbg(&master->dev, "DMA channel teardown\n");
> +		if (sspi->tx_buf)
> +			dmaengine_terminate_sync(master->dma_tx);
> +		if (sspi->rx_buf)
> +			dmaengine_terminate_sync(master->dma_rx);
> +	}

That can be addressed through additional labels.

> +	sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);

Why are you doing a PIO access here?

> +	return ret;
> +}
> +
> +static int sun6i_spi_transfer_one_pio(struct spi_device *spi,
> +				      struct spi_transfer *tfr)
> +{
> +	struct spi_master *master = spi->master;
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +	unsigned int trig_level;
> +	int ret;
> +
> +	/* Disable DMA requests */
> +	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, 0);
> +
> +	/*
> +	 * Setup FIFO interrupt trigger level
> +	 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
> +	 * value used in old generation of Allwinner SPI controller.
> +	 * (See spi-sun4i.c)
> +	 */
> +	trig_level = sspi->fifo_depth / 4 * 3;
> +	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> +			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS));
> +
> +	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> +
> +	/* Enable the interrupts */
> +	sun6i_spi_set(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC |
> +					       SUN6I_INT_CTL_RF_RDY);
> +
> +	/* Start transfer */
> +	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> +
> +	ret = sun6i_spi_wait_for_transfer(spi, tfr);
> +
> +	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> +
> +	return ret;
> +}
> +
>  static int sun6i_spi_transfer_one(struct spi_master *master,
>  				  struct spi_device *spi,
>  				  struct spi_transfer *tfr)
>  {
>  	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> -	unsigned int mclk_rate, div, timeout;
> -	unsigned int start, end, tx_time;
> -	unsigned int trig_level;
> +	unsigned int mclk_rate, div;
>  	unsigned int tx_len = 0;
> -	int ret = 0;
>  	u32 reg;
>  
>  	/* A zero length transfer never finishes if programmed
> @@ -285,10 +473,15 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	if (!tfr->len)
>  		return 0;
>  
> -	/* Don't support transfer larger than the FIFO */
> -	if (tfr->len > sspi->fifo_depth)
> +	if (tfr->len > SUN6I_MAX_XFER_SIZE)
>  		return -EMSGSIZE;
>  
> +	if (!master->can_dma) {
> +		/* Don't support transfer larger than the FIFO */
> +		if (tfr->len > sspi->fifo_depth)
> +			return -EMSGSIZE;
> +	}
> +
>  	sspi->tx_buf = tfr->tx_buf;
>  	sspi->rx_buf = tfr->rx_buf;
>  	sspi->len = tfr->len;
> @@ -300,16 +493,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
>  			SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>  
> -	/*
> -	 * Setup FIFO interrupt trigger level
> -	 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
> -	 * value used in old generation of Allwinner SPI controller.
> -	 * (See spi-sun4i.c)
> -	 */
> -	trig_level = sspi->fifo_depth / 4 * 3;
> -	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> -			(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
> -
>  	/*
>  	 * If it's a TX only transfer, we don't want to fill the RX
>  	 * FIFO with bogus data
> @@ -364,22 +547,10 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>  	sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
>  			SUN6I_BURST_CTL_CNT_STC(tx_len));
>  
> -	/* Fill the TX FIFO */
> -	sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> -
> -	/* Enable the interrupts */
> -	sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
> -					 SUN6I_INT_CTL_RF_RDY);
> -
> -	/* Start the transfer */
> -	sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> -
> -	/* Wait for completion */
> -	ret = sun6i_spi_wait_for_transfer(spi, tfr);
> -
> -	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> +	if (sun6i_spi_can_dma(master, spi, tfr))
> +		return sun6i_spi_transfer_one_dma(spi, tfr);
>  
> -	return ret;
> +	return sun6i_spi_transfer_one_pio(spi, tfr);

You should really split the function creation here into a separate patch

>  }
>  
>  static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> @@ -409,6 +580,77 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>  	return IRQ_NONE;
>  }
>  
> +static int sun6i_spi_dma_setup(struct platform_device *pdev,
> +			       struct resource *res)
> +{
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +	struct dma_slave_config dma_sconf;
> +	int ret;
> +
> +	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 TX channel\n");
> +		ret = PTR_ERR(master->dma_tx);
> +		goto out;
> +	}
> +
> +	dma_sconf.direction = DMA_MEM_TO_DEV;
> +	dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconf.dst_addr = res->start + SUN6I_TXDATA_REG;
> +	dma_sconf.dst_maxburst = 1;
> +	dma_sconf.src_maxburst = 1;
> +
> +	ret = dmaengine_slave_config(master->dma_tx, &dma_sconf);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to configure DMA TX slave\n");
> +		goto err_rel_tx;
> +	}
> +
> +	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 RX channel\n");
> +		ret = PTR_ERR(master->dma_rx);
> +		goto err_rel_tx;
> +	}
> +
> +	dma_sconf.direction = DMA_DEV_TO_MEM;
> +	dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconf.src_addr = res->start + SUN6I_RXDATA_REG;
> +	dma_sconf.src_maxburst = 1;
> +	dma_sconf.dst_maxburst = 1;
> +
> +	ret = dmaengine_slave_config(master->dma_rx, &dma_sconf);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to configure DMA RX slave\n");
> +		goto err_rel_rx;
> +	}
> +
> +	/* don't set can_dma unless both channels are valid*/
> +	master->can_dma = sun6i_spi_can_dma;
> +
> +	return 0;
> +
> +err_rel_rx:
> +	dma_release_channel(master->dma_rx);
> +err_rel_tx:
> +	dma_release_channel(master->dma_tx);
> +out:
> +	master->dma_tx = NULL;
> +	master->dma_rx = NULL;
> +	return ret;
> +}
> +
> +static void sun6i_spi_dma_release(struct spi_master *master)
> +{
> +	if (master->can_dma) {
> +		dma_release_channel(master->dma_rx);
> +		dma_release_channel(master->dma_tx);
> +	}
> +}
> +
>  static int sun6i_spi_runtime_resume(struct device *dev)
>  {
>  	struct spi_master *master = dev_get_drvdata(dev);
> @@ -530,6 +772,16 @@ static int sun6i_spi_probe(struct platform_device *pdev)
>  		goto err_free_master;
>  	}
>  
> +	ret = sun6i_spi_dma_setup(pdev, res);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER) {
> +			/* wait for the dma driver to load */

There's no need for that comment.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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