[U-Boot] [PATCH/RFC, 0/2] I2C rework -- what do you think?

[Heiko Schocher]

Hello ksi,

ksi at koi8.net schrieb:
> On Tue, 27 Jan 2009, Heiko Schocher wrote:
>> ksi at koi8.net wrote:
>>> Hi everyone,
>>>
>>> Here is the first draft of major I2C subsystem rework. This is not a
>> patch
[...]
>>> the new code. It is a big job and I don't want to spend a lot of time
>> doing
>>> something that nobody would accept.
>>>
>>
>> Yes, this would be a big job, and how do you want to test this for ~200
>> boards?
> 
> It's physically impossible -- I simply don't have all those board even if I
> had time to do it. However we have people with those boards who can test
> their parts.

Ack.

> I can test if it compiles for all boards. It is not black magic, just
> logical code and if it worked for several boards I can't see why it
> wouldn't
> work for majority of others. Those few that required some kind of magical
> touch and got broken we'll fix.

Ack.

>>> My goal is to somehow untie I2C subsystem from platform-specific
>> parts. This
>>>
>>
>> Isn t this so?
>>
[...]
>>> Primary goal is to allow multiple I2C busses on a board served by
>> different
>>> I2C controllers (on-SoC, external, bit-banging.) As of now we have
>>> rudimentary support for multiple BUILT-IN adapters in some SoCs (e.g.
>>> fsl_i2c.c) but this support is platform specific and does NOT allow
>> any
>>> additional (external etc.) adapters.
>>>
>>
>> Ah... okay, this lacks in the actual u-boot i2c layer. I see here to
>> ways:
>>
>> a) Rewriting the complete code. This is really a big change (maybe, I am
>> not
>>    a I2C expert, the cleaner way, but this will need a long time and a
>> good test
>>    period ...)
> 
> Eh, it ain't rocket science... Yes, it would require some time but nothing
> special...

Ack.

>> b) It should be possible to extend the i2c hardware specific functions,
>> to allow
>>     more then one i2c platform adapter. Switching between those should
>> possible
>>     with the "i2c dev" command (Which I think is the reason for this
>> command)
>>     also "i2c bus" should be extend to choose, which "real" I2C Bus
>>     it uses. Something like this comes in my mind:
>>             i2c bus devid=[muxtype:muxaddr:muxchannel]
>>                 with  devid (an existing device id see "i2c dev"
>> command)
>>                 if "devid=" is missing, old behaviour (backward
>> compatible)
> 
> It makes no sence to me. Having just several busses--some direct, some via
> muxes--looks more natural, IMHO. If one wanted to check the exact path to
> that bus he could use "i2c bus bus_no" command. Other than that I can't see
> any reason for such information.
> 
> And remember, all those "i2c xyz" commands are just interactive commands
> that are usually used for debugging purposes or during board bringup
> process. The internal interface is more important and it is much easier an
> logical to just do "i2c_set_bus(N)" and then use regular i2c_read/i2_write
> or whatever is required. That makes the entire code uniform, without
> special
> treatment for muxed busses etc. Kinda like Unix where everything's a
> file :)

I wrote:

>> That has nothing to do with the "i2c bus" command. This command
>> only adds new virtual buss to the "i2c dev" list (device number).
>> you can then select this bus with "i2c dev [device number]" or in
>> code with i2c_set_bus_num([device number]) . You see, this is also
>> just an integer, you have to choose!

Once again, you must with actual code also just do a i2c_set_bus_num ()
to switch between busses (real or virtual).

[...]
>> Ah, now I understand why you have to change all config files for all
>> existing
>> boards which uses I2C. Think of my suggestion to integrate the "multi
>> real bus"
>> in the existing code, so you dont have to do this ... boards which have
>> only
>> one Hardware I2C Bus automagically use this bus, because i2c_bus_cur =
>> 0!
> 
> That is exactly what I'm going to do as a first step. For the new code to
> take effect one must define "CONFIG_NEW_I2C" in config file. If it is not
> defined no new code is used. Then I will work on each and every board/CPU
> one by one and when all of them are converted I will remove that define.

OK,. But if I look in your patch you delete the "i2c bus" command, and
this breaks at least 2 boards.

> As a matter of fact I can only see 2 boards that use that existing mux
> code,
> namely mgsuvd and mgcoge. That will be trivial to rework.

Yes, for those I made the "i2c bus" command. And it will follow soon
2 more boards ...

>>> board's config file. That means we can use I2C from flash before
>> U-Boot is
>>> relocated to DRAM thus allowing us to use SPD for DRAM initialization.
>> It is
>>>
>>
>> You can also use in actual code SPD EEprom for DRAM initialization.
>> Guess
>> the following example:
>>
>> SPD EEprom reached over 2 muxes:
>>
>> define an Environment var like:
>> SPD_EEprom =pca9554a:70:5:pca9544a:71:4
>>
>> now make bevor accessing the SPD EEprom:
>>
>> i2c_mux_ident_muxstring_f (getenv ("SPD_EEprom");
>>
>> and you can access the SPD EEProm! (And nice side effect, the way to
>> your
>> SPD EEprom is not fix in the code, you can choose it easy per Env var!)
> 
> I don't think it is a good idea. It breaks uniformity, i.e. one would have
> to have different functions for different boards (or unnecessary overloaded
> functions that would fit all.)

It was just a example, to show, that it is possible, not actually used.

>>> also means we do NOT need that "i2c bus" or whatever command to setup
>> a bus
>>> behind muxes for consecutive access--we just choose that bus number
>> and
>>> that's it.
>>>
>>
>> That has nothing to do with the "i2c bus" command. This command
>> only adds new virtual buss to the "i2c dev" list (device number).
>> you can then select this bus with "i2c dev [device number]" or in
>> code with i2c_set_bus_num([device number]) . You see, this is also
>> just an integer, you have to choose!
> 
> I don't like an idea of adding virtual busses dynamically with an
> interactive command. Also it is not needed at all, one can easily switch
> muxes manually, with simple memory write command for some muxes that are
> not
> a part of board's permanent hardware.

It is needed! If, for example an EEprom, is not always on the same virtual
bus. Some boards have no mux, some 1, some 2 ... and if we have this virtual
busses statically, we must have for all boards an extra u-boot binary.
The only Hardware difference for this boards is, how things are connected
to the I2C bus ... and it is not OK for this manufacturer, to have
for every board a different u-boot binary!

So, why shouldnt it possible to add to your proposal a possibility to
add virtual i2c busses dynamically? (using a list instead of a fix table
for example)

>>> All busses are explicitely defined in boards' config files so we know
>>> exactly where all accesses are going.
>>>
>>
>> Actual Code too. Make a "i2c dev" and you get the actual bus number, if
>> it is a virtual
>> bus, you can use "i2c bus" to get a list of this virtual busses.
>>> I did test it on MPC8548CDS system and it works OK. There is a patch
>> for
>>> MPC8548CDS.h in the patchset that changes it to the new I2C code and a
>>> speculative example of multiadapter multibus configuration with 3
>> adapters
>>> and 5 busses 3 of which are connected with I2C muxes, 2 from them are
>>> multihop.
>>>
>>
>> Nice, but why you ignored the existing interface for handling with
>> muxes?
>> I think it is easier to extend the existing interface to support
>> multiple "real"
>> interfaces ...
> 
> I don't think it is needed at all. One has some number of busses on the
> board and all of them are active. Just switch to another bus and you're
> good
> to talk to that bus. If one needs something special, he can easily achieve
> it with writing to those muxes with existing read/write commands.

But it is in the code, because it is needed!

>>> Please let me know guys what you think so I would be able to start
>> with a
>>> big patch.
>>
>>
>> I think, the mux functionality is independent from the problem to have
>> more
>> then one hardware I2C Bus, or?
> 
> No, it is dependent. Bus is a bus, no matter if it is a directly connected
> one or reached through some muxes. It is easier to use one entity (bus)
> instead of two separate ones (bus and mux.)

You are here wrong. You didnt read my answer. Again:

with "i2c bus" you add virtual busses
with "i2c dev" you can switch between real *and* this virtual busses,
               without thinking about what sort of bus it is.

just one command for switching between busses!

>> So, maybe you could integrate your "multiple hardware I2C Bus"
>> concept (which looks good at a first look) in the existing code?
>> I vote for accesing the I2C Bus over a mux with the actual way:
>> - defining with the "i2c bus" command a new "virtual" i2c device
>>  and
>> - accesing this with the standard command "i2c dev"
>> because this is a more flexible way.
> 
> This is the command interface that is only of limited use. And there is no
> need to make a CLI user manually setup anything that is already defined in
> board config.

Really? If commands are saved in Environmentvars, they maybe executed automagic
on every reset ... thinking of for example "run net_nfs" which uses at least
"tftp", "bootm", "setenv", ...

> CLI is easy, I'm now trying to make a template driven, self-generating from
> config file multi-bus software I2C driver...

or just try to make this

i2c_adap_t	*i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] = CONFIG_SYS_I2C_ADAPTERS;

dynamically and extendable and I am happy ;-)

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