Skip to contentSkip to conversation

spi: Add support for DMA transfers in sun4i SPI driver

PATCH Superseded Sergey Suloev 3 participants 18 messages Apr 3, 2018 – Apr 4, 2018

Subsystems (2)
SPI Arm/Allwinner sunXi SoC support

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Sergey Suloev

author
#0 lore raw

v2 cover spi: Add support for DMA transfers in sun4i SPI driver

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

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

2) Debug log enhancements.

Sergey Suloev (6):
spi: core: handle timeout error from transfer_one()
spi: sun4i: restrict transfer length in PIO-mode
spi: sun4i: coding style/readability improvements
spi: sun4i: use completion provided by SPI core driver
spi: sun4i: introduce register set/unset helpers
spi: sun4i: add DMA transfers support

 drivers/spi/spi-sun4i.c | 442 +++++++++++++++++++++++++++++++++++++-----------
 drivers/spi/spi.c       |   5 +-
 2 files changed, 347 insertions(+), 100 deletions(-)
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH v2 1/6 spi: core: handle timeout error from transfer_one()

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/spi/spi.c +3 -2
index b33a727..2dcd4f6 100644
@@ -1028,7 +1028,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 			reinit_completion(&ctlr->xfer_completion);
 
 			ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
-			if (ret < 0) {
+			if (ret < 0 && ret != -ETIMEDOUT) {
 				SPI_STATISTICS_INCREMENT_FIELD(statm,
 							       errors);
 				SPI_STATISTICS_INCREMENT_FIELD(stats,

@@ -1051,7 +1051,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 								 msecs_to_jiffies(ms));
 			}
 
-			if (ms == 0) {
+			if (ms == 0 || ret == -ETIMEDOUT) {
 				SPI_STATISTICS_INCREMENT_FIELD(statm,
 							       timedout);
 				SPI_STATISTICS_INCREMENT_FIELD(stats,

@@ -1059,6 +1059,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 				dev_err(&msg->spi->dev,
 					"SPI transfer timed out\n");
 				msg->status = -ETIMEDOUT;
+				ret = 0;
 			}
 		} else {
 			if (xfer->len)
Signature · 1 line
2.16.2
maintainer
#2 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Why is this connected to those drivers specifically?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180403/5f6f43b2/attachment.sig>

author
#3 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 5 lines · Mark Brown
Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Why is this connected to those drivers specifically?

These 2 drivers have their own "waiting" code and not using the code from SPI core.

maintainer
#4 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 10 lines · Sergey Suloev
Quoted 6 lines · Mark Brown
Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Why is this connected to those drivers specifically?

These 2 drivers have their own "waiting" code and not using the code from SPI core.

Does this not apply to any other driver - why is this something we only
have to do when these drivers do it? That's what's setting off alarm
bells.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180403/d4208218/attachment.sig>

author
#5 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 6 lines · Mark Brown
Quoted 5 lines · Sergey Suloev
Quoted 4 lines · Mark Brown
Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Why is this connected to those drivers specifically?

These 2 drivers have their own "waiting" code and not using the code from SPI core.

Does this not apply to any other driver - why is this something we only have to do when these drivers do it? That's what's setting off alarm bells.

sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a fixed interval to wait.

I can't say for every SPI driver in kernel, that's outside of my area of expertise.

#6 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 14 lines · Sergey Suloev
Quoted 6 lines · Mark Brown
Quoted 5 lines · Sergey Suloev
Quoted 4 lines · Mark Brown
Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Why is this connected to those drivers specifically?

These 2 drivers have their own "waiting" code and not using the code from SPI core.

Does this not apply to any other driver - why is this something we only have to do when these drivers do it? That's what's setting off alarm bells.

Quoted 5 lines

sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a fixed interval to wait.

I can't say for every SPI driver in kernel, that's outside of my area of expertise.

I'm not sure what's specific about the sun4i / sun6i case here. Your patch doesn't have anything to do with the delay before the timeout, but the fact that we return -ETIMEDOUT in the first place.

And I'm pretty sure that papering over an error returned by a driver is not the right thing to do.

Maxime

Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/614b2c6d/attachment.sig>
maintainer
#7 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

Quoted 2 lines · Maxime Ripard

And I'm pretty sure that papering over an error returned by a driver is not the right thing to do.

We've got specific error handling for timeouts - they get accounted for
separately in the stats. It *shouldn't* affect actual operation and
AFAICT it doesn't. I think the main problem here is that the commit
message is very unclear.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/8f12f834/attachment.sig>

author
#8 lore raw

Re: [PATCH v2 1/6] spi: core: handle timeout error from transfer_one()

In reply to Maxime Ripard
Quoted 26 lines · Maxime Ripard
Quoted 10 lines · Sergey Suloev
Quoted 9 lines · Mark Brown
Quoted 8 lines · Sergey Suloev
Quoted 5 lines · Mark Brown
Quoted 3 lines · Sergey Suloev

As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one().

Why is this connected to those drivers specifically?

These 2 drivers have their own "waiting" code and not using the code from SPI core.

Quoted 8 lines
Quoted 3 lines

Does this not apply to any other driver - why is this something we only have to do when these drivers do it? That's what's setting off alarm bells.

sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a fixed interval to wait.

I can't say for every SPI driver in kernel, that's outside of my area of expertise.

I'm not sure what's specific about the sun4i / sun6i case here. Your patch doesn't have anything to do with the delay before the timeout, but the fact that we return -ETIMEDOUT in the first place.

And I'm pretty sure that papering over an error returned by a driver is not the right thing to do.

Quoted 8 lines

Maxime

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

do you mean the changes in spi.c are not required at all ?

My point was to eat ETIMEDOUT error from transfer_one() as it is just a mark and

shouldn't be handled as a normal error.

author
#9 lore raw

PATCH v2 2/6 spi: sun4i: restrict transfer length in PIO-mode

In reply to Sergey Suloev

There is no need to handle the 3/4 FIFO empty interrupt as the maximum supported transfer length in PIO mode is 64 bytes.
As long as a problem was reported previously with filling FIFO on A10s we want to stick with 63 bytes depth.

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

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)
drivers/spi/spi-sun4i.c +10 -27
index 4141003..08fd007 100644
@@ -22,7 +22,12 @@
 
 #include <linux/spi/spi.h>
 
-#define SUN4I_FIFO_DEPTH		64
+/*
+ * FIFO length is 64 bytes
+ * But filling the FIFO fully might cause a timeout
+ * on some devices, for example on spi2 on A10s
+ */
+#define SUN4I_FIFO_DEPTH		63
 
 #define SUN4I_RXDATA_REG		0x00
 

@@ -202,7 +207,7 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
 
 static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
 {
-	return SUN4I_FIFO_DEPTH - 1;
+	return SUN4I_FIFO_DEPTH;
 }
 
 static int sun4i_spi_transfer_one(struct spi_master *master,

@@ -216,11 +221,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	int ret = 0;
 	u32 reg;
 
-	/* We don't support transfer larger than the FIFO */
-	if (tfr->len > SUN4I_MAX_XFER_SIZE)
-		return -EMSGSIZE;
-
-	if (tfr->tx_buf && tfr->len >= SUN4I_MAX_XFER_SIZE)
+	/* We don't support transfers larger than FIFO depth */
+	if (tfr->len > SUN4I_FIFO_DEPTH)
 		return -EMSGSIZE;
 
 	reinit_completion(&sspi->done);

@@ -313,17 +315,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 
 	/*
 	 * Fill the TX FIFO
-	 * Filling the FIFO fully causes timeout for some reason
-	 * at least on spi2 on A10s
 	 */
-	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
 
 	/* Enable the interrupts */
 	sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
 					 SUN4I_INT_CTL_RF_F34);
-	/* Only enable Tx FIFO interrupt if we really need it */
-	if (tx_len > SUN4I_FIFO_DEPTH)
-		sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);
 
 	/* Start the transfer */
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);

@@ -371,20 +368,6 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
-	/* Transmit FIFO 3/4 empty */
-	if (status & SUN4I_INT_CTL_TF_E34) {
-		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
-
-		if (!sspi->len)
-			/* nothing left to transmit */
-			sun4i_spi_disable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);
-
-		/* Only clear the interrupt _after_ re-seeding the FIFO */
-		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TF_E34);
-
-		return IRQ_HANDLED;
-	}
-
 	return IRQ_NONE;
 }
Signature · 1 line
2.16.2
#10 lore raw

Re: [PATCH v2 2/6] spi: sun4i: restrict transfer length in PIO-mode

Quoted 19 lines · Sergey Suloev

There is no need to handle the 3/4 FIFO empty interrupt as the maximum supported transfer length in PIO mode is 64 bytes.
As long as a problem was reported previously with filling FIFO on A10s we want to stick with 63 bytes depth.

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

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)
drivers/spi/spi-sun4i.c +0 -0
index 4141003..08fd007 100644

drivers/spi/spi-sun4i.c line 30

Quoted 10 lines
drivers/spi/spi-sun4i.c +6 -1
@@ -22,7 +22,12 @@
 
 #include <linux/spi/spi.h>
 
-#define SUN4I_FIFO_DEPTH		64
+/*
+ * FIFO length is 64 bytes
+ * But filling the FIFO fully might cause a timeout
+ * on some devices, for example on spi2 on A10s
+ */
+#define SUN4I_FIFO_DEPTH		63

The FIFO depth is 64 bytes, so the code should remain the same at least from that regard.

Quoted 13 lines
drivers/spi/spi-sun4i.c +1 -1
 #define SUN4I_RXDATA_REG		0x00
 

@@ -202,7 +207,7 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
 
 static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
 {
-	return SUN4I_FIFO_DEPTH - 1;
+	return SUN4I_FIFO_DEPTH;
 }
 
 static int sun4i_spi_transfer_one(struct spi_master *master,

@@ -216,11 +221,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	int ret = 0;

drivers/spi/spi-sun4i.c static int sun4i_spi_transfer_one(struct spi_master *master, · line 226

Quoted 10 lines
drivers/spi/spi-sun4i.c +2 -5
 	u32 reg;
 
-	/* We don't support transfer larger than the FIFO */
-	if (tfr->len > SUN4I_MAX_XFER_SIZE)
-		return -EMSGSIZE;
-
-	if (tfr->tx_buf && tfr->len >= SUN4I_MAX_XFER_SIZE)
+	/* We don't support transfers larger than FIFO depth */
+	if (tfr->len > SUN4I_FIFO_DEPTH)
 		return -EMSGSIZE;

This essentially reverts 196737912da5, why?

Maxime

Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/7de4c81b/attachment.sig>
author
#11 lore raw

PATCH v2 3/6 spi: sun4i: coding style/readability improvements

In reply to Sergey Suloev

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

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
drivers/spi/spi-sun4i.c +17 -15
index 08fd007..899e956 100644
@@ -83,8 +83,11 @@
 #define SUN4I_FIFO_STA_TF_CNT_MASK		0x7f
 #define SUN4I_FIFO_STA_TF_CNT_BITS		16
 
+#define SUN4I_SPI_MAX_SPEED_HZ		100 * 1000 * 1000
+#define SUN4I_SPI_MIN_SPEED_HZ		3 * 1000
+#define SUN4I_SPI_MODE_BITS		(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
+
 struct sun4i_spi {
-	struct spi_master	*master;
 	void __iomem		*base_addr;
 	struct clk		*hclk;
 	struct clk		*mclk;

@@ -418,12 +421,23 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	struct resource	*res;
 	int ret = 0, irq;
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct sun4i_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 = SUN4I_SPI_MAX_SPEED_HZ;
+	master->min_speed_hz = SUN4I_SPI_MIN_SPEED_HZ;
+	master->num_chipselect = 4;
+	master->mode_bits = SUN4I_SPI_MODE_BITS;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->set_cs = sun4i_spi_set_cs;
+	master->transfer_one = sun4i_spi_transfer_one;
+	master->max_transfer_size = sun4i_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);
 

@@ -442,24 +456,12 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
-			       0, "sun4i-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;
-	master->max_speed_hz = 100 * 1000 * 1000;
-	master->min_speed_hz = 3 * 1000;
-	master->set_cs = sun4i_spi_set_cs;
-	master->transfer_one = sun4i_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 = sun4i_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");
Signature · 1 line
2.16.2
#12 lore raw

Re: [PATCH v2 3/6] spi: sun4i: coding style/readability improvements

Quoted 2 lines · Sergey Suloev

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

Again, this is a very subjective statement. Overall, I'm not convinced by the idea of moving code around and shuffling variables assignments just for the sake of it.

Maxime

Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/65893e53/attachment.sig>
author
#13 lore raw

PATCH v2 4/6 spi: sun4i: use completion provided by SPI core driver

In reply to Sergey Suloev

As long as the completion 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.

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 62 ++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 27 deletions(-)
drivers/spi/spi-sun4i.c +35 -27
index 899e956..9d1bc20 100644
@@ -92,8 +92,6 @@ struct sun4i_spi {
 	struct clk		*hclk;
 	struct clk		*mclk;
 
-	struct completion	done;
-
 	const u8		*tx_buf;
 	u8			*rx_buf;
 	int			len;

@@ -213,13 +211,36 @@ static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
 	return SUN4I_FIFO_DEPTH;
 }
 
+static int sun4i_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;
+
+	/* calc required timeout from given speed & len values */
+	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 sun4i_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *tfr)
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(master);
-	unsigned int mclk_rate, div, timeout;
-	unsigned int start, end, tx_time;
+	unsigned int mclk_rate, div;
 	unsigned int tx_len = 0;
 	int ret = 0;
 	u32 reg;

@@ -228,7 +249,6 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	if (tfr->len > SUN4I_FIFO_DEPTH)
 		return -EMSGSIZE;
 
-	reinit_completion(&sspi->done);
 	sspi->tx_buf = tfr->tx_buf;
 	sspi->rx_buf = tfr->rx_buf;
 	sspi->len = tfr->len;

@@ -329,22 +349,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
 
-	tx_time = max(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;
-	}
-
+	ret = sun4i_spi_wait_for_transfer(spi, tfr);
 
-out:
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
 
 	return ret;

@@ -352,14 +358,18 @@ out:
 
 static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
 {
-	struct sun4i_spi *sspi = dev_id;
-	u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
+	struct spi_master *master = dev_id;
+	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+	u32 status;
+
+	status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
 
 	/* Transfer complete */
 	if (status & SUN4I_INT_CTL_TC) {
-		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
+		sun4i_spi_write(sspi, SUN4I_INT_STA_REG,
+				SUN4I_INT_CTL_TC);
 		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
-		complete(&sspi->done);
+		spi_finalize_current_transfer(master);
 		return IRQ_HANDLED;
 	}
 

@@ -456,7 +466,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun4i_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;

@@ -476,8 +486,6 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
-	init_completion(&sspi->done);
-
 	/*
 	 * This wake-up/shutdown pattern is to be able to have the
 	 * device woken up, even if runtime_pm is disabled
Signature · 1 line
2.16.2
#14 lore raw

Re: [PATCH v2 4/6] spi: sun4i: use completion provided by SPI core driver

Quoted 3 lines · Sergey Suloev

As long as the completion 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 should also be split in two parts.

Maxime

Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/d615ec4b/attachment.sig>
author
#15 lore raw

PATCH v2 5/6 spi: sun4i: introduce register set/unset helpers

In reply to Sergey Suloev

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

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)
drivers/spi/spi-sun4i.c +19 -21
index 9d1bc20..d81d31c 100644
@@ -107,29 +107,29 @@ static inline void sun4i_spi_write(struct sun4i_spi *sspi, u32 reg, u32 value)
 	writel(value, sspi->base_addr + reg);
 }
 
-static inline u32 sun4i_spi_get_tx_fifo_count(struct sun4i_spi *sspi)
+static inline void sun4i_spi_set(struct sun4i_spi *sspi, u32 addr, u32 val)
 {
-	u32 reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
-
-	reg >>= SUN4I_FIFO_STA_TF_CNT_BITS;
+	u32 reg = sun4i_spi_read(sspi, addr);
 
-	return reg & SUN4I_FIFO_STA_TF_CNT_MASK;
+	reg |= val;
+	sun4i_spi_write(sspi, addr, reg);
 }
 
-static inline void sun4i_spi_enable_interrupt(struct sun4i_spi *sspi, u32 mask)
+static inline void sun4i_spi_unset(struct sun4i_spi *sspi, u32 addr, u32 val)
 {
-	u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
+	u32 reg = sun4i_spi_read(sspi, addr);
 
-	reg |= mask;
-	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
+	reg &= ~val;
+	sun4i_spi_write(sspi, addr, reg);
 }
 
-static inline void sun4i_spi_disable_interrupt(struct sun4i_spi *sspi, u32 mask)
+static inline u32 sun4i_spi_get_tx_fifo_count(struct sun4i_spi *sspi)
 {
-	u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
+	u32 reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
 
-	reg &= ~mask;
-	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
+	reg >>= SUN4I_FIFO_STA_TF_CNT_BITS;
+
+	return reg & SUN4I_FIFO_STA_TF_CNT_MASK;
 }
 
 static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)

@@ -256,13 +256,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	/* Clear pending interrupts */
 	sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);
 
+	/* Reset FIFOs */
+	sun4i_spi_set(sspi, SUN4I_CTL_REG,
+		      SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
 
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 
-	/* Reset FIFOs */
-	sun4i_spi_write(sspi, SUN4I_CTL_REG,
-			reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
-
 	/*
 	 * Setup the transfer control register: Chip Select,
 	 * polarities, etc.

@@ -342,12 +341,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
 
 	/* Enable the interrupts */
-	sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
-					 SUN4I_INT_CTL_RF_F34);
+	sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
+					       SUN4I_INT_CTL_RF_F34);
 
 	/* Start the transfer */
-	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
-	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
+	sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
 
 	ret = sun4i_spi_wait_for_transfer(spi, tfr);
Signature · 1 line
2.16.2
#16 lore raw

Re: [PATCH v2 5/6] spi: sun4i: introduce register set/unset helpers

Quoted 2 lines · Sergey Suloev

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

Again, the diffstat is pretty neutral, so what is the benefit?

Maxime

Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180404/f7abb560/attachment.sig>
author
#17 lore raw

PATCH v2 6/6 spi: sun4i: add DMA transfers support

In reply to Sergey Suloev

DMA transfers are now available for sun4i-family SoCs. The DMA mode is used automatically as soon as requested transfer length is more than FIFO length.

Changes in v2:
1) Debug log enhancements.

Signed-off-by Sergey Suloev <[email protected]>
---
 drivers/spi/spi-sun4i.c | 299 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 277 insertions(+), 22 deletions(-)
drivers/spi/spi-sun4i.c +277 -22
index d81d31c..dda7922 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>

@@ -39,6 +41,7 @@
 #define SUN4I_CTL_CPHA				BIT(2)
 #define SUN4I_CTL_CPOL				BIT(3)
 #define SUN4I_CTL_CS_ACTIVE_LOW			BIT(4)
+#define SUN4I_CTL_DMA_DEDICATED			BIT(5)
 #define SUN4I_CTL_LMTF				BIT(6)
 #define SUN4I_CTL_TF_RST			BIT(8)
 #define SUN4I_CTL_RF_RST			BIT(9)

@@ -58,6 +61,8 @@
 #define SUN4I_INT_STA_REG		0x10
 
 #define SUN4I_DMA_CTL_REG		0x14
+#define SUN4I_CTL_DMA_RF_READY			BIT(0)
+#define SUN4I_CTL_DMA_TF_NOT_FULL		BIT(10)
 
 #define SUN4I_WAIT_REG			0x18
 

@@ -169,6 +174,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
 	}
 }
 
+static bool sun4i_spi_can_dma(struct spi_master *master,
+			      struct spi_device *spi,
+			      struct spi_transfer *tfr)
+{
+	return tfr->len > SUN4I_FIFO_DEPTH;
+}
+
 static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(spi->master);

@@ -208,6 +220,11 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
 
 static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
 {
+	struct spi_master *master = spi->master;
+
+	if (master->can_dma)
+		return SUN4I_MAX_XFER_SIZE;
+
 	return SUN4I_FIFO_DEPTH;
 }
 

@@ -235,6 +252,164 @@ static int sun4i_spi_wait_for_transfer(struct spi_device *spi,
 	return 0;
 }
 
+static void sun4i_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 sun4i_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 = sun4i_spi_dma_callback;
+	chan_desc->callback_param = master;
+
+	*cookie = dmaengine_submit(chan_desc);
+	dma_async_issue_pending(master->dma_tx);
+
+	return 0;
+}
+
+static int sun4i_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 = sun4i_spi_dma_callback;
+	chan_desc->callback_param = master;
+
+	*cookie = dmaengine_submit(chan_desc);
+	dma_async_issue_pending(master->dma_rx);
+
+	return 0;
+}
+
+static int sun4i_spi_transfer_one_dma(struct spi_device *spi,
+				      struct spi_transfer *tfr)
+{
+	struct spi_master *master = spi->master;
+	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+	dma_cookie_t tx_cookie = 0, rx_cookie = 0;
+	enum dma_status status;
+	int ret;
+	u32 reg = 0;
+
+	dev_dbg(&master->dev, "Using DMA mode for transfer\n");
+
+	/* Disable interrupts */
+	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+
+	if (sspi->tx_buf) {
+		ret = sun4i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
+		if (ret)
+			goto out;
+
+		reg |= SUN4I_CTL_DMA_TF_NOT_FULL;
+	}
+
+	if (sspi->rx_buf) {
+		ret = sun4i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
+		if (ret)
+			goto out;
+
+		reg |= SUN4I_CTL_DMA_RF_READY;
+	}
+
+	sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, reg);
+
+	/* Dedicated DMA requests */
+	sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_DMA_DEDICATED);
+
+	/* Start transfer */
+	sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
+
+	/* Wait for completion */
+	ret = sun4i_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);
+	}
+
+	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+
+	return ret;
+}
+
+static int sun4i_spi_transfer_one_pio(struct spi_device *spi,
+				      struct spi_transfer *tfr)
+{
+	struct spi_master *master = spi->master;
+	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+	int ret;
+
+	/* Explicit disable DMA requests */
+	sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+	sun4i_spi_unset(sspi, SUN4I_CTL_REG, SUN4I_CTL_DMA_DEDICATED);
+
+	/* Fill the TX FIFO */
+	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+
+	/* Enable the interrupts */
+	sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
+					       SUN4I_INT_CTL_RF_F34);
+
+	/* Start transfer */
+	sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
+
+	ret = sun4i_spi_wait_for_transfer(spi, tfr);
+
+	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+
+	return ret;
+}
+
 static int sun4i_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *tfr)

@@ -242,13 +417,22 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	struct sun4i_spi *sspi = spi_master_get_devdata(master);
 	unsigned int mclk_rate, div;
 	unsigned int tx_len = 0;
-	int ret = 0;
 	u32 reg;
 
-	/* We don't support transfers larger than FIFO depth */
-	if (tfr->len > SUN4I_FIFO_DEPTH)
+	/* A zero length transfer never finishes if programmed
+	   in the hardware */
+	if (!tfr->len)
+		return 0;
+
+	if (tfr->len > SUN4I_MAX_XFER_SIZE)
 		return -EMSGSIZE;
 
+	if (!master->can_dma) {
+		/* Don't support transfer larger than the FIFO */
+		if (tfr->len > SUN4I_FIFO_DEPTH)
+			return -EMSGSIZE;
+	}
+
 	sspi->tx_buf = tfr->tx_buf;
 	sspi->rx_buf = tfr->rx_buf;
 	sspi->len = tfr->len;

@@ -335,23 +519,10 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
 	sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
 
-	/*
-	 * Fill the TX FIFO
-	 */
-	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
-
-	/* Enable the interrupts */
-	sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
-					       SUN4I_INT_CTL_RF_F34);
-
-	/* Start the transfer */
-	sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
-
-	ret = sun4i_spi_wait_for_transfer(spi, tfr);
-
-	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+	if (sun4i_spi_can_dma(master, spi, tfr))
+		return sun4i_spi_transfer_one_dma(spi, tfr);
 
-	return ret;
+	return sun4i_spi_transfer_one_pio(spi, tfr);
 }
 
 static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)

@@ -364,8 +535,7 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
 
 	/* Transfer complete */
 	if (status & SUN4I_INT_CTL_TC) {
-		sun4i_spi_write(sspi, SUN4I_INT_STA_REG,
-				SUN4I_INT_CTL_TC);
+		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
 		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
 		spi_finalize_current_transfer(master);
 		return IRQ_HANDLED;

@@ -422,6 +592,76 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
 	return 0;
 }
 
+static int sun4i_spi_dma_setup(struct device *dev,
+			       struct resource *res)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	struct dma_slave_config dma_sconf;
+	int ret;
+
+	master->dma_tx = dma_request_slave_channel_reason(dev, "tx");
+	if (IS_ERR(master->dma_tx)) {
+		dev_err(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 + SUN4I_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(dev, "Unable to configure DMA TX slave\n");
+		goto err_rel_tx;
+	}
+
+	master->dma_rx = dma_request_slave_channel_reason(dev, "rx");
+	if (IS_ERR(master->dma_rx)) {
+		dev_err(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 + SUN4I_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(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 = sun4i_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 sun4i_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 sun4i_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;

@@ -484,6 +724,16 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
+	ret = sun4i_spi_dma_setup(&pdev->dev, 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

@@ -500,7 +750,7 @@ static int sun4i_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;
 	}
 

@@ -510,14 +760,19 @@ err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	sun4i_spi_runtime_suspend(&pdev->dev);
 err_free_master:
+	sun4i_spi_dma_release(master);
 	spi_master_put(master);
 	return ret;
 }
 
 static int sun4i_spi_remove(struct platform_device *pdev)
 {
+	struct spi_master *master = platform_get_drvdata(pdev);
+
 	pm_runtime_force_suspend(&pdev->dev);
 
+	sun4i_spi_dma_release(master);
+
 	return 0;
 }
Signature · 1 line
2.16.2