[U-Boot] [PATCH 3/8] ARMV7: Modify i2c driver for more reliable operation on OMAP4
Nishanth Menon
menon.nishanth at gmail.com
Thu Jul 22 01:53:16 CEST 2010
On 07/21/2010 06:41 PM, Steve Sakoman wrote:
> On Wed, Jul 21, 2010 at 4:22 PM, Nishanth Menon
> <menon.nishanth at 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 at 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.
not as a config option, but a #define in the C file so that it is
readable, the 10 makes not much sense at a glance.. but as I mentioned,
it is just a nitpick.. it just helps to quickly figure out which are the
timeouts used in the driver.. I mean we cant proceed with i2c_init or
use it as i2c master (aka no point in using the driver) if that happens
in the system rt?
>
>>>
>>> /* 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?
print might e ok here, but at this point, we are looking to check what
state we are in - master/slave if the state did not change aka
controller is in slave mode, makes no sense in trying to dump data out..
it is doomed to failure :(.. quitting with an appropriate message?
>
>>>
>>> - 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?
lets see here -> interrupt status never gets cleared OR we are in slave
mode.. my argument still stands in the case of master mode..
we can return error value for the caller of read_byte to handle, maybe
we should be using that along with a print to scare the user ;) ?
>
>>
>>> }
>>> }
>>> @@ -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.
>
Apologies on not being clear - I dont have a problem with the print as
such, BUT, do we need a diff here which changes printf ("... to
printf("... ? makes no reason why I need that for reliable OMAP4
operation :D..
> Regards,
>
> Steve
>
More information about the U-Boot
mailing list