[U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4

[ksi at koi8.net]

On Fri, 20 Feb 2009, Heiko Schocher wrote:

> Hello ksi,
> 
> ksi at koi8.net wrote:
> > On Thu, 19 Feb 2009, Heiko Schocher wrote:
> > 
> >> Hello ksi,
> >>
> >> ksi at koi8.net wrote:
> >>> On Wed, 18 Feb 2009, Heiko Schocher wrote:
> >>>
> >>>> 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.0902142019520.6240 at home-gw.koi8.net> you wrote:
> >>>>>>> That means you have to make changes in two places instead of one -- config
> >>>>>>> file AND $(BOARD).c. Also you use functions instead of macros and you can
> >>>>>>> NOT make them inline because they come from a separate object file. This
> >>>>>>> essentially defeats the very purpose of that common soft_i2c.c driver. If
> >>>>>>> you want to make functions for bitbanged I2C into the $(BOARD).c there is no
> >>>>>>> reason to have them as a base for that driver. It is much more logical to do
> >>>>>>> everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> >>>>>>> drivers and those I2C_SDA and friends as its building blocks make those
> >>>>>>> i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
> >>>>>>> build the actual driver in the $(BOARD).c itself. Just convert that
> >>>>>>> soft_i2c.c into a header file with macros for real functions (soft_i2c_read
> >>>>>>> etc.) and instantiate them in the $(BOARD).c.
> >>>>>> Ecept that the code you posted is unreadable and you will need lots of
> >>>>>> very good arguments to make me accept it.
> >>>>> What is unreadable in that code?
> >>>> I wouldn;t say unreadable but unnecessary swollen.
> >>>>
> >>>>> Take e.g. this:
> >>>>>
> >>>>> === Cut ===
> >>>>> #define I2C_SOFT_SEND_START(n) \
> >>>>> static void send_start##n(void) \
> >>>>> { \
> >>>> [...]
> >>>>>         I2C_DELAY2;
> >>>>>         I2C_SDA2(0);
> >>>>>         I2C_DELAY2;
> >>>>> }
> >>>>> === Cut ===
> >>>>>
> >>>>> This will be generated at compile time and fed to gcc.
> >>>>>
> >>>>> What is so unreadable here?
> >>>>>
> >>>>> Sure I can make all the instances manually and avoid those #define's but it
> >>>>> will not make that source file any more readable by simply repeating those
> >>>>> functions several times with just that "##n" different. And it will make
> >>>>> that source file 4 times bigger with 4 instances or twice as big if there
> >>>>> are only two of them.
> >>>> Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t
> >>>> have to change anything in this driver (I posted such a patch as a proposal)
> >>>>
> >>>> And again, you don;t need to do, as i did in this proposal, make this
> >>>> I2C_SDA, ... in function. You can of course make this in macros. OK, you
> >>>> have one more if but that shouldn;t be such a problem!
> >>> What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building
> >>> blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_
> >>> parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for
> >>> different adapters. There is _NOTHING_ common between them.
> >> How SDA, SCL are get/set is common, just how SDA and SCL are accessed is
> >> different! So there is no need for different i2c_write(), ... only SDA,SCL
> >> accessors are different.
> > 
> > Argh... Do you understand that those send_start etc. are _NOT_ the
> > functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
> > 
> > Template is _NOT_ a code, it is a _TOOL_ that generates code.
> 
> I know this.
> 
> >>> In my case you simply make N i2c_write() functions for N adapters and assign
> >> Thats not needed.
> > 
> > Hm... Please explain how are you going to use 2 different sets of pins with
> > different access methods with one function?
> 
> I explained you this in more than one EMail. You have one more "if" in
> your SDA, SCL functions/macro. And don;t say to me, your processor is not
> fast enough to do one more if for every SDA, SCL access. When running u-boot,
> you have full 100% from your CPU for doing this. If your CPU is to
> slow to do this, I bet you have no fun when starting Linux. And look
> deeper in the i2c-bitbang driver from linux. When using this with
> the gpio API you get for every SDA, SCL access a function call, in
> this fn you must decide which pin, and what state for that pin ->
> min 2 "if" ... and so we are not worser than Linux.

Why would we need _ADDITIONAL_ ifs in extremely simple macros that just
access some memory/io location? They are so simple and small that you can
not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_
make them any smaller. You can _NOT_ exclude actual code from them, you can
only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have
_ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from
them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in
those macros.

But that is not all. You make those macros in function-like ones so you have
to pass an argument to them. That requires for that argument to come from
somewhere. For that you add an additional external global variable and
additional member to the i2c_adap structure.

Less for extreme ugliness of using an external global variable in object
methods that work on that particular object, you add a whole bunch of
totally unnecessary entities. You are _NOT_ removing anything, you _ADD_
things.

Look, every adapter is represented by its very own structure with pointers
to its own methods. There is absolutely no need for anything else, each such
object (I2C adapter) has its own methods (accessor functions or whatever)
and its own set of local variables. You pick the object then call its method
that already knows how to deal with that very object.

Now you propose to scrap all that and replace object methods with some
generic ones that do _NOT_ know what they are supposed to work on. There is
no "this" pointer in C so you have to somehow let 'em know what they are
supposed to work on. For this you add a HORRENDEOUS kludge, an external
global variable. But the story does not end here, that variable does _NOT_
hold that argument, it is just some index in the _GLOBAL_ array of objects.
To get that argument you add yet another entity, hwadapnr or whatever to the
object and make yet another level of indirection to get that argument.

Here is the path to the real action in my case:

bus_no -> adap_no -> function -> action


Here is yours:

bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action


Not only that makes the path longer, not only it requires a blasphemy of
using a global variable in object methods, but it also defeats a purpose of
having a separate object for each adapter. Why do we need those separate
objects if they contain exactly the same poiners to the very same functions?

There is already an index into the objects array and it is already used to
choose a proper object. Why should we use that index again in the already
chosen object? Why go twice over the same rake? And even if we were going
that way what do we gain by using such horrible kludges? Is there any
benefit?

> >>> pointers to those functions to appropriate adapter struct members at
> >>> _COMPLILE_ time. And that's all to it.
> >>>
> >>> In your case you stick those functions in one monstrous i2c_write() where
> >>> you still have those N functions as "case" bodies of some switch so you
> >> No, just the SDA,SCL accesses have such a switch.
> > 
> > So we should make a monster with switches off of each and every send_start()
> > and friends and pass them a value to decide which set to use?
> 
> It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent
> you a soft-i2c driver proposal. If you look in this you see, no fn
> in it is changed!
> 
> We want this, because we don;t need to change everything in Sourcecode,
> as Wolfgang mentioned to you.

But you _DO_ want to change something! It is impossible to change something
without changing it!

Furthermore, you also must change everything because you'll have _ALL_ the
functions and templates in that same driver to take an additional argument
and pass it all the way down to the bare metal, those accessor macros! That
means you will have to add an access to that global variable to each and
every I2C method (i2c_read etc.,) then change all utility functions
(send_start etc.) to take that argument and pass it further to those macros.

And that will _NOT_ make resulting code any smaller because you'll still
have all the actual guts of all those macros _PLUS_ register loads for
arguments you are passing _PLUS_ conditional branches in each and every
macro (and they will be multi-step if you have more than 2 adapters...)

I fail to see how this can be simpler, or better, or smaller, or any more
logical than just multiplying that code ADAPTER_NUMBER times and use
separate methods for each adapter.

Then comes an issue of where those macros should come from. In existing code
you simply define methods to set/reset/tristate particular pins for separate
adapters in separate macros.

In your case instead of N sets of such macros you must put a single set but
each macro will have _ALL_ those macros in a switch statement with a
separate case for each adapter. It is wrong to put a complex C code in a
config file and it is much more prone to errors (think about all those macro
expansion pitfalls that are aplenty) and more difficult to comprehense.

But there is not the end of a story yet... There is another thing that is
probably overlooked. When adding an adapter to the config file one must
define a separate set of macros for it in my case. If he forgot to define
some macro U-Boot build will fail with "XYZ undefined" so it will be
immediately known and very easy to fix.

In your case there will be just one set of macros with actual guts going
into "case" statements of that omnipresent switch. When one adds an adapter
he must add another "case" with the guts to that switch. The problem is that
it will be still perfectly legal C code that will build without any errors
of warnings if it is forgotten. That makes it much more difficult to debug.

That all is just basic principles of good engineering, not something special
to U-Boot, C, or programming.

> > That makes everything more messy and has a performance penalty -- instead of
> > simple branch to those simple blocks you are adding at least one register
> > load per call to load that argument.
> 
> Look above. If speed is here your problem, you have a lot of other
> problems with that, when running Linux.
> 
> > And what are the advantages?
> 
> No Sourcecode change is needed.

How can you change sourcecode without changing sourcecode?

> >>> still have that same code size _PLUS_ switch. Than, you assign a pointer to
> >>> that monstrous function to i2c_write() member of _ALL_ adapter structures so
> >>> a call to i2c_write() ends up calling that function. Now you have to somehow
> >>> find out which switch case to execute. For that you need an additional
> >>> global variable and additional member in each adapter struct. And those must

> >> No problem with this.

Less for regular principles of good engineering there are other problems
some of which are described above.

> > There IS a problem First, that means you have _ABSOLUTELY_ no way to access
> > any other adapter than default one until that variable made writable.
> 
> And we make it writeable, so no problem. But you have the _same_ problem,
> and as I told you. + you cannot switch actually busses with your approach!
> 
> > Second, all class functions must be self-sufficient, they should NOT rely on
> > some external global variable to work. That might sound C++ese but no matter
> > how I don't like C++ it does many things right.
> 
> Hmm.. think this is not so a big problem here.
> 
> >>> be writeable because you don't have any means of executing something like
> >>> "adap[N]->i2c_write(....)", you must prepend it with i2c_set_bus_num(M)
> >>> because in your case that "N" in "adap[N]" has absolutely no effect...
> >> ? you again mixing thinks. With my approach no need for changing the API
> >> in i2c-core.c. I think, thats you don;t got.
> > 
> > No, it is you who didn't get it. As we russians use to say, the bride is
> > very good, just a little bit pregnant. You want to use a global variable in
> > I2C object member functions. Global variables are _EVIL_ by theirself and
> > should be only used as last resort, but in member functions they are not
> > just plain evil, they are absolute NO-NO.
> 
> see above.
> 
> >>> Please explain how it is better than simple and straightforward function per
> >>> adapter? Which one is more complex?
> >>> It looks like I simply don't understand something. You must've meant
> >>> something else that I didn't get because the above comparison is SO obvious
> >>> that it is almost impossible to somehow misunderstand it...
> >>>
> >>>>> Why should we avoid using CPP feature that is SPECIALLY made for cases like
> >>>>> this?
> >>>> What CPP feature?
> >>> Source file preprocessing.
> >> I think you mean gcc, right?
> > 
> > No, I mean CPP. GCC is a frontend for cpp, as, ld etc. It does NOT
> > preprocess files, cpp does.
> 
> Ah, okay, thanks.
> 
> >>>>> Not rocket science and not much of black magic, just simple and
> >>>>> straightforward token pasting...
> >>>>>
> >>>>>>> The only problem with that is it breaks uniformity and makes another mess.
> >>>>>>> The whole idea was to bring _ALL_ I2C drivers to a single place and make
> >>>>>>> them totally transparent and uniform. Something like e.g. Linux VFS.
> >>>>>> This is a boot loader with limited resources, not a general purpose
> >>>>>> OS.
> >>>>> It doesn't matter. It is much better to have a uniform API for all the
> >>>>> future developers to use than to multiply horrible hacks and reinventing the
> >>>>> wheel again and again.
> >>>> ? We didn;t want to change the API, you mix things. We only want to
> >>>> prevent such a define monster in the bitbang driver.
> >>> Make several of those for several drivers if it looks easier. Multiply it.
> >> No need for it.
> > 
> > Please show me the code.
> 
> which code? The soft-i2c driver I posted here as a proposal, also how a
> I2C_SDA macro can look, if you have more than one bitbang driver. Look
> in the archive.

Please see above. It is not just plain ugly and violates good engineering
basics, it has a bunch of problems. And there is absolutely no benefits.

Those guys who wrote that driver as a template, generating actual code from
a set of configuration macros did a wonderful job and I can't see any valid
reason to reinvent the wheel.

> >>>>>>> And remember, the devil is in details. How are you going to assign
> >>>>>>> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> >>>>>>> going to work on an adapter other that "current" in a situation when you can
> >>>>>>> NOT change "current" adapter (e.g. perform all I2C layer initialization
> >>>>>>> while still running from flash?) Remember, this is plain C and there is no
> >>>>>> What makes you insist that we cannot change a variable if we need to
> >>>>>> be able to change one?
> >>>>> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> >>>>> number of busses can be bigger than number of adapters (e.g. when some
> >>>>> busses a reached via muxes or switches.) When doing i2c_set_current_bus()
> >>>>> you are switching _NOT_ adapters, but busses. That involves not only
> >>>> What has this to do with soft_i2c.c?
> >>> Please read above.
> >> So you to ;-)
> >>
> >>>>> changing that global variable but also reprogramming muxes/switches for
> >>>> Yes, and this is independent of changing also this current pointer.
> >>> No, it is _NOT_.
> >> It is.
> > 
> > That means you have another design for that. Please explain it.
> 
> I explained it. I posted the soft-i2c driver, also how i2c_set_bus_num can
> look, to support switching busses and init hardware adapters from it.
> 
> >>>>> i2c_set_current_bus() to be consistent and hardware independent. Otherwise
> >>>> It is this also with changing this current pointer!
> >>> No, it is _NOT_.
> >> It is.
> > 
> > Please show your code.
> 
> posted here.
> 
> >>>>> your code should know if that particular bus it is switching to is directly
> >>>>> connected or switched and check the bus it is switching from for muxes. If
> >>>>> they are switched, your code should disconnect the current bus switches,
> >>>> Yes, and this you did perfectly in i2c-core.c, where is your problem?
> >>> The problem is you can _NOT_ change the bus if adapter is not initialized
> >>> and you can _NOT_ initialize adapter because you _MUST_ change the bus to do
> >>> this. No matter running from RAM or from ROM, you have exactly the same
> >>> Catch 22.
> >> You have the same problem. I wrote you in an EMail, that your i2c_set_bus_number()
> >> will not work from flash, so how works this for you?
> > 
> > I can call a member function for any adapter directly if needed with
> > adap[N]->function(). You can NOT because in your case that "N" does not have
> > any effect and your functions rely on an external global variable that you
> > can not change.
> > 
> > BTW, in my design i2c_cur_bus variable is NOT global. It has file scope.
> 
> Okay.
> 
> >>>>> then do that i2c_set_current_bus() and connect the switches to the new bus
> >>>>> after that.
> >>>> I don;t understand you know, really. Nobody in this discussion criticize
> >>>> the API, we just discuss the soft_i2c.c driver, and how we can prevent
> >>>> this defines ... or I lost something ...
> >>> You can _NOT_. The soft_i2c.c file is _ALREADY_ a template, that builds
> >>> actual source code from set of external defines. One more time -- it is
> >>> _ALREADY_ a template.
> >> Yes, I know! But different adapters just differnet in how they access
> >> SDA, SCL not in i2c_write,..
> > 
> > That i2c_write uses a bunch of helper functions that are GENERATED from
> > I2C_SDA and other macros. All those helper functions are different for
> > different pin sets. And there is no compilable source code for them in
> > soft_i2c.c (_EXISTING_ one, not just new,) only TEMPLATEs. Function is
> > something you can get an address for. Templates do NOT have such address.
> > You can NOT make a pointer to a template.
> 
> I give up.
> 
> >>> I did nothing to that file, just added an option of generating several
> >>> drivers instead of one. There is absolutely nothing I changed in that driver
> >>> per se, that is _EXACTLY_ the same code.
> >>>
> >>> You can make N such files for N adapters, or you can multiply those
> >>> functions in that file N times to make N adapters. I'm doing the latter and
> >>> nothing else.
> >>>
> >>> What are you trying to prevent? I simply don't understand. Do you want me to
> >>> just make several sets of functions in soft_i2c.c file because those defines
> >>> look scary? No problems, just say it and I will do that. But there is no
> >>> need for reinventing a wheel or adding sophisticated crutches to the
> >>> obvious...
> >>>
> >>>>> That means that code MUST somehow know the topology to take appropriate
> >>>>> actions and properly configure those switches. That means you should somehow
> >>>>> describe that topology for each and every board in CONFIG_* terms and make
> >>>>> each and every place at U-Boot that invokes _ANY_ i2c function to take care
> >>>>> of that switching.
> >>>> Yep, this we(you did it ;-) have this in i2c-core.c ...
> >>>>
> >>>> (And, I want to start this discuss again, you just dropped the support for
> >>>> adding new such busses per command shell ... you could not do this! But
> >>>> I have a solution for this on top of your patches, but I want start this
> >>>> discussion, if we have your patches in a testing branch in u-boot-i2c.git)
> >>> We'll return to those dynamic busses later. I personally can't see any
> >>> viable reason for that.
> >> Maybe you not, but I told you, that there is a hardware manufacturer, that
> >> has his DTTs, EEProms,... on different hardware on different I2C busses.
> >> With this dynamic busses, he can have one image for all his boardvariants,
> >> and this is a need.
> > 
> > That's a separate issue. I can offer N other ways to achieve this other than
> > using that horror. And if they use the same binary image for different
> > boards those boards are not different. If they put a chip on different bus
> > that means the hardware is different. Different hardware means quite a
> > lengthy process involving changes to the schematics, then re-layout, new set
> > of gerbers, new PCBs, new BOM, new P&P programs etc. Such an effort
> > definitely deserves a separate config file and simple repompile of U-Boot
> > that takes less than a minute time.
> 
> Thats your opinion, but there are others you cannot ignore.
> 
> > But again, that is a separate issue. We do NOT have anything yet, just a
> > cloned repository with no new source.
> 
> I agree here with you. First let us make some base, and then we
> can go on with that issue.
> 
> >>>>> My code does it transparently in the single place, i2c_set_current_bus() so
> >>>>> higher level code doesn't have to bother with details.
> >>>> Again, what has this to do with introducing a current pointer?
> >>> It solves Catch 22.
> >>>
> >>>>> Then, all those I2C multiplexers/switches are I2C devices theirself that
> >>>>> means you can NOT talk to them if the adapter they connected to is not
> >>>>> initialized.
> >>>> Ok, come, read my previous EMail, you can init this adapter before
> >>>> switching the muxes.
> >>> You can _NOT_. Please read above.
> >> Why?
> > 
> > Because you use an external global variable to choose which adapter's
> > function to call and that variable is not writable. You can NOT use
> > adap[N]->function() because in your case that N does not have any effect.
> 
> But we can make this variable "writeable", so there is no problem with
> that.

There is a problem of multiplying entities beyond necessity. Entia non sunt
multiplicanda praeter necessitatem. Occam's razor. It does _NOT_ solve any
problems, does not give any benefits, and creates additional problems.

> >>>>>>> You are adding unnecessary complexity to the code. And you break uniformity.
> >>>>>> He. I must have thought the same before about someone else's code ;-)
> >>>>> Eh, I'm trying to make things simpler... For that particular board I'm
> >>>>> expecting from assembly house by the end of this week I can make its
> >>>>> particular hardware work with a bunch of one-time hacks in its $(BOARD).c... 
> >>>>>
> >>>>> But I think I'm not the first one to face such a problem and not the last
> >>>>> one. So why wouldn't we make the proper API to get rid of all those hacks? I
> >>>>> can do it now while paid by my current employer but there is no guarantee my
> >>>>> next one would allow for such a waste from business standpoint...
> >>>> I don;t understand why you have such problems with introducing a current
> >>>> pointer. And again, that has nothing to do with the API.
> >>> We do already have current bus. There is absolutely no need for such a
> >>> pointer. Occam's razor.
> >> OK, do we not make a pointer, just an integer, but it is the same
> >> problem, the integer must also be writeable! How would you change busses
> >> when running in flash?
> > 
> > I'm not going to change busses when I'm running from flash. But I can call
> > functions on any adapter I want if needed.
> 
> Is it needed?

It might be. In my case there is nothing special required for this, this
comes as a free extra.

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