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

Naveen Krishna Ch
2013-09-30T10:03:49Z

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.
>>
>> 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 :)