From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 31 Jul 2013 07:01:22 +0200 Subject: [U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework In-Reply-To: <51F81261.7020008@wwwdotorg.org> References: <1367668903-29653-1-git-send-email-hs@denx.de> <1367668903-29653-9-git-send-email-hs@denx.de> <51F6946F.8010500@wwwdotorg.org> <51F740FD.4080505@denx.de> <51F81261.7020008@wwwdotorg.org> Message-ID: <51F89A22.3000902@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 Stephen, Am 30.07.2013 21:22, schrieb Stephen Warren: > On 07/29/2013 10:28 PM, Heiko Schocher wrote: >> Hello Stephen, >> >> Am 29.07.2013 18:12, schrieb Stephen Warren: >>> On 05/04/2013 06:01 AM, Heiko Schocher wrote: >>>> From: Simon Glass >>>> >>>> This enables CONFIG_SYS_I2C on Tegra, updating existing boards and >>>> the Tegra >>>> i2c driver to support this. >>> >>> Heiko, the latest U-Boot tree hangs during boot on Tegra, and "git >>> bisect" points at this patch. Olof reported the issue to me. >>> Can you take a look at the code and see what might be wrong? Thanks. >>> >>> I suspect some kind of initialization ordering issue, since the boot >>> messages are: >>> >>> ----- >>> U-Boot SPL 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37) >>> U-Boot 2013.07-rc3-00038-g880540d (Jul 29 2013 - 10:04:37) >>> >>> TEGRA30 >>> Board: NVIDIA Beaver >>> I2C: Caller requested bad clock: periph=-49, parent=2 >>> ----- >>> >>> ... and that "bad clock" message implies to me that the I2C driver is >>> initializing before it has parsed the correct clock ID out of device >>> tree. >> >> Hmm... looking in the patch ... I can see nothing which changes >> some initializing order ... > > Yes, there's some initialization order issue; before this patch, I see > the I2C controller initialization, followed by some usage of it: Ok, great! > ---------- > U-Boot SPL 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47) > U-Boot 2013.07-rc3-00036-gd84eb85-dirty (Jul 30 2013 - 13:04:47) > > TEGRA30 > Board: NVIDIA Beaver > DRAM: 2 GiB > i2c: controller bus 0 at 7000d000, periph_id 47, speed 100000: ok > i2c: controller bus 1 at 7000c000, periph_id 12, speed 100000: ok > i2c: controller bus 2 at 7000c400, periph_id 54, speed 100000: ok > i2c: controller bus 3 at 7000c500, periph_id 67, speed 100000: ok > i2c: controller bus 4 at 7000c700, periph_id 103, speed 100000: ok > MMC: i2c_write: chip=0x2d, addr=0x32, len=0x1 > ---------- > > However with this patch applied, something starts using the controller > immediately, without it having been "probed" from device-tree: Hmm... do you have a debugger? > ---------- > U-Boot SPL 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28) > U-Boot 2013.07-rc3-00037-g1f2ba72-dirty (Jul 30 2013 - 13:08:28) > > TEGRA30 > Board: NVIDIA Beaver > I2C: i2c_init(speed=100000, slaveaddr=0xfe) > ---------- > > i2c_init touches HW, but since process_nodes() hasn't run, none of the > parameters like register address or clock ID are yet known. > i> I think this call comes from init_sequence_f[] -> init_func_i2c() -> > i2c_init() -> i2c_init() == __i2c_init() -> i2c_init_bus() -> No, we have now defined CONFIG_SYS_I2C and this calls i2c_init_all() -> i2c_init_board() and I think, on nvidia board i2c_init_board is defined. Yes, in the i2c driver there it is ... calling process_nodes() ... so, the i2c busses should be setup ... > I2C_ADAP->init(), although I didn't validate that in the running code, > just by code inspection. > > The issue here is that the I2C core and/or Tegra driver seems to be > statically registering the I2C device objects, even though they should > be dynamically registered from device tree. Feel free to post patches. > Should Tegra move its call of i2c_init_board() out of board_init() to > board_init_f(), and/or override __i2c_init() to call i2c_init_board()? No, i2c_init_board gets called here very early: init_func_i2c() -> i2c_init_all(): static int init_func_i2c(void) { puts("I2C: "); #ifdef CONFIG_SYS_I2C i2c_init_all(); #else i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); #endif puts("ready\n"); return (0); } and we have CONFIG_SYS_I2C defined (or, did you have it?) and in drivers/i2c/i2c_core.c: void i2c_init_all(void) { i2c_init_board(); i2c_set_bus_num(CONFIG_SYS_SPD_BUS_NUM); return; } Ah, I see, it is also called in board_init ... > I think when init_sequence_f[] is running, there may be no serial > console to report errors. If so, moving the I2C initialization to that > early point sounds like a really bad idea. No, we need i2c before relocation for example to read SPD data from eeprom, but this is on powerpc. puts() is working, as your log shows. I added in the comment from i2c_init_all: /* * i2c_init_all(): * * not longer needed, will deleted. Actual init the SPD_BUS * for compatibility. * i2c_adap[] must be initialized beforehead with function pointers and * data, including speed and slaveaddr. */ So the question raises, do we need this on arm? bye, Heiko