[04/18] spi-xilinx: Simplify spi_fill_tx_fifo

Commit Message

Ricardo Ribalda Delgado Jan. 23, 2015, 4:08 p.m. UTC

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 <[e-mail]>
---
 drivers/spi/spi-xilinx.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

Comments

Mark Brown Jan. 26, 2015, 11:54 p.m. UTC | #1

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.

Patch

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