From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Sun, 15 Feb 2009 09:15:54 +0100 Subject: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C In-Reply-To: References: <499526CD.5080206@denx.de> <49968511.40309@denx.de> Message-ID: <4997CF3A.7060902@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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