[U-Boot] [PATCH 4/7] tegra: Add I2C driver

[Simon Glass <sjg@chromium.org>]

Hi Stephen,

On Thu, Jan 12, 2012 at 11:14 AM, Stephen Warren <swarren@nvidia.com> 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.

OK I have added the new define.

>
>> >> +int i2c_init_board(void)
>> >> +{
>> >> + ? ? ? struct i2c_bus *i2c_bus;
>> >> + ? ? ? int index = 0;
>> >> + ? ? ? int i;
>> >> +
>> >> + ? ? ? /* build the i2c_controllers[] for each controller */
>> >> + ? ? ? for (i = 0; i < CONFIG_SYS_MAX_I2C_BUS; ++i) {
>> >> + ? ? ? ? ? ? ? i2c_bus = &i2c_controllers[i];
>> >> + ? ? ? ? ? ? ? i2c_bus->id = i;
>> >> +
>> >> + ? ? ? ? ? ? ? if (i2c_get_config(&index, i2c_bus)) {
>> >> + ? ? ? ? ? ? ? ? ? ? ? printf("i2c_init_board: failed to find bus %d\n", i);
>> >> + ? ? ? ? ? ? ? ? ? ? ? return -1;
>> >> + ? ? ? ? ? ? ? }
>> >> +
>> >> + ? ? ? ? ? ? ? if (i2c_bus->periph_id == PERIPH_ID_DVC_I2C)
>> >> + ? ? ? ? ? ? ? ? ? ? ? i2c_bus->control =
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &((struct dvc_ctlr *)i2c_bus->regs)->control;
>> >> + ? ? ? ? ? ? ? else
>> >> + ? ? ? ? ? ? ? ? ? ? ? i2c_bus->control = &i2c_bus->regs->control;
>> >
>> > When instantiating controllers from device tree (as opposed to the
>> > static !CONFIG_OF_CONTROL case), that conditional should be driven by
>> > device tree properties. The kernel already represents this using two
>> > separate compatible values for the different HW: nvidia,tegra20-i2c and
>> > nvidia,tegra20-i2c-dvc.
>>
>> Not in the device tree file I got from the kernel...has it changed?
>
> Yes, the latest is:
>
> ? ? ? ?i2c at 7000c000 {
> ? ? ? ? ? ? ? ?compatible = "nvidia,tegra20-i2c";
> ? ? ? ?i2c at 7000c400 {
> ? ? ? ? ? ? ? ?compatible = "nvidia,tegra20-i2c";
> ? ? ? ?i2c at 7000c500 {
> ? ? ? ? ? ? ? ?compatible = "nvidia,tegra20-i2c";
> ? ? ? ?i2c at 7000d000 {
> ? ? ? ? ? ? ? ?compatible = "nvidia,tegra20-i2c-dvc";

OK I think I have this now.

>
>
>> >> +
>> >> + ? ? ? ? ? ? ? i2c_init_controller(i2c_bus);
>> >> + ? ? ? }
>> >> +
>> >> + ? ? ? return 0;
>> >> +}
>> >
>> >> +void i2c_init(int speed, int slaveaddr)
>> >> +{
>> >> + ? ? ? debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
>> >> +}
>> >
>> > 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.
>
> Then, fix do_i2c_reset() to use i2c_get_bus_speed() so it interacts
> correctly with those functions.

The reset will override the fdt speed, but there's no easy way around
that. U-Boot expects a single speed for all I2C at present.

>
> 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.

I think we should leave this until the I2C refactor is in. In fact I
would go further and say that U-Boot should probably support more
flexibility here - but the API is what it is at the moment and I feel
uncomfortable changing it just for Tegra. As you probably saw I sent
up Heiko's patches and one or other of us may get to this.

Regards,
Simon

>
> --
> nvpublic
>