[U-Boot] [PATCH/RFC, 0/2] I2C rework -- what do you think?
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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@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
On Tue, 27 Jan 2009, Heiko Schocher wrote:
Hello ksi,
ksi@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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net schrieb:
On Tue, 27 Jan 2009, Heiko Schocher wrote:
ksi@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
On Wed, 28 Jan 2009, Heiko Schocher wrote:
Hello ksi,
ksi@koi8.net schrieb:
On Tue, 27 Jan 2009, Heiko Schocher wrote:
ksi@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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hello ksi,
ksi@koi8.net wrote:
On Wed, 28 Jan 2009, Heiko Schocher wrote:
Hello ksi, ksi@koi8.net schrieb:
On Tue, 27 Jan 2009, Heiko Schocher wrote:
ksi@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
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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
participants (2)
-
Heiko Schocher -
ksi＠koi8.net