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

Tue Oct 1 08:36:51 CEST 2013

Hello Naveen,

Am 01.10.2013 08:19, schrieb Naveen Krishna Ch:
> Hello Heiko,
>
> On 1 October 2013 11:35, Heiko Schocher<hs at denx.de>  wrote:
>> 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 at 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.
[...]
>>> 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 ...
> Will try and implement it this way.

Ok.

>>>>> 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?
> That's right, even if there is a need for  i2c_reset_port_fdt().
> it must be a i2c-core function instead of being in a driver.
>>
>>
>>>> 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:
[...]
> Thanks for this explanation.

! It is just theoretical ... you must try it ;-)

>> 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 ...
>
> Will try and do this.
> Mean while can we get the other 3 patches reviewed.
> Patch 1: http://patchwork.ozlabs.org/patch/278933/

This patch seems good to me, added to u-boot-i2c.git
as it seems it is a bugfix.

> Patch 2: http://patchwork.ozlabs.org/patch/278931/
> Patch 3: http://patchwork.ozlabs.org/patch/278930/

Think, they are OK, but they go after the next release to mainline,
as we are close to the next release ...

> They were tested without the change to the new i2c framework.

Thanks for your work!

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