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

[Heiko Schocher <hs@denx.de>]

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

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

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

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

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.

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

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

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