| Message ID | 1401894990-30092-2-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/04/2014 09:16 AM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > This patch adds the device tree binding documentation for the XUSB pad > controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY > capabilities. > diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > +- #phy-cells: Should be 1. The specifier is the index of the PHY to reference. > + Possible values are: > + - 0: PCIe > + - 1: SATA Those values are defined in include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider the <dt-bindings/> header files to be part of the binding itself, rather than being derived from the binding. As such, I'd suggest the following changes: * Make this patch 1 not patch 2 * Move pinctrl-tegra-xusb.h into this patch. * Remove the list of values above, and replace it with the text "See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the set of valid values". We'll need to make sure this patch is applied to a topic branch that can be merged into the pinctrl tree and the Tegra tree for 3.17, assuming that you'll be sending patches for 3.17 that use pinctrl-tegra-xusb.h. > +Example: > +======== > + > +SoC file extract: > +----------------- > + > + padctl@0,7009f000 { > + compatible = "nvidia,tegra124-xusb-padctl"; > + reg = <0x0 0x7009f000 0x0 0x1000>; > + resets = <&tegra_car 142>; > + reset-names = "padctl"; > + > + #address-cells = <0>; > + #size-cells = <0>; Why are those two properties required? Yes, this node has sub-nodes, but those sub-nodes don't have a reg property or unit address. The main Tegra pinctrl nodes don't have #address/size-cells. > +Board file extract: > +------------------- > + padctl: padctl@0,7009f000 { > + pinmux { > + pinctrl-0 = <&padctl_default>; > + pinctrl-names = "default"; Isn't there one extra level of nodes here. In the DT patches later in this series, pinctrl-0/pinctrl-names are directly inside the top-level padctl node.
On Thu, Jun 05, 2014 at 10:47:45AM -0600, Stephen Warren wrote: > On 06/04/2014 09:16 AM, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > This patch adds the device tree binding documentation for the XUSB pad > > controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY > > capabilities. > > > diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt > > > +- #phy-cells: Should be 1. The specifier is the index of the PHY to reference. > > + Possible values are: > > + - 0: PCIe > > + - 1: SATA > > Those values are defined in > include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider > the <dt-bindings/> header files to be part of the binding itself, rather > than being derived from the binding. As such, I'd suggest the following > changes: > > * Make this patch 1 not patch 2 > * Move pinctrl-tegra-xusb.h into this patch. > * Remove the list of values above, and replace it with the text "See > <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the set of valid values". I remember discussions where people explicitly said that relying on the symbolic names in the DT bindings was a mistake because it would mean that everyone would need to have access to a mechanism similar to what we have in the Linux kernel (and that the header files would always need to be shipped with the DT bindings). > > +Example: > > +======== > > + > > +SoC file extract: > > +----------------- > > + > > + padctl@0,7009f000 { > > + compatible = "nvidia,tegra124-xusb-padctl"; > > + reg = <0x0 0x7009f000 0x0 0x1000>; > > + resets = <&tegra_car 142>; > > + reset-names = "padctl"; > > + > > + #address-cells = <0>; > > + #size-cells = <0>; > > Why are those two properties required? Yes, this node has sub-nodes, but > those sub-nodes don't have a reg property or unit address. The main > Tegra pinctrl nodes don't have #address/size-cells. I seem to remember that there was a reason but I'm pulling a blank. I'll do some testing with those removed and verify that they are indeed not needed. > > +Board file extract: > > +------------------- > > > + padctl: padctl@0,7009f000 { > > + pinmux { > > + pinctrl-0 = <&padctl_default>; > > + pinctrl-names = "default"; > > Isn't there one extra level of nodes here. In the DT patches later in > this series, pinctrl-0/pinctrl-names are directly inside the top-level > padctl node. Yes, this is left-over from an earlier version of the patch. Thanks for catching this. Thierry
On 06/05/2014 04:08 PM, Thierry Reding wrote: > On Thu, Jun 05, 2014 at 10:47:45AM -0600, Stephen Warren wrote: >> On 06/04/2014 09:16 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> This patch adds the device tree binding documentation for the XUSB pad >>> controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY >>> capabilities. >> >>> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> >>> +- #phy-cells: Should be 1. The specifier is the index of the PHY to reference. >>> + Possible values are: >>> + - 0: PCIe >>> + - 1: SATA >> >> Those values are defined in >> include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider >> the <dt-bindings/> header files to be part of the binding itself, rather >> than being derived from the binding. As such, I'd suggest the following >> changes: >> >> * Make this patch 1 not patch 2 >> * Move pinctrl-tegra-xusb.h into this patch. >> * Remove the list of values above, and replace it with the text "See >> <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the set of valid values". > > I remember discussions where people explicitly said that relying on the > symbolic names in the DT bindings was a mistake because it would mean > that everyone would need to have access to a mechanism similar to what > we have in the Linux kernel (and that the header files would always need > to be shipped with the DT bindings). The entire point of the headers is so that the binding definitions and code using them can't get out of sync. In my opinion, the headers *are* part of the bindings, so of course anyone either reading DT bindings, or parsing DT, must have the headers, and hence the headers must be bundled with the DT files or with the bindings wherever they go.
On Fri, Jun 6, 2014 at 12:57 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > In my opinion, the headers *are* > part of the bindings, so of course anyone either reading DT bindings, or > parsing DT, must have the headers, and hence the headers must be bundled > with the DT files or with the bindings wherever they go. I agree 100% with Stephen's stance. Reference the headers. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt new file mode 100644 index 000000000000..8af59bbcdaa0 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt @@ -0,0 +1,135 @@ +Device tree binding for NVIDIA Tegra XUSB pad controller +======================================================== + +The Tegra XUSB pad controller manages a set of lanes, each of which can be +assigned to one out of a set of different pads. Some of these pads have an +associated PHY that must be powered up before the pad can be used. + +This document defines the device-specific binding for the XUSB pad controller. + +Refer to pinctrl-bindings.txt in this directory for generic information about +pin controller device tree bindings and ../phy/phy-bindings.txt for details on +how to describe and reference PHYs in device trees. + +Required properties: +-------------------- +- compatible: should be "nvidia,tegra124-xusb-padctl" +- reg: Physical base address and length of the controller's registers. +- resets: Must contain an entry for each entry in reset-names. + See ../reset/reset.txt for details. +- reset-names: Must include the following entries: + - padctl +- #address-cells: Should be 0. +- #size-cells: Should be 0. +- #phy-cells: Should be 1. The specifier is the index of the PHY to reference. + Possible values are: + - 0: PCIe + - 1: SATA + +Lane muxing: +------------ + +Child nodes contain the pinmux configurations following the conventions from +the pinctrl-bindings.txt document. Typically a single, static configuration is +given and applied at boot time. + +Each subnode describes groups of lanes along with parameters and pads that +they should be assigned to. The name of these subnodes is not important. All +subnodes should be parsed solely based on their content. + +Each subnode only applies the parameters that are explicitly listed. In other +words, if a subnode that lists a function but no pin configuration parameters +implies no information about any pin configuration parameters. Similarly, a +subnode that describes only an IDDQ parameter implies no information about +what function the pins are assigned to. For this reason even seemingly boolean +values are actually tristates in this binding: unspecified, off or on. +Unspecified is represented as an absent property, and off/on are represented +as integer values 0 and 1. + +Required properties: +- nvidia,lanes: An array of strings. Each string is the name of a lane. + +Optional properties: +- nvidia,function: A string that is the name of the function (pad) that the + pin or group should be assigned to. Valid values for function names are + listed below. +- nvidia,iddq: Enables IDDQ mode of the lane. (0: no, 1: yes) + +Note that not all of these properties are valid for all lanes. Lanes can be +divided into three groups: + + - otg-0, otg-1, otg-2: + + Valid functions for this group are: "snps", "xusb", "uart", "rsvd". + + The nvidia,iddq property does not apply to this group. + + - ulpi-0, hsic-0, hsic-1: + + Valid functions for this group are: "snps", "xusb". + + The nvidia,iddq property does not apply to this group. + + - pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, sata-0: + + Valid functions for this group are: "pcie", "usb3", "sata", "rsvd". + + +Example: +======== + +SoC file extract: +----------------- + + padctl@0,7009f000 { + compatible = "nvidia,tegra124-xusb-padctl"; + reg = <0x0 0x7009f000 0x0 0x1000>; + resets = <&tegra_car 142>; + reset-names = "padctl"; + + #address-cells = <0>; + #size-cells = <0>; + #phy-cells = <1>; + }; + +Board file extract: +------------------- + + pcie-controller@0,01003000 { + ... + + phys = <&padctl 0>; + phy-names = "pcie"; + + ... + }; + + ... + + padctl: padctl@0,7009f000 { + pinmux { + pinctrl-0 = <&padctl_default>; + pinctrl-names = "default"; + + padctl_default: pinmux { + usb3 { + nvidia,lanes = "pcie-0", "pcie-1"; + nvidia,function = "usb3"; + nvidia,iddq = <0>; + }; + + pcie { + nvidia,lanes = "pcie-2", "pcie-3", + "pcie-4"; + nvidia,function = "pcie"; + nvidia,iddq = <0>; + }; + + sata { + nvidia,lanes = "sata-0"; + nvidia,function = "sata"; + nvidia,iddq = <0>; + }; + }; + }; + };