spi/sunxi patches

From: Emilio L?pez <[email protected]>

The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over 1MHz SPI bus takes way longer than that. Calculate the timeout from the actual time the transfer is supposed to take and multiply by 2 for good measure.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/spi/spi-sun4i.c | 10 +++++++++-
 drivers/spi/spi-sun6i.c | 10 +++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

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

index fbb0a4d..48532ec 100644
@@ -170,6 +170,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(master);
 	unsigned int mclk_rate, div, timeout;
+	unsigned int start, end, tx_time;
 	unsigned int tx_len = 0;
 	int ret = 0;
 	u32 reg;

@@ -279,9 +280,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
 
+	tx_time = max_t(int, tfr->len * 8 * 2 / (speed / 1000), 100);

When testing SPI without DMA I noticed that filling the FIFO on the spi controller causes timeout.

Always leave room for one byte in the FIFO.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/spi/spi-sun4i.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

index 48532ec..707f61b 100644

drivers/spi/spi-sun4i.c +1 -0

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

Signature · 1 line

2.1.4

When the maximum transfer speed is not set for a SPI slave the value remains 0 and the code in sunxi SPI divides by it. Use an arbitrary speed instead in that case.

Signed-off-by Michal Suchanek <[email protected]>

From: Emilio L?pez <[email protected]>

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

Signed-off-by Emilio L?pez <[email protected]>

Tested-by Michal Suchanek <[email protected]>

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

drivers/spi/spi-sun4i.c +41 -12

index 4dda366..63242a7 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>

@@ -34,6 +36,7 @@
 #define SUN4I_CTL_CPHA				BIT(2)
 #define SUN4I_CTL_CPOL				BIT(3)
 #define SUN4I_CTL_CS_ACTIVE_LOW			BIT(4)
+#define SUN4I_CTL_DMAMC_DEDICATED		BIT(5)
 #define SUN4I_CTL_LMTF				BIT(6)
 #define SUN4I_CTL_TF_RST			BIT(8)
 #define SUN4I_CTL_RF_RST			BIT(9)

@@ -51,6 +54,8 @@
 #define SUN4I_INT_STA_REG		0x10
 
 #define SUN4I_DMA_CTL_REG		0x14
+#define SUN4I_DMA_CTL_RF_READY			BIT(0)
+#define SUN4I_DMA_CTL_TF_NOT_FULL		BIT(10)
 
 #define SUN4I_WAIT_REG			0x18
 

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

@@ -169,17 +181,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 				  struct spi_transfer *tfr)
 {
 	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
 	unsigned int speed, mclk_rate, div, timeout;
 	unsigned int start, end, tx_time;
 	unsigned int tx_len = 0;
+	u32 reg, trigger = 0;
 	int ret = 0;
-	u32 reg;
-
-	/* We don't support transfer larger than the FIFO */
-	if (tfr->len > SUN4I_FIFO_DEPTH)
-		return -EINVAL;
-	if (tfr->tx_buf && tfr->len => SUN4I_FIFO_DEPTH)
-		return -EINVAL;
 
 	reinit_completion(&sspi->done);
 	sspi->tx_buf = tfr->tx_buf;

@@ -277,14 +284,67 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
 	sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
 
-	/* Fill the TX FIFO */
-	/* Filling the fifo fully causes timeout for some reason
-	 * at least on spi2 on a10s */
-	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
-
 	/* Enable the interrupts */
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
 
+	if (sun4i_spi_can_dma(master, spi, tfr)) {
+		dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+		if (sspi->tx_buf) {
+			desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+					tfr->tx_sg.sgl, tfr->tx_sg.nents,
+					DMA_TO_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_tx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			trigger |= SUN4I_DMA_CTL_TF_NOT_FULL;
+
+			dmaengine_submit(desc_tx);
+			dma_async_issue_pending(master->dma_tx);
+
+		}
+
+		if (sspi->rx_buf) {
+			desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+					tfr->rx_sg.sgl, tfr->rx_sg.nents,
+					DMA_FROM_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_rx) {

drivers/spi/spi-sun4i.c +20 -0

+		/* Enable Dedicated DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		reg |= SUN4I_CTL_DMAMC_DEDICATED;
+		sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger);
+	} else {
+		dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+		/* Disable DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		sun4i_spi_write(sspi, SUN4I_CTL_REG,
+				reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+		/* Fill the TX FIFO */
+		/* Filling the fifo fully causes timeout for some reason
+		 * at least on spi2 on a10s */
+		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+	}
+
 	/* Start the transfer */
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);

drivers/spi/spi-sun4i.c +29 -1

+	} else {
+		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
 
 out:
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);

@@ -368,6 +433,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
 
 static int sun4i_spi_probe(struct platform_device *pdev)
 {
+	struct dma_slave_config dma_sconfig;
 	struct spi_master *master;
 	struct sun4i_spi *sspi;
 	struct resource	*res;

@@ -403,7 +469,9 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
+	init_completion(&sspi->done);
 	sspi->master = master;
+	master->can_dma = sun4i_spi_can_dma;
 	master->set_cs = sun4i_spi_set_cs;
 	master->transfer_one = sun4i_spi_transfer_one;
 	master->num_chipselect = 4;

@@ -426,7 +494,45 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
-	init_completion(&sspi->done);
+	master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+	if (IS_ERR(master->dma_tx)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+		ret = PTR_ERR(master->dma_tx);
+		goto err_free_master;
+	}
+
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+		goto err_tx_dma_release;
+	}
+
+	master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+	if (IS_ERR(master->dma_rx)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");

Only SPI0 is enabled. The schematic denotes it as the only SPI bus. Other SPI pins are reserved for different peripherals.

Signed-off-by Michal Suchanek <[email protected]>

---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 7 +++++++
 1 file changed, 7 insertions(+)

arch/arm/boot/dts/sun7i-a20-cubieboard2.dts +0 -0

index 39a51d5..9861304 100644

According to datasheet some pins are available on A10s only while others are shared with A13.

Signed-off-by Michal Suchanek <[email protected]>

---
This time add all spi pins and make the CS pins separate as is seen with current sun4i DTs

---
 arch/arm/boot/dts/sun5i-a10s.dtsi | 21 +++++++++++++++++
 arch/arm/boot/dts/sun5i.dtsi      | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

arch/arm/boot/dts/sun5i-a10s.dtsi +11 -0

index f11efb7..d9610fa 100644
@@ -201,6 +201,27 @@
 		allwinner,drive = <SUN4I_PINCTRL_30_MA>;
 		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 	};
+
+	spi1_cs1_pins_a: spi1_cs1 at 0 {
+		allwinner,pins = "PG13";
+		allwinner,function = "spi1";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+	};
+
+	spi2_pins_a: spi2 at 0 {
+		allwinner,pins = "PB12", "PB13", "PB14";
+		allwinner,function = "spi1";

arch/arm/boot/dts/sun5i-a10s.dtsi +0 -0

 };
 
 &sram_a {

arch/arm/boot/dts/sun5i.dtsi +39 -0

index 54b0978..7d32d49 100644
@@ -516,6 +516,55 @@
 				allwinner,drive = <SUN4I_PINCTRL_30_MA>;
 				allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
 			};
+
+			spi0_pins_a: spi0 at 0 {
+				allwinner,pins = "PC00", "PC01", "PC02";
+				allwinner,function = "spi0";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
+			spi0_cs0_pins_a: spi0_cs0 at 0 {
+				allwinner,pins = "PC03";
+				allwinner,function = "spi0";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
+			spi1_pins_a: spi1 at 0 {
+				allwinner,pins = "PG10", "PG11", "PG12";
+				allwinner,function = "spi1";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
+			spi1_cs0_pins_a: spi1_cs0 at 0 {
+				allwinner,pins = "PG09";
+				allwinner,function = "spi1";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
+			spi2_pins_b: spi2 at 1 {
+				allwinner,pins = "PE01", "PE02", "PE03";
+				allwinner,function = "spi2";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
+			spi2_cs0_pins_b: spi2_cs1 at 1 {
+				allwinner,pins = "PE00";
+				allwinner,function = "spi2";

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

When the maximum transfer speed is not set for a SPI slave the value remains 0 and the code in sunxi SPI divides by it. Use an arbitrary speed instead in that case.

Signed-off-by Emilio L?pez <[email protected]>

Tested-by Michal Suchanek <[email protected]>

Quoted 2 lines · Michal Suchanek

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

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

Quoted 5 lines · Michal Suchanek

When the maximum transfer speed is not set for a SPI slave the value remains 0 and the code in sunxi SPI divides by it. Use an arbitrary speed instead in that case.

Signed-off-by Michal Suchanek <[email protected]>

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

Quoted 6 lines · Mark Brown

Quoted 2 lines · Michal Suchanek

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

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

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

Quoted 5 lines · Maxime Ripard

Quoted 4 lines · Mark Brown

Quoted 2 lines · Michal Suchanek

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

Quoted 4 lines

Quoted 1 line

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

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

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

Quoted 7 lines · Maxime Ripard

Quoted 5 lines · Michal Suchanek

When the maximum transfer speed is not set for a SPI slave the value remains 0 and the code in sunxi SPI divides by it. Use an arbitrary speed instead in that case.

Signed-off-by Michal Suchanek <[email protected]>

Quoted 3 lines

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

Spidev I guess.

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

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

Quoted 27 lines

drivers/spi/spi-sun4i.c +20 -0

+             /* Enable Dedicated DMA requests */
+             reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+             reg |= SUN4I_CTL_DMAMC_DEDICATED;
+             sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
+             sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger);
+     } else {
+             dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+             /* Disable DMA requests */
+             reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+             sun4i_spi_write(sspi, SUN4I_CTL_REG,
+                             reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+             sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+             /* Fill the TX FIFO */
+             /* Filling the fifo fully causes timeout for some reason
+              * at least on spi2 on a10s */
+             sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+     }
+
      /* Start the transfer */
      reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
      sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);

@@ -303,7 +363,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
              goto out;
      }
 

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

Quoted 4 lines

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

-     sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+     if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+             /* The receive transfer should be the last one to finish */
+             dma_wait_for_async_tx(desc_rx);

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

Quoted 28 lines

drivers/spi/spi-sun4i.c +27 -1

-     init_completion(&sspi->done);
+     master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+     if (IS_ERR(master->dma_tx)) {
+             dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+             ret = PTR_ERR(master->dma_tx);
+             goto err_free_master;
+     }
+
+     dma_sconfig.direction = DMA_MEM_TO_DEV;
+     dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+     dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+     dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+     dma_sconfig.src_maxburst = 1;
+     dma_sconfig.dst_maxburst = 1;
+
+     ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig);
+     if (ret) {
+             dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+             goto err_tx_dma_release;
+     }
+
+     master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+     if (IS_ERR(master->dma_rx)) {
+             dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+             ret = PTR_ERR(master->dma_rx);
+             goto err_tx_dma_release;
+     }
+

drivers/spi/spi-sun4i.c static int sun4i_spi_probe(struct platform_device *pdev) · line 529

Quoted 6 lines

drivers/spi/spi-sun4i.c +6 -0

+     dma_sconfig.direction = DMA_DEV_TO_MEM;
+     dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+     dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+     dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+     dma_sconfig.src_maxburst = 1;
+     dma_sconfig.dst_maxburst = 1;

We can't use a higher bust size?

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

Quoted 4 lines · Emilio L?pez

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

-     sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+     if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+             /* The receive transfer should be the last one to finish */
+             dma_wait_for_async_tx(desc_rx);

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

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

Quoted 3 lines

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

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

...which manually reads data from the FIFO which doesn't seem like what

we want, won't it conflict with the DMA?