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

[Simon Glass]

Hi Heiko,

On Thu, Jan 12, 2012 at 11:25 PM, Heiko Schocher <hs at denx.de> wrote:
> Hello Simon,
>
> 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 requires CONFIG_OF_CONTROL to obtain its configuration
>> from the device tree.
>>
>> (Simon Glass: sjg at chromium.org modified for upstream)
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>> Changes in v2:
>> - Use DIV_ROUND_UP() instead of a home-grown macro
>> - Tidy comment style
>> - Change i2c array to static
>> - Adjust definitions to fit new peripheral clock bindings
>> - Remove i2c configuring using CONFIG (use fdt instead)
>
> Why? Ah found it ... Hmm.. why we don't need the non OF
> case?

The intent is that Tegra boards will run with fdt turned on. It means
that we should be able to build U-Boot for a Tegra platform using the
Linux fdt file and not lots of manual config. Of course this works
better for some peripherals than others, but I2C works well enough.

>
>> - Make i2c/dvc decision come from fdt
>> - Use new fdtdec alias decode function
>> - Simplify code in i2c_addr_ok()
>> - Return an error if an unavailable i2c bus is selected
>>
>>  arch/arm/include/asm/arch-tegra2/tegra2_i2c.h |  160 +++++++
>>  drivers/i2c/Makefile                          |    1 +
>>  drivers/i2c/tegra2_i2c.c                      |  551 +++++++++++++++++++++++++
>>  include/fdtdec.h                              |    2 +
>>  lib/fdtdec.c                                  |    2 +
>>  5 files changed, 716 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
>>  create mode 100644 drivers/i2c/tegra2_i2c.c
> [...]
>> diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
>> new file mode 100644
>> index 0000000..a7db714
>> --- /dev/null
>> +++ b/drivers/i2c/tegra2_i2c.c
>> @@ -0,0 +1,551 @@
> [...]
>> +static void i2c_init_controller(struct i2c_bus *i2c_bus)
>> +{
>> +     /* TODO: Fix bug which makes us need to do this */
>> +     clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_OSC,
>> +                            i2c_bus->speed * (8 * 2 - 1));
>                                                 Can you use here some defines?
> What is (8 * 2 - 1) ?

I will look up the bug and find out.

>
>> +#ifndef CONFIG_OF_CONTROL
>> +#error "Please enable device tree support to use this driver"
>> +#endif
>
> Hmm.. see above question. Ok, if somebody need want to use this
> driver without CONFIG_OF_CONTROL it must be added ...

Yes and they need a way of getting the configuration in there. The fdt
is nicer...

>
>> +/*
>> + * Process a list of nodes, adding them to our list of I2C ports.
>> + *
>> + * @param blob               fdt blob
>> + * @param node_list  list of nodes to process (any <=0 are ignored)
>> + * @param count              number of nodes to process
>> + * @param is_dvc     1 if these are DVC ports, 0 if standard I2C
>> + * @return 0 if ok, -1 on error
>> + */
>> +static int process_nodes(const void *blob, int node_list[], int count,
>> +                      int is_dvc)
>> +{
>> +     struct i2c_bus *i2c_bus;
>> +     int i;
>> +
>> +     /* build the i2c_controllers[] for each controller */
>> +     for (i = 0; i < count; i++) {
>> +             int node = node_list[i];
>> +
>> +             if (node <= 0)
>> +                     continue;
>> +
>> +             i2c_bus = &i2c_controllers[i];
>> +             i2c_bus->id = i;
>> +
>> +             if (i2c_get_config(blob, node, i2c_bus)) {
>> +                     printf("i2c_init_board: failed to decode bus %d\n", i);
>> +                     return -1;
>> +             }
>> +
>> +             i2c_bus->is_dvc = is_dvc;
>> +             if (is_dvc) {
>> +                     i2c_bus->control =
>> +                             &((struct dvc_ctlr *)i2c_bus->regs)->control;
>> +             } else {
>> +                     i2c_bus->control = &i2c_bus->regs->control;
>> +             }
>> +             debug("%s: controller bus %d at %p, periph_id %d, speed %d: ",
>> +                   is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
>> +                   i2c_bus->periph_id, i2c_bus->speed);
>> +             i2c_init_controller(i2c_bus);
>> +             debug("ok\n");
>> +             i2c_bus->inited = 1;
>> +
>> +             /* Mark position as used */
>> +             node_list[i] = -1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +int i2c_init_board(void)
>> +{
>> +     int node_list[CONFIG_SYS_MAX_I2C_BUS];
>> +     const void *blob = gd->fdt_blob;
>> +     int count;
>> +
>> +     /* First get the normal i2c ports */
>> +     count = fdtdec_find_aliases_for_id(blob, "i2c",
>> +                     COMPAT_NVIDIA_TEGRA20_I2C, node_list,
>> +                     CONFIG_SYS_MAX_I2C_BUS);
>> +     if (process_nodes(blob, node_list, count, 0))
>> +             return -1;
>> +
>> +     /* Now look for dvc ports */
>> +     count = fdtdec_add_aliases_for_id(blob, "i2c",
>> +                     COMPAT_NVIDIA_TEGRA20_DVC, node_list,
>> +                     CONFIG_SYS_MAX_I2C_BUS);
>> +     if (process_nodes(blob, node_list, count, 1))
>> +             return -1;
>> +
>> +     return 0;
>> +}
>> +
>> +void i2c_init(int speed, int slaveaddr)
>> +{
>> +     debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
>> +}
>
> Hmm... i2c_init is called to init the i2c subsystem ... you do nothing
> here ... and use i2c_init_board for init the i2c bus, right?
>
> But i2c_init_board is not called from the driver ... ah, you do this
> in board code ... Ok ...
>
> I think, you do this, because i2c_init is called very early, and
> so processing fdt is slow?

That's not the main reason but it is true. We have no need of early
I2C on the platform. Also we don't want to set the speed as this is
defined individually per port in the fdt.

Regards,
Simon

>
> [...]
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany