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

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

[ksi at koi8.net]

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.

My goal is to somehow untie I2C subsystem from platform-specific parts. This
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.

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
such a board right now in PCB fab. It's MPC8548E based mATX motherboard that
uses both built-in I2C adapters (fsl_i2c.c) AND the third one off of SM502
chip sitting on PCI bus. All 3 busses are required for proper system
initialization and operation because we have 3 I2C devices with the same
address, 0x68 that can not be changed.

For existing boards we have just 2 MUTUALLY EXCLUSIVE options,
CONFIG_HARD_I2C and CONFIG_SOFT_I2C. That gives very little control over I2C
subsystem. It won't even allow for having a bit-banged I2C bus in addition
to a hardware adapter, less for several such busses or anything external
(e.g. some chipset adapters that would give some control over sensors and
such.) This is obvious even from that HARD/SOFT I2C exclusivness --
bit-banging I2C interface is ortogonal to a built-in hardware one so why
wouldn't we were able to use both? One can even have several bit-banging
busses as long as GPIO pins are available...

There are 2 different ways to address this issue -- make another platform
specific monster or rework the entire I2C subsystem to make that multibus
support generic and common U-Bootwide.

This is an attempt to make I2C uniform and scalable.

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

All busses are explicitely defined in boards' config files so we know
exactly where all accesses are going.

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.

Please let me know guys what you think so I would be able to start with a
big patch.

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

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

[Heiko Schocher]

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?

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

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)


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

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!

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

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.

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

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

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

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.

Wolfgang: Maybe we should have a "i2c" Tree for testing purposes,
if we step into this "multi hardware I2C busses" theme.

bye
Heiko

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

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

[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

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

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

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> 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:
[...]
>> 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.

Ok.

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

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

OK, let us think a little about it.

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

Ok.

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

Yes, I know, this is a problem.

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

Ok, thanks.

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

Hmm.. so we need no i2c mux support?

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

Ok.

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

:-)

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

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

[ksi at koi8.net]

On Thu, 29 Jan 2009, Heiko Schocher wrote:

[dd]

>> 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...
>
> Hmm.. so we need no i2c mux support?

Yep. It can be emulated by a couple of existing commands.

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