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

[ksi at koi8.net]

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.

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

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

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.

And what are the advantages?

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

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.

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.

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

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

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

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

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

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

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

But again, that is a separate issue. We do NOT have anything yet, just a
cloned repository with no new source.

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

> >>> And yes, we DO have some boards with switched I2C busses in U-Boot main tree
> >>> so this is NOT a hypothetical situation.
> >> Yes, and they add i2c busses frem env variables, which you dropped ...
> > 
> > Again, I can't see any reason for such a feature. If you want to attach
> > something to the already running board and do some accesses you can easily
> > do it with existing commands. If you want to use it for U-Boot itself, then
> > it does NOT belong to the environment; it belongs to board config file.
> 
> I think, you don;t read my EMails ...

I don't know what is the exact reason for such weird approach. Also those
EPROMs must be preprogrammed to be used for bus topology and that is more
expensive option than using a different U-Boot image.

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

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