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

[ksi at koi8.net]

On Wed, 28 Jan 2009, Heiko Schocher wrote:

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

Yep. As I said it was not a real patch, just a request for comments. I will
take care to make the real one work.

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

OK, I will look at those boards and make sure they work. Anyway it is just a
work in progress; no actual patch submitted so far...

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

OK.

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

It is definitely a possibility but it would break uniformity. One can not
use lists while U-Boot is running out of ROM and DRAM is not active. That
means we should use an additional mechanism for those busses added later on.

One solution is to make num_i2c_busses (or whatever) a variable initially
set by config file define and then modified when new busses are added. It
also means we should use pointers instead of just array of structures and
bring in the whole bunch of list management functions like find_next,
find_prev, find_first etc. It might make board developer's life slightly
easier but would add complexity to U-Boot and increase its memory footprint.

I'm not sure yet that that little convenience is worth that.

OK, I will return to it after I have that template driven multibus soft I2C
driver done.

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

It steers us into a bunch of board-specific hacks and quirks instead of
making something uniform. We do not have a luxury of working DRAM when we
starting up and that code is required to bring up DRAM. I'm trying to avoid
separate code for startup and full blown operation.

Furthermore, U-Boot is NOT an OS, it is a bootloader. It is nice to have a
bunch of utility commands in its CLI but it is not what it is for. Hardware
configuration is rigid as far as U-Boot concerned. One has such and such
hardware onboard and he is free to choose anything from whatever is in but
not to add anything else on the fly.

There is also no one-size-fits-all U-Boot, it is specifically built for each
and every specific board. If you look at e.g. PC BIOS'es, they are made for
exactly one specific motherboard. U-Boot is not different in this respect.

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

I will return to it when it comes to the real patch.

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

It is. But it's logical to reduce the number of entities and make everything
a bus instead of having 2 different entities. Occam's Razor...

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

It is more flexible, I agree. But it multiplies entities. And doesn't add
anything that couldn't be achieved with other existing commands.

Look, I2C bus behind muxes is just an already defined bus and a set of
regular single-register I2C chips connected to that bus. In order to "add"
that virtual bus one can simply switch to the appropriate existing bus and
write a byte or two to those chips with existing i2c write commands.

That means that that "i2c bus" command in nothing more than a shortcut. It
can be easily implemented even as U-Boot environmental variable that one can
run...

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

Exactly my point. Instead of making a cosmetic command one can simply use a
sequence of "i2c dev existing_bus", "i2c mw mux_addr 0.0 some_value".

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

No, one can NOT use a dynamic set of adapters because this set determines
which drivers got included into the final binary. This is COMPILE time
define.

OK, let me make some real patch and then we'll return to our discussion :)

Anyways thank you very much for a feedback, I really appreciate it. And I
will try to make you happy :)

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