DaVinci: Improve DaVinci SPI speed.

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from 2 to 1 and take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138 EVM, SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with a 2Mbyte image file. Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from 2 to 1 and
take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138 EVM,
SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with a 2Mbyte image file.
Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

---

Quoted 7 lines

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from

2

Quoted 4 lines

to 1 and
take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138 EVM,
SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with

a

Quoted 6 lines

2Mbyte image file.
Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

---

Checkpatch returned 4 errors.

Please fix and resubmit

Thanks,
Sandeep

Quoted 6 lines

Checkpatch returned 4 errors.

Please fix and resubmit

Thanks,
Sandeep

I fixed them myself.

Pushed to u-boot-ti

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from 2 to 1 and take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138 EVM, SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with a 2Mbyte image file. Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

Quoted 5 lines

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from

2 to 1 and

Quoted 1 line

take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138

EVM,

Quoted 1 line

SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with

a 2Mbyte image file.

Quoted 4 lines

Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

Sorry, I'm a bit late to the party on this.

I have an alternative patch that tries to be even quicker, but I don't have the same platform as Delio, so can't compare like with like.

This diff applies before Delio's patch. If it is any faster I am prepared to create a proper patch. If nobody can test it for speed I'll probably just drop it, since it produces a slightly bigger executable and I don't know that it is actually any faster...

In essence, it splits up read and write operations to avoid testing pointers in every loop iteration. It also unrolls the last iteration so that it doesn't have to test for loop ending twice each time round. Finally it avoids bit setting/clearing on each iteration when the results would only turn out to be the same anyway.

Here's the diff:

drivers/spi/davinci_spi.c +89 -7

index 60ba007..a90d2f4 100644
@@ -126,16 +126,98 @@ void spi_release_bus(struct spi_slave *slave)
 	writel(SPIGCR0_SPIRST_MASK, &ds->regs->gcr0);
 }
 
-int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
-		const void *dout, void *din, unsigned long flags)
+static inline u8 davinci_spi_read_data(struct davinci_spi_slave *ds, u32
 data)
+{
+	unsigned int	buf_reg_val;
+	/* wait till TXFULL is deasserted */
+	while (readl(&ds->regs->buf) & SPIBUF_TXFULL_MASK)
+		;
+	writel(data, &ds->regs->dat1);
+
+	/* read the data - wait for data availability */
+	while ((buf_reg_val = readl(&ds->regs->buf)) & SPIBUF_RXEMPTY_MASK)
+		;
+	return buf_reg_val & 0xFF;
+}
+
+static int davinci_spi_read(struct spi_slave *slave, unsigned int len,
+			    u8 *rxp, unsigned long flags)
 {
 	struct davinci_spi_slave *ds = to_davinci_spi(slave);
-	unsigned int	len, data1_reg_val = readl(&ds->regs->dat1);
-	int		ret, i;
-	const u8	*txp = dout; /* dout can be NULL for read operation */
-	u8		*rxp = din;  /* din can be NULL for write operation */
+	unsigned int data1_reg_val = readl(&ds->regs->dat1);
+
+	/* do an empty read to clear the current contents */
+	(void)readl(&ds->regs->buf);
+
+	/* enable CS hold */
+	data1_reg_val |= ((1 << SPIDAT1_CSHOLD_SHIFT) |
+			(slave->cs << SPIDAT1_CSNR_SHIFT));
+	data1_reg_val &= ~0xFFFF;
+
+	/* keep writing and reading 1 byte until only 1 byte left to read */
+	while ((len--) > 1) {
+		*rxp++ = davinci_spi_read_data(ds, data1_reg_val);
+	}
 
-	ret = 0;
+	/*
+	 * clear CS hold when we reach the end.
+	 */
+	if (flags & SPI_XFER_END)
+		data1_reg_val &= ~(1 << SPIDAT1_CSHOLD_SHIFT);
+
+	*rxp = davinci_spi_read_data(ds, data1_reg_val);
+
+	return 0;
+}
+
+static inline void davinci_spi_write_data(struct davinci_spi_slave *ds,
 u32 data)
+{
+	/* wait till TXFULL is deasserted */
+	while (readl(&ds->regs->buf) & SPIBUF_TXFULL_MASK)
+		;
+	writel(data, &ds->regs->dat1);
+
+	/* wait for read data availability */
+	while (readl(&ds->regs->buf) & SPIBUF_RXEMPTY_MASK)
+		;
+}
+
+static int davinci_spi_write(struct spi_slave *slave, unsigned int len,
+		const u8 *txp, unsigned long flags)
+{
+	struct davinci_spi_slave *ds = to_davinci_spi(slave);
+	unsigned int data1_reg_val = readl(&ds->regs->dat1);
+
+	/* do an empty read to clear the current contents */
+	(void)readl(&ds->regs->buf);
+
+	/* enable CS hold */
+	data1_reg_val |= ((1 << SPIDAT1_CSHOLD_SHIFT) |
+			(slave->cs << SPIDAT1_CSNR_SHIFT));
+	data1_reg_val &= ~0xFFFF;
+
+	/* keep writing and reading 1 byte until only 1 byte left to write
 */
+	while ((len--) > 1) {
+		/* write the data */
+		davinci_spi_write_data(ds, data1_reg_val | *txp++);
+	}
+
+	/*
+	 * clear CS hold when we reach the end.
+	 */
+	if (flags & SPI_XFER_END)
+		data1_reg_val &= ~(1 << SPIDAT1_CSHOLD_SHIFT);
+
+	/* write the data */
+	davinci_spi_write_data(ds, data1_reg_val | *txp);
+
+	return 0;
+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+		const void *dout, void *din, unsigned long flags)
+{
+	unsigned int len;

drivers/spi/davinci_spi.c +6 -44

 	if (bitlen == 0)
 		/* Finish any previously submitted transfers */

@@ -155,53 +237,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int
 bitlen,
 
 	len = bitlen / 8;
 
-	/* do an empty read to clear the current contents */
-	readl(&ds->regs->buf);
-
-	/* keep writing and reading 1 byte until done */
-	for (i = 0; i < len; i++) {
-		/* wait till TXFULL is asserted */
-		while (readl(&ds->regs->buf) & SPIBUF_TXFULL_MASK);
-
-		/* write the data */
-		data1_reg_val &= ~0xFFFF;
-		if (txp) {
-			data1_reg_val |= *txp;
-			txp++;
-		}
-
-		/*
-		 * Write to DAT1 is required to keep the serial transfer going.
-		 * We just terminate when we reach the end.
-		 */
-		if ((i == (len - 1)) && (flags & SPI_XFER_END)) {
-			/* clear CS hold */
-			writel(data1_reg_val &
-				~(1 << SPIDAT1_CSHOLD_SHIFT), &ds->regs->dat1);
-		} else {
-			/* enable CS hold */
-			data1_reg_val |= ((1 << SPIDAT1_CSHOLD_SHIFT) |
-					(slave->cs << SPIDAT1_CSNR_SHIFT));
-			writel(data1_reg_val, &ds->regs->dat1);
-		}
-
-		/* read the data - wait for data availability */
-		while (readl(&ds->regs->buf) & SPIBUF_RXEMPTY_MASK);
-
-		if (rxp) {
-			*rxp = readl(&ds->regs->buf) & 0xFF;
-			rxp++;
-		} else {
-			/* simply drop the read character */
-			readl(&ds->regs->buf);
-		}
-	}
-	return 0;
+	if (din)
+		return davinci_spi_read(slave, len, din, flags);
+	else
+		return davinci_spi_write(slave, len, dout, flags);
 
 out:
 	if (flags & SPI_XFER_END) {
-		writel(data1_reg_val &
-			~(1 << SPIDAT1_CSHOLD_SHIFT), &ds->regs->dat1);
+		u8 dummy = 0;
+		davinci_spi_write(slave, 1, &dummy, flags);
 	}
 	return 0;

}
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 12 lines · Delio Brignoli

Quoted 5 lines

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

Reduce the number of reads per byte transferred on the BUF register from

2 to 1 and

Quoted 1 line

take advantage of the TX buffer in the SPI module. On LogicPD OMAP-L138

EVM,

Quoted 8 lines

Quoted 1 line

SPI read throughput goes up from ~0.8Mbyte/s to ~1.3Mbyte/s. Tested with

a 2Mbyte image file.

Quoted 4 lines

Remove unused variables in the spi_xfer() function.

Signed-off-by Delio Brignoli <[email protected]>

Tested-by Ben Gardiner <[email protected]>

Sorry, I'm a bit late to the party on this.

It is late. Pull request already sent to Wolfgang

Quoted 4 lines

I have an alternative patch that tries to be even quicker, but I don't have the same platform as Delio, so can't compare like with like.

Compare it on your platform. I believe you have the OMAP L137. And post the results.

Quoted 3 lines · Delio Brignoli

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

I have an alternative patch that tries to be even quicker, but I don't have the same platform as Delio, so can't compare like with like.

In essence, it splits up read and write operations to avoid testing pointers in every loop iteration. It also unrolls the last iteration so that it doesn't have to test for loop ending twice each time round. Finally it avoids bit setting/clearing on each iteration when the results would only turn out to be the same anyway.

Hello Nick,

Quoted 4 lines · Nick Thompson

Quoted 3 lines · Delio Brignoli

I have updated this patch based on the comments [1] by Wolfgang Denk and removed unused variables. [1][http://lists.denx.de/pipermail/u-boot/2010-May/071728.html]

[...]

Quoted 3 lines

I have an alternative patch that tries to be even quicker, but I don't have the same platform as Delio, so can't compare like with like.

I will test your patch on my L138 and let you know.

Quoted 6 lines

In essence, it splits up read and write operations to avoid testing pointers in every loop iteration. It also unrolls the last iteration so that it doesn't have to test for loop ending twice each time round. Finally it avoids bit setting/clearing on each iteration when the results would only turn out to be the same anyway.

I am wondering if it is OK to split reading and writing: when I wrote the patch I assumed that spi_xfer() was supposed to be able to read and write to two distinct buffers at the same time. As I understand it, your patch allows spi_xfer() to be either a read or a write operation but not both and I do not know if this semantic change is acceptable. Maybe Sekhar or Sandeep can clarify this.

(I'm not sure if U-Boot uses SPI for anything else anyway...)