[U-Boot] [PATCH 2/2] WIP: tegra: i2c: Enable new CONFIG_SYS_I2C framework

[Heiko Schocher]

Hello Stephen,

On 01.11.2012 18:03, Stephen Warren wrote:
> On 11/01/2012 01:42 AM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> On 31.10.2012 17:25, Stephen Warren wrote:
>>> On 10/31/2012 09:56 AM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Wed, Oct 31, 2012 at 8:41 AM, Stephen
>>>> Warren<swarren at wwwdotorg.org>   wrote:
>>>>> On 10/31/2012 12:00 AM, Heiko Schocher wrote:
[...]
>>> If all in-use I2C adapters must be specified statically in
>>> CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any
>>> arbitrary subset of Tegra's I2C adapters, then we must always set
>>> CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we
>>> put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining
>>> the maximal set of I2C adapters that a board can enable, which means DT
>>> isn't specifying that, but rather the board config file is, and hence
>>> it's pointless to even use DT for this purpose.
>>
>> My current approach needs static specific adapters, yes. But I see not
>> the problem, if we define all tegra adapters per default and ...
>>
>>> Now, most boards won't use all I2C adapters, so presumably the Tegra I2C
>>> init routine will look for status="disabled" (or the inverse) in DT, and
>>> only initialize if the DT node for the adapter is present and enabled.
>>
>> ... and it should here be possible to add the used "i2c busses"!
>> dynamically from the info in the DT, or?
>
> Does the I2C adapter registration function call an I2C core function to
> dynamically register the actual buses that exist on the system? If so,
> then everything seems fine.
>
> However, your later responses re: CONFIG_SYS_I2C_BUSSES then would
> confuse me...
>
>> We use in U-Boot not direct the i2c adapters, instead i2c busses ...
>> so if we define all 4 tegra i2c adapters per default, but using on
>> one board only adapter 1 and 3 we have two i2c busses:
>> 0 (= adapter 1) and
>> 1 (= adapter 3) ...
>> no gaps between ...)
>>
>>> However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not
>>> enabled, which will presumably influence the I2C bus numbering in the
>>
>> No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS
>> define, without really use them in CONFIG_SYS_I2C_BUSSES. And it
>
> Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized,

CONFIG_SYS_I2C_ADAPTERS defines which i2c adapters (=drivers) (maybe)
used on this board.

> and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so
> CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a
> particular adapter, or could end up defining multiple buses that use a
> particular bus (in the presence of a mux). That all seems fine, but...
>
> However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at
> compile time, not something dynamic. If that's true, then I'll just
> re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I
> said "CONFIG_SYS_I2C_ADAPTERS"...

Yes, currently static ...

>> should be possible (not yet coded, but tried in an older version) to add
>> i2c busses after relocation, or while interpret DT ...
>>
>> something like I did in
>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13
>
> I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or
> CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this
> dynamic in the long run we should just make it dynamic from the start as
> part of the API rework. If we don't, we'll just have to do another pass
> over all the drivers converting them to dynamic registration later anyway.

This patchserie is thought only for static configuration (as current
state is) ...

But you can post patches for such a solution ... and we can move this way!

> I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each
> driver registers an arbitrary number of adapters and/or buses during its
> initialization.

Why should an i2c driver register buses? That is board specific.

> We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all;
> just have each driver define a structure that gets put into a special
> linker section, so that the I2C core can iterate over all linked drivers
> at boot time and call their initialization functions.
>
> Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we
> should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

How do we this, for example on some powerpc boards, which boot from
NOR flash and read a SPD EEprom data for RAM initialization (Note: we
have here not a lot of stack, nor RAM)? Maybe we should wait with this
hole patchserie until DM is ready? (And maybe rework it completly?)

> BTW, isn't buses spelled "buses" not "busses"? Thunderbird's
> spell-checker certainly thinks so!
>
>> function:
>> int i2c_add_one_bus(char *buf)
>>
>> ->  while interpreting DT i2c info for above board will result in calling:
>>     i2c_add_one_bus("tegra-i2c-1");
>>     i2c_add_one_bus("tegra-i2c-3");
>>
>> and results in two new i2c busses 0 and 1 ...
>> Maybe this is a way to go?
>
> Yes, that sounds like the right kind of direction.

Ok.

>>> U-Boot shell; there will be I2C busses that exist but cannot be used. Is
>>> this what we want? Perhaps it is in fact a good idea to always make the
>>
>> Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have
>> some i2c adapters which are defined but (maybe) not used ...
>>
>>> U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps
>>> when some ports aren't enabled? That would be a departure from the way
>>> USB is handled today though.
>>
>> Hmm.. but is this possible? If we have a board with 2 (or more) different
>> i2c adapters (which is now possible with the new framework), for example
>> 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra
>> adapters are the first 4 i2c busses, so we cannot longer say the
>> two soft i2c adapters are starting from 0 too (and vice versa) ...
>
> I was talking about the I2C HW in the SoC itself. There isn't any
> concept of an expected I2C bus/adapter ID for anything outside the SoC.

What do you mean here? There is such a concept when using multibus.
We have an ID for each bus. This patchserie just add the possibility
that we can use now more than one hw/sw adapter, introduce an i2c_core
file and reworks the i2c mux functionality ... and in the end we will
get rid of the defines HARD_I2C/SOFT_I2C, all i2c driver moved to
drivers/i2c ...

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