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

[Wolfgang Denk]

Dear Steve Sakoman,

In message <1287158873.7756.66.camel at 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_