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

[Stephen Warren]

Simon Glass wrote ednesday, January 11, 2012 9:18 PM:
> On Mon, Jan 9, 2012 at 2:07 PM, Stephen Warren <swarren at nvidia.com> wrote:
> > On 12/26/2011 11:11 AM, Simon Glass wrote:
> >> From: Yen Lin <yelin at 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.

> >> +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";


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

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.

-- 
nvpublic