spi: omap3: Fix timeout handling

The timeout value is never reset during the transfer. This means that when transferring more data we eventually trigger the timeout.

This was reported on the mailing list:
"Spansion SPI flash read timeout with AM335x"

Signed-off-by David Dueck <[email protected]>

CC Tom Rini <[email protected]>

CC Jagannadh Teki <[email protected]>

CC Stefan Roese <[email protected]>

CC Andy Pont <[email protected]>

---
Changes since v1:
- fix style issue
- fix CC line

 drivers/spi/omap3_spi.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

drivers/spi/omap3_spi.c +12 -9

index 651e46e..85f9e85 100644
@@ -20,7 +20,7 @@
 #include <asm/io.h>
 #include "omap3_spi.h"
 
-#define SPI_WAIT_TIMEOUT 3000000
+#define SPI_WAIT_TIMEOUT 10
 
 static void spi_reset(struct omap3_spi_slave *ds)
 {

@@ -227,7 +227,7 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int len, const void *txp,
 {
        struct omap3_spi_slave *ds = to_omap3_spi(slave);
        int i;
-       int timeout = SPI_WAIT_TIMEOUT;
+       ulong start;
        int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
 
        /* Enable the channel */

@@ -241,9 +241,10 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int len, const void *txp,
 
        for (i = 0; i < len; i++) {
                /* wait till TX register is empty (TXS == 1) */
+               start = get_timer(0);
                while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                         OMAP3_MCSPI_CHSTAT_TXS)) {
-                       if (--timeout <= 0) {
+                       if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                                printf("SPI TXS timed out, status=0x%08x\n",
                                       readl(&ds->regs->channel[ds->slave.cs].chstat));
                                return -1;

@@ -280,7 +281,7 @@ int omap3_spi_read(struct spi_slave *slave, unsigned int len, void *rxp,
 {
        struct omap3_spi_slave *ds = to_omap3_spi(slave);
        int i;
-       int timeout = SPI_WAIT_TIMEOUT;
+       ulong start;
        int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
 
        /* Enable the channel */

@@ -295,10 +296,11 @@ int omap3_spi_read(struct spi_slave *slave, unsigned int len, void *rxp,
        writel(0, &ds->regs->channel[ds->slave.cs].tx);
 
        for (i = 0; i < len; i++) {
+               start = get_timer(0);
                /* Wait till RX register contains data (RXS == 1) */
                while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                         OMAP3_MCSPI_CHSTAT_RXS)) {
-                       if (--timeout <= 0) {
+                       if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                                printf("SPI RXS timed out, status=0x%08x\n",
                                       readl(&ds->regs->channel[ds->slave.cs].chstat));
                                return -1;

@@ -332,7 +334,7 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int len,
                   const void *txp, void *rxp, unsigned long flags)
 {
        struct omap3_spi_slave *ds = to_omap3_spi(slave);
-       int timeout = SPI_WAIT_TIMEOUT;
+       ulong start;
        int chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
        int irqstatus = readl(&ds->regs->irqstatus);
        int i=0;

@@ -350,9 +352,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int len,
        for (i=0; i < len; i++){
                /* Write: wait for TX empty (TXS == 1)*/
                irqstatus |= (1<< (4*(ds->slave.bus)));
+               start = get_timer(0);
                while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                         OMAP3_MCSPI_CHSTAT_TXS)) {
-                       if (--timeout <= 0) {
+                       if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                                printf("SPI TXS timed out, status=0x%08x\n",
                                       readl(&ds->regs->channel[ds->slave.cs].chstat));
                                return -1;

@@ -368,9 +371,10 @@ int omap3_spi_txrx(struct spi_slave *slave, unsigned int len,
                        writel(((u8 *)txp)[i], tx);
 
                /*Read: wait for RX containing data (RXS == 1)*/
+               start = get_timer(0);
                while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                         OMAP3_MCSPI_CHSTAT_RXS)) {
-                       if (--timeout <= 0) {
+                       if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                                printf("SPI RXS timed out, status=0x%08x\n",
                                       readl(&ds->regs->channel[ds->slave.cs].chstat));
                                return -1;
--

2.3.4

drivers/spi/omap3_spi.c +2 -1

 	for (i = 0; i < len; i++) {
 		/* wait till TX register is empty (TXS == 1) */
+		start = get_timer(0);
 		while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
 			 OMAP3_MCSPI_CHSTAT_TXS)) {
-			if (--timeout <= 0) {
+			if (get_timer(start) > SPI_WAIT_TIMEOUT) {
 				printf("SPI TXS timed out, status=0x%08x\n",
 				       readl(&ds->regs->channel[ds-

Quoted 1 line

slave.cs].chstat));

return -1;

Hi David,

<snipped for brevity>

Quoted 9 lines

drivers/spi/omap3_spi.c +2 -1

 	for (i = 0; i < len; i++) {
 		/* wait till TX register is empty (TXS == 1) */
+		start = get_timer(0);
 		while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
 			 OMAP3_MCSPI_CHSTAT_TXS)) {
-			if (--timeout <= 0) {
+			if (get_timer(start) > SPI_WAIT_TIMEOUT) {
 				printf("SPI TXS timed out, status=0x%08x\n",
 				       readl(&ds->regs->channel[ds-

Quoted 2 lines

Quoted 1 line

slave.cs].chstat));

return -1;

I have a couple of questions...

Firstly, when in SPL is there access to the get_timer() function?

Secondly, when using Falcon mode to load Linux directly from SPI (Falcon mode) then we want to maximise the throughput and save every CPU cycle we possibly can. Adding yet another function call into the for loop and hence calling it a couple of million times seems, on the face of it, like it is going to slow things down.

Quoted 13 lines · Andy Pont

Hi David,

<snipped for brevity>

Quoted 9 lines

    for (i = 0; i < len; i++) {
            /* wait till TX register is empty (TXS == 1) */

drivers/spi/omap3_spi.c +2 -1

+           start = get_timer(0);
            while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                     OMAP3_MCSPI_CHSTAT_TXS)) {
-                   if (--timeout <= 0) {
+                   if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                            printf("SPI TXS timed out, status=0x%08x\n",
                                   readl(&ds->regs->channel[ds-

Quoted 6 lines

Quoted 2 lines

Quoted 1 line

slave.cs].chstat));

                            return -1;

I have a couple of questions...

Firstly, when in SPL is there access to the get_timer() function?

We call timer_init() from board_init_r() in SPL, prior to diving down into loading (or checking for Falcon vs Regular) so this is safe.

Quoted 5 lines

Secondly, when using Falcon mode to load Linux directly from SPI (Falcon mode) then we want to maximise the throughput and save every CPU cycle we possibly can. Adding yet another function call into the for loop and hence calling it a couple of million times seems, on the face of it, like it is going to slow things down.

I'd like to see measurements to prove me wrong but this both seems like a bad idea (optimizing by being incorrect, this gives us a correct timeout check like other drivers do) and really unlikely I would think to be noticable. Since we'll be doing the same code-paths in both regular and SPL, trying to time things (by loading a big file) would be easy enough I think. Thanks!

Quoted 5 lines · On 7 April 2015 at 05:55, Tom Rini <[email protected] <javascript:;>> wrote:

Quoted 4 lines · Andy Pont

Hi David,

<snipped for brevity>

Quoted 8 lines

Quoted 8 lines

drivers/spi/omap3_spi.c int omap3_spi_write(struct spi_slave *slave, unsigned int len, const void *txp,

Quoted 8 lines

    for (i = 0; i < len; i++) {
            /* wait till TX register is empty (TXS == 1) */

drivers/spi/omap3_spi.c +2 -1

+           start = get_timer(0);
            while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                     OMAP3_MCSPI_CHSTAT_TXS)) {
-                   if (--timeout <= 0) {
+                   if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                            printf("SPI TXS timed out,

status=0x%08x\n",

Quoted 13 lines

Quoted 7 lines

Quoted 3 lines

                                   readl(&ds->regs->channel[ds-

Quoted 1 line

slave.cs].chstat));

                            return -1;

I have a couple of questions...

Firstly, when in SPL is there access to the get_timer() function?

We call timer_init() from board_init_r() in SPL, prior to diving down into loading (or checking for Falcon vs Regular) so this is safe.

Quoted 2 lines

Secondly, when using Falcon mode to load Linux directly from SPI (Falcon mode) then we want to maximise the throughput and save every CPU cycle

we

Quoted 1 line

Quoted 1 line

possibly can. Adding yet another function call into the for loop and

hence

Quoted 1 line

Quoted 1 line

calling it a couple of million times seems, on the face of it, like it

is

Quoted 8 lines

Quoted 1 line

going to slow things down.

I'd like to see measurements to prove me wrong but this both seems like a bad idea (optimizing by being incorrect, this gives us a correct timeout check like other drivers do) and really unlikely I would think to be noticable. Since we'll be doing the same code-paths in both regular and SPL, trying to time things (by loading a big file) would be easy enough I think. Thanks!

Ping

thanks!
--
Jagan.

As requested:

Tested-by David Dueck <[email protected]>

Am Freitag, 24. April 2015 schrieb Jagan Teki :

Quoted 45 lines · Tom Rini

Quoted 35 lines · Andy Pont

Quoted 14 lines

Hi David,

<snipped for brevity>

Quoted 10 lines

    for (i = 0; i < len; i++) {
            /* wait till TX register is empty (TXS == 1) */

drivers/spi/omap3_spi.c +2 -1

+           start = get_timer(0);
            while (!(readl(&ds->regs->channel[ds->slave.cs].chstat) &
                     OMAP3_MCSPI_CHSTAT_TXS)) {
-                   if (--timeout <= 0) {
+                   if (get_timer(start) > SPI_WAIT_TIMEOUT) {
                            printf("SPI TXS timed out,

status=0x%08x\n",

                                   readl(&ds->regs->channel[ds-

Quoted 6 lines

Quoted 2 lines

Quoted 1 line

slave.cs].chstat));

                            return -1;

I have a couple of questions...

Firstly, when in SPL is there access to the get_timer() function?

We call timer_init() from board_init_r() in SPL, prior to diving down into loading (or checking for Falcon vs Regular) so this is safe.

Quoted 9 lines

Secondly, when using Falcon mode to load Linux directly from SPI (Falcon
mode) then we want to maximise the throughput and save every CPU cycle we
possibly can. Adding yet another function call into the for loop and hence
calling it a couple of million times seems, on the face of it, like it is
going to slow things down.

Quoted 6 lines

I'd like to see measurements to prove me wrong but this both seems like a bad idea (optimizing by being incorrect, this gives us a correct timeout check like other drivers do) and really unlikely I would think to be noticable. Since we'll be doing the same code-paths in both regular and SPL, trying to time things (by loading a big file) would be easy enough I think. Thanks!

Ping