spi: Add support for DMA transfers in sun6i SPI driver

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 <[email protected]>

---
 drivers/spi/spi-sun6i.c | 97 +++++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 39 deletions(-)

drivers/spi/spi-sun6i.c +31 -1

index 8533f4e..88ad45e 100644
@@ -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;
+

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 <[email protected]>

---
 drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

drivers/spi/spi-sun6i.c +0 -0

index 78acc1f..c09ad10 100644

drivers/spi/spi-sun6i.c +1 -2

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

Quoted 17 lines · 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 <[email protected]>

---
  drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
  1 file changed, 17 insertions(+), 24 deletions(-)

drivers/spi/spi-sun6i.c +0 -0

index 78acc1f..c09ad10 100644

drivers/spi/spi-sun6i.c static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) · line 213

Quoted 8 lines

drivers/spi/spi-sun6i.c +4 -1

@@ -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?

Quoted 8 lines

drivers/spi/spi-sun6i.c +0 -0

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

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 262

Quoted 4 lines

drivers/spi/spi-sun6i.c +2 -2

-	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.

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 264

Quoted 3 lines

drivers/spi/spi-sun6i.c +2 -0

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

Can that case even happen?

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 268

Quoted 4 lines

drivers/spi/spi-sun6i.c +3 -0

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

You're changing the return type, why?

Quoted 16 lines

drivers/spi/spi-sun6i.c +1 -2

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

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 350

Quoted 6 lines

drivers/spi/spi-sun6i.c +0 -3

  	/* 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.

drivers/spi/spi-sun6i.c out: · line 386

Quoted 12 lines

drivers/spi/spi-sun6i.c +3 -1

  	/* 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

Quoted 27 lines · Maxime Ripard

Quoted 17 lines · 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 <[email protected]>

---
  drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
  1 file changed, 17 insertions(+), 24 deletions(-)

drivers/spi/spi-sun6i.c +0 -0

index 78acc1f..c09ad10 100644

drivers/spi/spi-sun6i.c static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) · line 212

Quoted 7 lines

drivers/spi/spi-sun6i.c +4 -1

@@ -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 ?

Quoted 1 line

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.

Quoted 12 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 261

Quoted 10 lines

drivers/spi/spi-sun6i.c +2 -2

  }
  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

Quoted 4 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 264

Quoted 3 lines

drivers/spi/spi-sun6i.c +2 -0

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

Can that case even happen?

Not sure, just for safety.

Quoted 5 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 268

Quoted 4 lines

drivers/spi/spi-sun6i.c +3 -0

+	/* 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

Quoted 12 lines

Quoted 12 lines

drivers/spi/spi-sun6i.c +1 -2

  	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,

Quoted 8 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 350

Quoted 7 lines

drivers/spi/spi-sun6i.c +0 -3

  	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 ?

Quoted 12 lines

drivers/spi/spi-sun6i.c out: · line 386

Quoted 11 lines

drivers/spi/spi-sun6i.c +3 -1

  	/* 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.

Quoted 28 lines · Sergey Suloev

Quoted 27 lines · Maxime Ripard

Quoted 17 lines · 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 <[email protected]>

---
   drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
   1 file changed, 17 insertions(+), 24 deletions(-)

drivers/spi/spi-sun6i.c +0 -0

index 78acc1f..c09ad10 100644

drivers/spi/spi-sun6i.c static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) · line 212

Quoted 7 lines

drivers/spi/spi-sun6i.c +4 -1

@@ -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 ?

Quoted 2 lines

Quoted 1 line

Why do you need to do so?

Need what?

Why do you need to revert that change.

Quoted 3 lines

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.

Quoted 15 lines

Quoted 11 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 261

Quoted 9 lines

drivers/spi/spi-sun6i.c +2 -2

   }
   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

Quoted 3 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 264

Quoted 2 lines

drivers/spi/spi-sun6i.c +2 -0

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

Can that case even happen?

Quoted 6 lines

Not sure, just for safety.

Quoted 4 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 268

Quoted 3 lines

drivers/spi/spi-sun6i.c +3 -0

+	/* 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.

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

drivers/spi/spi-sun6i.c +1 -2

   	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,

Quoted 8 lines

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 350

Quoted 7 lines

drivers/spi/spi-sun6i.c +0 -3

   	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.

Quoted 1 line

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

Quoted 11 lines

Quoted 11 lines

drivers/spi/spi-sun6i.c out: · line 386

Quoted 10 lines

drivers/spi/spi-sun6i.c +3 -1

   	/* 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?

Quoted 1 line

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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 3 lines · Maxime Ripard

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.

Quoted 5 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 5 lines · Mark Brown

Quoted 4 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 10 lines · Sergey Suloev

Quoted 9 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 3 lines

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

Quoted 14 lines · Maxime Ripard

Quoted 13 lines · Sergey Suloev

Quoted 9 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 3 lines

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"

Quoted 12 lines · Sergey Suloev

Quoted 11 lines · Maxime Ripard

Quoted 10 lines · Sergey Suloev

Quoted 9 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 7 lines · Maxime Ripard

Quoted 4 lines · Sergey Suloev

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.

Quoted 5 lines · Sergey Suloev

Quoted 4 lines · Maxime Ripard

Quoted 3 lines · Sergey Suloev

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"

Quoted 3 lines

Quoted 1 line

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.

Quoted 8 lines · Mark Brown

Quoted 2 lines · Sergey Suloev

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.

Quoted 6 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 4 lines · Maxime Ripard

Quoted 3 lines · Sergey Suloev

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"

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 18 lines · Sergey Suloev

Quoted 6 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 4 lines · Maxime Ripard

Quoted 3 lines · Sergey Suloev

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"

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 1 line

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

Then it must be fixed, and not silently reverted.

Quoted 4 lines

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

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

Quoted 8 lines

Quoted 4 lines

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().

Quoted 10 lines · Sergey Suloev

Quoted 9 lines · Mark Brown

Quoted 5 lines · Sergey Suloev

Quoted 3 lines · Maxime Ripard

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.

Quoted 3 lines

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

Quoted 1 line · Sergey Suloev

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

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.

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

Signed-off-by Sergey Suloev <[email protected]>

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 <[email protected]>

---
 drivers/spi/spi-sun6i.c | 331 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 294 insertions(+), 37 deletions(-)

drivers/spi/spi-sun6i.c +2 -0

index 2fa9d6e..7f41871 100644
@@ -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>

drivers/spi/spi-sun6i.c +126 -2

 #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))) {

drivers/spi/spi-sun6i.c +48 -25

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

drivers/spi/spi-sun6i.c +71 -0

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