Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

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

On 6/1/2011 5:59 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Tuesday, May 31, 2011 12:43 PM:
>> On 5/31/2011 7:55 AM, Stephen Warren wrote:
>>> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>>>> ...
>>>> GPIOs share the need to "point across the tree to different nodes", but
>>>> it is unclear that there is a need for a entirely different hierarchy.
>>>>
>>>> That suggests the possibility of using the device tree addressing
>>>> mechanism as much as possible.  Normal device tree addresses could be
>>>> used to specify GPIO numbers.
>>>>
>>>> Suppose we have:
>>>>
>>>>       gpio-controller1: gpio-controller {
>>>>           #address-cells =<2>;
>>>>           #mode-cells =<2>;
>>>>           gpio1: gpio@12,0 {
>>>>               reg =<12 0>;
>>>>               mode =<55 66>;
>>>>               usage = "Audio Codec chip select";  /* Optional */
>>>>           }
>>>>       };
>>>>       gpio-controller2: gpio-controller {
>>>>           #address-cells =<1>;
>>>>           #mode-cells =<1>;
>>>>           gpio2: gpio@4 {
>>>>               reg =<4>;
>>>>               #mode-cells =<1>;
>>>>           }
>>>>       };
>>>
>>> A quick note on the way that Tegra's devicetree files are broken up in
>>> Grant's devicetree/test branch:
>>>
>>> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>>>     itself.
>>> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>>>     And additionally:
>>>     ** Defines all devices on the board
>>>     ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>>>        board.
>>>
>>> I like this model, because it shares the complete definition of the
>>> Tegra SoC in a single file, rather than duplicating it with cut/paste
>>> into every board file.
>>>
>>> As such, the code quoted above would be in tegra250.dtsi.
>>>
>>> This has two relevant implications here:
>>>
>>> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
>>> naming of those GPIO pins, and not any board-specific naming, e.g.
>>> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
>>> be at the client/reference site, not the GPIO definition site.
>>>
>>> 2) The GPIO mode should be defined by the client/user of the GPIO, not
>>> the SoC's definition of that GPIO.
>>>
>>> Without those two conditions, we couldn't share anything through
>>> tegra250.dtsi.
>>>
>>> Re-iteration of these implications below.
>>>
>>>>       [...]
>>>>       chipsel-gpio =<&gpio1>,
>>>>           <&gpio-controller1 13 0 55 77>,
>>>>           <0>, /* holes are permitted, means no GPIO 2 */
>>>>           <&gpio2 88>,
>>>>           <&gpio-controller2 5 1>;
>>>
>>>> A GPIO spec consist of:
>>>>
>>>> 1) A reference to either a gpio-controller or a gpio device node.
>>>>
>>>> 2) 0 or more address cells, according to the value of #address-cells in
>>>> the referenced node.  If the node has no #address-cells property, the
>>>> value is assumed to be 0.
>>>
>>> I'd expect address cells only if referencing a gpio-controller; if
>>> referencing a gpio device node, the address would be implicit.
>>
>> Yes.  But I think it's better if there is a single rule for interpreting
>> the GPIO spec, namely look at the #address-cells property, rather than
>> deciding implicitly based on the type of the referent node.
>>
>>>> 3) 0 or more mode cells, according to the value of #mode-cells in the
>>>> referenced node.
>>>
>>> Yes, I agree. Although, I think your (3) is inconsistent with the gpio
>>> controller definitions you wrote above, which include the mode
>>> definitions in the controller instead of the user.
>>
>> Hmmm.  I think I got the example right.
>
> You're right. The examples are consistent. I think what threw me was that
> the example code itself contained "<&gpio2 88>" whereas the description
> later referred to just "<gpio2>".
>
> Also, I hadn't noticed that the gpio2 definition repeated
> "#mode-cells =<1>;" whereas the gpio1 definition didn't.
>
> I have to say, I don't like that aspect; i.e. having to repeat
> #mode-cells in every gpio definition that's inside/underneath the
> controller definition; in my mind, "passing on" the requirement to
> define the mode would be the default state, so forcing the namer of
> GPIOs (i.e. whoever writes the "gpio1: gpio@12,0 {" definitions) to
> do this seems almost like busy work. Is there a way in *.dts to mark
> the #mode-cells field as inherited by children unless overridden?


That is a very good point.  Addressing it led me to a revised scheme 
that I like much better.  (Notice that in the notes below I address your 
reasonable desire for an immutable SoC core definition.)

The example:

     gpio-controller1: gpio-controller {
         #address-cells = <2>;
         #mode-cells = <2>;
         unbound_gpio1: gpio {
             /* No reg property */
             /* No mode property */
         }
         fully_bound_gpio1: audio-chipsel@12,0 {
             reg = <12 0>;
             mode = <55 66>;
             usage = "Audio Codec chip select";  /* Optional */
         }
         address_bound_gpio1: gpio@13,0 {
             reg = <13 0>;
             /* No mode property */
         }
         mode_bound_gpio1: open-drain-gpio {
             /* No reg property */
             mode = <77 88>;
         }
     };
     gpio-controller2: gpio-controller {
          #address-cells = <1>;
          #mode-cells = <1>;
          unbound_gpio2: gpio {
              /* No reg property */
              /* No mode property */
          }
          address_bound_gpio2: gpio@4 {
              reg = <4>;
              /* No mode property */
          }
     };
     [...]
     chipsel-gpio =
         <&fully_bound_gpio1>,
         <&unbound_gpio1 13 0 55 77>,
         <&mode_bound_gpio1 14 0>,
         <&address_bound_gpio2 88>,
         <&unbound_gpio2 5 1>;

Notes:

a) A reference to a GPIO always points to the child of a 
gpio-controller, i.e. to an individual gpio node.

b) If that gpio node has a "reg" property, the number of address cells 
in the reference is 0, otherwise it is #address-cells from the parent 
(gpio-controller) node.

c) If that gpio node has a "mode" property, the number of mode cells in 
the reference is 0, otherwise it is #mode-cells from the parent 
(gpio-controller) node.

d) It's unnecessary for all children of a gpio controller to be named 
just "gpio".  The important thing is that the semantics of the node - 
the set of properties (and, for Open Firmware systems, the set of 
methods) - meet the usage needs of the node's "client".

e) gpios that are mode-bound but not address-bound must have distinct
names from each other and from the unbound node name ("gpio"), because 
of the device tree rule that the combination of name+address must be 
unique among the children of a given node.  It is okay to have both 
"gpio" and "gpio@12", but you cannot have two nodes both named just "gpio".

f) SoC device tree blobs would probably use only the unbound form.  A 
given platform might choose to specialize the tree by adding additional 
bound nodes to the tree established by the unmodified SoC core blob.

g) reg-less nodes have been part of the Open Firmware spec forever; they 
are called "wildcard nodes".  Their intended use is to refer to a class 
of similar objects, potentially so numerous that full enumeration is 
undesireable.


>
>>>> In the example, the chipsel-gpio specs are interpreted as:
>>>>
>>>> <&gpio1>    -  refers to a pre-bound gpio device node, in which the
>>>> address (12 0) and mode (55 66) are specified within that node.
>>>
>>> Just re-iterating: I'd prefer mode to be solely in the reference, and
>>> not in the gpio controller.
>>
>> I agree that it doesn't make much sense for a controller node to specify
>> the mode.  My example doesn't do that.  The only mode value is in an
>> individual gpio node, not a controller node.
>
> Yes, I suppose that's true.
>
> But, in my mind at least, the controller definition would be part of the
> SoC definition, and provided by the SoC vendor in a separate and
> basically immutable file. As such, any gpio node inside the controller
> node really is part of the controller's/SoC's definition.
>
>>  From the standpoint of a SoC manufacturer, specifying modes in the
>> reference is probably a good idea.  But it's not necessarily best in all
>> cases.  If the focus of attention is a board design with numerous
>> variants and revisions, "binding" the modes of specific gpio pins
>> according to the board wiring may be the right thing.
>>
>> The way I specified it lets you choose.
>
> Granted.
>
> However, I'm still not convinced that's a great idea; just because a
> board vendor might want to cut/paste the entire SoC definition into their
> DTS file and hack it, rather than just including it, doesn't mean it's
> a good idea. If we agreed on that (which I'll grant we probably don't)
> it seems like we shouldn't aim to add features that are probably only
> needed to make the life of people who do that easier.
>
> My perspective is that cut/pasting the entire SoC definition into a board
> definition is the devicetree equivalent of having more than one driver
> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
> stuff that caused Linus to complain about the state of ARM Linux.
>
> Equally, a separation of SoC vs. board should make incorporating bugfixes
> to the SoC definition into board definitions easier; simply replace the
> file and recompile. And, it'll make it more obvious to board vendors which
> changes need to be upstreamed to the SoC vendor, i.e. if a board vendor
> finds a bug in the SoC definition file.
>
> I suppose the one area this flexibility might make sense is if a board
> vendor has N similar boards, they can setup a set of include files:
>
> board-a.dts:
> include board-common.dtsi
> Do board-a customizations
>
> board-b-dts:
> include board-common.dtsi
> Do board-b customizations
>
> board-common.dtsi: include soc.dtsi
> Could add the gpio definitions to the controller definition from
> soc.dtsi
>
> soc.dtsi:
> base SoC definition; gpio controller, etc.
>
> But I still don't entirely see the advantage of board-common.dtsi
> defining GPIOs and having board-*.dts use those GPIOs as parameters to
> HW modules, e.g. as a GPIO list for an SDHCI controller, rather than
> simply having board-common.dtsi simply specify the SDHCI controller
> directly, thus avoiding the new syntax.
>
>>> Does this SoC/board segregation make sense to everyone? Obviously it
>>> does to me:-)
>>
>> It makes perfect sense from one viewpoint, but I think that board
>> vendors may have a different focus.  The flexibility to specify a mode
>> in either place costs little, as the parsing rule is definite and
>> straightforward.  SoC vendors are free to defer mode decisions until
>> later, by omitting "mode" and supplying "#mode-cells" in their device
>> tree templates.  Board vendors could choose either to use the SoC
>> vendor's template verbatim, or to amend it with additional
>> board-specific information.
>
>