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