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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to