Hello,
I want now, because the merge window is open again, restart the the multibus/multiadapter discussion.
To have a base for this discusion, I made in the i2c git tree (git://git.denx.de/u-boot-i2c.git) the following new 2 branches:
"multibus":
Patches from Sergey Kubushyn ksi@koi8.net posted in the last merge window. see, http://lists.denx.de/pipermail/u-boot/2009-February/047465.html
They were discussed, but not accepted ... also some CodingStyle comments, changes in patch hierarchic are not done ... I only synced them to actuall code to have a base for the discussion.
"multibus_v2": (I couldn;t post the patches because one of them is bigger than 100k :-( and I cannot split it in 2 patches, because this would break git bisection compatibility. But the patches can be found in git://git.denx.de/u-boot-i2c.git):
Based on the i2c_core from Sergeys patches, added the following suggestions from Wolfgang and me:
- v2 is bisection compatible - commits are in (hopefully) logical blocks
- "cur_adap" added in gd_t: This points allways to the actuall used i2c adapter:
- because gd_t is writeable when running in flash, complete multiadapter/multibus functionality is usable, when running in flash - using a pointer brings faster accesses to the i2c_adapter_t struct and saves some bytes here and there (see later).
- init from a i2c controller: In the "multibus" branch (also in actual code) all i2c controllers, as a precaution, getting initialized. In the "multibus_v2" branch, a i2c controller gets only initialized if it is used. This is done in i2c_set_bus_num().
Actually, I let the i2c_init_all() in code, but just call in this function i2c_set_bus_num(CONFIG_SYS_SPD_BUS_NUM). This can be dropped in a second step completely.
Also, with this approach, we can easy add in a second step, a i2c_deinit() function in i2c_set_bus_num(), so we can deactivate a no longer used controller.
- added "hw_adapnr" in i2c_adapter_t struct: when for example a CPU has more then one i2c controller we can use this variable to differentiate which controller the actual i2c adapter uses. This results in lower codesize and lower sourcecode changes. Example:
fsl_i2c driver: (MPC8548CDS only with fsl driver) multibus:
[hs@pollux u-boot-i2c]$ ./MAKEALL MPC8548CDS Configuring for MPC8548CDS board... fsl_i2c.c: In function '__i2c_init': fsl_i2c.c:173: warning: assignment discards qualifiers from pointer target type text data bss dec hex filename 222168 17344 27256 266768 41210 ./u-boot
multibus_v2:
[hs@pollux u-boot-i2c]$ ./MAKEALL MPC8548CDS Configuring for MPC8548CDS board... text data bss dec hex filename 222080 17344 27256 266680 411b8 ./u-boot
also the bitbang driver gets better maintainable:
Sourcecode changes:
multibus:
drivers/i2c/soft_i2c.c | 704 ++++++++++++++++++++---------------
multibus_v2:
drivers/i2c/soft_i2c.c | 155 +++++---
Codesize bitbang driver: (MPC8548CDS with the following bus topology: * Busses: * * 0: Direct off of FSL_I2C[0] * 1: FSL_I2C[1]->PCA9542(0)->PCA9542(0) * 2: FSL_I2C[1]->PCA9542(0)->PCA9542(1) * 3: FSL_I2C[1]->PCA9542(1) * 4: Direct off of SOFT_I2C[0] * 5: Direct off of SOFT_I2C[1] )
multibus:
[hs@pollux u-boot-i2c]$ ./MAKEALL MPC8548CDS Configuring for MPC8548CDS board... fsl_i2c.c: In function '__i2c_init': fsl_i2c.c:173: warning: assignment discards qualifiers from pointer target type text data bss dec hex filename 227092 17864 27256 272212 42754 ./u-boot [hs@pollux u-boot-i2c]$
multibus_v2:
[hs@pollux u-boot-i2c]$ ./MAKEALL MPC8548CDS Configuring for MPC8548CDS board... text data bss dec hex filename 225048 17868 27256 270172 41f5c ./u-boot [hs@pollux u-boot-i2c]$
For this I call for all I2C_SDA, I2C_SCL, ... defines, which holds the board specific code, now functions (This is not a must, but I did this as an example see MPC8548CDS). In this functions I use "hw_adapnr" to switch to do the adapter specific gpio settings ...
If we look at the "multibus" approach, there, this is not needed but Sourcecode size and Codesize grows with each bitbang adapter ... and if I think for a board with n bitbang adapter (with n > 5) this results in nearly unmaintable source code ...
I just write this here because this has to be discussed (and this was a point, which drove the last discussion in chaos). We can now easy compare such a board ... see MPC8548CDS) and discuss on facts, which version we want to have in mainline.
- adding a base_addr to i2c_adap_t struct (Did this not yet, but would also a good thing to have, because when I ported for example the ppc_4xx i2c hardware adapter, I saw, that some CPUs have more than one controller, and they just differ in the base addr, so this variable would be a "good to have"). I solved this actually in adding a function in this driver, which returns the base addr depending on hw_adapnr, which is a suboptimal way ...
see for an example new ppc_4xx i2c driver:
in drivers/i2c/ppc4xx_i2c.c: ppc4xx_get_base ()
Codesize:
for a board with one soft_i2c driver:
[hs@pollux u-boot-i2c]$ git checkout master Switched to branch "master" Your branch is ahead of 'origin/master' by 143 commits. [hs@pollux u-boot-i2c]$ ./MAKEALL CPU86 Configuring for CPU86 board... ... booting from 64-bit flash text data bss dec hex filename 146572 22068 24144 192784 2f110 ./u-boot [hs@pollux u-boot-i2c]$ git checkout multibus Switched to branch "multibus" [hs@pollux u-boot-i2c]$ ./MAKEALL CPU86 Configuring for CPU86 board... ... booting from 64-bit flash text data bss dec hex filename 147816 22124 24144 194084 2f624 ./u-boot [hs@pollux u-boot-i2c]$ git checkout multibus_v2 Switched to branch "multibus_v2" [hs@pollux u-boot-i2c]$ ./MAKEALL CPU86 Configuring for CPU86 board... ... booting from 64-bit flash text data bss dec hex filename 147688 22128 24144 193960 2f5a8 ./u-boot [hs@pollux u-boot-i2c]$
for a board with the fsl driver:
[hs@pollux u-boot-i2c]$ git checkout master ./MA Switched to branch "master" Your branch is ahead of 'origin/master' by 143 commits. [hs@pollux u-boot-i2c]$ ./MAKEALL MPC8360EMDS Configuring for MPC8360EMDS board... text data bss dec hex filename 203504 11588 26900 241992 3b148 ./u-boot [hs@pollux u-boot-i2c]$ git checkout multibus Switched to branch "multibus" [hs@pollux u-boot-i2c]$ ./MAKEALL MPC8360EMDS Configuring for MPC8360EMDS board... fsl_i2c.c: In function '__i2c_init': fsl_i2c.c:173: warning: assignment discards qualifiers from pointer target type text data bss dec hex filename 204288 11628 26900 242816 3b480 ./u-boot [hs@pollux u-boot-i2c]$ git checkout multibus_v2 Switched to branch "multibus_v2" [hs@pollux u-boot-i2c]$ ./MAKEALL MPC8360EMDS Configuring for MPC8360EMDS board... text data bss dec hex filename 204256 11632 26900 242788 3b464 ./u-boot [hs@pollux u-boot-i2c]$
at last here comes a TODO list what should be done:
- add README for the multibus functionality
- list of drivers to port:
drivbers/i2c: bfin-twi_i2c.c mxc_i2c.c omap1510_i2c.c omap24xx_i2c.c tsi108_i2c.c
grep -lr HARD_I2C cpu/ cpu/arm920t/at91rm9200/i2c.c cpu/arm920t/s3c24x0/i2c.c cpu/mpc512x/i2c.c cpu/mpc5xxx/i2c.c cpu/mpc8220/i2c.c cpu/mpc824x/drivers/i2c/i2c.c cpu/mpc8xx/i2c.c cpu/pxa/i2c.c
Hope didn;t forget some more ...
- test, test, test ...
- i2c devices must ported to new multibus functionality. Ideas here are welcome ;-) (They must "know", on which bus they are ...)
- In actuall code it is possible to add new i2c busses with the "i2c bus" command (and from code with i2c_mux_ident_muxstring()) This actual is not included in the new code, but that could be done the following way (for the multibus_v2 version, for the approach in "multibus" branch I have no idea how to make this):
When running in flash, we just can use the i2c busses defined in CONFIG_SYS_I2C_ADAPTERS (should be okay in the first step. When relocating code, the i2c busses in CONFIG_SYS_I2C_ADAPTERS are converted in a dynamcial list, so we can then easy add new busses to this list.
Also we define a "i2c_bus" environment variable, which contains i2c busses, that gets added when relocating. This is needed for the keymile boards. There are a lot of board versions which differ only in the i2c bus topology. With this approach, we can use one u-boot binary for all board versions, just defining the i2c bus topology in the environment ...
BTW: This is also a point for using "cur_adap", because the rest of the multibus functionality doesn;t have to be changed ...
So, I hope I didn;t forget something ... lets start with the discussion ...
bye heiko