[U-Boot] [PATCH 3/8] ARMV7: Modify i2c driver for more reliable operation on OMAP4

[Nishanth Menon <menon.nishanth@gmail.com>]

Steve,
just minor nitpick below:

On 07/21/2010 02:25 PM, Steve Sakoman wrote:
> In addition to modifying the init routine to follow the TRM
> recommendations, this patch adds a retry count to two loops
> to avoid the possibility of infinite loops.  It also corrects
> error message typos in the i2c_write routine.
>
> Signed-off-by:  Steve Sakoman<steve@sakoman.com>
> ---
>   arch/arm/include/asm/arch-omap3/i2c.h |    4 ++-
>   drivers/i2c/omap24xx_i2c.c            |   37 ++++++++++++++++++++++----------
>   drivers/i2c/omap24xx_i2c.h            |    4 +++
>   3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-omap3/i2c.h b/arch/arm/include/asm/arch-omap3/i2c.h
> index 7a4a73a..d2e7488 100644
> --- a/arch/arm/include/asm/arch-omap3/i2c.h
> +++ b/arch/arm/include/asm/arch-omap3/i2c.h
> @@ -34,7 +34,9 @@ struct i2c {
>   	unsigned short stat;	/* 0x08 */
>   	unsigned short res3;
>   	unsigned short iv;	/* 0x0C */
> -	unsigned short res4[3];
> +	unsigned short res4;
> +	unsigned short syss;	/* 0x10 */
> +	unsigned short res4a;
>   	unsigned short buf;	/* 0x14 */
>   	unsigned short res5;
>   	unsigned short cnt;	/* 0x18 */
> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
> index 3256133..a91fe55 100644
> --- a/drivers/i2c/omap24xx_i2c.c
> +++ b/drivers/i2c/omap24xx_i2c.c
> @@ -41,6 +41,7 @@ void i2c_init (int speed, int slaveadd)
>   	int psc, fsscll, fssclh;
>   	int hsscll = 0, hssclh = 0;
>   	u32 scll, sclh;
> +	int reset_timeout = 10;
I am guessing we  can life with a u8, we timeout in 10ms.. would asking 
for a #define I2C_TIMEOUT_RESET is too much here?
>
>   	/* Only handle standard, fast and high speeds */
>   	if ((speed != OMAP_I2C_STANDARD)&&
> @@ -102,15 +103,22 @@ void i2c_init (int speed, int slaveadd)
>   		sclh = (unsigned int)fssclh;
>   	}
>
> -	writew(0x2,&i2c_base->sysc); /* for ES2 after soft reset */
> -	udelay(1000);
> -	writew(0x0,&i2c_base->sysc); /* will probably self clear but */
> -
>   	if (readw (&i2c_base->con)&  I2C_CON_EN) {
>   		writew (0,&i2c_base->con);
>   		udelay (50000);
>   	}
>
> +	writew(0x2,&i2c_base->sysc); /* for ES2 after soft reset */
> +	udelay(1000);
> +
> +	writew(I2C_CON_EN,&i2c_base->con);
> +	while (!(readw(&i2c_base->syss)&  I2C_SYSS_RDONE)&&  reset_timeout--) {
> +		if (reset_timeout<= 0)
> +			printf("ERROR: Timeout in soft-reset\n");
> +		udelay(1000);
> +	}
> +
> +	writew(0,&i2c_base->con);
>   	writew(psc,&i2c_base->psc);
>   	writew(scll,&i2c_base->scll);
>   	writew(sclh,&i2c_base->sclh);
> @@ -134,6 +142,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>   {
>   	int i2c_error = 0;
>   	u16 status;
> +	u16 retries;
u8 sounds good here - this does not seem to take more than 10..
>
>   	/* wait until bus not busy */
>   	wait_for_bb ();
> @@ -159,15 +168,16 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>   	}
>
>   	if (!i2c_error) {
> -		/* free bus, otherwise we can't use a combined transction */
> -		writew (0,&i2c_base->con);
> -		while (readw (&i2c_base->stat) || (readw (&i2c_base->con)&  I2C_CON_MST)) {
> +		retries = 10;
#define sounds nice here..
> +		while ((readw(&i2c_base->stat)&  0x14) ||
> +			(readw(&i2c_base->con)&  I2C_CON_MST)) {
>   			udelay (10000);
>   			/* Have to clear pending interrupt to clear I2C_STAT */
>   			writew (0xFFFF,&i2c_base->stat);
> +			if (!retries--)
> +				break;
>   		}
do we just proceed in case of retry timeout?
>
> -		wait_for_bb ();
>   		/* set slave address */
>   		writew (devaddr,&i2c_base->sa);
>   		/* read one byte from slave */
> @@ -190,11 +200,14 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>   		}
>
>   		if (!i2c_error) {
> +			retries = 10;
>   			writew (I2C_CON_EN,&i2c_base->con);
>   			while (readw (&i2c_base->stat)
>   			       || (readw (&i2c_base->con)&  I2C_CON_MST)) {
>   				udelay (10000);
>   				writew (0xFFFF,&i2c_base->stat);
> +				if (!retries--)
> +					break;
>   			}
same comment on retry failure here..

>   		}
>   	}
> @@ -276,7 +289,7 @@ static void flush_fifo(void)
>   	 * you get a bus error
>   	 */
>   	while(1){
> -		stat = readw(&i2c_base->stat);
> +		stat = readw(&i2c_base->stat)&  I2C_STAT_RRDY;
>   		if(stat == I2C_STAT_RRDY){
>   #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
>       defined(CONFIG_OMAP44XX)
> @@ -357,18 +370,18 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
>   	int i;
>
>   	if (alen>  1) {
> -		printf ("I2C read: addr len %d not supported\n", alen);
> +		printf("I2C write: addr len %d not supported\n", alen);
err.. do we need this change?
>   		return 1;
>   	}
>
>   	if (addr + len>  256) {
> -		printf ("I2C read: address out of range\n");
> +		printf("I2C write: address out of range\n");
err.. do we need this change?
>   		return 1;
>   	}
>
>   	for (i = 0; i<  len; i++) {
>   		if (i2c_write_byte (chip, addr + i, buffer[i])) {
> -			printf ("I2C read: I/O error\n");
> +			printf("I2C write: I/O error\n");
err.. do we need this change?
>   			i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>   			return 1;
>   		}
> diff --git a/drivers/i2c/omap24xx_i2c.h b/drivers/i2c/omap24xx_i2c.h
> index 92a3416..650e33a 100644
> --- a/drivers/i2c/omap24xx_i2c.h
> +++ b/drivers/i2c/omap24xx_i2c.h
> @@ -85,6 +85,10 @@
>   #define I2C_SYSTEST_SDA_I	(1<<  1)  /* SDA line sense input value */
>   #define I2C_SYSTEST_SDA_O	(1<<  0)  /* SDA line drive output value */
>
> +/* I2C System Status Register (I2C_SYSS): */
> +
> +#define I2C_SYSS_RDONE          (1<<  0)  /* Internel reset monitoring */
> +
>   #define I2C_SCLL_SCLL		0
>   #define I2C_SCLL_SCLL_M		0xFF
>   #define I2C_SCLL_HSSCLL		8

Regards,
Nishanth Menon