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

[Stephen Warren]

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 at 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 at 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 at 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?

> >> 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.