From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 01 Oct 2013 08:05:02 +0200 Subject: [U-Boot] [PATCH 0/3] i2c: improve s3c24x0 with High-speed and new SYS_I2C framework support In-Reply-To: References: <1363085897-19814-1-git-send-email-ch.naveen@samsung.com> <1380524290-9644-1-git-send-email-ch.naveen@samsung.com> <524930DB.3050703@denx.de> Message-ID: <524A660E.40908@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 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 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 ... >>> 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? >> 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++; } 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 ... bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany