[U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Sat, 14 Feb 2009, Heiko Schocher wrote:
>
>   
>> Hello ksi,
>>
>> ksi at koi8.net wrote:
>>     
>>> On Fri, 13 Feb 2009, Heiko Schocher wrote:
>>>       
>>>> ksi at koi8.net wrote:
>>>>         
>>>>> Here is the second attempt for initial portion of multibus/multiadapter
>>>>> I2C support.
>>>>>   
>>>>>           
>>>> Can you please send your patches with some better commit messages.
>>>> You only send your Signed-off-by, without any explanation. Please
>>>> change this.
>>>>         
>>> There is not much sense in extensive commit messages in this case, IMHO. It
>>> is not a bug fix or added feature at one particular place; it is a major
>>> rework. The only message I can give is something like "Changes for
>>> multiadapter/multibus I2C support..."
>>>
>>> I'll add it to the second attempt that I will make later today.
>>>
>>>       
>>>>> This includes a set of common files, all drivers in drivers/i2c and all
>>>>> boards affected by these changes (config files, board files, and lib_xx
>>>>> files.)
>>>>>
>>>>> There is an illustrative example of multiadapter multibus I2C config in
>>>>> MPC8548CDS.h config file (#if 0'd.) Definitions in that example are
>>>>> bogus so please don't expect it to work. It will compile though...
>>>>>
>>>>> This set also includes big rework for soft_i2c.c that makes it template
>>>>> version that allows up to 4 bitbanged adapters. This number can be
>>>>>   
>>>>>           
>>>> Didn;t you try my suggestion? This is a really big define monster now,
>>>> which I think, we can avoid, and without to change nearly all lines of
>>>> the existing driver.
>>>>         
>>> We can not avoid it. At least I can not see an easy way to do this. SOFT_I2C
>>>       
>> Yes, we can. Look again deeper in my approach, this _is_ an easy way!
>>
>> I also looked again in your changes in the fsl_i2c.c driver, and we
>> can make this also simplier, by using cur_adap_nr->hwadapnr!
>>     
>
> OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also
>   

When running from ram, this is no problem. It should be set in
i2c_set_bus_num().

> explain how can one invoke a function on other adapter than "current".
>   

Is this needed? If so, you must before call a i2c_set_bus_num(), and after
you finished call it again with the old busnumber. So it is done for example
in do_date () common/cmd_date.c

> Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
> so you can NOT change "current" adapter.
>   

Yes, thats a point. But do we need this before running from ram (except
one hardwareadapter)?

> Please also note that you will loose a capability of working with more than
> one adapter before the code is relocated to RAM.
>   

Do we really need this?

>   
>> We have not to define for both hardware adapter each function in
>> i2c_adap_t! For example:
>>
>> You did:
>> static void fsl_i2c1_init(int speed, int slaveadd)
>> {
>> 	__i2c_init(0, speed, slaveadd);
>> }
>>
>> instead we only need
>>
>> i2c_init(cur_adap_nr->hwadapnr, speed, slaveadd);
>>
>> with
>>
>> i2c_adap_t	fsl_i2c_adap[] = {
>> 	{
>> 		.init		=	i2c_init,
>> [...]
>> 		.hwadapnr	=	0,
>> 		.name		=	FSL_NAME(CONFIG_SYS_FSL_I2C_OFFSET)
>> 	},
>> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>> 	{
>> 		.init		=	i2c_init,
>> [...]
>> 		.hwadapnr	=	1,
>> 		.name		=	FSL_NAME(CONFIG_SYS_FSL_I2C2_OFFSET)
>> 	},
>> #endif
>>     
>
> It would've been easy if we had had "this" pointer. That would allow us to
> find out what adapter we are running on by using something like
> "this->hwadapnr." Unfortunately we do NOT have such a pointer, we're plain
> C. Function in a structure does not have a way to find out how to access a
> member of that structure. The only way to somehow find which "hwadapnr" we
> are running at is using a global variable, cur_i2c_bus as a starting point.
> But that is meaningless until the code is relocated to RAM and that variable
> became writable. And that robs us of added possibility of using any adapter
> other than a single one preset in config file before relocating to RAM.
>   

Yes, I know. But again, do we need this?

> That is if we want to keep the original I2C API. The other, simpler way is
> to add an argument to each and every function, a pointer to i2c_adap_t
> structure or its index or something similar. But that defeats the entire
> purpose of this code by requiring to find and change each and every call to
> any I2C function in the entire U-Boot source thus totally breaking ALL
> existing code 99.99% of which only use single I2C adapter/bus...
>   

That would be a hard way.

>> Please change this driver also!
>>     
>
> I can't. Please read above.
>
>   
>> If I think more, we never even need to change the function parameters
>> like you did for example for i2c_int ()! We can use at the beginning
>> of every function who go in i2c_adap_t, the "cur_adap_nr->hwadapnr"
>> and make the settings we need for this function... and wow we saved
>> one function parameter.
>>     
>
> Devil is in the details... Please read above.
>   

Thats why we discuss it ;-)

>>> is special. Those multiple e.g. MXC or OMAP3 adapters can be parameterized
>>> because different channels do only differ in their base address that can be
>>> made into a parameter. Software I2C is totally different because it has
>>>       
>> Why is this different? If you change a base or the way to the pins?
>>     
>
> Because the pins on different channels can be accessesed in absolutely
> different way.
>
>   
>>> totally different functions for different channels, there is nothing we can
>>>       
>> Think about my explanation to the soft_i2c.c driver in previous EMail and
>> above function.
>>
>> It also works!
>>     
>
> Partially and with handicaps. Please read my reply to that message.
>   

If we really need more then one bus when running from flash, this is
a problem.

>   
>>> make into a parameter. All those I2C_SDA etc. are NOT DEFINES, they are
>>> MACROS. Every function for every channel is built of those macros that can
>>>       
>> I know this in your approach, but we _don;t_ need this. We simply can make
>> one "common" board function and switch in this function dependent on the
>> "cur_adap_nr->hwadapnr" to the particular GPIO pin functions, wherever the
>> are!
>>     
>
> Please read above.
>
>   
>>> be absolutely different for each channel. They define NOT some PARAMETERS
>>> but function TEXT that will be compiled into executable code.
>>>       
>> And this additional TEXT I save too!
>>     
>
> You don't save anything. And you add complexity and break uniformity. BTW,
>   

I save text when having 4 bitbang drivers running. And I don;t see
where it is complexer nor where it breaks uniformity.

> what is a reason to save on text?
>   

We are only a bootloader and have often to fit in a maybe small flash.

bye
Heiko

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