[PATCH 00/18] spi/xilinx: Speed-up

[PATCH 00/18] spi/xilinx: Speed-up

[Ricardo Ribalda Delgado]

This set of patches has been developed after using the core to access a spi
nor flash. The initial performance was not as desired. The following changes
have been done.

-This series of patches add driver support for hardware implemented modes:
LSB_FIRS, LOOP and CS_HIGH.

-There is support for irq less operations.

-The buffer size of the core is detected at probe time, speeding up the
operations.

-Code has been simplify and cleaned

This are the results, before and after the patchet

Original
root@qt5022:~# dd if=/dev/mtd0 of=/dev/null
16384+0 records in
16384+0 records out
8388608 bytes (8.4 MB) copied, 117.985 s, 71.1 kB/s

 37:        107      82853  Xilinx INTC Level   1  b0010000.spi-flash

After patchset (irq):
root@qt5022:~# dd if=/dev/mtd0 of=/dev/null
16384+0 records in
16384+0 records out
8388608 bytes (8.4 MB) copied, 49.009 s, 171 kB/s

After patchset (irq):
root@qt5022:~# dd if=/dev/mtd0 of=/dev/null
16384+0 records in
16384+0 records out
8388608 bytes (8.4 MB) copied, 41.2526 s, 203 kB/s

IRQS: ZERO

This set of patches supersedes the previous patchet with only LSB_FIRST and LOOP

Ricardo Ribalda Delgado (18):
  spi/xilinx: Support for spi mode LSB_FIRST
  spi/xilinx: Support for spi mode LOOP
  spi/xilinx: Simplify data read from the Rx FIFO
  spi-xilinx: Simplify spi_fill_tx_fifo
  spi/xilinx: Leave the IRQ always enabled.
  spi/xilinx: Code cleanup
  spi/xilinx: Use cached value of register
  spi/xilinx: Support cores with no interrupt
  spi/xilinx: Do not inhibit transmission in polling mode
  spi/xilinx: Support for spi mode CS_HIGH
  spi/xilinx: Remove rx_fn and tx_fn pointer
  spi/xilinx: Make spi_tx and spi_rx simmetric
  spi/xilinx: Convert remainding_bytes in remaining words
  spi/xilinx: Convert bits_per_word in bytes_per_word
  spi/xilinx: Remove iowrite/ioread wrappers
  spi/xilinx: Reset core before buffer_size detection
  spi/xilinx: Remove remaining_words driver data variable
  spi/xilinx: Check number of slaves range

 drivers/spi/spi-xilinx.c | 306 ++++++++++++++++++++++-------------------------
 1 file changed, 146 insertions(+), 160 deletions(-)

-- 
2.1.4

[PATCH 01/18] spi/xilinx: Support for spi mode LSB_FIRST

[Ricardo Ribalda Delgado]

Hardware supports LSB_FIRST mode. Support it also in the driver.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 79bd84f..d4edeee 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -34,7 +34,8 @@
 #define XSPI_CR_MASTER_MODE	0x04
 #define XSPI_CR_CPOL		0x08
 #define XSPI_CR_CPHA		0x10
-#define XSPI_CR_MODE_MASK	(XSPI_CR_CPHA | XSPI_CR_CPOL)
+#define XSPI_CR_MODE_MASK	(XSPI_CR_CPHA | XSPI_CR_CPOL | \
+				 XSPI_CR_LSB_FIRST)
 #define XSPI_CR_TXFIFO_RESET	0x20
 #define XSPI_CR_RXFIFO_RESET	0x40
 #define XSPI_CR_MANUAL_SSELECT	0x80
@@ -194,6 +195,8 @@ static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
 			cr |= XSPI_CR_CPHA;
 		if (spi->mode & SPI_CPOL)
 			cr |= XSPI_CR_CPOL;
+		if (spi->mode & SPI_LSB_FIRST)
+			cr |= XSPI_CR_LSB_FIRST;
 		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 
 		/* We do not check spi->max_speed_hz here as the SPI clock
@@ -353,7 +356,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/* the spi->mode bits understood by this driver: */
-	master->mode_bits = SPI_CPOL | SPI_CPHA;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
 
 	xspi = spi_master_get_devdata(master);
 	xspi->bitbang.master = master;
-- 
2.1.4

Re: [PATCH 01/18] spi/xilinx: Support for spi mode LSB_FIRST

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:33PM +0100, Ricardo Ribalda Delgado wrote:
> Hardware supports LSB_FIRST mode. Support it also in the driver.

Applied, thanks.

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

[PATCH 02/18] spi/xilinx: Support for spi mode LOOP

[Ricardo Ribalda Delgado]

Hardware supports LOOP mode. Support it also in the driver.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index d4edeee..e1d9a20 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -35,7 +35,7 @@
 #define XSPI_CR_CPOL		0x08
 #define XSPI_CR_CPHA		0x10
 #define XSPI_CR_MODE_MASK	(XSPI_CR_CPHA | XSPI_CR_CPOL | \
-				 XSPI_CR_LSB_FIRST)
+				 XSPI_CR_LSB_FIRST | XSPI_CR_LOOP)
 #define XSPI_CR_TXFIFO_RESET	0x20
 #define XSPI_CR_RXFIFO_RESET	0x40
 #define XSPI_CR_MANUAL_SSELECT	0x80
@@ -197,6 +197,8 @@ static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
 			cr |= XSPI_CR_CPOL;
 		if (spi->mode & SPI_LSB_FIRST)
 			cr |= XSPI_CR_LSB_FIRST;
+		if (spi->mode & SPI_LOOP)
+			cr |= XSPI_CR_LOOP;
 		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 
 		/* We do not check spi->max_speed_hz here as the SPI clock
@@ -356,7 +358,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/* the spi->mode bits understood by this driver: */
-	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_LOOP;
 
 	xspi = spi_master_get_devdata(master);
 	xspi->bitbang.master = master;
-- 
2.1.4

Re: [PATCH 02/18] spi/xilinx: Support for spi mode LOOP

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:34PM +0100, Ricardo Ribalda Delgado wrote:
> Hardware supports LOOP mode. Support it also in the driver.

Applied, thanks.

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

[PATCH 03/18] spi/xilinx: Simplify data read from the Rx FIFO

[Ricardo Ribalda Delgado]

The number of words in the read buffer will be exactly the same as the
number of words written on write buffer, once the transaction has
finished.

Instead of cheking the rx_empty flags for every word simply save the
number of words written by fill_tx_fifo.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index e1d9a20..416b227 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -221,9 +221,10 @@ static int xilinx_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi)
+static int xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi)
 {
 	u8 sr;
+	int n_words = 0;
 
 	/* Fill the Tx FIFO with as many bytes as possible */
 	sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
@@ -234,7 +235,10 @@ static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi)
 			xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
 		xspi->remaining_bytes -= xspi->bits_per_word / 8;
 		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
+		n_words++;
 	}
+
+	return n_words;
 }
 
 static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
@@ -259,9 +263,9 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 	for (;;) {
 		u16 cr;
-		u8 sr;
+		int n_words;
 
-		xilinx_spi_fill_tx_fifo(xspi);
+		n_words = xilinx_spi_fill_tx_fifo(xspi);
 
 		/* Start the transfer by not inhibiting the transmitter any
 		 * longer
@@ -282,11 +286,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 			       xspi->regs + XSPI_CR_OFFSET);
 
 		/* Read out all the data from the Rx FIFO */
-		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+		while (n_words--)
 			xspi->rx_fn(xspi);
-			sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-		}
 
 		/* See if there is more data to send */
 		if (xspi->remaining_bytes <= 0)
-- 
2.1.4

Re: [PATCH 03/18] spi/xilinx: Simplify data read from the Rx FIFO

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:35PM +0100, Ricardo Ribalda Delgado wrote:
> The number of words in the read buffer will be exactly the same as the
> number of words written on write buffer, once the transaction has
> finished.

Applied, thanks.

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

[PATCH 04/18] spi-xilinx: Simplify spi_fill_tx_fifo

[Ricardo Ribalda Delgado]

Instead of checking the TX_FULL flag for every transaction, find out the
size of the buffer at probe time and use it.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 416b227..17480a2 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -88,6 +88,7 @@ struct xilinx_spi {
 	const u8 *tx_ptr;	/* pointer in the Rx buffer */
 	int remaining_bytes;	/* the number of bytes left to transfer */
 	u8 bits_per_word;
+	int buffer_size;	/* buffer size in words */
 	unsigned int (*read_fn)(void __iomem *);
 	void (*write_fn)(u32, void __iomem *);
 	void (*tx_fn)(struct xilinx_spi *);
@@ -221,24 +222,16 @@ static int xilinx_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static int xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi)
+static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
 {
-	u8 sr;
-	int n_words = 0;
+	xspi->remaining_bytes -= n_words * xspi->bits_per_word / 8;
 
-	/* Fill the Tx FIFO with as many bytes as possible */
-	sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-	while ((sr & XSPI_SR_TX_FULL_MASK) == 0 && xspi->remaining_bytes > 0) {
+	while (n_words--)
 		if (xspi->tx_ptr)
 			xspi->tx_fn(xspi);
 		else
 			xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
-		xspi->remaining_bytes -= xspi->bits_per_word / 8;
-		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-		n_words++;
-	}
-
-	return n_words;
+	return;
 }
 
 static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
@@ -265,7 +258,10 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		u16 cr;
 		int n_words;
 
-		n_words = xilinx_spi_fill_tx_fifo(xspi);
+		n_words = (xspi->remaining_bytes * 8) / xspi->bits_per_word;
+		n_words = min(n_words, xspi->buffer_size);
+
+		xilinx_spi_fill_tx_fifo(xspi, n_words);
 
 		/* Start the transfer by not inhibiting the transmitter any
 		 * longer
@@ -322,6 +318,21 @@ static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int xilinx_spi_find_buffer_size(struct xilinx_spi *xspi)
+{
+	u8 sr;
+	int n_words = 0;
+
+	/* Fill the Tx FIFO with as many words as possible */
+	do {
+		xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
+		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
+		n_words++;
+	} while (!(sr & XSPI_SR_TX_FULL_MASK));
+
+	return n_words;
+}
+
 static const struct of_device_id xilinx_spi_of_match[] = {
 	{ .compatible = "xlnx,xps-spi-2.00.a", },
 	{ .compatible = "xlnx,xps-spi-2.00.b", },
@@ -413,6 +424,8 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		goto put_master;
 	}
 
+	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
+
 	/* SPI controller initializations */
 	xspi_init_hw(xspi);
 
-- 
2.1.4

Re: [PATCH 04/18] spi-xilinx: Simplify spi_fill_tx_fifo

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:36PM +0100, Ricardo Ribalda Delgado wrote:
> Instead of checking the TX_FULL flag for every transaction, find out the
> size of the buffer at probe time and use it.

So, I see what's going on here and the potential performance benefit
from avoiding the MMIO access.  However I can't help but worry that this
makes things more fragile - the current code will transparently handle
any races or anything which result in a misfilling of the FIFO for some
reason either now or in the future as performance is improved and the
driver gets more fancy.

As things stand if I look at the code it's fairly clear it should be
safe and unfortunately we only have an underrun interrupt which will be
triggered normally (no overflow interrupt) so we can't really use that
to add a bit of robustness.  I'm tempted to suggest checking that we did
trigger the FIFO full flag when we fill the FIFO but that feels like
overengineering.

Let me think about this, I'll probably apply it but if you can think
about ways of making this more robust that'd be good.

> @@ -413,6 +424,8 @@ static int xilinx_spi_probe(struct platform_device *pdev)
>  		goto put_master;
>  	}
>  
> +	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
> +
>  	/* SPI controller initializations */
>  	xspi_init_hw(xspi);

It seems safer to reset the hardware, probe the FIFO size and then reset
the hardware again in case something decided to leave some data in the
FIFO (perhaps some bootloader wasn't as well programmed as one might
desire for example).  Not cricial now but it could again become
important with future optimizations.

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

[PATCH 05/18] spi/xilinx: Leave the IRQ always enabled.

[Ricardo Ribalda Delgado]

Instead of enabling the IRQ and disabling it for every transaction.

Specially the small transactions (1,2 words) benefit from removing 3 bus
accesses.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 17480a2..408a842 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -167,8 +167,11 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
 	/* Reset the SPI device */
 	xspi->write_fn(XIPIF_V123B_RESET_MASK,
 		regs_base + XIPIF_V123B_RESETR_OFFSET);
-	/* Disable all the interrupts just in case */
-	xspi->write_fn(0, regs_base + XIPIF_V123B_IIER_OFFSET);
+	/* Enable the transmit empty interrupt, which we use to determine
+	 * progress on the transmission.
+	 */
+	xspi->write_fn(XSPI_INTR_TX_EMPTY,
+			regs_base + XIPIF_V123B_IIER_OFFSET);
 	/* Enable the global IPIF interrupt */
 	xspi->write_fn(XIPIF_V123B_GINTR_ENABLE,
 		regs_base + XIPIF_V123B_DGIER_OFFSET);
@@ -237,7 +240,6 @@ static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
 static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 {
 	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
-	u32 ipif_ier;
 
 	/* We get here with transmitter inhibited */
 
@@ -246,14 +248,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	xspi->remaining_bytes = t->len;
 	reinit_completion(&xspi->done);
 
-
-	/* Enable the transmit empty interrupt, which we use to determine
-	 * progress on the transmission.
-	 */
-	ipif_ier = xspi->read_fn(xspi->regs + XIPIF_V123B_IIER_OFFSET);
-	xspi->write_fn(ipif_ier | XSPI_INTR_TX_EMPTY,
-		xspi->regs + XIPIF_V123B_IIER_OFFSET);
-
 	for (;;) {
 		u16 cr;
 		int n_words;
@@ -290,9 +284,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 			break;
 	}
 
-	/* Disable the transmit empty interrupt */
-	xspi->write_fn(ipif_ier, xspi->regs + XIPIF_V123B_IIER_OFFSET);
-
 	return t->len - xspi->remaining_bytes;
 }
 
-- 
2.1.4

[PATCH 06/18] spi/xilinx: Code cleanup

[Ricardo Ribalda Delgado]

Trivial fix

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 408a842..978fbdd 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -248,7 +248,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	xspi->remaining_bytes = t->len;
 	reinit_completion(&xspi->done);
 
-	for (;;) {
+	while (xspi->remaining_bytes) {
 		u16 cr;
 		int n_words;
 
@@ -278,10 +278,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		/* Read out all the data from the Rx FIFO */
 		while (n_words--)
 			xspi->rx_fn(xspi);
-
-		/* See if there is more data to send */
-		if (xspi->remaining_bytes <= 0)
-			break;
 	}
 
 	return t->len - xspi->remaining_bytes;
-- 
2.1.4

Re: [PATCH 06/18] spi/xilinx: Code cleanup

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:38PM +0100, Ricardo Ribalda Delgado wrote:
> Trivial fix

But it's best to say what it is in the changelog.

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

[PATCH 07/18] spi/xilinx: Use cached value of register

[Ricardo Ribalda Delgado]

The control register has not changed since the previous access.
Therefore we can use the cached value and safe one bus access.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 978fbdd..3f8450d 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -271,7 +271,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		 * transmitter while the Isr refills the transmit register/FIFO,
 		 * or make sure it is stopped if we're done.
 		 */
-		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
 		xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
 			       xspi->regs + XSPI_CR_OFFSET);
 
-- 
2.1.4

[PATCH 08/18] spi/xilinx: Support cores with no interrupt

[Ricardo Ribalda Delgado]

The core can run in polling mode. In fact, the performance of the core
is similar (or even better), due to the fact most of the spi
transactions are just a couple of bytes and there is one irq per
transactions.

When an mtd device is connected via spi, reading 8MB of data produces
more than 80K interrupts (with irq disabling, context swith....)

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 3f8450d..8a28cab 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -173,8 +173,11 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
 	xspi->write_fn(XSPI_INTR_TX_EMPTY,
 			regs_base + XIPIF_V123B_IIER_OFFSET);
 	/* Enable the global IPIF interrupt */
-	xspi->write_fn(XIPIF_V123B_GINTR_ENABLE,
-		regs_base + XIPIF_V123B_DGIER_OFFSET);
+	if (xspi->irq >= 0)
+		xspi->write_fn(XIPIF_V123B_GINTR_ENABLE,
+			regs_base + XIPIF_V123B_DGIER_OFFSET);
+	else
+		xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET);
 	/* Deselect the slave on the SPI bus */
 	xspi->write_fn(0xffff, regs_base + XSPI_SSR_OFFSET);
 	/* Disable the transmitter, enable Manual Slave Select Assertion,
@@ -264,7 +267,12 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 							~XSPI_CR_TRANS_INHIBIT;
 		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 
-		wait_for_completion(&xspi->done);
+		if (xspi->irq >= 0)
+			wait_for_completion(&xspi->done);
+		else
+			while (!(xspi->read_fn(xspi->regs + XSPI_SR_OFFSET) &
+						XSPI_SR_TX_EMPTY_MASK))
+				;
 
 		/* A transmit has just completed. Process received data and
 		 * check for more data to transmit. Always inhibit the
@@ -412,20 +420,17 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 
 	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
 
-	/* SPI controller initializations */
-	xspi_init_hw(xspi);
-
 	xspi->irq = platform_get_irq(pdev, 0);
-	if (xspi->irq < 0) {
-		ret = xspi->irq;
-		goto put_master;
+	if (xspi->irq >= 0) {
+		/* Register for SPI Interrupt */
+		ret = devm_request_irq(&pdev->dev, xspi->irq, xilinx_spi_irq, 0,
+				dev_name(&pdev->dev), xspi);
+		if (ret)
+			goto put_master;
 	}
 
-	/* Register for SPI Interrupt */
-	ret = devm_request_irq(&pdev->dev, xspi->irq, xilinx_spi_irq, 0,
-			       dev_name(&pdev->dev), xspi);
-	if (ret)
-		goto put_master;
+	/* SPI controller initializations */
+	xspi_init_hw(xspi);
 
 	ret = spi_bitbang_start(&xspi->bitbang);
 	if (ret) {
-- 
2.1.4

Re: [PATCH 08/18] spi/xilinx: Support cores with no interrupt

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:40PM +0100, Ricardo Ribalda Delgado wrote:

> The core can run in polling mode. In fact, the performance of the core
> is similar (or even better), due to the fact most of the spi
> transactions are just a couple of bytes and there is one irq per
> transactions.

> When an mtd device is connected via spi, reading 8MB of data produces
> more than 80K interrupts (with irq disabling, context swith....)

Right, for short transfers polling tends to win every time over
interrupts - if you look at other controller drivers you'll see a lot of
them use this technique.  The best practice here is generally to use a
copy break and do very short transfers in polling mode and go back to
interrupts for larger ones.  This break is typically done at the point
where we go over a FIFO in transfer size since normally if the device is
designed to be run with DMA the FIFO will be quite small.

Obviously this isn't quite the same case since not only is there no DMA
support (yet?) but the FIFO could be any size but similar things apply
especially if someone configured the device with a large FIFO.

> -	xspi_init_hw(xspi);
> -
>  	xspi->irq = platform_get_irq(pdev, 0);
> -	if (xspi->irq < 0) {
> -		ret = xspi->irq;
> -		goto put_master;
> +	if (xspi->irq >= 0) {
> +		/* Register for SPI Interrupt */
> +		ret = devm_request_irq(&pdev->dev, xspi->irq, xilinx_spi_irq, 0,
> +				dev_name(&pdev->dev), xspi);
> +		if (ret)
> +			goto put_master;
>  	}
>  
> -	/* Register for SPI Interrupt */
> -	ret = devm_request_irq(&pdev->dev, xspi->irq, xilinx_spi_irq, 0,
> -			       dev_name(&pdev->dev), xspi);
> -	if (ret)
> -		goto put_master;
> +	/* SPI controller initializations */
> +	xspi_init_hw(xspi);

This appears to move the interrupt request before the hardware reset.
Are you sure the interrupt handler won't misbehave if it fires before we
finished initializing?  One thing the hardware reset should do is get
the device in a known good state.

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

Re: [PATCH 08/18] spi/xilinx: Support cores with no interrupt

[Ricardo Ribalda Delgado]

Hello Mark

> Right, for short transfers polling tends to win every time over
> interrupts - if you look at other controller drivers you'll see a lot of
> them use this technique.  The best practice here is generally to use a
> copy break and do very short transfers in polling mode and go back to
> interrupts for larger ones.  This break is typically done at the point
> where we go over a FIFO in transfer size since normally if the device is
> designed to be run with DMA the FIFO will be quite small.

Seems very reasonable. Only problem I can see is if the spi clk is
veeery slow, then a irq approach, even for transfer size bellow
buffer_size will be better.


>
> Obviously this isn't quite the same case since not only is there no DMA
> support (yet?) but the FIFO could be any size but similar things apply
> especially if someone configured the device with a large FIFO.

The hardware does not support dma (at least without making use of
another core called cdma....)

We can always argue that the system engineer can select polling by not
specifying an irq on the device tree.


>> +     xspi_init_hw(xspi);
>
> This appears to move the interrupt request before the hardware reset.
> Are you sure the interrupt handler won't misbehave if it fires before we
> finished initializing?  One thing the hardware reset should do is get
> the device in a known good state.

The hardware will be reseted and in good state by the function
find_buffer_size, so this should be good. Also the initialization is
slightly different for polling and irq mode.

So far I have fixed what you have sent and implemented a new patch for
the "heuristic" irq mode. I will try to run in on hw tomorrow and send
a new patchset with the patches that have not been merged yet.

Maybe you want to consider also this patch from Michal Simek to your
topic/xilinx branch

https://patchwork.kernel.org/patch/5648351/

Thanks!



-- 
Ricardo Ribalda

Re: [PATCH 08/18] spi/xilinx: Support cores with no interrupt

[Mark Brown]

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

On Tue, Jan 27, 2015 at 08:05:57PM +0100, Ricardo Ribalda Delgado wrote:

> > Right, for short transfers polling tends to win every time over
> > interrupts - if you look at other controller drivers you'll see a lot of
> > them use this technique.  The best practice here is generally to use a
> > copy break and do very short transfers in polling mode and go back to
> > interrupts for larger ones.  This break is typically done at the point
> > where we go over a FIFO in transfer size since normally if the device is
> > designed to be run with DMA the FIFO will be quite small.

> Seems very reasonable. Only problem I can see is if the spi clk is
> veeery slow, then a irq approach, even for transfer size bellow
> buffer_size will be better.

Yeah, there's a bit of guesswork in there though practically speaking
people tend not to make SPI *that* slow since one of the reasons to use
it over I2C is performance.  You can do things like estimate how long
the transfer should be based on the clock rate and shove a msleep() in
there as well.

> > Obviously this isn't quite the same case since not only is there no DMA
> > support (yet?) but the FIFO could be any size but similar things apply
> > especially if someone configured the device with a large FIFO.

> The hardware does not support dma (at least without making use of
> another core called cdma....)

Yes, that's standard - it's very rare to see a SPI controller with
integrated DMA.  Usually the IP will bring out signals which can
handshake with a DMA controller to control flow.

> We can always argue that the system engineer can select polling by not
> specifying an irq on the device tree.

The DT should describe the hardware, performance tuning doesn't really
come into that - what if the driver gets better and can be smarter about
interrupts or the best tuning varies at runtime?

> >> +     xspi_init_hw(xspi);

> > This appears to move the interrupt request before the hardware reset.
> > Are you sure the interrupt handler won't misbehave if it fires before we
> > finished initializing?  One thing the hardware reset should do is get
> > the device in a known good state.

> The hardware will be reseted and in good state by the function
> find_buffer_size, so this should be good. Also the initialization is
> slightly different for polling and irq mode.

> So far I have fixed what you have sent and implemented a new patch for
> the "heuristic" irq mode. I will try to run in on hw tomorrow and send
> a new patchset with the patches that have not been merged yet.

OK.

> Maybe you want to consider also this patch from Michal Simek to your
> topic/xilinx branch

> https://patchwork.kernel.org/patch/5648351/

Please include plain text descriptions of things in your mails rather
than just links/numbers.

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

Re: [PATCH 08/18] spi/xilinx: Support cores with no interrupt

[Ricardo Ribalda Delgado]

Hello

On Tue, Jan 27, 2015 at 8:49 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> https://patchwork.kernel.org/patch/5648351/
>
> Please include plain text descriptions of things in your mails rather
> than just links/numbers.

spi: xilinx: Use standard num-cs binding


Regards!

-- 
Ricardo Ribalda

[PATCH 09/18] spi/xilinx: Do not inhibit transmission in polling mode

[Ricardo Ribalda Delgado]

When no irq is used, there is no need to inhibit the transmission for
every transaction. This inhibition was implemented to avoid a race
condition with the irq handler.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 8a28cab..a70cb91 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -163,6 +163,7 @@ static void xspi_rx32(struct xilinx_spi *xspi)
 static void xspi_init_hw(struct xilinx_spi *xspi)
 {
 	void __iomem *regs_base = xspi->regs;
+	u32 inhibit;
 
 	/* Reset the SPI device */
 	xspi->write_fn(XIPIF_V123B_RESET_MASK,
@@ -173,16 +174,19 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
 	xspi->write_fn(XSPI_INTR_TX_EMPTY,
 			regs_base + XIPIF_V123B_IIER_OFFSET);
 	/* Enable the global IPIF interrupt */
-	if (xspi->irq >= 0)
+	if (xspi->irq >= 0) {
 		xspi->write_fn(XIPIF_V123B_GINTR_ENABLE,
 			regs_base + XIPIF_V123B_DGIER_OFFSET);
-	else
+		inhibit = XSPI_CR_TRANS_INHIBIT;
+	} else {
 		xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET);
+		inhibit = 0;
+	}
 	/* Deselect the slave on the SPI bus */
 	xspi->write_fn(0xffff, regs_base + XSPI_SSR_OFFSET);
 	/* Disable the transmitter, enable Manual Slave Select Assertion,
 	 * put SPI controller into master mode, and enable it */
-	xspi->write_fn(XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT |
+	xspi->write_fn(inhibit | XSPI_CR_MANUAL_SSELECT |
 		XSPI_CR_MASTER_MODE | XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET |
 		XSPI_CR_RXFIFO_RESET, regs_base + XSPI_CR_OFFSET);
 }
@@ -252,7 +256,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	reinit_completion(&xspi->done);
 
 	while (xspi->remaining_bytes) {
-		u16 cr;
+		u16 cr = 0;
 		int n_words;
 
 		n_words = (xspi->remaining_bytes * 8) / xspi->bits_per_word;
@@ -263,13 +267,13 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		/* Start the transfer by not inhibiting the transmitter any
 		 * longer
 		 */
-		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) &
-							~XSPI_CR_TRANS_INHIBIT;
-		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 
-		if (xspi->irq >= 0)
+		if (xspi->irq >= 0) {
+			cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) &
+							~XSPI_CR_TRANS_INHIBIT;
+			xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 			wait_for_completion(&xspi->done);
-		else
+		} else
 			while (!(xspi->read_fn(xspi->regs + XSPI_SR_OFFSET) &
 						XSPI_SR_TX_EMPTY_MASK))
 				;
@@ -279,7 +283,8 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		 * transmitter while the Isr refills the transmit register/FIFO,
 		 * or make sure it is stopped if we're done.
 		 */
-		xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
+		if (xspi->irq >= 0)
+			xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
 			       xspi->regs + XSPI_CR_OFFSET);
 
 		/* Read out all the data from the Rx FIFO */
-- 
2.1.4

[PATCH 10/18] spi/xilinx: Support for spi mode CS_HIGH

[Ricardo Ribalda Delgado]

The core controls the chip select lines individually.

By default, all the lines are consider active_low. After
spi_setup_transfer, it has its real value.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 62 +++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index a70cb91..29edf1b 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -89,6 +89,7 @@ struct xilinx_spi {
 	int remaining_bytes;	/* the number of bytes left to transfer */
 	u8 bits_per_word;
 	int buffer_size;	/* buffer size in words */
+	u32 cs_inactive;	/* Level of the CS pins when inactive*/
 	unsigned int (*read_fn)(void __iomem *);
 	void (*write_fn)(u32, void __iomem *);
 	void (*tx_fn)(struct xilinx_spi *);
@@ -194,33 +195,37 @@ static void xspi_init_hw(struct xilinx_spi *xspi)
 static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
 {
 	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
+	u16 cr;
+	u32 cs;
 
 	if (is_on == BITBANG_CS_INACTIVE) {
 		/* Deselect the slave on the SPI bus */
-		xspi->write_fn(0xffff, xspi->regs + XSPI_SSR_OFFSET);
-	} else if (is_on == BITBANG_CS_ACTIVE) {
-		/* Set the SPI clock phase and polarity */
-		u16 cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET)
-			 & ~XSPI_CR_MODE_MASK;
-		if (spi->mode & SPI_CPHA)
-			cr |= XSPI_CR_CPHA;
-		if (spi->mode & SPI_CPOL)
-			cr |= XSPI_CR_CPOL;
-		if (spi->mode & SPI_LSB_FIRST)
-			cr |= XSPI_CR_LSB_FIRST;
-		if (spi->mode & SPI_LOOP)
-			cr |= XSPI_CR_LOOP;
-		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
-
-		/* We do not check spi->max_speed_hz here as the SPI clock
-		 * frequency is not software programmable (the IP block design
-		 * parameter)
-		 */
-
-		/* Activate the chip select */
-		xspi->write_fn(~(0x0001 << spi->chip_select),
-			xspi->regs + XSPI_SSR_OFFSET);
+		xspi->write_fn(xspi->cs_inactive, xspi->regs + XSPI_SSR_OFFSET);
+		return;
 	}
+
+	/* Set the SPI clock phase and polarity */
+	cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET)	& ~XSPI_CR_MODE_MASK;
+	if (spi->mode & SPI_CPHA)
+		cr |= XSPI_CR_CPHA;
+	if (spi->mode & SPI_CPOL)
+		cr |= XSPI_CR_CPOL;
+	if (spi->mode & SPI_LSB_FIRST)
+		cr |= XSPI_CR_LSB_FIRST;
+	if (spi->mode & SPI_LOOP)
+		cr |= XSPI_CR_LOOP;
+	xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+
+	/* We do not check spi->max_speed_hz here as the SPI clock
+	 * frequency is not software programmable (the IP block design
+	 * parameter)
+	 */
+
+	cs = xspi->cs_inactive;
+	cs ^= BIT(spi->chip_select);
+
+	/* Activate the chip select */
+	xspi->write_fn(cs, xspi->regs + XSPI_SSR_OFFSET);
 }
 
 /* spi_bitbang requires custom setup_transfer() to be defined if there is a
@@ -229,6 +234,13 @@ static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
 static int xilinx_spi_setup_transfer(struct spi_device *spi,
 		struct spi_transfer *t)
 {
+	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
+
+	if (spi->mode & SPI_CS_HIGH)
+		xspi->cs_inactive &= ~BIT(spi->chip_select);
+	else
+		xspi->cs_inactive |= BIT(spi->chip_select);
+
 	return 0;
 }
 
@@ -369,9 +381,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/* the spi->mode bits understood by this driver: */
-	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_LOOP;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_LOOP |
+			    SPI_CS_HIGH;
 
 	xspi = spi_master_get_devdata(master);
+	xspi->cs_inactive = 0xffffffff;
 	xspi->bitbang.master = master;
 	xspi->bitbang.chipselect = xilinx_spi_chipselect;
 	xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
-- 
2.1.4

[PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer

[Ricardo Ribalda Delgado]

Simplify the code by removing the tx and and rx function pointers and
substitute them by a single function.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 69 +++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 51 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 29edf1b..6b6c658 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -92,8 +92,6 @@ struct xilinx_spi {
 	u32 cs_inactive;	/* Level of the CS pins when inactive*/
 	unsigned int (*read_fn)(void __iomem *);
 	void (*write_fn)(u32, void __iomem *);
-	void (*tx_fn)(struct xilinx_spi *);
-	void (*rx_fn)(struct xilinx_spi *);
 };
 
 static void xspi_write32(u32 val, void __iomem *addr)
@@ -116,49 +114,32 @@ static unsigned int xspi_read32_be(void __iomem *addr)
 	return ioread32be(addr);
 }
 
-static void xspi_tx8(struct xilinx_spi *xspi)
-{
-	xspi->write_fn(*xspi->tx_ptr, xspi->regs + XSPI_TXD_OFFSET);
-	xspi->tx_ptr++;
-}
-
-static void xspi_tx16(struct xilinx_spi *xspi)
-{
-	xspi->write_fn(*(u16 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET);
-	xspi->tx_ptr += 2;
-}
-
-static void xspi_tx32(struct xilinx_spi *xspi)
+static void xilinx_spi_tx(struct xilinx_spi *xspi)
 {
 	xspi->write_fn(*(u32 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET);
-	xspi->tx_ptr += 4;
+	xspi->tx_ptr += xspi->bits_per_word/8;
 }
 
-static void xspi_rx8(struct xilinx_spi *xspi)
+static void xilinx_spi_rx(struct xilinx_spi *xspi)
 {
 	u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET);
-	if (xspi->rx_ptr) {
-		*xspi->rx_ptr = data & 0xff;
-		xspi->rx_ptr++;
-	}
-}
 
-static void xspi_rx16(struct xilinx_spi *xspi)
-{
-	u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET);
-	if (xspi->rx_ptr) {
-		*(u16 *)(xspi->rx_ptr) = data & 0xffff;
-		xspi->rx_ptr += 2;
-	}
-}
+	if (!xspi->rx_ptr)
+		return;
 
-static void xspi_rx32(struct xilinx_spi *xspi)
-{
-	u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET);
-	if (xspi->rx_ptr) {
+	switch (xspi->bits_per_word) {
+	case 8:
+		*(u8 *)(xspi->rx_ptr) = data;
+		break;
+	case 16:
+		*(u16 *)(xspi->rx_ptr) = data;
+		break;
+	case 32:
 		*(u32 *)(xspi->rx_ptr) = data;
-		xspi->rx_ptr += 4;
+		break;
 	}
+
+	xspi->rx_ptr += xspi->bits_per_word/8;
 }
 
 static void xspi_init_hw(struct xilinx_spi *xspi)
@@ -250,7 +231,7 @@ static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
 
 	while (n_words--)
 		if (xspi->tx_ptr)
-			xspi->tx_fn(xspi);
+			xilinx_spi_tx(xspi);
 		else
 			xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
 	return;
@@ -301,7 +282,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 		/* Read out all the data from the Rx FIFO */
 		while (n_words--)
-			xspi->rx_fn(xspi);
+			xilinx_spi_rx(xspi);
 	}
 
 	return t->len - xspi->remaining_bytes;
@@ -423,20 +404,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 
 	master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word);
 	xspi->bits_per_word = bits_per_word;
-	if (xspi->bits_per_word == 8) {
-		xspi->tx_fn = xspi_tx8;
-		xspi->rx_fn = xspi_rx8;
-	} else if (xspi->bits_per_word == 16) {
-		xspi->tx_fn = xspi_tx16;
-		xspi->rx_fn = xspi_rx16;
-	} else if (xspi->bits_per_word == 32) {
-		xspi->tx_fn = xspi_tx32;
-		xspi->rx_fn = xspi_rx32;
-	} else {
-		ret = -EINVAL;
-		goto put_master;
-	}
-
 	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
 
 	xspi->irq = platform_get_irq(pdev, 0);
-- 
2.1.4

Re: [PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:43PM +0100, Ricardo Ribalda Delgado wrote:

> +	switch (xspi->bits_per_word) {
> +	case 8:
> +		*(u8 *)(xspi->rx_ptr) = data;
> +		break;
> +	case 16:
> +		*(u16 *)(xspi->rx_ptr) = data;
> +		break;
> +	case 32:
>  		*(u32 *)(xspi->rx_ptr) = data;
> -		xspi->rx_ptr += 4;
> +		break;
>  	}

Perhaps I'm missing something here but we only seem to be incrementing
rx_ptr for the 32 bit case here...

> +	xspi->rx_ptr += xspi->bits_per_word/8;

...which looks to duplicate this which handles all cases.  Also there's
a coding style thing - spaces around the / please.

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

Re: [PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer

[Ricardo Ribalda Delgado]

Hello Mark


> Perhaps I'm missing something here but we only seem to be incrementing
> rx_ptr for the 32 bit case here...

The unified diff is a bit confusing this time.

This is how the function looks after the patch.
All the modes are handled in a generic way.

Which I believe it is right:

static void xilinx_spi_rx(struct xilinx_spi *xspi)
{
u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET);

if (!xspi->rx_ptr)
    return;

switch (xspi->bits_per_word) {
case 8:
    *(u8 *)(xspi->rx_ptr) = data;
break;
case 16:
    *(u16 *)(xspi->rx_ptr) = data;
break;
case 32:
    *(u32 *)(xspi->rx_ptr) = data;
break;
}

xspi->rx_ptr += xspi->bits_per_word/8;
}



>
>> +     xspi->rx_ptr += xspi->bits_per_word/8;
>
> ...which looks to duplicate this which handles all cases.  Also there's
> a coding style thing - spaces around the / please.

I am fixing this on the next version. (and  xspi->tx_ptr +=
xspi->bits_per_word/8;)

I am also cc: the maintainers of checkpatch, because for whatever
reason, this was not found by the script

ricardo@neopili:~/curro/qtec/linux-upstream$ scripts/checkpatch.pl
xilinx-spi2/0011-spi-xilinx-Remove-rx_fn-and-tx_fn-pointer.patch
total: 0 errors, 0 warnings, 109 lines checked

xilinx-spi2/0011-spi-xilinx-Remove-rx_fn-and-tx_fn-pointer.patch has
no obvious style problems and is ready for submission.

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer

[Joe Perches]

On Tue, 2015-01-27 at 11:00 +0100, Ricardo Ribalda Delgado wrote:
> >> +     xspi->rx_ptr += xspi->bits_per_word/8;
> > Also there's
> > a coding style thing - spaces around the / please.
[]
> I am also cc: the maintainers of checkpatch, because for whatever
> reason, this was not found by the script

Because it's personal preference.

checkpatch would note if a space was used
inconsistently (before but not after the /,
or the other way 'round)

Re: [PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer

[Ricardo Ribalda Delgado]

Hello Joe

On Tue, Jan 27, 2015 at 4:42 PM, Joe Perches <joe@perches.com> wrote:

>
> Because it's personal preference.
>
> checkpatch would note if a space was used
> inconsistently (before but not after the /,
> or the other way 'round)

Thanks for the explanation (and the great script :) )

Regards!

-- 
Ricardo Ribalda

[PATCH 12/18] spi/xilinx: Make spi_tx and spi_rx simmetric

[Ricardo Ribalda Delgado]

spi_rx handles the case where the buffer is null. Nevertheless spi_tx
did not handle it, and was handled by the caller function.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 6b6c658..514ad01 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -116,6 +116,10 @@ static unsigned int xspi_read32_be(void __iomem *addr)
 
 static void xilinx_spi_tx(struct xilinx_spi *xspi)
 {
+	if (!xspi->tx_ptr) {
+		xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
+		return;
+	}
 	xspi->write_fn(*(u32 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET);
 	xspi->tx_ptr += xspi->bits_per_word/8;
 }
@@ -230,10 +234,7 @@ static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
 	xspi->remaining_bytes -= n_words * xspi->bits_per_word / 8;
 
 	while (n_words--)
-		if (xspi->tx_ptr)
-			xilinx_spi_tx(xspi);
-		else
-			xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
+		xilinx_spi_tx(xspi);
 	return;
 }
 
-- 
2.1.4

[PATCH 13/18] spi/xilinx: Convert remainding_bytes in remaining words

[Ricardo Ribalda Delgado]

Simplify the code by using the unit used on most of the code logic.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 514ad01..fce5d0b 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -86,7 +86,7 @@ struct xilinx_spi {
 
 	u8 *rx_ptr;		/* pointer in the Tx buffer */
 	const u8 *tx_ptr;	/* pointer in the Rx buffer */
-	int remaining_bytes;	/* the number of bytes left to transfer */
+	int remaining_words;	/* the number of words left to transfer */
 	u8 bits_per_word;
 	int buffer_size;	/* buffer size in words */
 	u32 cs_inactive;	/* Level of the CS pins when inactive*/
@@ -231,7 +231,7 @@ static int xilinx_spi_setup_transfer(struct spi_device *spi,
 
 static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
 {
-	xspi->remaining_bytes -= n_words * xspi->bits_per_word / 8;
+	xspi->remaining_words -= n_words;
 
 	while (n_words--)
 		xilinx_spi_tx(xspi);
@@ -246,15 +246,14 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 	xspi->tx_ptr = t->tx_buf;
 	xspi->rx_ptr = t->rx_buf;
-	xspi->remaining_bytes = t->len;
+	xspi->remaining_words = (t->len * 8) / xspi->bits_per_word;
 	reinit_completion(&xspi->done);
 
-	while (xspi->remaining_bytes) {
+	while (xspi->remaining_words) {
 		u16 cr = 0;
 		int n_words;
 
-		n_words = (xspi->remaining_bytes * 8) / xspi->bits_per_word;
-		n_words = min(n_words, xspi->buffer_size);
+		n_words = min(xspi->remaining_words, xspi->buffer_size);
 
 		xilinx_spi_fill_tx_fifo(xspi, n_words);
 
@@ -286,7 +285,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 			xilinx_spi_rx(xspi);
 	}
 
-	return t->len - xspi->remaining_bytes;
+	return t->len;
 }
 
 
-- 
2.1.4

[PATCH 14/18] spi/xilinx: Convert bits_per_word in bytes_per_word

[Ricardo Ribalda Delgado]

Simplify the code by using the unit used on most of the code logic.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index fce5d0b..5d3503a 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -87,7 +87,7 @@ struct xilinx_spi {
 	u8 *rx_ptr;		/* pointer in the Tx buffer */
 	const u8 *tx_ptr;	/* pointer in the Rx buffer */
 	int remaining_words;	/* the number of words left to transfer */
-	u8 bits_per_word;
+	u8 bytes_per_word;
 	int buffer_size;	/* buffer size in words */
 	u32 cs_inactive;	/* Level of the CS pins when inactive*/
 	unsigned int (*read_fn)(void __iomem *);
@@ -121,7 +121,7 @@ static void xilinx_spi_tx(struct xilinx_spi *xspi)
 		return;
 	}
 	xspi->write_fn(*(u32 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET);
-	xspi->tx_ptr += xspi->bits_per_word/8;
+	xspi->tx_ptr += xspi->bytes_per_word;
 }
 
 static void xilinx_spi_rx(struct xilinx_spi *xspi)
@@ -131,19 +131,19 @@ static void xilinx_spi_rx(struct xilinx_spi *xspi)
 	if (!xspi->rx_ptr)
 		return;
 
-	switch (xspi->bits_per_word) {
-	case 8:
+	switch (xspi->bytes_per_word) {
+	case 1:
 		*(u8 *)(xspi->rx_ptr) = data;
 		break;
-	case 16:
+	case 2:
 		*(u16 *)(xspi->rx_ptr) = data;
 		break;
-	case 32:
+	case 4:
 		*(u32 *)(xspi->rx_ptr) = data;
 		break;
 	}
 
-	xspi->rx_ptr += xspi->bits_per_word/8;
+	xspi->rx_ptr += xspi->bytes_per_word;
 }
 
 static void xspi_init_hw(struct xilinx_spi *xspi)
@@ -246,7 +246,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 	xspi->tx_ptr = t->tx_buf;
 	xspi->rx_ptr = t->rx_buf;
-	xspi->remaining_words = (t->len * 8) / xspi->bits_per_word;
+	xspi->remaining_words = t->len / xspi->bytes_per_word;
 	reinit_completion(&xspi->done);
 
 	while (xspi->remaining_words) {
@@ -403,7 +403,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	}
 
 	master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word);
-	xspi->bits_per_word = bits_per_word;
+	xspi->bytes_per_word = bits_per_word / 8;
 	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
 
 	xspi->irq = platform_get_irq(pdev, 0);
-- 
2.1.4

[PATCH 15/18] spi/xilinx: Remove iowrite/ioread wrappers

[Ricardo Ribalda Delgado]

Save a stack level and cleanup code.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 5d3503a..704613c 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -94,26 +94,6 @@ struct xilinx_spi {
 	void (*write_fn)(u32, void __iomem *);
 };
 
-static void xspi_write32(u32 val, void __iomem *addr)
-{
-	iowrite32(val, addr);
-}
-
-static unsigned int xspi_read32(void __iomem *addr)
-{
-	return ioread32(addr);
-}
-
-static void xspi_write32_be(u32 val, void __iomem *addr)
-{
-	iowrite32be(val, addr);
-}
-
-static unsigned int xspi_read32_be(void __iomem *addr)
-{
-	return ioread32be(addr);
-}
-
 static void xilinx_spi_tx(struct xilinx_spi *xspi)
 {
 	if (!xspi->tx_ptr) {
@@ -391,15 +371,15 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	 * Setup little endian helper functions first and try to use them
 	 * and check if bit was correctly setup or not.
 	 */
-	xspi->read_fn = xspi_read32;
-	xspi->write_fn = xspi_write32;
+	xspi->read_fn = ioread32;
+	xspi->write_fn = iowrite32;
 
 	xspi->write_fn(XSPI_CR_LOOP, xspi->regs + XSPI_CR_OFFSET);
 	tmp = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
 	tmp &= XSPI_CR_LOOP;
 	if (tmp != XSPI_CR_LOOP) {
-		xspi->read_fn = xspi_read32_be;
-		xspi->write_fn = xspi_write32_be;
+		xspi->read_fn = ioread32be;
+		xspi->write_fn = iowrite32be;
 	}
 
 	master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word);
-- 
2.1.4

[PATCH 16/18] spi/xilinx: Reset core before buffer_size detection

[Ricardo Ribalda Delgado]

If the core had some data on the buffer, the size detection could show
up a smaller size than real, affecting performance.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 704613c..8c25c59 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -295,6 +295,13 @@ static int xilinx_spi_find_buffer_size(struct xilinx_spi *xspi)
 	u8 sr;
 	int n_words = 0;
 
+	/*
+	 * Before the buffer_size detection we reset the core
+	 * to make sure we start with a clean state.
+	 */
+	xspi->write_fn(XIPIF_V123B_RESET_MASK,
+		xspi->regs + XIPIF_V123B_RESETR_OFFSET);
+
 	/* Fill the Tx FIFO with as many words as possible */
 	do {
 		xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET);
-- 
2.1.4

Re: [PATCH 16/18] spi/xilinx: Reset core before buffer_size detection

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:48PM +0100, Ricardo Ribalda Delgado wrote:
> If the core had some data on the buffer, the size detection could show
> up a smaller size than real, affecting performance.

This should've been part of your earlier patch.

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

[PATCH 17/18] spi/xilinx: Remove remaining_words driver data variable

[Ricardo Ribalda Delgado]

The variable never leaves the scope of txrx_bufs.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 8c25c59..026f4c5 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -86,7 +86,6 @@ struct xilinx_spi {
 
 	u8 *rx_ptr;		/* pointer in the Tx buffer */
 	const u8 *tx_ptr;	/* pointer in the Rx buffer */
-	int remaining_words;	/* the number of words left to transfer */
 	u8 bytes_per_word;
 	int buffer_size;	/* buffer size in words */
 	u32 cs_inactive;	/* Level of the CS pins when inactive*/
@@ -209,33 +208,27 @@ static int xilinx_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words)
-{
-	xspi->remaining_words -= n_words;
-
-	while (n_words--)
-		xilinx_spi_tx(xspi);
-	return;
-}
-
 static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 {
 	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
+	int remaining_words;	/* the number of words left to transfer */
 
 	/* We get here with transmitter inhibited */
 
 	xspi->tx_ptr = t->tx_buf;
 	xspi->rx_ptr = t->rx_buf;
-	xspi->remaining_words = t->len / xspi->bytes_per_word;
+	remaining_words = t->len / xspi->bytes_per_word;
 	reinit_completion(&xspi->done);
 
-	while (xspi->remaining_words) {
+	while (remaining_words) {
 		u16 cr = 0;
-		int n_words;
+		int n_words, tx_words, rx_words;
 
-		n_words = min(xspi->remaining_words, xspi->buffer_size);
+		n_words = min(remaining_words, xspi->buffer_size);
 
-		xilinx_spi_fill_tx_fifo(xspi, n_words);
+		tx_words = n_words;
+		while (tx_words--)
+			xilinx_spi_tx(xspi);
 
 		/* Start the transfer by not inhibiting the transmitter any
 		 * longer
@@ -261,8 +254,11 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 			       xspi->regs + XSPI_CR_OFFSET);
 
 		/* Read out all the data from the Rx FIFO */
-		while (n_words--)
+		rx_words = n_words;
+		while (rx_words--)
 			xilinx_spi_rx(xspi);
+
+		remaining_words -= n_words;
 	}
 
 	return t->len;
-- 
2.1.4

[PATCH 18/18] spi/xilinx: Check number of slaves range

[Ricardo Ribalda Delgado]

The core only supports up to 32 slaves, and the chipselect function
expects the same.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/spi/spi-xilinx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 026f4c5..134d8cd 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -22,6 +22,8 @@
 #include <linux/spi/xilinx_spi.h>
 #include <linux/io.h>
 
+#define MAX_CS			32
+
 #define XILINX_SPI_NAME "xilinx_spi"
 
 /* Register definitions as per "OPB Serial Peripheral Interface (SPI) (v1.00e)
@@ -340,6 +342,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	if (num_cs > MAX_CS) {
+		dev_err(&pdev->dev, "Invalid number of spi slaves\n");
+		return -EINVAL;
+	}
+
 	master = spi_alloc_master(&pdev->dev, sizeof(struct xilinx_spi));
 	if (!master)
 		return -ENODEV;
-- 
2.1.4

Re: [PATCH 00/18] spi/xilinx: Speed-up

[Mark Brown]

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

On Fri, Jan 23, 2015 at 05:08:32PM +0100, Ricardo Ribalda Delgado wrote:
> This set of patches has been developed after using the core to access a spi
> nor flash. The initial performance was not as desired. The following changes
> have been done.

In general the best way to present a series is to start off with any bug
fixes (so they can be sent to Linus and stable if needed), then move on
to mechanical and stylistic cleanups (since they tend to be easy to
review and apply) and finally move on to new functionality (since that
is most likely to run into review problems and the cleanups should help
review).  

>   spi/xilinx: Support for spi mode LOOP
>   spi/xilinx: Simplify data read from the Rx FIFO
>   spi-xilinx: Simplify spi_fill_tx_fifo

spi-xilinx?

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

Re: [PATCH 00/18] spi/xilinx: Speed-up

[Ricardo Ribalda Delgado]

Hello Mark

On Tue, Jan 27, 2015 at 1:14 AM, Mark Brown <broonie@kernel.org> wrote:

>
> In general the best way to present a series is to start off with any bug
> fixes (so they can be sent to Linus and stable if needed), then move on
> to mechanical and stylistic cleanups (since they tend to be easy to
> review and apply) and finally move on to new functionality (since that
> is most likely to run into review problems and the cleanups should help
> review).

Will try to follow that pattern the next time.

The development of the patchset has been a bit chaotic. I opened up
the code to support more modes and then I realize that I could improve
the poor performance I was having with the driver....

>
>>   spi/xilinx: Support for spi mode LOOP
>>   spi/xilinx: Simplify data read from the Rx FIFO
>>   spi-xilinx: Simplify spi_fill_tx_fifo
>
> spi-xilinx?

Thanks! Fixed on the next version.

BTW what is preferred spi/xilinx: or spi: xilinx: ?I have seen both naming.

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH 00/18] spi/xilinx: Speed-up

[Mark Brown]

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

On Tue, Jan 27, 2015 at 11:17:42AM +0100, Ricardo Ribalda Delgado wrote:
> On Tue, Jan 27, 2015 at 1:14 AM, Mark Brown <broonie@kernel.org> wrote:

> >>   spi/xilinx: Support for spi mode LOOP
> >>   spi/xilinx: Simplify data read from the Rx FIFO
> >>   spi-xilinx: Simplify spi_fill_tx_fifo

> > spi-xilinx?

> Thanks! Fixed on the next version.

> BTW what is preferred spi/xilinx: or spi: xilinx: ?I have seen both naming.

Traditionally SPI always used spi/xilinx but that's too far out from the
normal kernel standards so it's hard to get people to follow it at all
so I don't worry about that specific bit too much.

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