diff mbox

[RFC] dt: Tegra XUSB padctl: per-lane PHYs and USB lane map

Message ID 1445297442-21439-1-git-send-email-swarren@wwwdotorg.org
State Superseded, archived
Headers show

Commit Message

Stephen Warren Oct. 19, 2015, 11:30 p.m. UTC
From: Stephen Warren <swarren@nvidia.com>

Convert the binding to provide a PHY per lane, rather than a PHY per
"pad" block in the hardware. This will allow the driver to easily know
which lanes are used by clients, and thus only enable those lanes, and
generally better aligns with the fact the hardware has configuration per
lane rather than solely configuration per "pad" block.

Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
Tegra210, which will allow an XUSB DT node to reference the PHYs it needs.

Add an nvidia,ss-port-map register to allow configuration of the
XUSB_PADCTL_SS_PORT_MAP register.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Thierry, Jon, here's a start at where I think the XUSB padctl binding
should go. What are your thoughts on this approach?

Kishon, this implements a "PHY-per-lane" approach for Tegra's XUSB padctl
module. Does the result look good to you? You may need to apply the patch
for the binding doc to make sense since I guess you aren't familiar with
the binding or HW.

 .../pinctrl/nvidia,tegra124-xusb-padctl.txt        | 122 +++++++++++++++------
 include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h   |  24 +++-
 2 files changed, 108 insertions(+), 38 deletions(-)

Comments

Jon Hunter Oct. 20, 2015, 10:10 a.m. UTC | #1
On 20/10/15 00:30, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Convert the binding to provide a PHY per lane, rather than a PHY per
> "pad" block in the hardware. This will allow the driver to easily know
> which lanes are used by clients, and thus only enable those lanes, and
> generally better aligns with the fact the hardware has configuration per
> lane rather than solely configuration per "pad" block.
> 
> Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
> Tegra210, which will allow an XUSB DT node to reference the PHYs it needs.
> 
> Add an nvidia,ss-port-map register to allow configuration of the
> XUSB_PADCTL_SS_PORT_MAP register.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Thierry, Jon, here's a start at where I think the XUSB padctl binding
> should go. What are your thoughts on this approach?
> 
> Kishon, this implements a "PHY-per-lane" approach for Tegra's XUSB padctl
> module. Does the result look good to you? You may need to apply the patch
> for the binding doc to make sense since I guess you aren't familiar with
> the binding or HW.
> 
>  .../pinctrl/nvidia,tegra124-xusb-padctl.txt        | 122 +++++++++++++++------
>  include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h   |  24 +++-
>  2 files changed, 108 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> index 3952d893635c..9b39f3283636 100644
> --- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> @@ -1,16 +1,31 @@
>  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.
> +The Tegra XUSB pad controller manages a set of "pads". Each pad drives output
> +to (or receives input from) one or more SoC IO signals, or (pad) lanes. Note
> +that in many contexts, a "pad" is an individual pin/signal/ball on a chip
> +package. In the context of this documentation, a "pad" refers to a sub-block
> +of the XUSB padctl HW module, which in turn is attached to potentially more
> +than one pin/signal/ball on the chip. This unusual use of terminology is
> +consistent with Tegra hardware documentation.

Ugh, you are right I see things like "7-lane pad" in the documentation.
In my mind pad has always been one physical ball and lane came from pcie
for a differential pair.

> +These pads must typically be powered up and configured before they can be
> +used. Individual lanes may also need various configuration applied to be
> +useful. This binding allows specification of the set of pads and lanes to be
> +used in a particular system, and their associated configuration.
> +
> +Various IO controllers are attached to the pad controller internally to Tegra.
> +Examples are USB2, XUSB, SATA, and PCIe. The XUSB padctl contains muxes that
> +route the IO controllers' lanes to the lanes of the pads. This binding allows
> +specification of the intended configuration of these muxes.
>  
>  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.
>  
> +Each PHY provided by this binding refers to a single (differential, and
> +typicallly bi-directional) lane of a pad.
>
>  Required properties:
>  --------------------
>  - compatible: Valid options are:
> @@ -24,8 +39,19 @@ Required properties:
>    See ../reset/reset.txt for details.
>  - reset-names: Must include the following entries:
>    - padctl
> -- #phy-cells: Should be 1. The specifier is the index of the PHY to reference.
> -  See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of valid values.
> +- #phy-cells: Should be 2. The first cell indicates the pad that implements
> +  the lane. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of
> +  valid values. The second cell indicates the lane within the pad.
> +
> +Optional properties:
> +--------------------
> +- nvidia,ss-port-map: This value to program into the XUSB_PADCTL_SS_PORT_MAP
> +  register. If this value is missing, no programming of the register should
> +  occur.
> +
> +FIXME: Probably need other global properties to initialize registers such as
> +XUSB_PADCTL_SNPS_OC_MAP_0, XUSB_PADCTL_USB2_OC_MAP_0,
> +XUSB_PADCTL_VBUS_OC_MAP_0...
>  
>  Lane muxing:
>  ------------
> @@ -34,28 +60,34 @@ 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 describes arbitary sets of (pad) lanes along with muxing and
> +configuration parameters that should be applied to them. 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
> +words, 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.
> +subnode that describes only an configuration 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 (pad).
> +- nvidia,lanes: An array of strings. Each string is the name of a (pad) lane.
>    Valid values for lanes are listed below.
>  
>  Optional properties:
>  - nvidia,function: A string that is the name of the function (IO controller)
>    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)
> +- FIXME: Need to add e.g. nvidia,hsic-strobe-trim,
> +  nvidia,otg-hs-curr-level-offset, ...
> +
> +Deprecated properties:
> +- nvidia,iddq: (single-cell) Integer. Programming this value statically
> +  violates prescribed HW programming rules, hence this property is deprecated.
>  
>  Note that not all of these properties are valid for all lanes. Lanes can be
>  divided into three groups:
> @@ -66,7 +98,8 @@ divided into three groups:
>  
>      Valid functions for this group are: "snps", "xusb", "uart", "rsvd".
>  
> -    The nvidia,iddq property does not apply to this group.
> +    FIXME: I'm not sure if usb2-bias needs to be exposed as a PHY, or if XUSB
> +    padctl can simply configure/enable it itself?
>  
>    - ulpi-0, hsic-0, hsic-1:
>  
> @@ -74,8 +107,6 @@ divided into three groups:
>  
>      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, pcie-5, pcie-6, sata-0:
>  
>      (pcie-5, pcie-6 are valid on Tegra210 only.)
> @@ -86,7 +117,6 @@ divided into three groups:
>      On Tegra210, valid functions for this group are "pcie-x1", "usb3",
>      "sata", "pcie-x4".
>  
> -
>  Example:
>  ========
>  
> @@ -99,45 +129,65 @@ SoC file extract:
>  		resets = <&tegra_car 142>;
>  		reset-names = "padctl";
>  
> -		#phy-cells = <1>;
> +		#phy-cells = <2>;
>  	};
>  
>  Board file extract:
>  -------------------
>  
>  	pcie-controller@0,01003000 {
> -		...
> -
> -		phys = <&padctl 0>;
> -		phy-names = "pcie";
> +		status = "okay";
> +
> +		pci@1,0 {
> +			status = "okay";
> +			nvidia,num-lanes = <4>;
> +			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 1>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 2>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 3>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 4>;
> +			/* These are the lane IDs within this PCIe port */
> +			phy-names = "0", "1", "2", "3";

Looks good. So the above simply indicates what pad lane is used, but the
actual pin mux is still setup by the default pin mux below? Would the
xlate verify the pad lane is setup correctly?

> +		};
>  
> -		...
> +		pci@2,0 {
> +			status = "okay";
> +			nvidia,num-lanes = <1>;
> +			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 0>;
> +			phy-names = "0";
> +		};
>  	};
>  
> -	...
> +	padctl@0,7009f000 {
> +		status = "okay";
>  
> -	padctl: padctl@0,7009f000 {
>  		pinctrl-0 = <&padctl_default>;
>  		pinctrl-names = "default";
>  
>  		padctl_default: pinmux {
> +			xusb {
> +				nvidia,lanes = "otg-1", "otg-2";
> +				nvidia,function = "xusb";
> +			};
> +
>  			usb3 {
> -				nvidia,lanes = "pcie-0", "pcie-1";
> +				nvidia,lanes = "pcie-5", "pcie-6";
>  				nvidia,function = "usb3";
> -				nvidia,iddq = <0>;
>  			};
>  
> -			pcie {
> -				nvidia,lanes = "pcie-2", "pcie-3",
> -					       "pcie-4";
> -				nvidia,function = "pcie";
> -				nvidia,iddq = <0>;
> +			pcie-x1 {
> +				nvidia,lanes = "pcie-0";
> +				nvidia,function = "pcie-x1";
> +			};
> +
> +			pcie-x4 {
> +				nvidia,lanes = "pcie-1", "pcie-2",
> +					       "pcie-3", "pcie-4";
> +				nvidia,function = "pcie-x4";

It is worth having a separate example for t210 versus changing the above
because it is still valid for t124.

>  			};
>  
>  			sata {
>  				nvidia,lanes = "sata-0";
>  				nvidia,function = "sata";
> -				nvidia,iddq = <0>;
>  			};
>  		};
>  	};
> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> index 914d56da9324..edc961202d1c 100644
> --- a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> +++ b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> @@ -1,7 +1,27 @@
>  #ifndef _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H
>  #define _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H 1
>  
> -#define TEGRA_XUSB_PADCTL_PCIE 0
> -#define TEGRA_XUSB_PADCTL_SATA 1
> +/*
> + * These legacy specifiers represent a client that uses an unspecified subset
> + * of the lanes within a particular "pad". Drivers that handle these
> + * specifiers should enable all lanes of the pad.
> + */
> +#define TEGRA_XUSB_PADCTL_PCIE_LEGACY	0
> +#define TEGRA_XUSB_PADCTL_SATA_LEGACY	1

Do you know if the pcie and sata drivers for tegra actually use the
"phys" and "phy-names" properties? I had a quick grep for "phy_get" and
"phy-names" in the pcie and ata drivers but did not see any usage for
tegra. I am wondering if tegra124 is really just relying on the default
pin-mux as opposed to the phy properties. If so may be we could get rid
of the above and update the binding without breaking anything?

> +
> +/*
> + * These represent the set of "pads" that exist within XUSB padctl hardware.
> + * Different pads contain a different number of lanes. See the TRM for
> + * details. The second cell of any specifier that uses these values will
> + * represent the specific lane that the client uses.
> + */
> +#define TEGRA_XUSB_PADCTL_PCIE		2
> +#define TEGRA_XUSB_PADCTL_SATA		3
> +#define TEGRA_XUSB_PADCTL_HSIC		4
> +#define TEGRA_XUSB_PADCTL_USB2_BIAS	5
> +#define TEGRA_XUSB_PADCTL_USB2_0	6
> +#define TEGRA_XUSB_PADCTL_USB2_1	7
> +#define TEGRA_XUSB_PADCTL_USB2_2	8
> +#define TEGRA_XUSB_PADCTL_USB2_3	9
>  
>  #endif /* _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H */
>

Jon

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Oct. 20, 2015, 3:56 p.m. UTC | #2
On 10/20/2015 04:10 AM, Jon Hunter wrote:
>
> On 20/10/15 00:30, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Convert the binding to provide a PHY per lane, rather than a PHY per
>> "pad" block in the hardware. This will allow the driver to easily know
>> which lanes are used by clients, and thus only enable those lanes, and
>> generally better aligns with the fact the hardware has configuration per
>> lane rather than solely configuration per "pad" block.
>>
>> Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
>> Tegra210, which will allow an XUSB DT node to reference the PHYs it needs.
>>
>> Add an nvidia,ss-port-map register to allow configuration of the
>> XUSB_PADCTL_SS_PORT_MAP register.

>> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt

>> +The Tegra XUSB pad controller manages a set of "pads". Each pad drives output
>> +to (or receives input from) one or more SoC IO signals, or (pad) lanes. Note
>> +that in many contexts, a "pad" is an individual pin/signal/ball on a chip
>> +package. In the context of this documentation, a "pad" refers to a sub-block
>> +of the XUSB padctl HW module, which in turn is attached to potentially more
>> +than one pin/signal/ball on the chip. This unusual use of terminology is
>> +consistent with Tegra hardware documentation.
>
> Ugh, you are right I see things like "7-lane pad" in the documentation.
> In my mind pad has always been one physical ball and lane came from pcie
> for a differential pair.

Yes, so the "7-lane pad" probably has something like 28 physical 
pads/pins/balls ignoring any power/ground/...

>>   Board file extract:
>>   -------------------
>>
>>   	pcie-controller@0,01003000 {
>> -		...
>> -
>> -		phys = <&padctl 0>;
>> -		phy-names = "pcie";
>> +		status = "okay";
>> +
>> +		pci@1,0 {
>> +			status = "okay";
>> +			nvidia,num-lanes = <4>;
>> +			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 1>,
>> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 2>,
>> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 3>,
>> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 4>;
>> +			/* These are the lane IDs within this PCIe port */
>> +			phy-names = "0", "1", "2", "3";
>
> Looks good. So the above simply indicates what pad lane is used, but the
> actual pin mux is still setup by the default pin mux below?

Yes.

> Would the xlate verify the pad lane is setup correctly?

I don't think so. of_xlate() doesn't have any context such as what type 
of driver is parsing the DT or for what purpose, so it couldn't validate 
that the pinctrl settings for that node had actually assigned the 
correct option to the mux register for that lane.

>> +	padctl@0,7009f000 {
>> +		status = "okay";
>>
>> -	padctl: padctl@0,7009f000 {
>>   		pinctrl-0 = <&padctl_default>;
>>   		pinctrl-names = "default";
>>
>>   		padctl_default: pinmux {
>> +			xusb {
>> +				nvidia,lanes = "otg-1", "otg-2";
>> +				nvidia,function = "xusb";
>> +			};
>> +
>>   			usb3 {
>> -				nvidia,lanes = "pcie-0", "pcie-1";
>> +				nvidia,lanes = "pcie-5", "pcie-6";
>>   				nvidia,function = "usb3";
>> -				nvidia,iddq = <0>;
>>   			};
>>
>> -			pcie {
>> -				nvidia,lanes = "pcie-2", "pcie-3",
>> -					       "pcie-4";
>> -				nvidia,function = "pcie";
>> -				nvidia,iddq = <0>;
>> +			pcie-x1 {
>> +				nvidia,lanes = "pcie-0";
>> +				nvidia,function = "pcie-x1";
>> +			};
>> +
>> +			pcie-x4 {
>> +				nvidia,lanes = "pcie-1", "pcie-2",
>> +					       "pcie-3", "pcie-4";
>> +				nvidia,function = "pcie-x4";
>
> It is worth having a separate example for t210 versus changing the above
> because it is still valid for t124.

Perhaps I should just modify the example rather than pasting a new 
version over the top. I did a paste since I already had the DT content, 
but IIRC the only change I really need to make here is to delete the 
nvidia,iddq properties.

>> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h

>> -#define TEGRA_XUSB_PADCTL_PCIE 0
>> -#define TEGRA_XUSB_PADCTL_SATA 1
>> +/*
>> + * These legacy specifiers represent a client that uses an unspecified subset
>> + * of the lanes within a particular "pad". Drivers that handle these
>> + * specifiers should enable all lanes of the pad.
>> + */
>> +#define TEGRA_XUSB_PADCTL_PCIE_LEGACY	0
>> +#define TEGRA_XUSB_PADCTL_SATA_LEGACY	1
>
> Do you know if the pcie and sata drivers for tegra actually use the
> "phys" and "phy-names" properties? I had a quick grep for "phy_get" and
> "phy-names" in the pcie and ata drivers but did not see any usage for
> tegra. I am wondering if tegra124 is really just relying on the default
> pin-mux as opposed to the phy properties. If so may be we could get rid
> of the above and update the binding without breaking anything?

In drivers/pci/host/pci-tegra.c tegra_pcie_get_resources() I see a call 
to devm_phy_optional_get().

The SATA driver doesn't seem to do anything with phys at the moment, 
although tegra124.dtsi does put phy-related properties into the SATA DT 
node.

So, we can either:
a) Just ignore DT ABI for these cases claiming the DT binding is not yet 
declared stable.

b) Continue to support those specifier values for ABI reasons, which 
needs the "_LEGACY" values shown above.

(a) is certainly simpler.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 21, 2015, 12:15 p.m. UTC | #3
On Mon, Oct 19, 2015 at 05:30:42PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Convert the binding to provide a PHY per lane, rather than a PHY per
> "pad" block in the hardware. This will allow the driver to easily know
> which lanes are used by clients, and thus only enable those lanes, and
> generally better aligns with the fact the hardware has configuration per
> lane rather than solely configuration per "pad" block.
> 
> Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
> Tegra210, which will allow an XUSB DT node to reference the PHYs it needs.
> 
> Add an nvidia,ss-port-map register to allow configuration of the
> XUSB_PADCTL_SS_PORT_MAP register.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Thierry, Jon, here's a start at where I think the XUSB padctl binding
> should go. What are your thoughts on this approach?
> 
> Kishon, this implements a "PHY-per-lane" approach for Tegra's XUSB padctl
> module. Does the result look good to you? You may need to apply the patch
> for the binding doc to make sense since I guess you aren't familiar with
> the binding or HW.
> 
>  .../pinctrl/nvidia,tegra124-xusb-padctl.txt        | 122 +++++++++++++++------
>  include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h   |  24 +++-
>  2 files changed, 108 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> index 3952d893635c..9b39f3283636 100644
> --- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> @@ -1,16 +1,31 @@
>  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.
> +The Tegra XUSB pad controller manages a set of "pads". Each pad drives output
> +to (or receives input from) one or more SoC IO signals, or (pad) lanes. Note
> +that in many contexts, a "pad" is an individual pin/signal/ball on a chip
> +package. In the context of this documentation, a "pad" refers to a sub-block
> +of the XUSB padctl HW module, which in turn is attached to potentially more
> +than one pin/signal/ball on the chip. This unusual use of terminology is
> +consistent with Tegra hardware documentation.
> +
> +These pads must typically be powered up and configured before they can be
> +used. Individual lanes may also need various configuration applied to be
> +useful. This binding allows specification of the set of pads and lanes to be
> +used in a particular system, and their associated configuration.
> +
> +Various IO controllers are attached to the pad controller internally to Tegra.
> +Examples are USB2, XUSB, SATA, and PCIe. The XUSB padctl contains muxes that
> +route the IO controllers' lanes to the lanes of the pads. This binding allows
> +specification of the intended configuration of these muxes.
>  
>  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.
>  
> +Each PHY provided by this binding refers to a single (differential, and
> +typicallly bi-directional) lane of a pad.
> +
>  Required properties:
>  --------------------
>  - compatible: Valid options are:
> @@ -24,8 +39,19 @@ Required properties:
>    See ../reset/reset.txt for details.
>  - reset-names: Must include the following entries:
>    - padctl
> -- #phy-cells: Should be 1. The specifier is the index of the PHY to reference.
> -  See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of valid values.
> +- #phy-cells: Should be 2. The first cell indicates the pad that implements
> +  the lane. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of
> +  valid values. The second cell indicates the lane within the pad.
> +
> +Optional properties:
> +--------------------
> +- nvidia,ss-port-map: This value to program into the XUSB_PADCTL_SS_PORT_MAP
> +  register. If this value is missing, no programming of the register should
> +  occur.
> +
> +FIXME: Probably need other global properties to initialize registers such as
> +XUSB_PADCTL_SNPS_OC_MAP_0, XUSB_PADCTL_USB2_OC_MAP_0,
> +XUSB_PADCTL_VBUS_OC_MAP_0...
>  
>  Lane muxing:
>  ------------
> @@ -34,28 +60,34 @@ 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 describes arbitary sets of (pad) lanes along with muxing and
> +configuration parameters that should be applied to them. 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
> +words, 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.
> +subnode that describes only an configuration 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 (pad).
> +- nvidia,lanes: An array of strings. Each string is the name of a (pad) lane.
>    Valid values for lanes are listed below.
>  
>  Optional properties:
>  - nvidia,function: A string that is the name of the function (IO controller)
>    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)
> +- FIXME: Need to add e.g. nvidia,hsic-strobe-trim,
> +  nvidia,otg-hs-curr-level-offset, ...
> +
> +Deprecated properties:
> +- nvidia,iddq: (single-cell) Integer. Programming this value statically
> +  violates prescribed HW programming rules, hence this property is deprecated.
>  
>  Note that not all of these properties are valid for all lanes. Lanes can be
>  divided into three groups:
> @@ -66,7 +98,8 @@ divided into three groups:
>  
>      Valid functions for this group are: "snps", "xusb", "uart", "rsvd".
>  
> -    The nvidia,iddq property does not apply to this group.
> +    FIXME: I'm not sure if usb2-bias needs to be exposed as a PHY, or if XUSB
> +    padctl can simply configure/enable it itself?
>  
>    - ulpi-0, hsic-0, hsic-1:
>  
> @@ -74,8 +107,6 @@ divided into three groups:
>  
>      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, pcie-5, pcie-6, sata-0:
>  
>      (pcie-5, pcie-6 are valid on Tegra210 only.)
> @@ -86,7 +117,6 @@ divided into three groups:
>      On Tegra210, valid functions for this group are "pcie-x1", "usb3",
>      "sata", "pcie-x4".
>  
> -
>  Example:
>  ========
>  
> @@ -99,45 +129,65 @@ SoC file extract:
>  		resets = <&tegra_car 142>;
>  		reset-names = "padctl";
>  
> -		#phy-cells = <1>;
> +		#phy-cells = <2>;
>  	};
>  
>  Board file extract:
>  -------------------
>  
>  	pcie-controller@0,01003000 {
> -		...
> -
> -		phys = <&padctl 0>;
> -		phy-names = "pcie";
> +		status = "okay";
> +
> +		pci@1,0 {
> +			status = "okay";
> +			nvidia,num-lanes = <4>;
> +			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 1>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 2>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 3>,
> +			       <&padctl TEGRA_XUSB_PADCTL_PCIE 4>;
> +			/* These are the lane IDs within this PCIe port */
> +			phy-names = "0", "1", "2", "3";
> +		};
>  
> -		...
> +		pci@2,0 {
> +			status = "okay";
> +			nvidia,num-lanes = <1>;
> +			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 0>;
> +			phy-names = "0";
> +		};
>  	};
>  
> -	...
> +	padctl@0,7009f000 {
> +		status = "okay";
>  
> -	padctl: padctl@0,7009f000 {
>  		pinctrl-0 = <&padctl_default>;
>  		pinctrl-names = "default";
>  
>  		padctl_default: pinmux {
> +			xusb {
> +				nvidia,lanes = "otg-1", "otg-2";
> +				nvidia,function = "xusb";
> +			};
> +
>  			usb3 {
> -				nvidia,lanes = "pcie-0", "pcie-1";
> +				nvidia,lanes = "pcie-5", "pcie-6";
>  				nvidia,function = "usb3";
> -				nvidia,iddq = <0>;
>  			};
>  
> -			pcie {
> -				nvidia,lanes = "pcie-2", "pcie-3",
> -					       "pcie-4";
> -				nvidia,function = "pcie";
> -				nvidia,iddq = <0>;
> +			pcie-x1 {
> +				nvidia,lanes = "pcie-0";
> +				nvidia,function = "pcie-x1";
> +			};
> +
> +			pcie-x4 {
> +				nvidia,lanes = "pcie-1", "pcie-2",
> +					       "pcie-3", "pcie-4";
> +				nvidia,function = "pcie-x4";
>  			};
>  
>  			sata {
>  				nvidia,lanes = "sata-0";
>  				nvidia,function = "sata";
> -				nvidia,iddq = <0>;
>  			};
>  		};
>  	};

According to Kishon's latest recommendation, the padctl binding should
probably look more like this:

	padctl@0,7009f000 {
		...

		phys {
			pcie {
				/* 5 subnodes on Tegra124, 7 on Tegra210 */
				pcie-0 {
					...
				};

				...
			};

			sata {
				sata-0 {
					...
				};
			};

			utmi {
				/* 3 subnodes on Tegra124, 4 on Tegra210 */
				utmi-0 {
					...
				};

				...
			};

			hsic {
				/* 2 subnodes */
				hsic-0 {
					...
				};

				...
			};

			/* on Tegra124 only */
			ulpi {
				ulpi-0 {
					...
				};
			};
		};
	};

This is pretty much a direct transcription of the padctl block diagram
in the TRM (e.g. page 1312 in vB4 of the Tegra X1 TRM).

Once we have this, I'm beginning to think that we should just drop the
pinctrl component, because we now have subnodes for each lane that can
carry the configuration. As an aside, going with this more verbose DT
representation should also give us an easy way of providing backwards-
compatibility. The driver could look for the existence of the phys
sub-node and fallback to the old code in its absence.

The above is also nice because it gives us a complete picture of what
lanes are being used. For example, if some of the lanes are unused the
corresponding device tree nodes could be marked status = "disabled",
which would come in handy for the programming (e.g. IDDQ).

I think for XUSB we'll need some additional information, though. Your
proposal already adds the nvidia,ss-port-map property to add some of the
information (mapping of SS ports to USB2 ports), but I think that's a)
not very readable and b) doesn't give us enough flexibility to describe
additional meta-data. I'm aware of at least two other pieces of
information that we need: VBUS power supplies and port mode. Originally
I think the binding used indexed names (vbus0-supply, vbus1-supply, ...)
for the VBUS supplies but that's also suboptimally structured in my
opinion. I think we could combine both into something of this sort
(within the padctl node above):

		ports {
			usb3-0 {
				vbus-supply = <&vbus1_reg>;
				nvidia,port = <1>;
			};

			usb3-1 {
				vbus-supply = <&vbus2_reg>;
				nvidia,port = <2>;
			};

			...

			utmi-0 {
				vbus-supply = <&vbus0_reg>;
				mode = "otg";
			};

			utmi-1 {
				vbus-supply = <&vbus1_reg>;
				mode = "host";
			};

			utmi-2 {
				vbus-supply = <&vbus2_reg>;
				mode = "host";
			};
		};

Note how both usb3-* and utmi-* ports specify the same regulators for
the same physical port. It would probably be enough to simply have them
listed in one place, but I suspect boards could omit USB2 fallback mode
and wire up only the SS lanes, though I'm not sure if that's permitted.

Granted, the above is rather verbose, but I also think it's a very
readable and at the same time accurate representation of the hardware
and its capabilities.

> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> index 914d56da9324..edc961202d1c 100644
> --- a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> +++ b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
> @@ -1,7 +1,27 @@
>  #ifndef _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H
>  #define _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H 1
>  
> -#define TEGRA_XUSB_PADCTL_PCIE 0
> -#define TEGRA_XUSB_PADCTL_SATA 1
> +/*
> + * These legacy specifiers represent a client that uses an unspecified subset
> + * of the lanes within a particular "pad". Drivers that handle these
> + * specifiers should enable all lanes of the pad.
> + */
> +#define TEGRA_XUSB_PADCTL_PCIE_LEGACY	0
> +#define TEGRA_XUSB_PADCTL_SATA_LEGACY	1

With the above binding changes I think we could leave this in place.
These indices would be purely for backwards-compatibility and newer
device trees would instead refer to the PHYs directly by the phandle
(with #phy-cells = <0>).

Thierry
Stephen Warren Oct. 21, 2015, 5:40 p.m. UTC | #4
On 10/21/2015 06:15 AM, Thierry Reding wrote:
> On Mon, Oct 19, 2015 at 05:30:42PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Convert the binding to provide a PHY per lane, rather than a PHY per
>> "pad" block in the hardware. This will allow the driver to easily know
>> which lanes are used by clients, and thus only enable those lanes, and
>> generally better aligns with the fact the hardware has configuration per
>> lane rather than solely configuration per "pad" block.
>>
>> Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
>> Tegra210, which will allow an XUSB DT node to reference the PHYs it needs.
>>
>> Add an nvidia,ss-port-map register to allow configuration of the
>> XUSB_PADCTL_SS_PORT_MAP register.

> According to Kishon's latest recommendation, the padctl binding should
> probably look more like this:
>
> 	padctl@0,7009f000 {
> 		...
>
> 		phys {
> 			pcie {
> 				/* 5 subnodes on Tegra124, 7 on Tegra210 */
> 				pcie-0 {
> 					...
> 				};
>
> 				...
> 			};

I noticed that he mentioned a separate node per PHY brick or PHY.

That seems like an odd requirement, or even recommendation, since the 
PHY bindings, like (almost?) all other DT provider/consumer bindings, 
use a phandle+specifier to indicate which resource is being provided. As 
such, there's no absolute need to represent objects as DT nodes, 
although there may be other good arguments for doing so.

> This is pretty much a direct transcription of the padctl block diagram
> in the TRM (e.g. page 1312 in vB4 of the Tegra X1 TRM).
>
> Once we have this, I'm beginning to think that we should just drop the
> pinctrl component, because we now have subnodes for each lane that can
> carry the configuration. As an aside, going with this more verbose DT
> representation should also give us an easy way of providing backwards-
> compatibility. The driver could look for the existence of the phys
> sub-node and fallback to the old code in its absence.

I would assert that if we're going to make this kind of radical change 
to the binding, we should simply change the compatible value. This will 
allow us to have two completely separate drivers for the two different 
DT representations. In turn, that will keep each driver simpler, in that 
we don't have two different drivers intermixed into one file.

The two drivers could share some common code e.g. by calling into a 
common C file for some purposes, and only have separate DT parsing 
logic, if that works out well.

In the kernel I guess we'd technically need to keep each driver around 
"forever" due to DT ABI requirements.

In U-Boot, I'd suggest simply dropping support for the old 
compatible-value since we know that the DT is part of the U-Boot source 
tree and tightly coupled. U-Boot is firmware after all.

> The above is also nice because it gives us a complete picture of what
> lanes are being used. For example, if some of the lanes are unused the
> corresponding device tree nodes could be marked status = "disabled",
> which would come in handy for the programming (e.g. IDDQ).

I worry that it's a bit verbose. Still, it's quite readable and as you 
say obviously maps to the documentation, so it may be worth going with this.

> I think for XUSB we'll need some additional information, though. Your
> proposal already adds the nvidia,ss-port-map property to add some of the
> information (mapping of SS ports to USB2 ports), but I think that's a)
> not very readable and b) doesn't give us enough flexibility to describe
> additional meta-data.

That property was only intended to represent that single piece of 
information. If there's other information that needs to be represented, 
we should certainly add more properties. I didn't enumerate all of them 
in my proposal since I was mainly concentrating on discussion the 
representation/specification of a PHY object, and not all the other 
miscellaneous configuration that the HW module has.

For ss-port-map in particular, I believe we should just use the same 
encoding that the HW register itself has. This keeps DT parsing simple, 
and keeps the binding directly in line with the HW documentation.

> I'm aware of at least two other pieces of
> information that we need: VBUS power supplies and port mode.

We also need to configure the mapping between USB controller ports and 
"OC" (over-current) pins, and various per-lane tuning parameters. For 
reference, there's a patch in Andrew Bresticker's tree that adds the 
tuning parameters:

git://github.com/abrestic/linux.git tegra-xhci-v8-test
bff711f935a989c220eabeeffffc150f18f54d7e
pinctrl: Update Tegra XUSB pad controller binding for USB

> Originally
> I think the binding used indexed names (vbus0-supply, vbus1-supply, ...)
> for the VBUS supplies but that's also suboptimally structured in my
> opinion. I think we could combine both into something of this sort
> (within the padctl node above):
>
> 		ports {
> 			usb3-0 {
> 				vbus-supply = <&vbus1_reg>;
> 				nvidia,port = <1>;
> 			};

Is there a standard binding for USB connectors? I've heard hints of a 
standard binding for e.g. HDMI connectors recently. Things like VBUS 
supply, ID pin reading, port mode, OC detection, etc. seem like they'd 
be quite generally applicable, although I must admit I'd rather not 
rat-hole this thread into designing any kind of universal standard...

> 			utmi-1 {
> 				vbus-supply = <&vbus1_reg>;
> 				mode = "host";
> 			};

> Note how both usb3-* and utmi-* ports specify the same regulators for
> the same physical port. It would probably be enough to simply have them
> listed in one place, but I suspect boards could omit USB2 fallback mode
> and wire up only the SS lanes, though I'm not sure if that's permitted.

I don't think that's permitted by USB spec.

It seems like it'd be better to represent the concept of a physical USB 
port. In this case, you wouldn't need to represent the VBUS supply 
multiple times since the physical port only has a single supply. Mapping 
the physical port to the various USB2 and USB3 controller ports could 
happen either elsewhere, or as properties within the physical port object.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I Oct. 21, 2015, 8:10 p.m. UTC | #5
Hi,

On Wednesday 21 October 2015 11:10 PM, Stephen Warren wrote:
> On 10/21/2015 06:15 AM, Thierry Reding wrote:
>> On Mon, Oct 19, 2015 at 05:30:42PM -0600, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Convert the binding to provide a PHY per lane, rather than a PHY per
>>> "pad" block in the hardware. This will allow the driver to easily know
>>> which lanes are used by clients, and thus only enable those lanes, and
>>> generally better aligns with the fact the hardware has configuration per
>>> lane rather than solely configuration per "pad" block.
>>>
>>> Add entries to pinctrl-tegra-xusb.h to enumerate all "pad" blocks on
>>> Tegra210, which will allow an XUSB DT node to reference the PHYs it
>>> needs.
>>>
>>> Add an nvidia,ss-port-map register to allow configuration of the
>>> XUSB_PADCTL_SS_PORT_MAP register.
> 
>> According to Kishon's latest recommendation, the padctl binding should
>> probably look more like this:
>>
>>     padctl@0,7009f000 {
>>         ...
>>
>>         phys {
>>             pcie {
>>                 /* 5 subnodes on Tegra124, 7 on Tegra210 */
>>                 pcie-0 {
>>                     ...
>>                 };
>>
>>                 ...
>>             };
> 
> I noticed that he mentioned a separate node per PHY brick or PHY.
> 
> That seems like an odd requirement, or even recommendation, since the
> PHY bindings, like (almost?) all other DT provider/consumer bindings,
> use a phandle+specifier to indicate which resource is being provided. As

A lot of that was added before PHY core was better able to handle multi
phy PHY provider. Using phandle+specifier makes the driver code do lot
of stuff just to find the PHY which was unnecessary. This can be avoided
just by modeling the dt node properly and using the correct phandle in
the controller dt node.

> such, there's no absolute need to represent objects as DT nodes,
> although there may be other good arguments for doing so.

yeah, that would be a better representation of the hw and avoid lot of
useless stuff in the driver.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen Oct. 22, 2015, 1:03 p.m. UTC | #6
On 10/20/2015 06:56 PM, Stephen Warren wrote:
> ...
>
> In drivers/pci/host/pci-tegra.c tegra_pcie_get_resources() I see a call
> to devm_phy_optional_get().
>
> The SATA driver doesn't seem to do anything with phys at the moment,
> although tegra124.dtsi does put phy-related properties into the SATA DT
> node.

FWIW, the SATA driver does use the PHY - just through the 
ahci_platform_{get,enable,...}_resources API. (so is life..)

Mikko

>
> So, we can either:
> a) Just ignore DT ABI for these cases claiming the DT binding is not yet
> declared stable.
>
> b) Continue to support those specifier values for ABI reasons, which
> needs the "_LEGACY" values shown above.
>
> (a) is certainly simpler.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
index 3952d893635c..9b39f3283636 100644
--- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
@@ -1,16 +1,31 @@ 
 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.
+The Tegra XUSB pad controller manages a set of "pads". Each pad drives output
+to (or receives input from) one or more SoC IO signals, or (pad) lanes. Note
+that in many contexts, a "pad" is an individual pin/signal/ball on a chip
+package. In the context of this documentation, a "pad" refers to a sub-block
+of the XUSB padctl HW module, which in turn is attached to potentially more
+than one pin/signal/ball on the chip. This unusual use of terminology is
+consistent with Tegra hardware documentation.
+
+These pads must typically be powered up and configured before they can be
+used. Individual lanes may also need various configuration applied to be
+useful. This binding allows specification of the set of pads and lanes to be
+used in a particular system, and their associated configuration.
+
+Various IO controllers are attached to the pad controller internally to Tegra.
+Examples are USB2, XUSB, SATA, and PCIe. The XUSB padctl contains muxes that
+route the IO controllers' lanes to the lanes of the pads. This binding allows
+specification of the intended configuration of these muxes.
 
 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.
 
+Each PHY provided by this binding refers to a single (differential, and
+typicallly bi-directional) lane of a pad.
+
 Required properties:
 --------------------
 - compatible: Valid options are:
@@ -24,8 +39,19 @@  Required properties:
   See ../reset/reset.txt for details.
 - reset-names: Must include the following entries:
   - padctl
-- #phy-cells: Should be 1. The specifier is the index of the PHY to reference.
-  See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of valid values.
+- #phy-cells: Should be 2. The first cell indicates the pad that implements
+  the lane. See <dt-bindings/pinctrl/pinctrl-tegra-xusb.h> for the list of
+  valid values. The second cell indicates the lane within the pad.
+
+Optional properties:
+--------------------
+- nvidia,ss-port-map: This value to program into the XUSB_PADCTL_SS_PORT_MAP
+  register. If this value is missing, no programming of the register should
+  occur.
+
+FIXME: Probably need other global properties to initialize registers such as
+XUSB_PADCTL_SNPS_OC_MAP_0, XUSB_PADCTL_USB2_OC_MAP_0,
+XUSB_PADCTL_VBUS_OC_MAP_0...
 
 Lane muxing:
 ------------
@@ -34,28 +60,34 @@  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 describes arbitary sets of (pad) lanes along with muxing and
+configuration parameters that should be applied to them. 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
+words, 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.
+subnode that describes only an configuration 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 (pad).
+- nvidia,lanes: An array of strings. Each string is the name of a (pad) lane.
   Valid values for lanes are listed below.
 
 Optional properties:
 - nvidia,function: A string that is the name of the function (IO controller)
   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)
+- FIXME: Need to add e.g. nvidia,hsic-strobe-trim,
+  nvidia,otg-hs-curr-level-offset, ...
+
+Deprecated properties:
+- nvidia,iddq: (single-cell) Integer. Programming this value statically
+  violates prescribed HW programming rules, hence this property is deprecated.
 
 Note that not all of these properties are valid for all lanes. Lanes can be
 divided into three groups:
@@ -66,7 +98,8 @@  divided into three groups:
 
     Valid functions for this group are: "snps", "xusb", "uart", "rsvd".
 
-    The nvidia,iddq property does not apply to this group.
+    FIXME: I'm not sure if usb2-bias needs to be exposed as a PHY, or if XUSB
+    padctl can simply configure/enable it itself?
 
   - ulpi-0, hsic-0, hsic-1:
 
@@ -74,8 +107,6 @@  divided into three groups:
 
     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, pcie-5, pcie-6, sata-0:
 
     (pcie-5, pcie-6 are valid on Tegra210 only.)
@@ -86,7 +117,6 @@  divided into three groups:
     On Tegra210, valid functions for this group are "pcie-x1", "usb3",
     "sata", "pcie-x4".
 
-
 Example:
 ========
 
@@ -99,45 +129,65 @@  SoC file extract:
 		resets = <&tegra_car 142>;
 		reset-names = "padctl";
 
-		#phy-cells = <1>;
+		#phy-cells = <2>;
 	};
 
 Board file extract:
 -------------------
 
 	pcie-controller@0,01003000 {
-		...
-
-		phys = <&padctl 0>;
-		phy-names = "pcie";
+		status = "okay";
+
+		pci@1,0 {
+			status = "okay";
+			nvidia,num-lanes = <4>;
+			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 1>,
+			       <&padctl TEGRA_XUSB_PADCTL_PCIE 2>,
+			       <&padctl TEGRA_XUSB_PADCTL_PCIE 3>,
+			       <&padctl TEGRA_XUSB_PADCTL_PCIE 4>;
+			/* These are the lane IDs within this PCIe port */
+			phy-names = "0", "1", "2", "3";
+		};
 
-		...
+		pci@2,0 {
+			status = "okay";
+			nvidia,num-lanes = <1>;
+			phys = <&padctl TEGRA_XUSB_PADCTL_PCIE 0>;
+			phy-names = "0";
+		};
 	};
 
-	...
+	padctl@0,7009f000 {
+		status = "okay";
 
-	padctl: padctl@0,7009f000 {
 		pinctrl-0 = <&padctl_default>;
 		pinctrl-names = "default";
 
 		padctl_default: pinmux {
+			xusb {
+				nvidia,lanes = "otg-1", "otg-2";
+				nvidia,function = "xusb";
+			};
+
 			usb3 {
-				nvidia,lanes = "pcie-0", "pcie-1";
+				nvidia,lanes = "pcie-5", "pcie-6";
 				nvidia,function = "usb3";
-				nvidia,iddq = <0>;
 			};
 
-			pcie {
-				nvidia,lanes = "pcie-2", "pcie-3",
-					       "pcie-4";
-				nvidia,function = "pcie";
-				nvidia,iddq = <0>;
+			pcie-x1 {
+				nvidia,lanes = "pcie-0";
+				nvidia,function = "pcie-x1";
+			};
+
+			pcie-x4 {
+				nvidia,lanes = "pcie-1", "pcie-2",
+					       "pcie-3", "pcie-4";
+				nvidia,function = "pcie-x4";
 			};
 
 			sata {
 				nvidia,lanes = "sata-0";
 				nvidia,function = "sata";
-				nvidia,iddq = <0>;
 			};
 		};
 	};
diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
index 914d56da9324..edc961202d1c 100644
--- a/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
+++ b/include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h
@@ -1,7 +1,27 @@ 
 #ifndef _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H
 #define _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H 1
 
-#define TEGRA_XUSB_PADCTL_PCIE 0
-#define TEGRA_XUSB_PADCTL_SATA 1
+/*
+ * These legacy specifiers represent a client that uses an unspecified subset
+ * of the lanes within a particular "pad". Drivers that handle these
+ * specifiers should enable all lanes of the pad.
+ */
+#define TEGRA_XUSB_PADCTL_PCIE_LEGACY	0
+#define TEGRA_XUSB_PADCTL_SATA_LEGACY	1
+
+/*
+ * These represent the set of "pads" that exist within XUSB padctl hardware.
+ * Different pads contain a different number of lanes. See the TRM for
+ * details. The second cell of any specifier that uses these values will
+ * represent the specific lane that the client uses.
+ */
+#define TEGRA_XUSB_PADCTL_PCIE		2
+#define TEGRA_XUSB_PADCTL_SATA		3
+#define TEGRA_XUSB_PADCTL_HSIC		4
+#define TEGRA_XUSB_PADCTL_USB2_BIAS	5
+#define TEGRA_XUSB_PADCTL_USB2_0	6
+#define TEGRA_XUSB_PADCTL_USB2_1	7
+#define TEGRA_XUSB_PADCTL_USB2_2	8
+#define TEGRA_XUSB_PADCTL_USB2_3	9
 
 #endif /* _DT_BINDINGS_PINCTRL_TEGRA_XUSB_H */