[U-Boot] [PATCH RFC] ARMV7: OMAP: I2C driver: Reduce excessively long udelay calls

[U-Boot] [PATCH RFC] ARMV7: OMAP: I2C driver: Reduce excessively long udelay calls

[Steve Sakoman]

I've been preparing a patch series for Beagle and Overo that detects expansion
board configuration information by reading a 128 byte I2C EEPROM on each
expansion board.

Using the OMAP I2C driver in its current form takes about 5-6 seconds to read
this small number of bytes!

Examining the code I see that there are a large number of fairly long udelay calls
throughout the driver (10 - 50 milliseconds). I looked through the linux driver
and did not see equivalent delays in that code.  In fact the longest delay in the
linux code was one millisecond.

In looking at the TRM I2C section for OMAP3 and OMAP4 I don't see any requirement
for delays in the programming model description.

In a series of test builds I found that (at least on OMAP3 and OMAP4) I could
eliminate just about all of these delays.

I don't have access to earlier OMAP family hardware, so I don't know for certain
that they do or do not require these delays.

This patch takes the conservative approach of shortening all the existing delays
to 1 msec, to match the few Linux driver delays.  It also modifies the driver's
wait_for_bb routine to timeout after 1 second, again to match the Linux driver
behavior.

Tested on OMAP3 and OMAP4 -- the EEPROM read is now instantaneous, as is the i2c probe
command, whaich also used to be extremely slow.

Comments welcome, as well as test results from those who might have access to old OMAP
hardware.

Steve



diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index 3febd1f..c3a25df 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -27,7 +27,7 @@
 
 #include "omap24xx_i2c.h"
 
-#define I2C_TIMEOUT	10
+#define I2C_TIMEOUT	1000
 
 static void wait_for_bb (void);
 static u16 wait_for_pin (void);
@@ -108,7 +108,7 @@ void i2c_init (int speed, int slaveadd)
 
 	if (readw (&i2c_base->con) & I2C_CON_EN) {
 		writew (0, &i2c_base->con);
-		udelay (50000);
+		udelay (1000);
 	}
 
 	writew(0x2, &i2c_base->sysc); /* for ES2 after soft reset */
@@ -164,7 +164,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 	if (status & I2C_STAT_XRDY) {
 		/* Important: have to use byte access */
 		writeb (regoffset, &i2c_base->data);
-		udelay (20000);
+		udelay (1000);
 		if (readw (&i2c_base->stat) & I2C_STAT_NACK) {
 			i2c_error = 1;
 		}
@@ -176,7 +176,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 		writew (I2C_CON_EN, &i2c_base->con);
 		while (readw(&i2c_base->stat) &
 			(I2C_STAT_XRDY | I2C_STAT_ARDY)) {
-			udelay (10000);
+			udelay (1000);
 			/* Have to clear pending interrupt to clear I2C_STAT */
 			writew (0xFFFF, &i2c_base->stat);
 		}
@@ -197,7 +197,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 #else
 			*value = readw (&i2c_base->data);
 #endif
-			udelay (20000);
+			udelay (1000);
 		} else {
 			i2c_error = 1;
 		}
@@ -206,7 +206,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 			writew (I2C_CON_EN, &i2c_base->con);
 			while (readw (&i2c_base->stat) &
 				(I2C_STAT_RRDY | I2C_STAT_ARDY)) {
-				udelay (10000);
+				udelay (1000);
 				writew (0xFFFF, &i2c_base->stat);
 			}
 		}
@@ -256,7 +256,7 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 		writew ((value << 8) + regoffset, &i2c_base->data);
 #endif
 		/* must have enough delay to allow BB bit to go low */
-		udelay (50000);
+		udelay (1000);
 		if (readw (&i2c_base->stat) & I2C_STAT_NACK) {
 			i2c_error = 1;
 		}
@@ -322,7 +322,7 @@ int i2c_probe (uchar chip)
 	/* stop bit needed here */
 	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c_base->con);
 	/* enough delay for the NACK bit set */
-	udelay (50000);
+	udelay (1000);
 
 	if (!(readw (&i2c_base->stat) & I2C_STAT_NACK)) {
 		res = 0;      /* success case */
@@ -331,7 +331,7 @@ int i2c_probe (uchar chip)
 	} else {
 		writew(0xFFFF, &i2c_base->stat);	 /* failue, clear sources*/
 		writew (readw (&i2c_base->con) | I2C_CON_STP, &i2c_base->con); /* finish up xfer */
-		udelay(20000);
+		udelay(1000);
 		wait_for_bb ();
 	}
 	flush_fifo();
@@ -392,13 +392,13 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
 
 static void wait_for_bb (void)
 {
-	int timeout = 10;
+	int timeout = I2C_TIMEOUT;
 	u16 stat;
 
 	writew(0xFFFF, &i2c_base->stat);	 /* clear current interruts...*/
 	while ((stat = readw (&i2c_base->stat) & I2C_STAT_BB) && timeout--) {
 		writew (stat, &i2c_base->stat);
-		udelay (50000);
+		udelay (1000);
 	}
 
 	if (timeout <= 0) {
@@ -411,7 +411,7 @@ static void wait_for_bb (void)
 static u16 wait_for_pin (void)
 {
 	u16 status;
-	int timeout = 10;
+	int timeout = I2C_TIMEOUT;
 
 	do {
 		udelay (1000);

[U-Boot] [PATCH RFC] ARMV7: OMAP: I2C driver: Reduce excessively long udelay calls

[Wolfgang Denk]

Dear Steve Sakoman,

In message <1287158873.7756.66.camel@quadra> you wrote:
> I've been preparing a patch series for Beagle and Overo that detects expansion
> board configuration information by reading a 128 byte I2C EEPROM on each
> expansion board.
> 
> Using the OMAP I2C driver in its current form takes about 5-6 seconds to read
> this small number of bytes!
> 
> Examining the code I see that there are a large number of fairly long udelay calls
> throughout the driver (10 - 50 milliseconds). I looked through the linux driver
> and did not see equivalent delays in that code.  In fact the longest delay in the
> linux code was one millisecond.

Eventually the long delays are needed to make the code work stable
even under rare conditions, which don't happen to often.  But this may
be caused by the way the code is written, too...


>  	if (readw (&i2c_base->con) & I2C_CON_EN) {
>  		writew (0, &i2c_base->con);
> -		udelay (50000);
> +		udelay (1000);
>  	}

Instead of a simple writew() followed by an eventually long delay it
might make more sense to wait in a (tight) loop for the intended
condition; in this case it looks as if the I2C_CON_EN bit should be
reset (plus some other bits?), so why not wait for it?

For example:

	tout = 1000;
	while ((tout > 0) && (readw (&i2c_base->con) & I2C_CON_EN)) {
		writew(0, &i2c_base->con);
		udelay (50);
	}
	if (!tout)
		error handling;

So if the hardware is really slow to respond you still get the fill 50
ms timeout that you had before, but if it's much faster in the regular
case - and you even get a chance to notify the user in case of errors.

[Not sure if the multiple writew() are ok here, but you get the idea.]

>  	writew(0x2, &i2c_base->sysc); /* for ES2 after soft reset */
> @@ -164,7 +164,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  	if (status & I2C_STAT_XRDY) {
>  		/* Important: have to use byte access */
>  		writeb (regoffset, &i2c_base->data);
> -		udelay (20000);
> +		udelay (1000);
>  		if (readw (&i2c_base->stat) & I2C_STAT_NACK) {
>  			i2c_error = 1;
>  		}

Here the harrdware spec should give an indication how quickly the NAK
can be read reliably.

> @@ -176,7 +176,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  		writew (I2C_CON_EN, &i2c_base->con);
>  		while (readw(&i2c_base->stat) &
>  			(I2C_STAT_XRDY | I2C_STAT_ARDY)) {
> -			udelay (10000);
> +			udelay (1000);
>  			/* Have to clear pending interrupt to clear I2C_STAT */
>  			writew (0xFFFF, &i2c_base->stat);
>  		}

Here I cannot think of good reasons for such a delay at all.
What does the spec say?


> @@ -206,7 +206,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  			writew (I2C_CON_EN, &i2c_base->con);
>  			while (readw (&i2c_base->stat) &
>  				(I2C_STAT_RRDY | I2C_STAT_ARDY)) {
> -				udelay (10000);
> +				udelay (1000);
>  				writew (0xFFFF, &i2c_base->stat);
>  			}
>  		}

Here we are in a loop anyway - use a much shrter cycle length combined
with a limit on the max number of cycles.


Etc.

My feeling is that this driver needs a more thorough rework.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Winners never talk about glorious victories. That's  because  they're
the  ones  who  see  what the battlefield looks like afterwards. It's
only the losers who have glorious victories.
                                      - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH RFC] ARMV7: OMAP: I2C driver: Reduce excessively long udelay calls

[Steve Sakoman]

On Fri, 2010-10-15 at 21:27 +0200, Wolfgang Denk wrote:
> Dear Steve Sakoman,
> 
> In message <1287158873.7756.66.camel@quadra> you wrote:
> > I've been preparing a patch series for Beagle and Overo that detects expansion
> > board configuration information by reading a 128 byte I2C EEPROM on each
> > expansion board.
> > 
> > Using the OMAP I2C driver in its current form takes about 5-6 seconds to read
> > this small number of bytes!
> > 
> > Examining the code I see that there are a large number of fairly long udelay calls
> > throughout the driver (10 - 50 milliseconds). I looked through the linux driver
> > and did not see equivalent delays in that code.  In fact the longest delay in the
> > linux code was one millisecond.
> 
> Eventually the long delays are needed to make the code work stable
> even under rare conditions, which don't happen to often.  But this may
> be caused by the way the code is written, too...

<snip>

> My feeling is that this driver needs a more thorough rework.

Unfortunately I am inclined to agree with you!

I'll start work on that.

Steve