[U-Boot] [PATCH 0/3] i2c: improve s3c24x0 with High-speed and new SYS_I2C framework support

[Heiko Schocher <hs@denx.de>]

Hello Naveen,

Am 30.09.2013 12:03, schrieb Naveen Krishna Ch:
> Helo Heiko,
>
> Thanks for timely reply.
>
> On 30 September 2013 13:35, Heiko Schocher<hs@denx.de>  wrote:
>> Hello Naveen,
>>
>> Am 30.09.2013 08:58, schrieb Naveen Krishna Chatradhi:
>>
>>> This patchset fixes few bugs in the existing s3c24x0.c code (standard i2c)
>>> and add support for High-speed i2c bus controller available on Exynos5
>>> SoCs
>>> from Samsung.
>>>
>>> Exynos5250 channels [0 ~ 3] can configured for I2C contoller or
>>>              new HSI2C controller
>>>              channels [3 ~ 7] are standard I2C controller channels
>>> Exynos5420 channels [0 ~ 3] are standard I2C controller channels and
>>>              channels [4 ~ 10] are High-speed controller channels
>>>
>>> Patchset:
>>> 1.  exynos: i2c: Fix i2c driver to handle NACKs properly
>>>       Improvements and fixes from Vadim Bendebury for standard i2c calls
>>>
>>> 2.  exynos: i2c: Change FDT bus setup code to enumerate ports correctly
>>>       FDT bus setup code from Simon Glass
>>>
>>> 3.  PATCH v5: i2c: s3c24xx: add hsi2c controller support
>>>       High-speed controller register description and defines new i2c
>>> read/write,
>>>       probe and set_bus calls.
>>>
>>>       This is been reviewed earlier at
>>>       http://lists.denx.de/pipermail/u-boot/2013-May/153245.html
>>>       Thanks for review and improvements from Vadim Bendebury.
>>>
>>> Question:
>>> 4. RFC: samsung: i2c: Enable new CONFIG_SYS_I2C framework
>>> I've tried to implement the new I2C multi bus framework in u-boot tree,
>>> taking tegra-i2c.c as reference.
[...]
>>> b). When i define 11 buses as in the case of Exynos5420, the "i2c bus"
>>> lists them
>>>    SMDK5420 # i2c bus
>>>    Bus 0:  s3c0
>>>    Bus 1:  s3c1
>>>    Bus 2:  s3c10
>>>    Bus 3:  s3c2
>>>    Bus 4:  s3c3
>>>    Bus 5:  s3c4
>>>    Bus 6:  s3c5
>>>    Bus 7:  s3c6
>>>    Bus 8:  s3c7
>>>    Bus 9:  s3c8
>>>    Bus 10: s3c9
>>>    or  (If i change the name to hsi2c)
>>>    SMDK5420 # i2c bus
>>>    Bus 0:  hsi2c10
>>>    Bus 1:  hsi2c4
>>>    Bus 2:  hsi2c5
>>>    Bus 3:  hsi2c6
>>>    Bus 4:  hsi2c7
>>>    Bus 5:  hsi2c8
>>>    Bus 6:  hsi2c9
>>>    Bus 7:  s3c0
>>>    Bus 8:  s3c1
>>>    Bus 9:  s3c2
>>>    Bus 10: s3c3
>>>
>>> Whats the expected behaviour. If the above result is correct, I need to
>>> changei
>>> the strings to get them in the correct order.
>>
>>
>> What, if you use two digits:
>>
>>    Bus 0:  hsi2c01
>>    Bus 1:  hsi2c02
>>    [...]
>>    Bus 7:  s3c00
>>    Bus 8:  s3c01
>>    [...]
>>
>> ?
>>
>> Or with one digit:
>>
>>    Bus 0:  hsi2c1
>>    Bus 1:  hsi2c2
>>    [...]
>>
>>    Bus 7:  s3c0
>>    Bus 8:  s3c1
>>    [...]
> In the Exynos5420 hardware
> channels are as below
>
> channel:  --0----1----2----3------4--------5---------6-------7--------8-------9-------10
> controller: i2c, i2c, i2c, i2c, hsi2c, hsi2c, hsi2c, hsi2c, hsi2c, hsi2c, hsi2c.
> But the "i2c bus" command is showing the bus number according to the "name"
> string comparison.  Which seems wrong. Isn't it ??

Hmm.. you are right, seems that the compiler sorts them alphabetical ...
So, two ways:
- use another name, saying first a two digit for the channel ?
- or, use CONFIG_SYS_NUM_I2C_BUSES, CONFIG_SYS_I2C_DIRECT_BUS
   and CONFIG_SYS_I2C_BUSES as described in the README (grep
   for CONFIG_SYS_I2C_BUSES in include/configs and you will find
   some examples ...) and you can define, which adapter is on which
   i2c_bus number ...

>>> c). What's the alternative for the
>>>      board_i2c_init(), i2c_get_bus_num_fdt(), i2c_reset_port_fdt().
>>>     "Functions to get the I2C bus number and reset I2C bus using FDT node"
>>>
>>>      I think, these functions are still needed.
>>
>>
>> Hmm.. that needs a general discussion, how to use fdt and i2c I think.
>>
>> I would prefer a way (not really nowing, if it is possible for all
>> configurations) where, if using fdt, the DT gets parsed and the
>> availiable i2c buses gets created ... After that, "normal" i2c access
>> with i2c_set_bus_num(), i2c_read/write should be possible ... this is
>> currently not possible with the i2c framework, but should not be so hard
>> to do. Except the restriction, that it would not work in SPL case, or
>> before relocation for i2c buses announced through dt
> once i2c_init_board() is done board_i2c_init() is not quite needed
> using i2c_init_board we can avoid the relocation problem aswell.
>
> by the definition of i2c_get_bus_num() in drivers/i2c/i2c_core.c
>
> unsigned int i2c_get_bus_num(void)
> {
>          return gd->cur_i2c_bus;
> }
>
> we don't need a special function i2c_get_bus_num_fdt()

Ah, ok!

> IMHO, i2c_reset_port_fdt() is the only function to be discussed

And why is i2c_init() not good? Why must we have here a new function?

>> Define CONFIG_SYS_I2C_MAX_HOPS ->  CONFIG_SYS_I2C_DIRECT_BUS is not defined
>> so i2c_bus[] is used in drivers/i2c/i2c_core.c. Define the fix i2c buses in
>> the board config file with CONFIG_SYS_I2C_BUSES (if you have no fix buses,
>> let this empty) and add a function in drivers/i2c/i2c_core.c, which adds
>> new i2c buses to i2c_bus[] after the fix buses and call this new function,
>> from where you interpret the fdt ...
> I din't quite understood this.
> Can you point me to some readme or Doc or discussion Please.

just the U-Boot README ... The above was just a fast idea, how it
is possible to add i2c buses from the info in the fdt ...

Here some theoretical code, how it could look like:

in the board config file add:

#define CONFIG_SYS_I2C_DIRECT_BUS       1
#define CONFIG_SYS_I2C_MAX_HOPS         0
#define CONFIG_SYS_NUM_I2C_BUSES	10 /* maximum possible i2c buses used on the hw */
#define CONFIG_SYS_FIX_I2C_BUSES	1 /* just as an example, here with one fix i2c bus */
#define CONFIG_SYS_I2C_BUSES    {       {0}, /* adapter 0 is fix on i2c_bus number 0 */
                                 }

in drivers/i2c/i2c_core.c:

static int i2c_fix_bus = CONFIG_SYS_FIX_I2C_BUSES;

/* add one i2c bus without muxes ... ToDo: how to add i2c buses with muxes */
static int i2c_add_one_bus(char *adapter_name)
{
	struct i2c_bus_hose *tmp;

	if (i2c_fix_bus >= CONFIG_SYS_NUM_I2C_BUSES)
		return -ENOMEM;

	/* search adapter with name */
	adap = i2c_search_adapter_name(name); /* Code this function */
	if (adap < 0)
		return -ENODEV;

	tmp = kalloc(sizeof(struct i2c_bus_hose));
	tmp->adapter = adap;
	/* if found add it into i2c_bus */
	memcpy(&i2c_bus[i2c_fix_bus], tmp, sizeof(struct i2c_bus_hose));
	i2c_fix_bus++;
}

And maybe here and there some adaptions for getting this running...
Thinking of i2c_set_bus_num(), there must be now a check for
i2c_fix_bus I think ...

Adapt the README ...

And then, from the place where you interpret the fdt, call if
you have the information for one i2c bus, i2c_add_one_bus() ...

Not sure, if I overlooked here something ...

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