Re: devicetree: Tegra I2C muxing, and of_i2c_register_devices

[Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]

On 4/27/2011 6:05 AM, Grant Likely wrote:
> [cc'ing devicetree-discuss, and also John Bonesio who is working on
> Tegra device tree support]
>
> On Tue, Apr 26, 2011 at 5:13 PM, Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> Tegra's I2C driver has a unique feature built in (although not mainlined
>> yet); it can multiplex the I2C bus onto different sets of pins using the
>> Tegra pinmux module, and hence can register more than 1 I2C bus with the
>> I2C core for each controller. The exact number of busses registers, and
>> the pinmux settings to be used, are provided by platform data. See:
>>
>> http://git.chromium.org/git/kernel-next.git chromeos-2.6.38
>> include/linux/i2c-tegra.h
>> arch/arm/mach-tegra/board-seaboard.c
>>
>> If I understand the direction of devicetree on ARM, there should be a
>> single devicetree node for the Tegra I2C controller, which will get
>> matched up with a static platform device registration in e.g.
>> mach-tegra/board-dt.c
>>
>> I can easily see how to provide the appropriate platform data to the
>> Tegra I2C driver, by definining the appropriate fields in a custom
>> devicetree binding.
>>
>> To cater for the multiple child busses, the simplest solution seems to
>> be to define each child node's reg property to be a tuple,<sub_bus_id,
>> i2c_address>  (c.f. existing<i2c_address>). E.g.:
>>
>> IIC0: i2c@00000000 {
>>         compatible = "nvidia,tegra250-iic";
>>         #address-cells =<2>;
>>         #size-cells =<0>;
>>         bus_muxes =<pinmuxid_1 pinmux_value_1 pinmuxid_2 pinmux_value_2>;
>>         rtc@68 {
>>                 compatible = "stm,m41t80";
>>                 reg =<0 0x68>;
>>         };
>>         sttm@48 {
>>                 compatible = "ad,ad7414";
>>                 reg =<1 0x48>;
>>         };
>> };
>>
>> The problem is then that of_i2c_register_devices won't work well for the
>> Tegra I2C controller, since it enforces that each child node's reg
>> property be a single value, the I2C address.
>>
>> To solve this, should we:
>>
>> a) Have each mux'd bus have a separate devicetree node underneath the
>> main I2C device, e.g.:
>
> Yes, this is exactly what you should do.
>
>> IIC0: i2c@00000000 {
>>         compatible = "nvidia,tegra250-iic";
>>         // @0 doesn't really end up matching a
>>         // reg property within the child though
>>         bus@0 {
>>                 #address-cells =<1>;
>>                 #size-cells =<0>;
>>                 pinmux_group =<N>;
>>                 pinmux_value =<M>;
>>
>>                 rtc@68 {
>>                         compatible = "stm,m41t80";
>>                         reg =<0x68>;
>>                 };
>>         }
>>
>>         bus@1 {
>>                 #address-cells =<1>;
>>                 #size-cells =<0>;
>>                 pinmux_group =<N>;
>>                 pinmux_value =<M>;
>>
>>                 sttm@48 {
>>                         compatible = "ad,ad7414";
>>                         reg =<0x48>;
>>                 };
>>         }
>> };


The right way to name this is:

   i2cmux@00000000 {
       i2c@0 {
          rtc@68 {
          }
       }
       i2c@1 {
          sttm@48 {
          }
       }
   }

The reason is because the rtc and sttm devices are i2c devices so their 
parents should be i2c buses.  Those intermediate "bus" things are in 
fact actual i2c buses as far as the children are concerned.

In a full OFW implementation, it would have to be set up that way so 
that the children would see the correct set of support methods in their 
direct parent, regardless of whether they were attached to simple 
one-bus controllers or to a muxing controller.

As further evidence for the correctness of this approach, the existence 
of the "pinmux_group" properties in the intermediate nodes make it clear 
that the toplevel parent (tegra250-iic) is not exactly an iic bus 
device, but rather something special that needs such properties.

The same sort of situation existed for PCI IDE controllers, which had 
primary and secondary IDE strings, each of which could have master and 
slave IDE devices.  The best hierarchy turned out to be:

   pci-ide@<pci_address> {
      ide@0 {   // Primary string
         disk@0 {  // Master
         }
         disk@1 {  // Slave
         }
      }
      ide@ {    // Secondary string
         disk@0 {  // Master
         }
         disk@1 {  // Slave
         }
      }
   }




>>
>> Then, the bus@0 or bus@1 nodes could be placed into adap->dev.of_node,
>> since the devices are hung off there.
>
> Right.
>
>>
>> b)
>>
>> Implement custom devicetree child enumeration code in the Tegra I2C
>> driver to replace of_i2c_register_devices, which implements the
>> two-entry reg format. The first devicetree example in this email could
>> then be used.
>
> Blech!!!  :-)
>
>>
>> c)
>>
>> Perhaps remove the bus mux functionality from the Tegra I2C driver, and
>> implement a separate I2C mux driver for it.
>
> This would also be useful.  There are other systems that I think would
> find this to be a useful feature.  The device tree layout would look
> much the same as for option a), but the support code would become more
> generic.  I've been thinking about trying to implement both i2c and
> spi 'gateway' or 'bridge' drivers for a while now, so I think this is
> worth exploring.
>
>>
>> I'm not sure if devicetree has a defined representation for I2C bus
>> muxes, and even if it does, since the mux control isn't accessed by I2C
>> registers (as most standalone I2C bus muxes are), but rather Tegra-
>> specific pinmux API calls, I'm not quite sure how to represent it in
>> device-tree.
>
> There isn't an existing binding, but it will be pretty easy to key the
> required behaviour off a new compatible value for the specific i2c
> mux.  You just need a node for the i2c mux, and another node for each
> child bus.  Alternately, the mux could do what you suggest for option
> b which might end up being cleaner.  *HOWEVER* it is only cleaner if
> the common i2c bus population code is modified to support it instead
> of creating something custom.
>
> g.
>
>>
>> Thanks for any thoughts!
>>
>> --
>> nvpublic
>>
>>
>> _______________________________________________
>> linaro-dev mailing list
>> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
>> http://lists.linaro.org/mailman/listinfo/linaro-dev
>>
>
>
>