Hello Heiko, On 1 October 2013 11:35, Heiko Schocher <h...@denx.de> wrote: > Hello Naveen, > > Am 30.09.2013 12:03, schrieb Naveen Krishna Ch: > >> Helo Heiko, >> >> Thanks for timely reply. >> >> On 30 September 2013 13:35, Heiko Schocher<h...@denx.de> wrote: >>> >>> Hello Naveen, >>> >>> Am 30.09.2013 08:58, schrieb Naveen Krishna Chatradhi: >>> >>>> This patchset fixes few bugs in the existing s3c24x0.c code (standard >>>> i2c) >>>> and add support for High-speed i2c bus controller available on Exynos5 >>>> SoCs >>>> from Samsung. >>>> >>>> Exynos5250 channels [0 ~ 3] can configured for I2C contoller or >>>> new HSI2C controller >>>> channels [3 ~ 7] are standard I2C controller channels >>>> Exynos5420 channels [0 ~ 3] are standard I2C controller channels and >>>> channels [4 ~ 10] are High-speed controller channels >>>> >>>> Patchset: >>>> 1. exynos: i2c: Fix i2c driver to handle NACKs properly >>>> Improvements and fixes from Vadim Bendebury for standard i2c calls >>>> >>>> 2. exynos: i2c: Change FDT bus setup code to enumerate ports correctly >>>> FDT bus setup code from Simon Glass >>>> >>>> 3. PATCH v5: i2c: s3c24xx: add hsi2c controller support >>>> High-speed controller register description and defines new i2c >>>> read/write, >>>> probe and set_bus calls. >>>> >>>> This is been reviewed earlier at >>>> http://lists.denx.de/pipermail/u-boot/2013-May/153245.html >>>> Thanks for review and improvements from Vadim Bendebury. >>>> >>>> Question: >>>> 4. RFC: samsung: i2c: Enable new CONFIG_SYS_I2C framework >>>> I've tried to implement the new I2C multi bus framework in u-boot tree, >>>> taking tegra-i2c.c as reference. > > [...] > >>>> b). When i define 11 buses as in the case of Exynos5420, the "i2c bus" >>>> lists them >>>> SMDK5420 # i2c bus >>>> Bus 0: s3c0 >>>> Bus 1: s3c1 >>>> Bus 2: s3c10 >>>> Bus 3: s3c2 >>>> Bus 4: s3c3 >>>> Bus 5: s3c4 >>>> Bus 6: s3c5 >>>> Bus 7: s3c6 >>>> Bus 8: s3c7 >>>> Bus 9: s3c8 >>>> Bus 10: s3c9 >>>> or (If i change the name to hsi2c) >>>> SMDK5420 # i2c bus >>>> Bus 0: hsi2c10 >>>> Bus 1: hsi2c4 >>>> Bus 2: hsi2c5 >>>> Bus 3: hsi2c6 >>>> Bus 4: hsi2c7 >>>> Bus 5: hsi2c8 >>>> Bus 6: hsi2c9 >>>> Bus 7: s3c0 >>>> Bus 8: s3c1 >>>> Bus 9: s3c2 >>>> Bus 10: s3c3 >>>> >>>> Whats the expected behaviour. If the above result is correct, I need to >>>> changei >>>> the strings to get them in the correct order. >>> >>> >>> >>> What, if you use two digits: >>> >>> Bus 0: hsi2c01 >>> Bus 1: hsi2c02 >>> [...] >>> Bus 7: s3c00 >>> Bus 8: s3c01 >>> [...] >>> >>> ? >>> >>> Or with one digit: >>> >>> Bus 0: hsi2c1 >>> Bus 1: hsi2c2 >>> [...] >>> >>> Bus 7: s3c0 >>> Bus 8: s3c1 >>> [...] >> >> In the Exynos5420 hardware >> channels are as below >> >> channel: >> --0----1----2----3------4--------5---------6-------7--------8-------9-------10 >> controller: i2c, i2c, i2c, i2c, hsi2c, hsi2c, hsi2c, hsi2c, hsi2c, hsi2c, >> hsi2c. >> But the "i2c bus" command is showing the bus number according to the >> "name" >> string comparison. Which seems wrong. Isn't it ?? > > > Hmm.. you are right, seems that the compiler sorts them alphabetical ... > So, two ways: > - use another name, saying first a two digit for the channel ? > - or, use CONFIG_SYS_NUM_I2C_BUSES, CONFIG_SYS_I2C_DIRECT_BUS > and CONFIG_SYS_I2C_BUSES as described in the README (grep > for CONFIG_SYS_I2C_BUSES in include/configs and you will find > some examples ...) and you can define, which adapter is on which > i2c_bus number ... Will try and implement it this way. > > >>>> c). What's the alternative for the >>>> board_i2c_init(), i2c_get_bus_num_fdt(), i2c_reset_port_fdt(). >>>> "Functions to get the I2C bus number and reset I2C bus using FDT >>>> node" >>>> >>>> I think, these functions are still needed. >>> >>> >>> >>> Hmm.. that needs a general discussion, how to use fdt and i2c I think. >>> >>> I would prefer a way (not really nowing, if it is possible for all >>> configurations) where, if using fdt, the DT gets parsed and the >>> availiable i2c buses gets created ... After that, "normal" i2c access >>> with i2c_set_bus_num(), i2c_read/write should be possible ... this is >>> currently not possible with the i2c framework, but should not be so hard >>> to do. Except the restriction, that it would not work in SPL case, or >>> before relocation for i2c buses announced through dt >> >> once i2c_init_board() is done board_i2c_init() is not quite needed >> using i2c_init_board we can avoid the relocation problem aswell. >> >> by the definition of i2c_get_bus_num() in drivers/i2c/i2c_core.c >> >> unsigned int i2c_get_bus_num(void) >> { >> return gd->cur_i2c_bus; >> } >> >> we don't need a special function i2c_get_bus_num_fdt() > > > Ah, ok! > > >> IMHO, i2c_reset_port_fdt() is the only function to be discussed > > > And why is i2c_init() not good? Why must we have here a new function? That's right, even if there is a need for i2c_reset_port_fdt(). it must be a i2c-core function instead of being in a driver. > > >>> Define CONFIG_SYS_I2C_MAX_HOPS -> CONFIG_SYS_I2C_DIRECT_BUS is not >>> defined >>> so i2c_bus[] is used in drivers/i2c/i2c_core.c. Define the fix i2c buses >>> in >>> the board config file with CONFIG_SYS_I2C_BUSES (if you have no fix >>> buses, >>> let this empty) and add a function in drivers/i2c/i2c_core.c, which adds >>> new i2c buses to i2c_bus[] after the fix buses and call this new >>> function, >>> from where you interpret the fdt ... >> >> I din't quite understood this. >> Can you point me to some readme or Doc or discussion Please. > > > just the U-Boot README ... The above was just a fast idea, how it > is possible to add i2c buses from the info in the fdt ... > > Here some theoretical code, how it could look like: > > in the board config file add: > > #define CONFIG_SYS_I2C_DIRECT_BUS 1 > #define CONFIG_SYS_I2C_MAX_HOPS 0 > #define CONFIG_SYS_NUM_I2C_BUSES 10 /* maximum possible i2c buses > used on the hw */ > #define CONFIG_SYS_FIX_I2C_BUSES 1 /* just as an example, here with > one fix i2c bus */ > #define CONFIG_SYS_I2C_BUSES { {0}, /* adapter 0 is fix on i2c_bus > number 0 */ > } > > in drivers/i2c/i2c_core.c: > > static int i2c_fix_bus = CONFIG_SYS_FIX_I2C_BUSES; > > /* add one i2c bus without muxes ... ToDo: how to add i2c buses with muxes > */ > static int i2c_add_one_bus(char *adapter_name) > { > struct i2c_bus_hose *tmp; > > if (i2c_fix_bus >= CONFIG_SYS_NUM_I2C_BUSES) > return -ENOMEM; > > /* search adapter with name */ > adap = i2c_search_adapter_name(name); /* Code this function */ > if (adap < 0) > return -ENODEV; > > tmp = kalloc(sizeof(struct i2c_bus_hose)); > tmp->adapter = adap; > /* if found add it into i2c_bus */ > memcpy(&i2c_bus[i2c_fix_bus], tmp, sizeof(struct i2c_bus_hose)); > i2c_fix_bus++; > } Thanks for this explanation. > > And maybe here and there some adaptions for getting this running... > Thinking of i2c_set_bus_num(), there must be now a check for > i2c_fix_bus I think ... > > Adapt the README ... > > And then, from the place where you interpret the fdt, call if > you have the information for one i2c bus, i2c_add_one_bus() ... > > Not sure, if I overlooked here something ...
Will try and do this. Mean while can we get the other 3 patches reviewed. Patch 1: http://patchwork.ozlabs.org/patch/278933/ Patch 2: http://patchwork.ozlabs.org/patch/278931/ Patch 3: http://patchwork.ozlabs.org/patch/278930/ They were tested without the change to the new i2c framework. > > > bye, > Heiko > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany -- Shine bright, (: Nav :) _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
