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

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

[ksi at koi8.net]

Here is the second attempt for initial portion of multibus/multiadapter
I2C support.

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
reduced/increased; 4 is arbitrary chosen value. I'm not a CPP guru so I
did not find a way to make a version without a limit. Sure I could add
additional configuration variable for this limit but it is one more
variable and resulting soft_i2c.c would be much uglier.

The general rule for multi-adapter controllers (fsl_i2c, mxc_i2c, etc.)
is that all defines for the first controller do not have any number in
their names, the second one has "2" and so on. That allows for
compatibility with existing code. All existing defaults are kept.

I did test-compile all ARM and PPC boards. Unfortunately I have never
worked with m68k, mips, blackfin so I don't have toolchains for those
to do test builds.

The only board that failed to compile was SIMPC8313.h that fails to
link NAND bootstrap with "NAND bootstrap too big" but it has nothing to
do with new I2C code; it also fails without my patches.

All patches are against today's u-boot-i2c.git.

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

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

[Heiko Schocher]

Hello ksi,

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.

> 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.
> reduced/increased; 4 is arbitrary chosen value. I'm not a CPP guru so I
> did not find a way to make a version without a limit. Sure I could add
> additional configuration variable for this limit but it is one more
> variable and resulting soft_i2c.c would be much uglier.
>
> The general rule for multi-adapter controllers (fsl_i2c, mxc_i2c, etc.)
> is that all defines for the first controller do not have any number in
> their names, the second one has "2" and so on. That allows for
> compatibility with existing code. All existing defaults are kept.
>
> I did test-compile all ARM and PPC boards. Unfortunately I have never
> worked with m68k, mips, blackfin so I don't have toolchains for those
> to do test builds.
>
> The only board that failed to compile was SIMPC8313.h that fails to
> link NAND bootstrap with "NAND bootstrap too big" but it has nothing to
> do with new I2C code; it also fails without my patches.
>
> All patches are against today's u-boot-i2c.git.
>   
bye
Heiko

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

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

[ksi at koi8.net]

On Fri, 13 Feb 2009, Heiko Schocher wrote:

> Hello ksi,
> 
> 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
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
totally different functions for different channels, there is nothing we can
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
be absolutely different for each channel. They define NOT some PARAMETERS
but function TEXT that will be compiled into executable code.

This is how it is done in Linux kernel (see e.g. drivers/hwmon/lm93.c.)

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

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

[Heiko Schocher]

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!

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

Please change this driver also!

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.

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

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

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

> 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!
I think, you think too much in C++?

> This is how it is done in Linux kernel (see e.g. drivers/hwmon/lm93.c.)

Thats no argument.

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

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

[ksi at koi8.net]

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
explain how can one invoke a function on other adapter than "current".
Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
so you can NOT change "current" adapter.

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

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

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

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

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

> > 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,
what is a reason to save on text?

> I think, you think too much in C++?

No, I'm not a C++ guy. I prefer assembly and only use C where it is
absolutely necessary :) I do not program in C++/Java/etc. at all and only
know C++ basics and a bit of other stuff required to fix occasional bugs in
other people's code.

It is not C++, it is common sence and good engineering practices. And I'm an
engineer first and only then a programmer, a machinist, an electronic
engineer, a gunsmith and so on.

> > This is how it is done in Linux kernel (see e.g. drivers/hwmon/lm93.c.)
> 
> Thats no argument.

Sure it is not. This is a suggestion.

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

[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 

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

[ksi at koi8.net]

On Sun, 15 Feb 2009, Heiko Schocher wrote:

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

Yep. But nobody's perfect and you can have a situation when you need to
access several busses before relocation. It is not hardware for U-Boot, it
is U-Boot for hardware. When hardware designers design their hardware they
don't make their decisions based on U-Boot limitation. That is us who should
accomodate what they designed.

There is also another consideration -- when having several adapters which
one should be initialized at boot time, before relocation? Another problem
is init() function that can be unique for each adapter. To make the lower
layer transparent I'm reprogramming muxes if any when switching busses. It
is necessary to make I2C API simple and uniform between muxed and non-muxed
busses. That essentially means that we can NOT do i2c_set_bus_num() to
execute init() for a particular adapter -- adapter MUST be initialized for
i2c_set_bus_num() to succeed.

Your suggestion requires total LOGIC change.

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

You can not do it before all adapters are initialized. And you WON'T be able
to initialize adapters because you will not be able to switch busses.

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

Yes, see above.

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

Eh, "640K ought to be enough to anybody..."

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

We do. Otherwise we can essentially throw everything to trash and start
over. This requires changing the logical design, architecture. And this is
that logic that is most difficult and takes most thinking. Coding is easy.

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

That is why I spent a week thinking about the design that would allow to
keep most of existing code.

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

No, that's not just that. There are multiple reasons why the original driver
had been made with macros.

First, it is _SMALLER_ when done this way. Most of those macros (I2C_SCL
etc.) translate into 1 to 3 assembly instructions depending on particular
processor code set. Except some special cases the most complex operation
they do is changing a bit at some address that takes 3 instructions if
particular CPU can not change set/reset bits directly - read->modify->write.
Many CPUs can make it in 1 to 2 instructions.

There is no way how you can avoid those instructions -- the work must be
done. You insist on making them into functions (there is no other way if
they reside in another object file.) That means that you do NOT eliminate
those instructions, you replace them with function calls. Even if we assume
that those function calls will not have any prolog/epilog many processors
would not be allow you to perform a function call with a single instruction
because they do not have e.g. automatic stack so you should save your link
register before the call ad restore it after. That already makes _MORE_ code
per operation than those macros had. And that is just function calls
overhead, the actual function body is extra...

Another reason why macros are used is speed. Not everyone is running U-Boot
on 10 GHz Pentium-9 with gigabyte of cache. In bitbanged I2C every
instruction counts if you want to run a bus at a decent speed (I won't even
start with regular 100kHz less for 400kHz; 50kHz would be very good.) Your
approach adds function call overhead to every memory/io bit change because
you must perform a function call every time you want to read SDA line or
change SCL state etc. It is ON TOP of what those macros were doing because
no matter what the real work must be done so that code in those macros still
must be executed. And remember, there is probably no instruction cache and
you're running off of flash so every instruction fetch is something like
100ns for a single word. That is for a single flash access but you would
probably need at least 2 such accesses for a single-word instruction fetch
because most of the CPUs are 32-bit and I really doubt anybody uses 32-bit
flash for U-Boot... That makes it 400 ns just to fetch _ONE_ that "branch"
instruction on 32-bit CPU running off of 8-bit flash. And another 400 ns to
fetch a "return" instruction after the real work is done. That is if your
CPU has automatic stack. Multiply it by two if it doesn't. That makes it
1.6 us just for a single function call overhead.

And you will NOT save a single byte of program code. It will probably get
even bigger because you will have to add that conditional logic for checking
what the current bus is and perform appropriate actions based on that.

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

You ain't gonna save anything. Quite in contrary that approach will almost
certainly make the code bigger.

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

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

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Sun, 15 Feb 2009, Heiko Schocher wrote:
>> ksi at koi8.net wrote:
>>> On Sat, 14 Feb 2009, Heiko Schocher wrote:
>>>> ksi at koi8.net wrote:
>>>>>> ksi at koi8.net wrote:
>>>>>>         
>>>>>>> Here is the second attempt for initial portion of multibus/multiadapter
>>>>>>> I2C support.
[...]
>> When running from ram, this is no problem. It should be set in
>> i2c_set_bus_num().
> 
> Yep. But nobody's perfect and you can have a situation when you need to
> access several busses before relocation. It is not hardware for U-Boot, it
> is U-Boot for hardware. When hardware designers design their hardware they
> don't make their decisions based on U-Boot limitation. That is us who should
> accomodate what they designed.

Din;t know, there is such a design.

> There is also another consideration -- when having several adapters which
> one should be initialized at boot time, before relocation? Another problem
> is init() function that can be unique for each adapter. To make the lower
> layer transparent I'm reprogramming muxes if any when switching busses. It
> is necessary to make I2C API simple and uniform between muxed and non-muxed
> busses. That essentially means that we can NOT do i2c_set_bus_num() to
> execute init() for a particular adapter -- adapter MUST be initialized for
> i2c_set_bus_num() to succeed.

Hmm.. okay, but we can also call from i2c_set_bus_num() when running in flash
first the init() function ... but that wouldn;t be nice, you are right.

>>> 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)?
> 
> Yes, see above.

Yes, thats is a problem in my approach, and if we need more then one
i2c bus when running from flash, maybe a no go.

[...]
>>> 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.
> 
> That is why I spent a week thinking about the design that would allow to
> keep most of existing code.

Thats why I discuss with you, to get this infos ;-)

[...]
>> If we really need more then one bus when running from flash, this is
>> a problem.
> 
> No, that's not just that. There are multiple reasons why the original driver
> had been made with macros.
> 
> First, it is _SMALLER_ when done this way. Most of those macros (I2C_SCL
> etc.) translate into 1 to 3 assembly instructions depending on particular
> processor code set. Except some special cases the most complex operation
> they do is changing a bit at some address that takes 3 instructions if
> particular CPU can not change set/reset bits directly - read->modify->write.
> Many CPUs can make it in 1 to 2 instructions.

Ok.

> There is no way how you can avoid those instructions -- the work must be
> done. You insist on making them into functions (there is no other way if
> they reside in another object file.) That means that you do NOT eliminate

As I said, it should be possible to do this also in macros. But you are right,
there is always a +switch, which will cost more code ...

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

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902131156250.26866@home-gw.koi8.net> you wrote:
> 
> > 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..."

This sounds very much as if there was a danger that the  patches  are
not  logically  cut  -  did you make sure that eahc of the commits is
still bisectable?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"A verbal contract isn't worth the paper it's printed on."
- Samuel Goldwyn

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902142104100.6240@home-gw.koi8.net> you wrote:
> 
> OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also
> explain how can one invoke a function on other adapter than "current".
> Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
> so you can NOT change "current" adapter.

We could assign an entry in the global data for it.

But then - how often will it bbe necessary to switch adapters before
relocation? What is I2C being used for? To read the SPD data for the
RAM init code. Which other adapter would be needed?


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

I don't see this.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Nobody will ever need more than 640k RAM!"       -- Bill Gates, 1981
"Windows 95 needs at least 8 MB RAM."             -- Bill Gates, 1996
"Nobody will ever need Windows 95."             -- logical conclusion

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902152236430.17769@home-gw.koi8.net> you wrote:
> 
> Yep. But nobody's perfect and you can have a situation when you need to
> access several busses before relocation. It is not hardware for U-Boot, it
> is U-Boot for hardware. When hardware designers design their hardware they
> don't make their decisions based on U-Boot limitation. That is us who should
> accomodate what they designed.

We don't have to make the mainline U-Boot implementation  unnecessary
complex  just  for  the  small  chance  that there is sombeody stupid
enough to design a broken system.

It is our task as software engineers to tell the  hardware  designers
which  designs  are  easy to support, nd which will cause problems in
the software.

> There is also another consideration -- when having several adapters which
> one should be initialized at boot time, before relocation? Another problem

The one that is needed there, if any at all.

> is init() function that can be unique for each adapter. To make the lower
> layer transparent I'm reprogramming muxes if any when switching busses. It
> is necessary to make I2C API simple and uniform between muxed and non-muxed
> busses. That essentially means that we can NOT do i2c_set_bus_num() to
> execute init() for a particular adapter -- adapter MUST be initialized for
> i2c_set_bus_num() to succeed.
> 
> Your suggestion requires total LOGIC change.

Maybe. But that's why we're discussing this here.

> > 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
> 
> You can not do it before all adapters are initialized. And you WON'T be able
> to initialize adapters because you will not be able to switch busses.

This sounds like a design problem to me, then.

Please keep in  mind  that  according  to  U-Boot  philosophy  it  is
forbidden to always initialize all adapters. Only those actually used
by U-Boot may be initialized, and shall be de-initialized after use.

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

Um... Maybe I missed something - did you give an example (except for
broken designs) where this really might be needed?

> > Yes, I know. But again, do we need this?
> 
> We do. Otherwise we can essentially throw everything to trash and start
> over. This requires changing the logical design, architecture. And this is
> that logic that is most difficult and takes most thinking. Coding is easy.

You say: "we do [need that]". I ask: why? what for?

> Another reason why macros are used is speed. Not everyone is running U-Boot
> on 10 GHz Pentium-9 with gigabyte of cache. In bitbanged I2C every
> instruction counts if you want to run a bus at a decent speed (I won't even
> start with regular 100kHz less for 400kHz; 50kHz would be very good.) Your

Are you sure? If I remember correctly soft-I2C can even run 400 KHz on
a slow 50 MHz MPC8xx system.

Do you have other numbers?

> must be executed. And remember, there is probably no instruction cache and
> you're running off of flash so every instruction fetch is something like

I tend to say that an U-Boot port where instruction cache is disabled
is misconfigured and should be fixed :-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is nothing in this world constant but inconstancy.      - Swift

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

[Wolfgang Denk]

Dear Heiko,

In message <49992BEB.30501@denx.de> you wrote:
> 
> >> Yes, thats a point. But do we need this before running from ram (except
> >> one hardwareadapter)?
> > 
> > Yes, see above.
> 
> Yes, thats is a problem in my approach, and if we need more then one
> i2c bus when running from flash, maybe a no go.

It is not an unsolvable problem. We do have the global data structure,
and it has been (mis-) used for worse purposes before.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"IBM uses what I like to call the 'hole-in-the-ground  technique'  to
destroy  the  competition.....  IBM digs a big HOLE in the ground and
covers it with leaves. It then puts a big POT OF GOLD nearby. Then it
gives the call, 'Hey, look at all this gold, get over here fast.'  As
soon  as  the competitor approaches the pot, he falls into the pit"
                                                     - John C. Dvorak

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

[ksi at koi8.net]

On Mon, 16 Feb 2009, Wolfgang Denk wrote:

> Dear ksi at koi8.net,
> 
> In message <Pine.LNX.4.64ksi.0902131156250.26866@home-gw.koi8.net> you wrote:
> > 
> > > 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..."
> 
> This sounds very much as if there was a danger that the  patches  are
> not  logically  cut  -  did you make sure that eahc of the commits is
> still bisectable?

I will rebase them and slice 'em in smaller pieces.

Unfortunately this is a major rework so there is no way one can make small
patches really independent. Changes to include/i2c.h, e.g. make the entire
old code uncompilable so they only make sence together with some other parts
that constitute the core of the new code. Everything else is much easier
when the core is in.

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

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

[ksi at koi8.net]

On Mon, 16 Feb 2009, Wolfgang Denk wrote:

> Dear ksi at koi8.net,
> 
> In message <Pine.LNX.4.64ksi.0902142104100.6240@home-gw.koi8.net> you wrote:
> > 
> > OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also
> > explain how can one invoke a function on other adapter than "current".
> > Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
> > so you can NOT change "current" adapter.
> 
> We could assign an entry in the global data for it.
> 
> But then - how often will it bbe necessary to switch adapters before
> relocation? What is I2C being used for? To read the SPD data for the
> RAM init code. Which other adapter would be needed?

I can not tell it right away. There might be several RAM DIMMs with their
own SPD EPROMS sitting on different busses or something else. It is not like
we absolutely need this feature right now, urgently but it is so easy to
implement that it would've been a crime to not to... :)

There is another problem with choosing a proper i2c_init() function with
i2c_set_bus_num() -- the latter also reprograms I2C multiplexers/switches if
there are any so you essentially need an initialized adapter to switch to it
to initialize it. Catch 22...

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

Why? There is no writable global variables before relocation...

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

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

[ksi at koi8.net]

On Mon, 16 Feb 2009, Wolfgang Denk wrote:

> Dear ksi at koi8.net,
> 
> In message <Pine.LNX.4.64ksi.0902152236430.17769@home-gw.koi8.net> you wrote:
> > 
> > Yep. But nobody's perfect and you can have a situation when you need to
> > access several busses before relocation. It is not hardware for U-Boot, it
> > is U-Boot for hardware. When hardware designers design their hardware they
> > don't make their decisions based on U-Boot limitation. That is us who should
> > accomodate what they designed.
> 
> We don't have to make the mainline U-Boot implementation  unnecessary
> complex  just  for  the  small  chance  that there is sombeody stupid
> enough to design a broken system.
> 
> It is our task as software engineers to tell the  hardware  designers
> which  designs  are  easy to support, nd which will cause problems in
> the software.

It is not complex. Quite in the contrary, it is much simpler and more
straightforward to have 2 sets of functions for 2 adapters in one place than
to have one that includes both of them in one place switchable using some
global variable at another place used by wrappers in a third place.

> 
> > There is also another consideration -- when having several adapters which
> > one should be initialized at boot time, before relocation? Another problem
> 
> The one that is needed there, if any at all.

You will have to pick that one for each and every board somehow...

> > is init() function that can be unique for each adapter. To make the lower
> > layer transparent I'm reprogramming muxes if any when switching busses. It
> > is necessary to make I2C API simple and uniform between muxed and non-muxed
> > busses. That essentially means that we can NOT do i2c_set_bus_num() to
> > execute init() for a particular adapter -- adapter MUST be initialized for
> > i2c_set_bus_num() to succeed.
> > 
> > Your suggestion requires total LOGIC change.
> 
> Maybe. But that's why we're discussing this here.
> 
> > > 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
> > 
> > You can not do it before all adapters are initialized. And you WON'T be able
> > to initialize adapters because you will not be able to switch busses.
> 
> This sounds like a design problem to me, then.
> 
> Please keep in  mind  that  according  to  U-Boot  philosophy  it  is
> forbidden to always initialize all adapters. Only those actually used
> by U-Boot may be initialized, and shall be de-initialized after use.

Eh, I don't see any problem initializing all I2C controllers at the same
time. What is the problem with that?

Then, we _DO_ already initialize _ALL_ controllers in U-Boot. Most of the
time the total number of controllers is 1 and we do initialize all of them.
In those rare cases when that number is not 1, we _DO_ initialize all of
them (look at fsl_i2c.c as it is in the main tree right now.) Existing
i2c_init() function does not have any provision for initializing any
particular controller; it initializes them all.

> > > Yes, thats a point. But do we need this before running from ram (except
> > > one hardwareadapter)?
> > 
> > Yes, see above.
> 
> Um... Maybe I missed something - did you give an example (except for
> broken designs) where this really might be needed?

We did not have such boards as of now but that does not mean they can't
exist. I would've agreed for making a handicapped version if it had been
difficult to make a full-featured one but it is not. Furthermore,
additional efforts required to put such a handicap on existing code.

> > > Yes, I know. But again, do we need this?
> > 
> > We do. Otherwise we can essentially throw everything to trash and start
> > over. This requires changing the logical design, architecture. And this is
> > that logic that is most difficult and takes most thinking. Coding is easy.
> 
> You say: "we do [need that]". I ask: why? what for?
> 
> > Another reason why macros are used is speed. Not everyone is running U-Boot
> > on 10 GHz Pentium-9 with gigabyte of cache. In bitbanged I2C every
> > instruction counts if you want to run a bus at a decent speed (I won't even
> > start with regular 100kHz less for 400kHz; 50kHz would be very good.) Your
> 
> Are you sure? If I remember correctly soft-I2C can even run 400 KHz on
> a slow 50 MHz MPC8xx system.
> 
> Do you have other numbers?

Is something wrong with those numbers?

> > must be executed. And remember, there is probably no instruction cache and
> > you're running off of flash so every instruction fetch is something like
> 
> I tend to say that an U-Boot port where instruction cache is disabled
> is misconfigured and should be fixed :-)

Eh, everything is easy when you have your RAM running and all handicaps
dropped off... Just try to run 100 meters with heavy weights on your legs
and you will see you can not compete with sprinters :)

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

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

[ksi at koi8.net]

On Mon, 16 Feb 2009, Wolfgang Denk wrote:

> Dear Heiko,
> 
> In message <49992BEB.30501@denx.de> you wrote:
> > 
> > >> Yes, thats a point. But do we need this before running from ram (except
> > >> one hardwareadapter)?
> > > 
> > > Yes, see above.
> > 
> > Yes, thats is a problem in my approach, and if we need more then one
> > i2c bus when running from flash, maybe a no go.
> 
> It is not an unsolvable problem. We do have the global data structure,
> and it has been (mis-) used for worse purposes before.

Not so fast... And remember, there are other similar interfaces that are
waiting in the line (e.g. SPI...)

I personally do not think we should use that global data structure for such
obscure purposes. And it must be writable for this to work.

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

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

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
>
>   
>> Dear ksi at koi8.net,
>>
>> In message <Pine.LNX.4.64ksi.0902142104100.6240@home-gw.koi8.net> you wrote:
>>     
>>> OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also
>>> explain how can one invoke a function on other adapter than "current".
>>> Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
>>> so you can NOT change "current" adapter.
>>>       
>> We could assign an entry in the global data for it.
>>
>> But then - how often will it bbe necessary to switch adapters before
>> relocation? What is I2C being used for? To read the SPD data for the
>> RAM init code. Which other adapter would be needed?
>>     
>
> I can not tell it right away. There might be several RAM DIMMs with their
> own SPD EPROMS sitting on different busses or something else. It is not like
> we absolutely need this feature right now, urgently but it is so easy to
> implement that it would've been a crime to not to... :)
>
> There is another problem with choosing a proper i2c_init() function with
> i2c_set_bus_num() -- the latter also reprograms I2C multiplexers/switches if
> there are any so you essentially need an initialized adapter to switch to it
> to initialize it. Catch 22...
>   

That would not be a problem:

if we are running from flash we could always init in i2c_set_bus_num()
first the new
controllor to which we switch. And if we are running in ram we can always
check in i2c_set_bus_num() if adap->init_done in i2c_adap_t is 1. If
this is not
so, we first init the bus! With this, it should be possible to get rid
of all this i2c_init
calls all over the code ...

One requirement for this:
All "old" code who directly call i2c_read, i2c_write, ... must, if this new
feature is activated, call a i2c_set_bus_num() before accessing the bus. But
this would be only "work" (grep and fix).

bye

Heiko

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

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902162123560.27482@home-gw.koi8.net> you wrote:
> 
> > But then - how often will it bbe necessary to switch adapters before
> > relocation? What is I2C being used for? To read the SPD data for the
> > RAM init code. Which other adapter would be needed?
> 
> I can not tell it right away. There might be several RAM DIMMs with their
> own SPD EPROMS sitting on different busses or something else. It is not like

I have never seen any such design in  real  life,  and  I  would  not
hesitate  to  call  it broken. We should not make the software design
unnecessarily complet just to support any worst case situations  that
might  eventually  happen. It's perfectly sufficient to cover 99.999%
of all existing systems :-)

> we absolutely need this feature right now, urgently but it is so easy to
> implement that it would've been a crime to not to... :)

Well, it's not so easy - it adds a lot of complexity, it seems.

> There is another problem with choosing a proper i2c_init() function with
> i2c_set_bus_num() -- the latter also reprograms I2C multiplexers/switches if
> there are any so you essentially need an initialized adapter to switch to it
> to initialize it. Catch 22...

One more reason not even to attempt to support such configurations.

> > > Please also note that you will loose a capability of working with more than
> > > one adapter before the code is relocated to RAM.
> > 
> > I don't see this.
> 
> Why? There is no writable global variables before relocation...

Of course there are. Guess what the "global data" strcuture has been
invented for?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It would be illogical to assume that all conditions remain stable
	-- Spock, "The Enterprise" Incident", stardate 5027.3

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902162132540.27482@home-gw.koi8.net> you wrote:
> 
> > It is our task as software engineers to tell the  hardware  designers
> > which  designs  are  easy to support, nd which will cause problems in
> > the software.
> 
> It is not complex. Quite in the contrary, it is much simpler and more
> straightforward to have 2 sets of functions for 2 adapters in one place than
> to have one that includes both of them in one place switchable using some
> global variable at another place used by wrappers in a third place.

To code I've seen so far looked pretty complex, and the fact that
we've spend many rounds of discussion here seems to indicate a certain
level of cmplexity, too.

> > > There is also another consideration -- when having several adapters which
> > > one should be initialized at boot time, before relocation? Another problem
> > 
> > The one that is needed there, if any at all.
> 
> You will have to pick that one for each and every board somehow...

Only for those boards that need it, whichis a tiny fraction of all
boards which use I2C. And there,it should be trivial to configure in
the board config file.

> > Please keep in  mind  that  according  to  U-Boot  philosophy  it  is
> > forbidden to always initialize all adapters. Only those actually used
> > by U-Boot may be initialized, and shall be de-initialized after use.
> 
> Eh, I don't see any problem initializing all I2C controllers at the same
> time. What is the problem with that?

Why would you do that? It makes little sense to initialize components
that may never be accessed at all.

> Then, we _DO_ already initialize _ALL_ controllers in U-Boot. Most of the
> time the total number of controllers is 1 and we do initialize all of them.
> In those rare cases when that number is not 1, we _DO_ initialize all of
> them (look at fsl_i2c.c as it is in the main tree right now.) Existing
> i2c_init() function does not have any provision for initializing any
> particular controller; it initializes them all.

When we re-implementing that code, we should fix that error instead
of copying it.

> > > Another reason why macros are used is speed. Not everyone is running U-Boot
> > > on 10 GHz Pentium-9 with gigabyte of cache. In bitbanged I2C every
> > > instruction counts if you want to run a bus at a decent speed (I won't even
> > > start with regular 100kHz less for 400kHz; 50kHz would be very good.) Your
> > 
> > Are you sure? If I remember correctly soft-I2C can even run 400 KHz on
> > a slow 50 MHz MPC8xx system.
> > 
> > Do you have other numbers?
> 
> Is something wrong with those numbers?

You seem to claim that soft-I2C cannot run at 400 kHz clock. I doubt
that claim.

> > I tend to say that an U-Boot port where instruction cache is disabled
> > is misconfigured and should be fixed :-)
> 
> Eh, everything is easy when you have your RAM running and all handicaps
> dropped off... Just try to run 100 meters with heavy weights on your legs
> and you will see you can not compete with sprinters :)

We're at the point: who needs I2C before relocation to  RAM?  Only  a
small fraction of all boards. How many of these use soft-I2C? An even
smaller  fraction. For how many of these is it critical to run at 100
kHz I2C clock or faster? Well, I think the answer is none here.

We're wasting time and efforts on a non-issue.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He had been eight years upon a project for extracting sunbeams out of
cucumbers, which were to be put in vials hermetically sealed, and let
out to warm the air in raw inclement summers.        - Jonathan Swift
              _Gulliver's Travels_ ``A Voyage to Laputa, etc.'' ch. 5

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

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0902162152530.27482@home-gw.koi8.net> you wrote:
> 
> > It is not an unsolvable problem. We do have the global data structure,
> > and it has been (mis-) used for worse purposes before.
> 
> Not so fast... And remember, there are other similar interfaces that are
> waiting in the line (e.g. SPI...)

So what?

SPI is an even more exotic interface, with even less need to use it
ever before relocation to RAM. Let's not artificially construct new
problems when there are none.

> I personally do not think we should use that global data structure for such
> obscure purposes. And it must be writable for this to work.

"obscure purposes"? Hm... The global data structure was  created  for
this   very  purpose:  to  provide  writable  global  storage  before
relocation. 

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Bradley's Bromide: If computers get too  powerful,  we  can  organize
them into a committee - that will do them in.