Re: [U-Boot] [PATCH 7/7] tegra: Enable I2C on Seaboard
+U-Boot, which I seemed to have dropped by mistake
Hi Stephen,
On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren swarren@nvidia.com wrote:
Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM:
On Mon, Jan 9, 2012 at 1:45 PM, Stephen Warren swarren@nvidia.com wrote:
On 12/26/2011 11:11 AM, Simon Glass wrote:
This enables I2C on Seaboard.
...
diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
...
+#define CONFIG_SYS_I2C_INIT_BOARD
I don't think that option is correct for Seaboard; the description in the README indicates this causes a function named i2c_init_board() to be called from boards/xxx/board.c, which is supposed to use GPIOs to unhang the I2C bus. However, this raises a couple of issues:
- Patch 5 in this series calls i2c_init_board() ifdef CONFIG_TEGRA2_I2C
rather than depending on this CONFIG option.
- Tegra's i2c_init_board() doesn't appear to be anything to do with bus
unhanging, but is instead regular I2C initialization. Perhaps the function should be renamed?
I have had a bit of a look at this. From what I can see the problem is that i2c_init() is passed a bus speed, but this is just CONFIG_SYS_I2C_SPEED. We want to control the speed individually for each port. Yes would could use i2c_init() and ignore the speed, but that seems odd also. At least with the way it is we don't ignore the setting.
We also don't define CONFIG_HARD_I2C which again seems odd, but I suppose is for the same reason (we don't want to call i2c_init()).
Hmm. It sounds like we should replace CONFIG_TEGRA2_I2C with CONFIG_HARD_I2C given the README description of the latter?
Well we could, but we would need to ignore the speed argument. Do you think that is better?
Also U-Boot tends to call i2c_init() before every use of i2c, whereas we just want to init once and be done with it.
I think the function name is correct, but perhaps I should change the #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for i2c to function on Tegra boards, this must be defined, so in fact this might be counterproductive. So perhaps a check that it is defined is best?
But README explicitly says that i2c_init_board() is for bus unhanging which isn't what the Tegra implementation does. How can the function name be correct given that?
Well we should rename the function IMO. It seems to me that with a a name like that it is for initing i2c on the board.
Don't we just want to make i2c_init() use a global/static variable to determine whether the device has already been initialized, and defer all the init until first usage or something like that? That seems in line with U-Boot's desire not to initialize drivers until they're actually used.
Actually that might work - if we keep i2c_init() a no-op, and wait until we get a request for a bus before we look up the fdt and init that port. But I suspect we might need to init port 0 immediately since there is no explicit call to i2c_set_bus_num() for that. It's a little wonky whatever we do. What do you think?
(have just sent the series again with fdt changes, but can update once we sort this out).
Regards, Simon
Simon Glass wrote at Thursday, January 12, 2012 12:10 PM:
On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren swarren@nvidia.com wrote:
Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM:
...
Also U-Boot tends to call i2c_init() before every use of i2c, whereas we just want to init once and be done with it.
I think the function name is correct, but perhaps I should change the #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for i2c to function on Tegra boards, this must be defined, so in fact this might be counterproductive. So perhaps a check that it is defined is best?
But README explicitly says that i2c_init_board() is for bus unhanging which isn't what the Tegra implementation does. How can the function name be correct given that?
Well we should rename the function IMO. It seems to me that with a a name like that it is for initing i2c on the board.
You mean change the function that's activated by CONFIG_SYS_I2C_INIT_BOARD to be something more like i2c_unhang_board()? That makes sense to me. I have no idea how much of a hassle it'd be to update any existing boards.
Don't we just want to make i2c_init() use a global/static variable to determine whether the device has already been initialized, and defer all the init until first usage or something like that? That seems in line with U-Boot's desire not to initialize drivers until they're actually used.
Actually that might work - if we keep i2c_init() a no-op, and wait until we get a request for a bus before we look up the fdt and init that port. But I suspect we might need to init port 0 immediately since there is no explicit call to i2c_set_bus_num() for that. It's a little wonky whatever we do. What do you think?
It does make sense to me to at least parse out all the DT stuff early on, so there's a single place to do all the alias processing etc. Given what I just wrote in the other email about fixing the I2C bus speed stuff, would making those changes here help this at all?
Hi Stephen,
On Thu, Jan 12, 2012 at 11:21 AM, Stephen Warren swarren@nvidia.com wrote:
Simon Glass wrote at Thursday, January 12, 2012 12:10 PM:
On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren swarren@nvidia.com wrote:
Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM:
...
Also U-Boot tends to call i2c_init() before every use of i2c, whereas we just want to init once and be done with it.
I think the function name is correct, but perhaps I should change the #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for i2c to function on Tegra boards, this must be defined, so in fact this might be counterproductive. So perhaps a check that it is defined is best?
But README explicitly says that i2c_init_board() is for bus unhanging which isn't what the Tegra implementation does. How can the function name be correct given that?
Well we should rename the function IMO. It seems to me that with a a name like that it is for initing i2c on the board.
You mean change the function that's activated by CONFIG_SYS_I2C_INIT_BOARD to be something more like i2c_unhang_board()? That makes sense to me. I have no idea how much of a hassle it'd be to update any existing boards.
Well we should change the README or change the function name. I'm not sure which is correct.
Don't we just want to make i2c_init() use a global/static variable to determine whether the device has already been initialized, and defer all the init until first usage or something like that? That seems in line with U-Boot's desire not to initialize drivers until they're actually used.
Actually that might work - if we keep i2c_init() a no-op, and wait until we get a request for a bus before we look up the fdt and init that port. But I suspect we might need to init port 0 immediately since there is no explicit call to i2c_set_bus_num() for that. It's a little wonky whatever we do. What do you think?
It does make sense to me to at least parse out all the DT stuff early on, so there's a single place to do all the alias processing etc. Given what I just wrote in the other email about fixing the I2C bus speed stuff, would making those changes here help this at all?
I think the device tree processing should be done later because i2c_init() is called very early, before the dcache is enabled. It is very s l o w to touch device tree before then. To be honest, that's another reason for leaving it as it is, with i2c_init() being a no-op. But we might be able to init the bus on first use with enough if() statements :-)
I will take a look. I am not going to refactor U-Boot I2C here, although I think it could use a few tweaked...
Regards, Simon
-- nvpublic
Hello Simon,
Simon Glass wrote:
+U-Boot, which I seemed to have dropped by mistake
Hi Stephen,
On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren swarren@nvidia.com wrote:
Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM:
On Mon, Jan 9, 2012 at 1:45 PM, Stephen Warren swarren@nvidia.com wrote:
On 12/26/2011 11:11 AM, Simon Glass wrote:
This enables I2C on Seaboard.
...
diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
...
+#define CONFIG_SYS_I2C_INIT_BOARD
I don't think that option is correct for Seaboard; the description in the README indicates this causes a function named i2c_init_board() to be called from boards/xxx/board.c, which is supposed to use GPIOs to unhang the I2C bus. However, this raises a couple of issues:
- Patch 5 in this series calls i2c_init_board() ifdef CONFIG_TEGRA2_I2C
rather than depending on this CONFIG option.
- Tegra's i2c_init_board() doesn't appear to be anything to do with bus
unhanging, but is instead regular I2C initialization. Perhaps the function should be renamed?
I have had a bit of a look at this. From what I can see the problem is that i2c_init() is passed a bus speed, but this is just CONFIG_SYS_I2C_SPEED. We want to control the speed individually for each port. Yes would could use i2c_init() and ignore the speed, but that seems odd also. At least with the way it is we don't ignore the setting.
We also don't define CONFIG_HARD_I2C which again seems odd, but I suppose is for the same reason (we don't want to call i2c_init()).
Hmm. It sounds like we should replace CONFIG_TEGRA2_I2C with CONFIG_HARD_I2C given the README description of the latter?
Well we could, but we would need to ignore the speed argument. Do you think that is better?
Without defining CONFIG_HARD_i2C, i2c_init() never gets called ... naming CONFIG_TEGRA2_I2C is absolutely OK for enabling the tegra i2c driver.
Also U-Boot tends to call i2c_init() before every use of i2c, whereas we just want to init once and be done with it.
I think the function name is correct, but perhaps I should change the #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for i2c to function on Tegra boards, this must be defined, so in fact this might be counterproductive. So perhaps a check that it is defined is best?
But README explicitly says that i2c_init_board() is for bus unhanging which isn't what the Tegra implementation does. How can the function name be correct given that?
Well we should rename the function IMO. It seems to me that with a a name like that it is for initing i2c on the board.
Yes, the name is missleading. On the other hand is a deblocking also some sort of "init" ... but I see no problem in using the i2c_init_board() for doing board specific i2c inits too ... maybe the README should be adapted ... but this problem results, that you don't want to use i2c_init() ...
Don't we just want to make i2c_init() use a global/static variable to determine whether the device has already been initialized, and defer all the init until first usage or something like that? That seems in line with U-Boot's desire not to initialize drivers until they're actually used.
Yep, that would be inline with that ... but how would you use "i2c reset" command? That only calls i2c_init() ... so, you never can reset/reinit your i2c bus controller.
Actually that might work - if we keep i2c_init() a no-op, and wait until we get a request for a bus before we look up the fdt and init that port. But I suspect we might need to init port 0 immediately since there is no explicit call to i2c_set_bus_num() for that. It's a little wonky whatever we do. What do you think?
Yep, the clean way would be to use the multibus/multiadapter branch:
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus...
but as I said in a previous EMail ... there is some work to do, before it is mainline acceptable ...
[...]
bye, Heiko
participants (3)
-
Heiko Schocher -
Simon Glass -
Stephen Warren