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

[ksi at koi8.net]

On Tue, 27 Jan 2009, Heiko Schocher wrote:

> Hello ksi,
>
> ksi at koi8.net wrote:
>> Hi everyone,
>>
>> Here is the first draft of major I2C subsystem rework. This is not a
> patch
>> that should be applied to the tree--it will break systems with I2C
> busses
>> over multiplexers and probably something else--but rather a Request
> For
>> Comments. I would like to here all the objections, suggestions etc.
> before I
>> go on and undertake a big job to change all the boards and other stuff
> to
>> 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.

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.

>> My goal is to somehow untie I2C subsystem from platform-specific
> parts. This
>>
>
> Isn t this so?
>
>> is not an attempt for a "code cleanup" though some cleanup could be
> achieved
>> in process. But this is not a goal, it is just a probable side effect.
>>
>> 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...

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

>> This is not a problem for a vast majority of supported boards that
> usually
>> have a single I2C bus but there are cases when boards have several
> busses
>> served by different I2C adapters. I personally have the first
> prototype of
>>
> [...]
>> Here is a little manifest...
>>
>> First of all, there is a basic entity, I2C bus. It can be a simple I2C
> bus
>> from the only I2C adapter on a SoC, several busses from several
> adapters,
>> both on-SoC and external, and several busses growing off of several
> adapters
>> via I2C multiplexers/switches (e.g. PCA9544 etc.) They are all the
> same for
>> the rest of code accessed with single set of i2c_read()/i2c_write()
> and
>> friends for all busses no matter what adapter they are on and if they
> are
>> directly connected to an adapter or reached through a set of I2C mux
> chips.
>>
>> Any I2C operation is performed on the CURRENT bus. Current bus is set
> with
>> i2c_set_bus_num(). In case of directly connected busses it merely
> changes a
>> global variable that is used as an index into an array. If some busses
> are
>> connected via I2C muxes, it disconnects the previously connected muxes
> if
>> any and sets all chips along the path to the new bus so next access
> will go
>> to that bus.
>>
>> All those paths are statically allocated at compile time according to
>>
>
> Why must be this statically defined? Think of a Hardwaremanufacturer who
> has several boardvariants. In your case, he must have for every board a
> config file -> different binaries. In the case using "i2c bus" he has
> one binary
> which is able to work with all of his variants! And he can connect on a
> running system on an actual not used I2C Bus, another mux for example,
> and can configure this "new" bus per commandline without generating
> a new binary, flashing, resetting the board!

Eh, first of all we do have a lot of configuration files for different
boards. And this is, IMHO, logical because every other board is different in
some respect. There is no way we can make a universal U-Boot binary that
works on each and every board. Furthermore, even if we could it would've
been very big binary with lots of excessive code. It is not a U-Boot goal to
be plug-and-pray do-all program -- it is a targeted highly specialized
firmware of a smallest size possible. It is built for every particular board
exactly to its hardware specs.

Then, if one even didn't use some available hardware I2C bus for anything at
all it is not such a big deal to have that particular bus also enabled in
the config file. It won't take much space and/or require additional effort
to do. Once one have a bus he can use for experiments there is not rocket
science to just configure possible muxes on that bus with a single mw
command and use it as he wants. This is exactly what all those interactive
commands for. All those additional busses/muxes/whatever will not be needed
once the board design is finalized.

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

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.

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

> But, so my feeling, it would be a good idea, to connect a SPD EEprom not
> over a mux, at least only on channel 0, so no mux commands are necessary
> to read from the SPD EEprom after reset.

I totally agree but it is not always possible.

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

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

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

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

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

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************