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

Steve Sakoman
2010-07-21T23:41:35Z

On Wed, Jul 21, 2010 at 4:22 PM, Nishanth Menon
<menon.nishanth@gmail.com> wrote:
> 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?

I can't imagine why we would ever want to make this a config option!

In normal operation the loop is only traversed one time. I only added
the timeout to prevent the possibility of an infinite loop.

>>
>> ? ? ? /* 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?

Yes. Again this timeout was added to prevent an infinite loop hang.

Perhaps an error message if the timeout is triggered?

>>
>> - ? ? ? ? ? ? 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..

Same response -- would you like to see an error message if the timeout
is triggered?

>
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>> @@ -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?

I think so -- otherwise the i2c_write function is printing messages
claiming to come from i2c_read! Same for the other messages you
flagged.

Regards,

Steve