Hi Heiko,
On Thu, Jan 12, 2012 at 11:12 PM, Heiko Schocher hs@denx.de wrote:
Hello Stephen,
Stephen Warren wrote:
Simon Glass wrote ednesday, January 11, 2012 9:18 PM:
On Mon, Jan 9, 2012 at 2:07 PM, Stephen Warren swarren@nvidia.com wrote:
On 12/26/2011 11:11 AM, Simon Glass wrote:
From: Yen Lin yelin@nvidia.com
Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver supports building both with and without CONFIG_OF_CONTROL.
Without CONFIG_OF_CONTROL a number of CONFIG options must be supplied in the board config header file:
I2CSPEED_KHZ - speed to run I2C bus at (typically 100000) CONFIG_I2Cx_PIN_MUX - pin mux setting for each port (P, 1, 2, 3) (typically this will be 0 to bring the port out the common pins)
...
...
diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
...
+struct i2c_bus i2c_controllers[CONFIG_SYS_MAX_I2C_BUS];
What if there are I2C bus extenders/muxes/... such that there are more I2C buses in the system than Tegra I2C controllers? I'd rather see an explicit TEGRA_I2C_NUM_CONTROLLERS define used throughout this patch.
We don't actually support CONFIG_I2C_MUX, so I can't see how that could happen. Can you please explain a bit more?
We may not support it now, but I see no reason we won't in the future. If we confuse the two defines now, it'll make it harder to allow muxes in the future. The fix is simply using the correct define name within the I2C driver isn't it; pretty simple.
If we really have this case, we *must* get the multibus/multiadapter branch from here:
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus...
to uboot mainline! With that approach we could handle this case in a clean way ... This branch is not in sync with current TOT, and it has to be tested again, as I didn't found time for this in the last year :-( Also, as its base is >2 years old, there are a lot of checkpatch errors in this patchserie, which have to be cleaned up ... hope I get some time for it ... help is welcome.
Hmm quite a few patches. I see this email from 2.5 years ago indicating that it was ready but needed more testing:
http://lists.denx.de/pipermail/u-boot/2009-July/056726.html
2a083b7 i2c: get rid of HARD_i2C ccb680c i2c: add dynamic bus allocation b68480a i2c, u8500: added new multibus/multiadapter support 692f037 i2c, marvell: added new multibus/multiadapter support 3b0258c i2c, spear: added new multibus/multiadapter support a06deff i2c, at91rm9200: added new multibus/multiadapter support 0758f6b i2c, kirkwood: added new multibus/multiadapter support 7919863 i2c, mv: added new multibus/multiadapter support b164243 i2c, mpc824x: added new multibus/multiadapter support a5f8bd1 i2c, tsi108: added new multibus/multiadapter support 9db40d8 i2c, s3c44b0: added new multibus/multiadapter support 67b95ef i2c, s3c24x0: added new multibus/multiadapter support 6b2c3f5 i2c, omap24xx: added new multibus/multiadapter support 1f8ffed i2c, omap1510: added new multibus/multiadapter support c41757c i2c, mxc: added new multibus/multiadapter support 5c22159 i2c, davinci: added new multibus/multiadapter support 3dd28ab i2c, bfin: added new multibus/multiadapter support 3d1c119 i2c, mpc8220: added new multibus/multiadapter support 8f4d062 i2c, mpc512x: added new multibus/multiadapter support 1bd40d4 i2c, mpc5xxx: added new multibus/multiadapter support 6225ccb i2c, 8xx: added new multibus/multiadapter support d94a632 i2c, ppc4xx_i2c: added new multibus/multiadapter support c34a705 i2c, mpc8260_i2c: added new multibus/multiadapter support 832b409 i2c, fsl_i2c: switch to new multibus/multiadapter support 06de378 i2c, soft-i2c: switch to new multibus/multiadapter support e47115f i2c, common: common changes for multibus/multiadapter support d7a70d7 i2c: add i2c_core and prepare for new multibus support
489 files changed, 8512 insertions(+), 7063 deletions(-)
checkpatch.pl found 295 error(s), 653 warning(s)
I have had a look through the patches. I agree that i2c needs a clean up and this seems to fit the bill. How can we get this committed?
The first two commits aren't too hard to bring over but it doesn't actually build. I could perhaps have a look at getting it to build correctly with and without CONFIG_SYS_I2C, and see if I can get the latter working on Tegra.
Then perhaps we could add the new support as just another option so we have CONFIG_HARD_I2C, CONFIG_SOFT_I2C and CONFIG_SYS_I2C. That might be a start, and drivers can move over to the new system. What do you think?
+int i2c_init_board(void) +{
[...]
Surely that function needs to actually do something, at least set up the clocks so that the (user?) requested rate is honored, or print an error message if you're only allowed to use the hard-coded bus rate.
See my other message. I suppose we could reinit, but we really don't want to honour the speed, since the fdt speed setting is then lost and irrecoverable. For now it feels like we should ignore it.
Hmm. I suspect the answer here is roughly to override the following in cmd_i2c.c:
/* TODO: Implement architecture-specific get/set functions */ unsigned int __def_i2c_get_bus_speed(void) { return CONFIG_SYS_I2C_SPEED; } unsigned int i2c_get_bus_speed(void) __attribute__((weak, alias("__def_i2c_get_bus_speed")));
int __def_i2c_set_bus_speed(unsigned int speed) { if (speed != CONFIG_SYS_I2C_SPEED) return -1;
return 0; } int i2c_set_bus_speed(unsigned int) __attribute__((weak, alias("__def_i2c_set_bus_speed")));
To actually read/write the rate in use by the driver.
Yep, for this reason this functions are weak, and you can define a driver specific function which returns the current used settings in the driver.
Code should compile, if tegra2 not define CONFIG_SYS_I2C_SPEED, as a "default" value for CONFIG_SYS_I2C_SPEED is defined in include/i2c.h ...
Then, fix do_i2c_reset() to use i2c_get_bus_speed() so it interacts correctly with those functions.
Yep, I think, you are right here, that should be fixed. As the default function for i2c_get_bus_speed returns CONFIG_SYS_I2C_SPEED, that should be OK.
Yes ok I will take a look.
Regards, Simon
There may be other places that need to be updates to use those function instead of hard-coding CONFIG_SYS_I2C_SPEED too. Perhaps we could even get away without defining CONFIG_SYS_I2C_SPEED for Tegra since it's meaningless. Instead, ifdef those default function definitions above based on whether CONFIG_SYS_I2C_SPEED is defined or not.
bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany