From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Wed, 21 Jul 2010 18:22:16 -0500 Subject: [U-Boot] [PATCH 3/8] ARMV7: Modify i2c driver for more reliable operation on OMAP4 In-Reply-To: <1279740329-11882-4-git-send-email-steve@sakoman.com> References: <1279740329-11882-1-git-send-email-steve@sakoman.com> <1279740329-11882-4-git-send-email-steve@sakoman.com> Message-ID: <4C478128.4000201@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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