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 Glasssjg@chromium.org
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