[U-Boot] i2c: samsung: register i2c busses for Exynso5420 and Exynos5250

[Heiko Schocher]

Hello Naveen,

Am 06.12.2013 06:47, schrieb Naveen Krishna Ch:
> Hello Heiko,
>
> On 5 December 2013 18:17, Heiko Schocher<hs at denx.de>  wrote:
>> Hello Naveen,
>>
>> Am 05.12.2013 13:21, schrieb Naveen Krishna Ch:
>>
>>> Hello Heiko,
>>>
>>> On 5 December 2013 16:56, Heiko Schocher<hs at denx.de>   wrote:
>>>>
>>>> Hello Naveen,
>>>>
>>>> Sorry for the late reply ...
>>>>
>>>> Am 25.11.2013 12:28, schrieb Naveen Krishna Ch:
>>>>
>>>>> This patch adds the U_BOOT_I2C_ADAP_COMPLETE defines for channels
>>>>> on Exynos5420 and Exynos5250 and also adds support for init function
>>>>> for hsi2c channels
>>>>>
>>>>> Signed-off-by: Naveen Krishna Chatradhi<ch.naveen at samsung.com>
>>>>>
>>>>> ---
>>>>> README                    |    6 ++
>>>>>     drivers/i2c/s3c24x0_i2c.c |  222
>>>>> +++++++++++++++++++++++++++++++++++----------
>>>>>     2 files changed, 180 insertions(+), 48 deletions(-)
>>>>>
>>>>> diff --git a/README b/README
>>>>> index c97ff0a..c04e352 100644
>>>>> --- a/README
>>>>> +++ b/README
>>>>> @@ -2076,6 +2076,12 @@ CBFS (Coreboot Filesystem) support
>>>>>                     - set CONFIG_SYS_I2C_ZYNQ_SPEED for speed setting
>>>>>                     - set CONFIG_SYS_I2C_ZYNQ_SLAVE for slave addr
>>>>>
>>>>> +               - drivers/i2c/s3c24x0_i2c.c:
>>>>> +                 - activate this driver with CONFIG_SYS_I2C_S3C24X0
>>>>> +                 - This driver adds i2c buses (11 for Exynos5250,
>>>>> Exynos5420
>>>>> +                   9 i2c buses for Exynos4 and 1 for S3C24X0 SoCs from
>>>>> Samsung)
>>>>> +                   with a fix speed from 100000 and the slave addr 0!
>>>>> +
>>>>>                   additional defines:
>>>>>
>>>>>                   CONFIG_SYS_NUM_I2C_BUSES
>>>>> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
>>>>> index 1e9dba0..6e719ec 100644
>>>>> --- a/drivers/i2c/s3c24x0_i2c.c
>>>>> +++ b/drivers/i2c/s3c24x0_i2c.c
>>>>> @@ -721,6 +721,15 @@ static unsigned int
>>>>> s3c24x0_i2c_set_bus_speed(struct
>>>>> i2c_adapter *adap,
>>>>>           return 0;
>>>>>     }
>>>>>
>>>>> +static void exynos_i2c_init(struct i2c_adapter *adap, int speed, int
>>>>> slaveaddr)
>>>>> +{
>>>>> +       /* This will override the speed selected in the fdt for that
>>>>> port
>>>>> */
>>>>> +       debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
>>>>> +       if (i2c_set_bus_speed(speed))
>>>>> +               printf("i2c_init: failed to init bus %d for speed =
>>>>> %d\n",
>>>>> +                                               adap->hwadapnr, speed);
>>>>> +}
>>>
>>> This init is not used for Exynos4 and lower series SoCs
>>>>>
>>>>> +
>>>>>     /*
>>>>>      * cmd_type is 0 for write, 1 for read.
>>>>>      *
>>>>> @@ -1071,51 +1080,168 @@ int i2c_reset_port_fdt(const void *blob, int
>>>>> node)
>>>>>     /*
>>>>>      * Register s3c24x0 i2c adapters
>>>>>      */
>>>>> -U_BOOT_I2C_ADAP_COMPLETE(s3c24x0_0, s3c24x0_i2c_init,
>>>>> s3c24x0_i2c_probe,
[...]
>>>>> +U_BOOT_I2C_ADAP_COMPLETE(i2c08, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>>> +                       s3c24x0_i2c_read, s3c24x0_i2c_write,
>>>>> +                       s3c24x0_i2c_set_bus_speed,
>>>>> +                       CONFIG_SYS_I2C_S3C24X0_SPEED,
>>>>> +                       CONFIG_SYS_I2C_S3C24X0_SLAVE, 8
> Is missing the closing braces. My local changes has it so i couldnt
> see the problem.

Ups?

>>>> leads in a compile error for trats and trats2, could you please fix?
>>>
>>> I ran "MAKEALL -v samsung"
>>> s3c24x0_i2c.c:724:13: warning: 'exynos_i2c_init' defined but not used
>>> [-Wunused-function]
>>> is the warning seen for trats and trats2 boards
>>> If you are talking about this one, will fix it in the next versio.
>>
>>
>> No, there is a missing ")" at the end!
>>
>> pollux:u-boot hs [master] $ ./MAKEALL trats
>> Configuring for trats board...
>> s3c24x0_i2c.c:1247:0: error: unterminated argument list invoking macro
>> "U_BOOT_I2C_ADAP_COMPLETE"
>> s3c24x0_i2c.c:1236:1: error: expected '=', ',', ';', 'asm' or
>> '__attribute__' at end of input
>> arm-linux-gnueabi-size: './u-boot': No such file
>> s3c24x0_i2c.c:1247:0: error: unterminated argument list invoking macro
>> "U_BOOT_I2C_ADAP_COMPLETE"
>> s3c24x0_i2c.c:1236:1: error: expected '=', ',', ';', 'asm' or
>> '__attribute__' at end of input
>>
>> s3c24x0_i2c.c:724:13: warning: 'exynos_i2c_init' defined but not used
>> [-Wunused-function]
>> make[1]: *** [s3c24x0_i2c.o] Fehler 1
>> make[1]: *** Warte auf noch nicht beendete Prozesse...
>> make: *** [drivers/i2c/built-in.o] Fehler 2
>> make: *** Warte auf noch nicht beendete Prozesse...
>>
>> --------------------- SUMMARY ----------------------------
>> Boards compiled: 1
>> Boards with errors: 1 ( trats )
>> ----------------------------------------------------------
> #pwclient git-am 292705
> #pwclient git-am 293889
> #pwclient git-am 292706
> #./MAKEALL trats
> Shows me this errors. Thanks for the catch.

Puuh...

>> pollux:u-boot hs [master] $
>>
>> pollux:u-boot hs [(kein Zweig, Neuaufbau begonnen bei master)] $ git bisect
>> log
>> git bisect start
>> # bad: [f5f40408ce3b00637c652fdabfb6f3c4e0d09635] arm: omap: i2c: don't zero
>> cnt in i2c_write
>> git bisect bad f5f40408ce3b00637c652fdabfb6f3c4e0d09635
>> # good: [f44483b57c49282299da0e5c10073b909cdad979] Merge branch 'serial' of
>> git://git.denx.de/u-boot-microblaze
>> git bisect good f44483b57c49282299da0e5c10073b909cdad979
>> # bad: [4d06a7c5707f813f4bc7fd7a9d700606c63fa4de] i2c: fti2c010: cosmetic:
>> coding style cleanup
>> git bisect bad 4d06a7c5707f813f4bc7fd7a9d700606c63fa4de
>> # good: [a6756bbdac43474193472b43309626e2c28d8100] driver:i2c:s3c24x0: fix
>> clock init for hsi2c
>> git bisect good a6756bbdac43474193472b43309626e2c28d8100
>> # bad: [c3b6bba20862ff6476c99aa6d3168f7097a5b4b2] i2c: samsung: register i2c
>> busses for Exynso5420 and Exynos5250
>> git bisect bad c3b6bba20862ff6476c99aa6d3168f7097a5b4b2
>> # first bad commit: [c3b6bba20862ff6476c99aa6d3168f7097a5b4b2] i2c: samsung:
>> register i2c busses for Exynso5420 and Exynos5250
>> pollux:u-boot hs [(kein Zweig, Neuaufbau begonnen bei master)] $
>>
>>
>>>>
>>>> Hmm... could you reformat this very long list in something like this:
>>>>
>>>> U_BOOT_I2C_ADAP_COMPLETE(s3c0, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>> if defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5420) ||
>>>> defined(CONFIG_EXYNOS5420)
>>>> U_BOOT_I2C_ADAP_COMPLETE(s3c1, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>> [...]
>>>> U_BOOT_I2C_ADAP_COMPLETE(s3c8, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>> #endif
>>>> if defined(CONFIG_EXYNOS5420) || defined(CONFIG_EXYNOS5420)
>>>> U_BOOT_I2C_ADAP_COMPLETE(s3c9, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>> U_BOOT_I2C_ADAP_COMPLETE(s3c10, s3c24x0_i2c_init, s3c24x0_i2c_probe,
>>>> #endif
> Even if we implement a common init function and reduce this list. The
> speeds would
> be fixed and the user need to set the speeds by calling i2c speed or
> the corresponding
> function.

Ok, I see... so please just sent the compileerrorfree patch, thanks!

>>> Basically, s3c24x0_i2c.c now support both HighSpeed I2C modules and
>>> Standard I2C
>>> Modules. They have different init functions.
>>> Exynos5250/5260 has 4 hsi2c channels hsi2c 0 ~ 3 and 7 i2c channels 4 ~ 10
>>> Exynos5420 has 4 i2c channels i2c 0 ~ 3 and 7 hsi2c channels 4 ~ 10
>>> Exynos4 has 9 i2c channels
>>> Hence, the long list. It would be helpful if anyone can suggest a way
>>> to reduce this list.
>>
>>
>> Ah, now I see it, thanks!
>>
>> Maybe we can define one s3c24x0_i2c_init function, and call from there
>> the SoC / adapater specific init func? So we can prevent this long list?
> I can think of something like this
> +static void samsung_i2c_init(struct i2c_adapter *adap, int speed, int slaveadd)
> +{
> +
> +#ifdef CONFIG_EXYNOS5
> +#ifdef CONFIG_EXYNOS5420
> +       if(adap->hwadapnr>  3)
> +               s3c24x0_i2c_init(adap, speed, slaveadd);
> +       else
> +               exynos_i2c_init(adap, speed, slaveadd);
> +#elif defined(CONFIG_EXYNOS5250) || defined(CONFIG_EXYNOS5260)
> +       if(adap->hwadapnr>  3)
> +               exynos_i2c_init(adap, speed, slaveadd);
> +       else
> +               s3c24x0_i2c_init(adap, speed, slaveadd);
> +#endif
> +#else
> +       s3c24x0_i2c_init(adap, speed, slaveadd);
> +#endif
> +}
>
> But, as mentioned earlier the speeds needs to be set from a seperate function
> for each board.
>>
>>
>>> Is U_BOOT_I2C_ADAP_COMPLETE is the only (best) way to register to new i2c
>>> framework ?
>
>>
>>
>> Yes.
> Is there any work going on to simplyfy U_BOOT_I2C_ADAP_COMPLETE ?

No, what do you thinking of? Simplify patches are always welcome!

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