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

[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.
>>
>> a). We have different number of channels on Exynos5250 and Exynos5420
>>     (as explained above). How are we supposed to define the
>> U_BOOT_I2C_ADAP_COMPLETE
>>     in such cases. Can i use #ifdef EXYNOS5420 or EXYNOS5250
>
>
> From my side, Yes.
Ok, Will do accordingly.
But, I would prefer a way of integrating it to FDT or passing as
platform data (board files)
instead of keeping it in the driver.

>
>
>> 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 ??

>
>
>> 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()

IMHO, i2c_reset_port_fdt() is the only function to be discussed
>
> 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.
>
> int i2c_get_bus_num_fdt(int node) should be integrated to
> drivers/i2c/i2c_core.c
>
> For what purpose is i2c_reset_port_fdt() ?
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Thanks,
-- 
Shine bright,
(: Nav :)