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

[Heiko Schocher <hs@denx.de>]

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