[U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework

[Heiko Schocher <hs@denx.de>]

Hello Stephen,

Am 31.07.2013 21:31, schrieb Stephen Warren:
> On 07/30/2013 11:46 PM, Heiko Schocher wrote:
> ...
>> Hmm.. each i2c adapter has its own init function ... why the tegra
>> driver do not use it? And do the necessary inits in it? So we
>> initialize an adapater only if we use it, which is also a rule
>> for u-boot ...
>>
>> I have no hw, but it should be possible to add to process_nodes()
>> a parameter "controller_number" and call
>> process_nodes(controller_number, ...) from the i2c drivers init
>> function ...
>
> There are two steps to initializing I2C on a Tegra system:
>
> 1) Parsing the DT to determine which/how-many I2C adapters exist in the
> SoC. This will yield information such as the register address of the I2C
> adapters, which clock/reset signal they rely on, perhaps the max bus
> clock rate, etc.
>
> This is a single global operation that needs to happen one single time
> before anything else.

Why?

> This stage initializes the i2c_controllers[] array, since that's what
> stores all the data parsed from DT.
>
> 2) Actually initializing or using the I2C HW. That could certainly be
> part of the per-I2C-controller init() function you mention.

For that purpose is the i2c_init function.

And why we could not do step 1 when we do step 2? Why doing step 1
for hw we later do not use?

saying something like this (just as an example, should be tested):

diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
index 9ac3969..7bbd3c7 100644
--- a/drivers/i2c/tegra_i2c.c
+++ b/drivers/i2c/tegra_i2c.c
@@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus)
   * @param is_scs       1 if this HW uses a single clock source (T114+)
   * @return 0 if ok, -1 on error
   */
-static int process_nodes(const void *blob, int node_list[], int count,
+static int process_nodes(const void *blob, int node_list[],
+                        struct i2c_adapter *adap, int count,
                          int is_dvc, int is_scs)
  {
         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];
+       int node = node_list[i];

-               if (node <= 0)
-                       continue;
+       bus = &i2c_controllers[adap->hwadapnr];
+       i2c_bus->id = adap->hwadapnr;

-               i2c_bus = &i2c_controllers[i];
-               i2c_bus->id = i;
+       if (node <= 0)
+               continue;

-               if (i2c_get_config(blob, node, i2c_bus)) {
-                       printf("i2c_init_board: failed to decode bus %d\n", i);
-                       return -1;
-               }
+       if (i2c_get_config(blob, node, i2c_bus)) {
+               printf("i2c_init_board: failed to decode bus %d\n", i);
+               return -1;
+       }

-               i2c_bus->is_scs = is_scs;
+       i2c_bus->is_scs = is_scs;

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

  /* Sadly there is no error return from this function */
-void i2c_init_board(void)
+static int tegra_i2c_init_board(struct i2c_adapter *adap)
  {
+       struct i2c_bus *i2c_bus;
         int node_list[TEGRA_I2C_NUM_CONTROLLERS];
         const void *blob = gd->fdt_blob;
         int count;

+       bus = tegra_i2c_get_bus(adap);
+        if (bus)
+                return 0;
+
         /* First check for newer (T114+) I2C ports */
         count = fdtdec_find_aliases_for_id(blob, "i2c",
                         COMPAT_NVIDIA_TEGRA114_I2C, node_list,
                         TEGRA_I2C_NUM_CONTROLLERS);
-       if (process_nodes(blob, node_list, count, 0, 1))
-               return;
+       if (process_nodes(blob, node_list, adap, count, 0, 1))
+               return -1;

         /* Now get the older (T20/T30) normal I2C ports */
         count = fdtdec_find_aliases_for_id(blob, "i2c",
                         COMPAT_NVIDIA_TEGRA20_I2C, node_list,
                         TEGRA_I2C_NUM_CONTROLLERS);
-       if (process_nodes(blob, node_list, count, 0, 0))
-               return;
+       if (process_nodes(blob, node_list, adap, count, 0, 0))
+               return -1;

         /* Now look for dvc ports */
         count = fdtdec_add_aliases_for_id(blob, "i2c",
                         COMPAT_NVIDIA_TEGRA20_DVC, node_list,
                         TEGRA_I2C_NUM_CONTROLLERS);
-       if (process_nodes(blob, node_list, count, 1, 0))
-               return;
+       if (process_nodes(blob, node_list, adap, count, 1, 0))
+               return -1;
+
+       return 0;
  }

  static void tegra_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr)
  {
+       int ret;
+
+       ret = tegra_i2c_init_board(adap);
+       if (ret) {
+               printf("Could not decode bus: %d\n", adap->hwadapnr);
+               return;
+       }
         /* This will override the speed selected in the fdt for that port */
         debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
         i2c_set_bus_speed(speed);

Which will call process_nodes() from the i2c_init function, which only
is called, when i2c bus get used ...

> Now, I think the big disconnect here is that historically, the U-Boot
> binary has hard-coded all the details that step (1) above parses out of
> DT, so that step (1) did not need to exist.
>
> However, Simon has been pushing Tegra towards not hard-coding any of
> this information, but instead having a single binary that can work on
> arbitrary Tegra boards and even across different Tegra SoCs which have a
> different number of I2C controllers at different register addresses.
> This is quite fundamentally different to how U-Boot has worked in the
> past, and evidently has some problems fitting into the typical U-Boot
> initialization sequence.

Yes ... but again, if we parse the DT in the moment we need to init
a hw, it would fit again (at least for the dt case). But we have a
problem, if we need to write (like the i2c_controllers[] array) before
relocation. So I2C and DT usage is not possible before relocation.
(And not only i2c in conjunction with dt I think)

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