[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Heiko Schocher]

Hello Dirk,

Dirk Behme wrote:
> Tom Rix wrote:
>> From: Syed Mohammed Khasim <khasim at ti.com>
>>
>> This was cherry-picked from
>>
>> repo: http://www.beagleboard.org/u-boot-arm.git
>> commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
>>
>> In addition to adding multibus support, this patch
>> also cleans up the register access.  The register
>> access has been changed from #defines to a structure.
> 
> Have you looked at my proposal I sent some hours before your patch?
> 
> http://lists.denx.de/pipermail/u-boot/2009-October/063556.html
> 
>> Signed-off-by: Syed Mohammed Khasim <khasim at ti.com>
>> Signed-off-by: Jason Kridner <jkridner at beagleboard.org>
>> Signed-off-by: Tom Rix <Tom.Rix at windriver.com>
>> ---
>>  drivers/i2c/omap24xx_i2c.c          |  220
>> ++++++++++++++++++++++-------------
>>  include/asm-arm/arch-omap24xx/i2c.h |   52 ++++++---
>>  include/asm-arm/arch-omap3/i2c.h    |   48 +++++---
>>  include/configs/omap3_beagle.h      |    1 +
>>  4 files changed, 209 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
>> index 1a4c8c9..763d2f8 100644
>> --- a/drivers/i2c/omap24xx_i2c.c
>> +++ b/drivers/i2c/omap24xx_i2c.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Basic I2C functions
>>   *
>> - * Copyright (c) 2004 Texas Instruments
>> + * Copyright (c) 2004, 2009 Texas Instruments
>>   *
>>   * This package is free software;  you can redistribute it and/or
>>   * modify it under the terms of the license found in the file
>> @@ -25,10 +25,18 @@
>>  #include <asm/arch/i2c.h>
>>  #include <asm/io.h>
>>  
>> +#ifdef CONFIG_OMAP34XX
>> +#define I2C_NUM_IF 3
>> +#else
>> +#define I2C_NUM_IF 2
>> +#endif
> 
> I prefer something like I2C_BUS_MAX for this. And move it to header
> file. Moving it OMAP2 and OMAP3 i2c.h headers will remove the #ifdef, too.

Yep, I2C_BUS_MAX would be better.


[...]
>> @@ -398,8 +406,58 @@ static u16 wait_for_pin (void)
>>  
>>      if (timeout <= 0) {
>>          printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
>> -            readw (I2C_STAT));
>> -            writew(0xFFFF, I2C_STAT);
>> +            readw(&i2c->stat));
>> +            writew(0xFFFF, &i2c->stat);
>>  }
>>      return status;
>>  }
>> +
>> +int i2c_set_bus_num(unsigned int bus)
> 
> Do we need an extern declaration in i2c.h for this? To be able to call
> it from somewhere else without warning?

Hmm.. isn;t it declared in i2c.h?

>> +{
>> +    if ((bus < 0) || (bus >= I2C_NUM_IF)) {
> 
> As mentioned above, I'd like something like bus max here.
> 
>> +        printf("Bad bus ID-%d\n", bus);
>> +        return -1;
>> +    }
>> +
>> +#if defined(CONFIG_OMAP34XX)
>> +    if (bus == 2)
>> +        i2c = (i2c_t *)I2C_BASE3;
>> +    else
>> +#endif
>> +    if (bus == 1)
>> +        i2c = (i2c_t *)I2C_BASE2;
>> +    else
>> +        i2c = (i2c_t *)I2C_BASE1;
>> +
>> +    return 0;
>> +}
> 
> I would remove the following three functions as they are not needed at
> the moment (i.e. without command line interface).

Hmm... really? If someone uses multibus support,
this functions are needed, or?

>> +unsigned int i2c_get_bus_num(void)
>> +{
>> +    if (i2c == (i2c_t *)I2C_BASE1)
>> +        return 0;
>> +    else
>> +    if (i2c == (i2c_t *)I2C_BASE2)
>> +        return 1;
>> +#if defined(CONFIG_OMAP34XX)
>> +    else
>> +    if (i2c == (i2c_t *)I2C_BASE3)
>> +        return 2;
>> +#endif
>> +    else
>> +        return 0xFFFFFFFF;
>> +}
>> +
>> +/*
>> + * To be Implemented
>> + */
>> +int i2c_set_bus_speed(unsigned int speed)
>> +{
>> +    return 0;
>> +}
>> +
>> +unsigned int i2c_get_bus_speed(void)
>> +{
>> +    return 0;
>> +}
>> +
[...]
>> diff --git a/include/configs/omap3_beagle.h
>> b/include/configs/omap3_beagle.h
>> index 19a5ec9..d5a0d49 100644
>> --- a/include/configs/omap3_beagle.h
>> +++ b/include/configs/omap3_beagle.h
>> @@ -128,6 +128,7 @@
>>  #define CONFIG_SYS_I2C_BUS        0
>>  #define CONFIG_SYS_I2C_BUS_SELECT    1
>>  #define CONFIG_DRIVER_OMAP34XX_I2C    1
>> +#define CONFIG_I2C_MULTI_BUS        1
> 
> Not needed.
> 
> 
> While all above are only style questions, what's about the main
> functionality topic:
> 
> Do we have to call i2c_init() for bus 1 and 2 if switching to it? If
> yes, who does it? For bus 0 it's already in the code. I'd like that the
> user doesn't have to care about it. Therefore, I added

I solved this in the multibus_v2 branch, see:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2

i2c_init() gets just called if switching to an I2C bus.
This is done in i2c_set_bus_num()

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blob;f=drivers/i2c/i2c_core.c;h=e1bb67f1c136f5904c195d31d2916b17a30c9632;hb=6ef1bb6e9a0c03d5cbc8ef296b5022b5698207a6

pro:

a I2C adapter gets only initialized, if used, and there
is the option to extend this functionality to:
- when switching to another i2c adapter call (a not yet existing)
  i2c adapter deinit function ...

> static unsigned int bus_initialized[3] = {0, 0, 0};
> static unsigned int current_bus = 0;
> 
> void i2c_init (int speed, int slaveadd) {
> ...
> bus_initialized[current_bus] = 1;
> }
> 
> int i2c_set_bus_num(unsigned int bus) {
> ...
> current_bus = bus;
> 
> if(!bus_initialized[current_bus])
>     i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> ...
> }

Ah, you also tend to this direction ;-) Great!

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany