[PATCH 0/2] Add NVIDIA Tegra XUSB pad controller bindings

[PATCH 0/2] Add NVIDIA Tegra XUSB pad controller bindings

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This is the result of my attempt to come up with a correct binding for
the XUSB pad controller found on NVIDIA Tegra SoCs. Note how this is a
completely new binding document. The old binding, which we should mark
deprecated if we settle on this new one, modelled the device as a
pinctrl/PHY hybrid, but it turns out that it's really not much of a
pin controller after all.

I have a driver, which is pretty much Andrew's work, heavily reworked
and put into drivers/phy. I've tested this on Jetson TK1 (for Tegra124)
and P2371-2180 (for Tegra210) and I successfully see PCIe, SATA and
super-speed USB working on Jetson TK1 as well as super-speed USB on the
P2371-2180 (root on NFS and USB 3.0 and USB 2.0 pen drives work fine).

Moving the driver into drivers/phy also gives us a trivial way to keep
backwards-compatibility by simply keeping the old driver in place and
calling into it from the new driver if a legacy DTB is detected.

I still need to do a bit of squashing and sorting of the driver patches
but I wanted to get this out now because I know Stephen is working on a
set of patches to support the XUSB pad controller on Tegra210 in U-Boot
and the discussion can hence continue irrespective of the driver code.

Note also that this isn't quite complete yet. I've left out most of the
HSIC-specific properties for now because there is no user for those in
the upstream kernel, so I didn't have an example nor test-case.

Thierry

Thierry Reding (2):
  dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding
  dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

 .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 689 +++++++++++++++++++++
 1 file changed, 689 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt

-- 
2.5.0

[PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
set of lanes that are used for PCIe, SATA and USB.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 359 +++++++++++++++++++++
 1 file changed, 359 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt

diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
new file mode 100644
index 000000000000..026e924ae54a
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
@@ -0,0 +1,359 @@
+Device tree binding for NVIDIA Tegra XUSB pad controller
+========================================================
+
+The Tegra XUSB pad controller manages a set of pads, each of which controls
+one or more lanes. Each lane can in turn be assigned to one out of a set of
+different outputs. A pad contains logic common for all its lanes. Each lane
+can additionally be separately configured and powered up.
+
+Some of the lanes are high-speed lanes, which can be used for PCIe, SATA or
+super-speed USB. Other lanes are for various types of low-speed, full-speed
+or high-speed USB (such as UTMI, ULPI and HSIC).
+
+In addition to per-lane configuration, USB 3.0 ports may require additional
+settings on a per-board basis.
+
+Pads will be represented as children of the top-level XUSB pad controller
+device tree node. Each lane exposed by the pad will be represented by its
+own subnode and can be referenced by users of the lane using the standard
+PHY bindings, as described by the phy-bindings.txt file in this directory.
+
+Required properties:
+--------------------
+- compatible: Must be:
+  - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132
+- reg: Physical base address and length of the controller's registers.
+- resets: Must contain an entry for each entry in reset-names.
+- reset-names: Must include the following entries:
+  - "padctl"
+- mboxes: Must contain an entry for each entry in mbox-names.
+- mbox-names: Must include the following entries:
+  - "xusb"
+
+
+Pad nodes:
+==========
+
+A required child node named "pads" contains a list of subnodes, one for each
+of the pads exposed by the XUSB pad controller. Each pad may need additional
+resources that can be referenced in its pad node.
+
+The "status" property is used to enable or disable the use of a pad. If set
+to "disabled", the pad will not be used on the given board. In order to use
+the pad and any of its lanes, this property must be set to "okay".
+
+For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
+and sata. No extra resources are required for operation of these pads.
+
+
+PHY nodes:
+==========
+
+Each pad node has one or more children, each representing one of the lanes
+controlled by the pad.
+
+Required properties:
+--------------------
+- status: Defines the operation status of the PHY. Valid values are:
+  - "disabled": the PHY is disabled
+  - "okay": the PHY is enabled
+- #phy-cells: Should be 0. Since each lane represents a single PHY, there is
+  no need for an additional specifier.
+- nvidia,function: The output function of the PHY. See below for a list of
+  valid functions per SoC generation.
+
+For Tegra124 and Tegra132, the list of valid PHY nodes is given below:
+- utmi: utmi-0, utmi-1, utmi-2
+  - functions: "snps", "xusb", "uart"
+- ulpi: ulpi-0
+  - functions: "snps", "xusb"
+- hsic: hsic-0, hsic-1
+  - functions: "snps", "xusb"
+- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4
+  - functions: "pcie", "usb3-ss"
+- sata: sata-0
+  - functions: "usb3-ss", "sata"
+
+Port nodes:
+===========
+
+A required child node named "ports" contains a list of all the ports exposed
+by the XUSB pad controller. Per-port configuration is only required for USB.
+
+UTMI ports:
+-----------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- mode: A string that determines the mode in which to run the port. Valid
+  values are:
+  - "host": for USB host mode
+  - "device": for USB device mode
+  - "otg": for USB OTG mode
+
+Optional properties:
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+- vbus-supply: phandle to a regulator supplying the VBUS voltage.
+
+ULPI ports:
+-----------
+
+Optional properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+
+HSIC ports:
+-----------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+
+Super-speed USB ports:
+----------------------
+
+Required properties:
+- status: Defines the operation status of the port. Valid values are:
+  - "disabled": the port is disabled
+  - "okay": the port is enabled
+- nvidia,port: A single cell that specifies the physical port number to map
+  this super-speed USB port to. The range of valid port numbers varies with
+  the SoC generation:
+  - 0-2: for Tegra124 and Tegra132
+
+Optional properties:
+- nvidia,internal: A boolean property whose presence determines that a port
+  is internal. In the absence of this property the port is considered to be
+  external.
+
+For Tegra124 and Tegra132, the XUSB pad controller exposes the following
+ports:
+- 3x UTMI: utmi-0, utmi-1, utmi-2
+- 1x ULPI: ulpi-0
+- 2x HSIC: hsic-0, hsic-1
+- 2x super-speed USB: usb3-0, usb3-1
+
+
+Examples:
+=========
+
+Tegra124 and Tegra132:
+----------------------
+
+SoC include:
+
+	padctl@0,7009f000 {
+		compatible = "nvidia,tegra124-xusb-padctl";
+		reg = <0x0 0x7009f000 0x0 0x1000>;
+		resets = <&tegra_car 142>;
+		reset-names = "padctl";
+		mboxes = <&xusb_mbox>;
+		mbox-names = "xusb";
+
+		pads {
+			utmi {
+				status = "disabled";
+
+				utmi-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				utmi-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				utmi-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			ulpi {
+				status = "disabled";
+
+				ulpi-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			hsic {
+				status = "disabled";
+
+				hsic-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				hsic-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			pcie {
+				status = "disabled";
+
+				pcie-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-3 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-4 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			sata {
+				status = "disabled";
+
+				sata-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+		};
+
+		ports {
+			utmi-0 {
+				status = "disabled";
+			};
+
+			utmi-1 {
+				status = "disabled";
+			};
+
+			utmi-2 {
+				status = "disabled";
+			};
+
+			ulpi-0 {
+				status = "disabled";
+			};
+
+			hsic-0 {
+				status = "disabled";
+			};
+
+			hsic-1 {
+				status = "disabled";
+			};
+
+			usb3-0 {
+				status = "disabled";
+			};
+
+			usb3-1 {
+				status = "disabled";
+			};
+		};
+	};
+
+Board file:
+
+	padctl@0,7009f000 {
+		status = "okay";
+
+		pads {
+			utmi {
+				status = "okay";
+
+				utmi-0 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				utmi-1 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				utmi-2 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+			};
+
+			pcie {
+				status = "okay";
+
+				pcie-0 {
+					nvidia,function = "usb3-ss";
+					status = "okay";
+				};
+
+				pcie-2 {
+					nvidia,function = "pcie";
+					status = "okay";
+				};
+
+				pcie-4 {
+					nvidia,function = "pcie";
+					status = "okay";
+				};
+			};
+
+			sata {
+				status = "okay";
+
+				sata-0 {
+					nvidia,function = "sata";
+					status = "okay";
+				};
+			};
+		};
+
+		ports {
+			/* Micro A/B */
+			utmi-0 {
+				status = "okay";
+				mode = "otg";
+			};
+
+			/* Mini PCIe */
+			utmi-1 {
+				status = "okay";
+				mode = "host";
+			};
+
+			/* USB3 */
+			utmi-2 {
+				status = "okay";
+				mode = "host";
+
+				vbus-supply = <&vdd_usb3_vbus>;
+			};
+
+			usb3-0 {
+				nvidia,port = <2>;
+				status = "okay";
+			};
+		};
+	};
-- 
2.5.0

[PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Extend the binding to cover the set of feature found in Tegra210.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 330 +++++++++++++++++++++
 1 file changed, 330 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
index 026e924ae54a..e7d7400bc981 100644
--- a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
@@ -22,6 +22,7 @@ Required properties:
 --------------------
 - compatible: Must be:
   - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132
+  - "nvidia,tegra210-xusb-padctl": for Tegra210
 - reg: Physical base address and length of the controller's registers.
 - resets: Must contain an entry for each entry in reset-names.
 - reset-names: Must include the following entries:
@@ -45,6 +46,44 @@ the pad and any of its lanes, this property must be set to "okay".
 For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
 and sata. No extra resources are required for operation of these pads.
 
+For Tegra210, the following pads exist: utmi, hsic, pcie and sata. Below is
+a description of the properties of each pad.
+
+UTMI pad:
+---------
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+  - "trk": phandle and specifier referring to the USB2 tracking clock
+
+HSIC pad:
+---------
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+  - "trk": phandle and specifier referring to the HSIC tracking clock
+
+PCIe pad:
+---------
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+  - "pll": phandle and specifier referring to the PLLE
+- resets: Must contain an entry for each entry in reset-names.
+- reset-names: Must contain the following entries:
+  - "phy": reset for the PCIe UPHY block
+
+SATA pad:
+---------
+
+Required properties:
+- resets: Must contain an entry for each entry in reset-names.
+- reset-names: Must contain the following entries:
+  - "phy": reset for the SATA UPHY block
+
 
 PHY nodes:
 ==========
@@ -74,6 +113,17 @@ For Tegra124 and Tegra132, the list of valid PHY nodes is given below:
 - sata: sata-0
   - functions: "usb3-ss", "sata"
 
+For Tegra210, the list of valid PHY nodes is given below:
+- utmi: utmi-0, utmi-1, utmi-2, utmi-3
+  - functions: "snps", "xusb", "uart"
+- hsic: hsic-0, hsic-1
+  - functions: "snps", "xusb"
+- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
+  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
+- sata: sata-0
+  - functions: "usb3-ss", "sata"
+
+
 Port nodes:
 ===========
 
@@ -129,6 +179,7 @@ Required properties:
   this super-speed USB port to. The range of valid port numbers varies with
   the SoC generation:
   - 0-2: for Tegra124 and Tegra132
+  - 0-3: for Tegra210
 
 Optional properties:
 - nvidia,internal: A boolean property whose presence determines that a port
@@ -142,6 +193,11 @@ ports:
 - 2x HSIC: hsic-0, hsic-1
 - 2x super-speed USB: usb3-0, usb3-1
 
+For Tegra210, the XUSB pad controller exposes the following ports:
+- 4x UTMI: utmi-0, utmi-1, utmi-2, utmi-3
+- 2x HSIC: hsic-0, hsic-1
+- 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3
+
 
 Examples:
 =========
@@ -357,3 +413,277 @@ Board file:
 			};
 		};
 	};
+
+Tegra210:
+---------
+
+SoC include:
+
+	padctl@0,7009f000 {
+		compatible = "nvidia,tegra210-xusb-padctl";
+		reg = <0x0 0x7009f000 0x0 0x1000>;
+		resets = <&tegra_car 142>;
+		reset-names = "padctl";
+		mboxes = <&xusb_mbox>;
+		mbox-names = "xusb";
+
+		status = "disabled";
+
+		pads {
+			utmi {
+				clocks = <&tegra_car TEGRA210_CLK_USB2_TRK>;
+				clock-names = "trk";
+				status = "disabled";
+
+				utmi-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				utmi-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				utmi-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				utmi-3 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			hsic {
+				clocks = <&tegra_car TEGRA210_CLK_HSIC_TRK>;
+				clock-names = "trk";
+				status = "disabled";
+
+				hsic-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				hsic-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			pcie {
+				clocks = <&tegra_car TEGRA210_CLK_PLL_E>;
+				clock-names = "pll";
+				resets = <&tegra_car 205>;
+				reset-names = "phy";
+				status = "disabled";
+
+				pcie-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-1 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-2 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-3 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-4 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-5 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+
+				pcie-6 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+
+			sata {
+				clocks = <&tegra_car TEGRA210_CLK_PLL_E>;
+				clock-names = "pll";
+				resets = <&tegra_car 204>;
+				reset-names = "phy";
+				status = "disabled";
+
+				sata-0 {
+					status = "disabled";
+					#phy-cells = <0>;
+				};
+			};
+		};
+
+		ports {
+			utmi-0 {
+				status = "disabled";
+			};
+
+			utmi-1 {
+				status = "disabled";
+			};
+
+			utmi-2 {
+				status = "disabled";
+			};
+
+			utmi-3 {
+				status = "disabled";
+			};
+
+			hsic-0 {
+				status = "disabled";
+			};
+
+			hsic-1 {
+				status = "disabled";
+			};
+
+			usb3-0 {
+				status = "disabled";
+			};
+
+			usb3-1 {
+				status = "disabled";
+			};
+
+			usb3-2 {
+				status = "disabled";
+			};
+
+			usb3-3 {
+				status = "disabled";
+			};
+		};
+	};
+
+Board file:
+
+	padctl@0,7009f000 {
+		status = "okay";
+
+		pads {
+			utmi {
+				status = "okay";
+
+				utmi-0 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				utmi-1 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				utmi-2 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+
+				utmi-3 {
+					nvidia,function = "xusb";
+					status = "okay";
+				};
+			};
+
+			pcie {
+				status = "okay";
+
+				pcie-0 {
+					nvidia,function = "pcie-x1";
+					status = "okay";
+				};
+
+				pcie-1 {
+					nvidia,function = "pcie-x4";
+					status = "okay";
+				};
+
+				pcie-2 {
+					nvidia,function = "pcie-x4";
+					status = "okay";
+				};
+
+				pcie-3 {
+					nvidia,function = "pcie-x4";
+					status = "okay";
+				};
+
+				pcie-4 {
+					nvidia,function = "pcie-x4";
+					status = "okay";
+				};
+
+				pcie-5 {
+					nvidia,function = "usb3-ss";
+					status = "okay";
+				};
+
+				pcie-6 {
+					nvidia,function = "usb3-ss";
+					status = "okay";
+				};
+			};
+
+			sata {
+				status = "okay";
+
+				sata-0 {
+					nvidia,function = "sata";
+					status = "okay";
+				};
+			};
+		};
+
+		ports {
+			utmi-0 {
+				status = "okay";
+				mode = "otg";
+			};
+
+			utmi-1 {
+				status = "okay";
+				vbus-supply = <&vdd_5v0_rtl>;
+				mode = "host";
+			};
+
+			utmi-2 {
+				status = "okay";
+				vbus-supply = <&vdd_usb_vbus>;
+				mode = "host";
+			};
+
+			utmi-3 {
+				status = "okay";
+				mode = "host";
+			};
+
+			usb3-0 {
+				status = "okay";
+				nvidia,lanes = "pcie-6";
+				nvidia,port = <1>;
+			};
+
+			usb3-1 {
+				status = "okay";
+				nvidia,lanes = "pcie-5";
+				nvidia,port = <2>;
+			};
+		};
+	};
-- 
2.5.0

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Stephen Warren]

On 11/04/2015 10:11 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.

>   .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 359 +++++++++++++++++++++

For Tegra bindings, we usually use the full compatible value as the 
filename, so I'd expect the chip number in the filename too.

I'd expect to see a patch in this series that edits the existing 
Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt and 
mentions that the binding is deprecated.

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

> +Device tree binding for NVIDIA Tegra XUSB pad controller
> +========================================================
> +
> +The Tegra XUSB pad controller manages a set of pads, each of which controls
> +one or more lanes.

The term "pad" usually refers to a single pad/pin/ball/signal on the 
chip. As such, saying "pad" here for something that's more than one pin 
is a bit confusing, even if that is what our HW documentation calls it.

> Each lane can in turn be assigned to one out of a set of
> +different outputs.

That doesn't sound correct. That phrasing implies that the mux is 
between the HW-blocks-known-as-pads and the "outputs", whereas the mux 
is actually between the IO controllers and the HW-blocks-known-as-pads

 > A pad contains logic common for all its lanes. Each lane
> +can additionally be separately configured and powered up.

I'd suggest rephrasing that all as:

The Tegra XUSB pad controller manages a set of IO lanes (differential 
signals) which connect directly to pins/pads on the SoC package. Each 
lane is controlled by a HW block referred to as a "pad" in the Tegra HW 
documentation. Each such "pad" may control either one or multiple lanes, 
and contains any logic common to all its lanes. Each lane can be 
separately configured and powered up.

> +Some of the lanes are high-speed lanes, which can be used for PCIe, SATA or
> +super-speed USB. Other lanes are for various types of low-speed, full-speed
> +or high-speed USB (such as UTMI, ULPI and HSIC).

Perhaps add the following after that?

The XUSB pad controller contains a software-configurable mux that sits 
between the IO controller ports (e.g. PCIe) and the lanes.

> +Required properties:
> +--------------------
> +- compatible: Must be:
> +  - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132

For Tegra132, we need both a "tegra124" and a "tegra132 value". I would 
suggest listing the valid complete property values for each SoC for 
simplicity and preciseness, as I did in the XUSB controller binding 
proposal I link to later:

  - compatible: Valid options are:
    - Tegra124: "nvidia,tegra124-xusb-padctl".
    - Tegra132: "nvidia,tegra132-xusb-padctl", \
                                  "nvidia,tegra124-xusb-padctl".

This also makes it very easy to add entries for future SoCs without 
editing the diff touching any existing lines of text.

> +- reg: Physical base address and length of the controller's registers.
> +- resets: Must contain an entry for each entry in reset-names.
> +- reset-names: Must include the following entries:
> +  - "padctl"

Are there no clocks or power domains that affect XUSB padctl? I suppose 
we can start off without any, and add them later if we need.

> +- mboxes: Must contain an entry for each entry in mbox-names.
> +- mbox-names: Must include the following entries:
> +  - "xusb"

I thought we'd decided not to use any mbox binding or drivers in XUSB 
now? See for example my proposed XUSB controller binding at:

http://www.spinics.net/lists/linux-tegra/msg23922.html
[PATCH V9] dt: add NVIDIA Tegra XUSB controller binding

The idea is that the mailbox should be entirely internal to the XUSB 
controller driver, and if the receipt of a mailbox message requires any 
change in the XUSB pad controller programming, the XUSB controller 
driver should simply call the XUSB pad controller driver to perform that 
operation.

> +Pad nodes:
> +==========

> +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
> +and sata. No extra resources are required for operation of these pads.

Judging by the diagram in the TRM (e.g. figure 41 in the Tegra124 TRM, 
figure 36 in the Tegra210 TRM), there is not a single "utmi" pad, but 
rather a separate pad per USB2 lane. However, the other types of pads 
are indeed multi-lane. The TRM also refers to "USB2" pads rather than 
"UTMI" pads. I don't see a ULPI pad in the diagram either. Assuming the 
diagram in the TRM is consistent with the register layout, I think we 
should have the following set of pad nodes:

utmi-0
utmi-1
utmi-2
hsic
pcie
sata

> +Required properties:
> +--------------------

> +For Tegra124 and Tegra132, the list of valid PHY nodes is given below:
> +- utmi: utmi-0, utmi-1, utmi-2
> +  - functions: "snps", "xusb", "uart"

I think that entry needs rework based on my comment above re: the set of 
utmi PHYs/pads that exist.

> +Port nodes:
> +===========
> +
> +A required child node named "ports" contains a list of all the ports exposed
> +by the XUSB pad controller. Per-port configuration is only required for USB.

I'm not sure this section clearly spells out there must be a node named 
"ports" inside the main node, and a node per port within "ports". The 
structure/text under "Pad nodes" seemed better at that.

Perhaps add the text "Each port represents the connection between one 
port on an IO controller, such as a PCIe root port, and the XUSB pad 
controller" either here, or mention something like this in the 
introduction to this binding? Otherwise, someone the distinction between 
lanes, ports, ... might not be clear to someone not familiar with Tegra. 
Perhaps an ASCII art diagram might make sense?

> +UTMI ports:
> +-----------

> +Optional properties:

> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.

This is the only port type that specifies vbus-supply as a valid 
property. There could well be control over VBUS even for ULPI. Shouldn't 
we add this property there too?

> +Super-speed USB ports:
> +----------------------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- nvidia,port: A single cell that specifies the physical port number to map
> +  this super-speed USB port to. The range of valid port numbers varies with
> +  the SoC generation:
> +  - 0-2: for Tegra124 and Tegra132

Wouldn't it be better to name the node after the physical port number, 
just like we name the PHY/lane nodes after the PHY/lane identity? We 
don't seem to have any such mapping information in the UTMI port nodes; 
how do we correlate the USB2 and USB3 ports?

I wonder if we shouldn't represent USB (physical) ports at the lane-side 
of the XUSB pad controller rather than the IO controller side. That way, 
we could pack both USB2- and USB3-related information into a single 
node. For example, vbus-supply really applies equally to the companion 
USB2 and USB3 ports, whereas this binding represents those two parts of 
the overall USB port separately, with the vbus-supply property appearing 
in only one of the two places. I guess this boils down to what a "port" 
actually is; the IO controller <-> XUSB pad controller interface, or the 
XUSB pad controller <-> SoC pins interface.

> +For Tegra124 and Tegra132, the XUSB pad controller exposes the following
> +ports:
> +- 3x UTMI: utmi-0, utmi-1, utmi-2
> +- 1x ULPI: ulpi-0
> +- 2x HSIC: hsic-0, hsic-1
> +- 2x super-speed USB: usb3-0, usb3-1

I suspect that chunk would be better placed directly inside the "Port 
nodes:" section, since it describes valid values for the nodes that are 
subsequently described.

Do we need port nodes for PCIe or SATA at all? If not, should we 
s/ports/usb-ports/ in the container node name? I suppose it doesn't 
matter, but it feels slightly odd to only represent some of the ports.

> +Examples:
> +=========
> +
> +Tegra124 and Tegra132:
> +----------------------

The example isn't valid for Tegra132 since the compatible values don't 
include any Tegra132 entry.

BTW, I've suggested a lot of phrasing changes. Once we've settled the 
other questions, just let me know if you want me to propose an updated 
version of the patch that contains all those phrasing changes so you 
don't have to do them all yourself.

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 11/04/2015 10:11 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Extend the binding to cover the set of feature found in Tegra210.

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

> +PCIe pad:
> +---------
> +
> +Required properties:
> +- clocks: Must contain an entry for each entry in clock-names.
> +- clock-names: Must contain the following entries:
> +  - "pll": phandle and specifier referring to the PLLE
> +- resets: Must contain an entry for each entry in reset-names.
> +- reset-names: Must contain the following entries:
> +  - "phy": reset for the PCIe UPHY block

I don't recall any clocks or resets properties in the pads for Tegra124. 
Do we really not need any?

> +SATA pad:
> +---------
> +
> +Required properties:
> +- resets: Must contain an entry for each entry in reset-names.
> +- reset-names: Must contain the following entries:
> +  - "phy": reset for the SATA UPHY block
> +
>
>   PHY nodes:

Nit: 2 blank lines there.

> +For Tegra210, the list of valid PHY nodes is given below:
> +- utmi: utmi-0, utmi-1, utmi-2, utmi-3
> +  - functions: "snps", "xusb", "uart"
> +- hsic: hsic-0, hsic-1
> +  - functions: "snps", "xusb"
> +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
> +  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
> +- sata: sata-0
> +  - functions: "usb3-ss", "sata"

usb2-bias also needs to be present.

> +
> +
>   Port nodes:
>   ===========

Nit: 2 blank lines there.

> +For Tegra210, the XUSB pad controller exposes the following ports:
> +- 4x UTMI: utmi-0, utmi-1, utmi-2, utmi-3
> +- 2x HSIC: hsic-0, hsic-1
> +- 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3
> +
>
>   Examples:
>   =========

Nit: 2 blank lines there.

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Jon Hunter]

On 04/11/15 17:11, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> set of lanes that are used for PCIe, SATA and USB.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 359 +++++++++++++++++++++
>  1 file changed, 359 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
> new file mode 100644
> index 000000000000..026e924ae54a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
> @@ -0,0 +1,359 @@
> +Device tree binding for NVIDIA Tegra XUSB pad controller
> +========================================================
> +
> +The Tegra XUSB pad controller manages a set of pads, each of which controls
> +one or more lanes. Each lane can in turn be assigned to one out of a set of
> +different outputs. A pad contains logic common for all its lanes. Each lane
> +can additionally be separately configured and powered up.
> +
> +Some of the lanes are high-speed lanes, which can be used for PCIe, SATA or
> +super-speed USB. Other lanes are for various types of low-speed, full-speed
> +or high-speed USB (such as UTMI, ULPI and HSIC).
> +
> +In addition to per-lane configuration, USB 3.0 ports may require additional
> +settings on a per-board basis.
> +
> +Pads will be represented as children of the top-level XUSB pad controller
> +device tree node. Each lane exposed by the pad will be represented by its
> +own subnode and can be referenced by users of the lane using the standard
> +PHY bindings, as described by the phy-bindings.txt file in this directory.
> +
> +Required properties:
> +--------------------
> +- compatible: Must be:
> +  - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132
> +- reg: Physical base address and length of the controller's registers.
> +- resets: Must contain an entry for each entry in reset-names.
> +- reset-names: Must include the following entries:
> +  - "padctl"
> +- mboxes: Must contain an entry for each entry in mbox-names.
> +- mbox-names: Must include the following entries:
> +  - "xusb"
> +
> +
> +Pad nodes:
> +==========
> +
> +A required child node named "pads" contains a list of subnodes, one for each
> +of the pads exposed by the XUSB pad controller. Each pad may need additional
> +resources that can be referenced in its pad node.
> +
> +The "status" property is used to enable or disable the use of a pad. If set
> +to "disabled", the pad will not be used on the given board. In order to use
> +the pad and any of its lanes, this property must be set to "okay".
> +
> +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
> +and sata. No extra resources are required for operation of these pads.
> +
> +
> +PHY nodes:
> +==========
> +
> +Each pad node has one or more children, each representing one of the lanes
> +controlled by the pad.
> +
> +Required properties:
> +--------------------
> +- status: Defines the operation status of the PHY. Valid values are:
> +  - "disabled": the PHY is disabled
> +  - "okay": the PHY is enabled
> +- #phy-cells: Should be 0. Since each lane represents a single PHY, there is
> +  no need for an additional specifier.
> +- nvidia,function: The output function of the PHY. See below for a list of
> +  valid functions per SoC generation.
> +
> +For Tegra124 and Tegra132, the list of valid PHY nodes is given below:
> +- utmi: utmi-0, utmi-1, utmi-2
> +  - functions: "snps", "xusb", "uart"
> +- ulpi: ulpi-0
> +  - functions: "snps", "xusb"
> +- hsic: hsic-0, hsic-1
> +  - functions: "snps", "xusb"
> +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4
> +  - functions: "pcie", "usb3-ss"

Per the patch I recently sent out [0], although from the register
description the above is correct, the reality is that the usb3-ss
function is not available on all pcie lanes. We really need to make this
clear somehow.

> +- sata: sata-0
> +  - functions: "usb3-ss", "sata"
> +
> +Port nodes:
> +===========
> +
> +A required child node named "ports" contains a list of all the ports exposed
> +by the XUSB pad controller. Per-port configuration is only required for USB.

What about pcie? For example, how/where can we map the pcie1-x2 to the
pcie lanes? Again per patch [0] there are only a finite number of
options for mapping pcie ports to lanes and so it would be good to
represent this as well.

> +UTMI ports:
> +-----------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- mode: A string that determines the mode in which to run the port. Valid
> +  values are:
> +  - "host": for USB host mode
> +  - "device": for USB device mode
> +  - "otg": for USB OTG mode
> +
> +Optional properties:
> +- nvidia,internal: A boolean property whose presence determines that a port
> +  is internal. In the absence of this property the port is considered to be
> +  external.
> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.
> +
> +ULPI ports:
> +-----------
> +
> +Optional properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- nvidia,internal: A boolean property whose presence determines that a port
> +  is internal. In the absence of this property the port is considered to be
> +  external.
> +
> +HSIC ports:
> +-----------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +
> +Super-speed USB ports:
> +----------------------
> +
> +Required properties:
> +- status: Defines the operation status of the port. Valid values are:
> +  - "disabled": the port is disabled
> +  - "okay": the port is enabled
> +- nvidia,port: A single cell that specifies the physical port number to map
> +  this super-speed USB port to. The range of valid port numbers varies with
> +  the SoC generation:
> +  - 0-2: for Tegra124 and Tegra132

I am a bit confused by what nvidia,port property is used for. Is this to
program XUSB_PADCTL_SS_PORT_MAP_0? If so then I think that this should
be an optional property because if you want to use the usb3 ports for
usb3, then we should not need to specify this here.

Also the XHCI has 3 usb2 ports and 2 usb3 ports and so when you have
port numbers 0-2 I am not sure what you are referring too.

Cheers
Jon

[0] https://lkml.org/lkml/2015/10/16/193

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Andrew Bresticker]

On Thu, Nov 5, 2015 at 1:55 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> +UTMI ports:
>> +-----------
>> +
>> +Required properties:
>> +- status: Defines the operation status of the port. Valid values are:
>> +  - "disabled": the port is disabled
>> +  - "okay": the port is enabled
>> +- mode: A string that determines the mode in which to run the port. Valid
>> +  values are:
>> +  - "host": for USB host mode
>> +  - "device": for USB device mode
>> +  - "otg": for USB OTG mode
>> +
>> +Optional properties:
>> +- nvidia,internal: A boolean property whose presence determines that a port
>> +  is internal. In the absence of this property the port is considered to be
>> +  external.
>> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.
>> +
>> +ULPI ports:
>> +-----------
>> +
>> +Optional properties:
>> +- status: Defines the operation status of the port. Valid values are:
>> +  - "disabled": the port is disabled
>> +  - "okay": the port is enabled
>> +- nvidia,internal: A boolean property whose presence determines that a port
>> +  is internal. In the absence of this property the port is considered to be
>> +  external.
>> +
>> +HSIC ports:
>> +-----------
>> +
>> +Required properties:
>> +- status: Defines the operation status of the port. Valid values are:
>> +  - "disabled": the port is disabled
>> +  - "okay": the port is enabled
>> +
>> +Super-speed USB ports:
>> +----------------------
>> +
>> +Required properties:
>> +- status: Defines the operation status of the port. Valid values are:
>> +  - "disabled": the port is disabled
>> +  - "okay": the port is enabled
>> +- nvidia,port: A single cell that specifies the physical port number to map
>> +  this super-speed USB port to. The range of valid port numbers varies with
>> +  the SoC generation:
>> +  - 0-2: for Tegra124 and Tegra132
>
> I am a bit confused by what nvidia,port property is used for. Is this to
> program XUSB_PADCTL_SS_PORT_MAP_0? If so then I think that this should
> be an optional property because if you want to use the usb3 ports for
> usb3, then we should not need to specify this here.

"nvidia,port" is used to program XUSB_PADCTL_SS_PORT_MAP and is
definitely required.  It specifies the UTMI port to which the SS port
is mapped to.  For example, Nyan-Big has 3 UTMI ports (0, 1
(internal), and 2) and 2 SS ports (0 and 1).  SS port 0 is mapped to
the same physical port as UTMI port 0 and SS port 1 is mapped to the
same physical port as UTMI port 2.

> Also the XHCI has 3 usb2 ports and 2 usb3 ports and so when you have
> port numbers 0-2 I am not sure what you are referring too.

The port numbers correspond tot he UTMI ports, so 3 UTMI ports =
possible port values of 0 - 2.

Maybe "port" isn't the best name here?

-Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Jon Hunter]

On 05/11/15 18:13, Andrew Bresticker wrote:
> On Thu, Nov 5, 2015 at 1:55 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> +UTMI ports:
>>> +-----------
>>> +
>>> +Required properties:
>>> +- status: Defines the operation status of the port. Valid values are:
>>> +  - "disabled": the port is disabled
>>> +  - "okay": the port is enabled
>>> +- mode: A string that determines the mode in which to run the port. Valid
>>> +  values are:
>>> +  - "host": for USB host mode
>>> +  - "device": for USB device mode
>>> +  - "otg": for USB OTG mode
>>> +
>>> +Optional properties:
>>> +- nvidia,internal: A boolean property whose presence determines that a port
>>> +  is internal. In the absence of this property the port is considered to be
>>> +  external.
>>> +- vbus-supply: phandle to a regulator supplying the VBUS voltage.
>>> +
>>> +ULPI ports:
>>> +-----------
>>> +
>>> +Optional properties:
>>> +- status: Defines the operation status of the port. Valid values are:
>>> +  - "disabled": the port is disabled
>>> +  - "okay": the port is enabled
>>> +- nvidia,internal: A boolean property whose presence determines that a port
>>> +  is internal. In the absence of this property the port is considered to be
>>> +  external.
>>> +
>>> +HSIC ports:
>>> +-----------
>>> +
>>> +Required properties:
>>> +- status: Defines the operation status of the port. Valid values are:
>>> +  - "disabled": the port is disabled
>>> +  - "okay": the port is enabled
>>> +
>>> +Super-speed USB ports:
>>> +----------------------
>>> +
>>> +Required properties:
>>> +- status: Defines the operation status of the port. Valid values are:
>>> +  - "disabled": the port is disabled
>>> +  - "okay": the port is enabled
>>> +- nvidia,port: A single cell that specifies the physical port number to map
>>> +  this super-speed USB port to. The range of valid port numbers varies with
>>> +  the SoC generation:
>>> +  - 0-2: for Tegra124 and Tegra132
>>
>> I am a bit confused by what nvidia,port property is used for. Is this to
>> program XUSB_PADCTL_SS_PORT_MAP_0? If so then I think that this should
>> be an optional property because if you want to use the usb3 ports for
>> usb3, then we should not need to specify this here.
> 
> "nvidia,port" is used to program XUSB_PADCTL_SS_PORT_MAP and is
> definitely required.  It specifies the UTMI port to which the SS port
> is mapped to.  For example, Nyan-Big has 3 UTMI ports (0, 1
> (internal), and 2) and 2 SS ports (0 and 1).  SS port 0 is mapped to
> the same physical port as UTMI port 0 and SS port 1 is mapped to the
> same physical port as UTMI port 2.

By port, you mean usb3 connector/receptacle right?

I had also been thinking about the use-case where you have an embedded
usb3 device (on the same pcb) directly connected to the tegra and so I
was wondering if setting this register would be still necessary.
However, I have checked with the designers and they told me that we do
still need to set this even if this case. So it does appear to be
required after all. Sorry for the noise.

Jon

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11471 bytes --]

On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote:
> On 11/04/2015 10:11 AM, Thierry Reding wrote:
> >From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> >The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
> >set of lanes that are used for PCIe, SATA and USB.
> 
> >  .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 359 +++++++++++++++++++++
> 
> For Tegra bindings, we usually use the full compatible value as the
> filename, so I'd expect the chip number in the filename too.

I specifically didn't do that to avoid confusion. The XUSB pad
controller was introduced on Tegra114, but patches weren't posted until
Tegra124. So nvidia,tegra114-xusb-padctl.txt would be the proper name
for this binding if we were following the conventions, but then we have
never specified that binding (though I think it would look mostly the
same as for Tegra124).

I can of course still go for nvidia,tegra114-xusb-padctl.txt, the
content would explicitly list valid compatible strings. It's just that
none of them would match the filename.

> I'd expect to see a patch in this series that edits the existing
> Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> and mentions that the binding is deprecated.

How about this:

--- >8 ---
diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
index 30676ded85bb..77dfba05ccfd 100644
--- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
@@ -1,6 +1,11 @@
 Device tree binding for NVIDIA Tegra XUSB pad controller
 ========================================================
 
+NOTE: It turns out that this binding isn't an accurate description of the XUSB
+pad controller. While the description is good enough for the functional subset
+required for PCIe and SATA, it lacks the flexibility to represent the features
+needed for USB. For the new binding, see ../phy/nvidia,tegra-xusb-padctl.txt.
+
 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.
--- >8 ---

> >diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
> >+- reg: Physical base address and length of the controller's registers.
> >+- resets: Must contain an entry for each entry in reset-names.
> >+- reset-names: Must include the following entries:
> >+  - "padctl"
> 
> Are there no clocks or power domains that affect XUSB padctl? I suppose we
> can start off without any, and add them later if we need.

I don't think there are any. The TRM specifies that it's in the ungated
Vaux_soc power partition, and doesn't mention any clocks.

> >+- mboxes: Must contain an entry for each entry in mbox-names.
> >+- mbox-names: Must include the following entries:
> >+  - "xusb"
> 
> I thought we'd decided not to use any mbox binding or drivers in XUSB now?
> See for example my proposed XUSB controller binding at:
> 
> http://www.spinics.net/lists/linux-tegra/msg23922.html
> [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding
> 
> The idea is that the mailbox should be entirely internal to the XUSB
> controller driver, and if the receipt of a mailbox message requires any
> change in the XUSB pad controller programming, the XUSB controller driver
> should simply call the XUSB pad controller driver to perform that operation.

Okay, I think that should work, but it'll require a rather large rewrite
of... well, everything. I think Martyn was going to look into that, so I
guess I'll just drop this for now.

Instead I think we'll need a phandle to the XUSB pad controller, so that
we can resolve it to the proper context. Something like this perhaps?

	- nvidia,padctl: phandle to the XUSB pad controller that is used
	  to configure the USB pads used by the XHCI controller.

That should of course go into the XHCI controller binding. The nice
thing about this would be that we get rid of the circular dependency
XHCI -> padctl -> mailbox -> XHCI.

> >+Pad nodes:
> >+==========
> 
> >+For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
> >+and sata. No extra resources are required for operation of these pads.
> 
> Judging by the diagram in the TRM (e.g. figure 41 in the Tegra124 TRM,
> figure 36 in the Tegra210 TRM), there is not a single "utmi" pad, but rather
> a separate pad per USB2 lane. However, the other types of pads are indeed
> multi-lane. The TRM also refers to "USB2" pads rather than "UTMI" pads. I
> don't see a ULPI pad in the diagram either. Assuming the diagram in the TRM
> is consistent with the register layout, I think we should have the following
> set of pad nodes:
> 
> utmi-0
> utmi-1
> utmi-2
> hsic
> pcie
> sata

I think the diagram is incomplete. If you look at the register
definitions there is clearly a ULPI lane that can be muxed to SNPS or
XUSB. Further there are two registers that allow configuration of the
ULPI link.

Similarly I think the UTMI pads are innacurately represented. First I
think naming them UTMI is more accurate, because USB2 could be either
UTMI or HSIC. Also there is common logic shared between all UTMI pads
which is why I think it makes sense to collect them in a separate top
level utmi pad node.

With Tegra210 this will become more important because having the top-
level utmi pad node provides for a good location to put shared resources
in the device tree.

> >+UTMI ports:
> >+-----------
> 
> >+Optional properties:
> 
> >+- vbus-supply: phandle to a regulator supplying the VBUS voltage.
> 
> This is the only port type that specifies vbus-supply as a valid property.
> There could well be control over VBUS even for ULPI. Shouldn't we add this
> property there too?

Yeah, I guess a vbus-supply property makes sense for all types of ports,
except maybe the USB3 ports because they share the VBUS voltage with the
USB2 port that they are paired up with.

> 
> >+Super-speed USB ports:
> >+----------------------
> >+
> >+Required properties:
> >+- status: Defines the operation status of the port. Valid values are:
> >+  - "disabled": the port is disabled
> >+  - "okay": the port is enabled
> >+- nvidia,port: A single cell that specifies the physical port number to map
> >+  this super-speed USB port to. The range of valid port numbers varies with
> >+  the SoC generation:
> >+  - 0-2: for Tegra124 and Tegra132
> 
> Wouldn't it be better to name the node after the physical port number, just
> like we name the PHY/lane nodes after the PHY/lane identity? We don't seem
> to have any such mapping information in the UTMI port nodes; how do we
> correlate the USB2 and USB3 ports?

The mapping of UTMI and HSIC ports seems to be static. I've filed an
internal bug to hopefully get this properly documented. There is an
internal document that lists these connections in a block diagram, they
are as follows. There are three physical ports, USB#1, USB#2 and USB#3.
They can have the following sources (using the naming in this binding):

  - USB#1: utmi-0
  - USB#2: utmi-1, hsic-0, ulpi-0
  - USB#3: utmi-2, hsic-1

That's for Tegra124. For Tegra114, which we don't support (yet), the
mapping is as follows:

  - USB#1: utmi-0
  - USB#2: hsic-0, ulpi-0
  - USB#3: utmi-1, hsic-1

Unfortunately the Tegra210 version of that document seems to have the
same block diagram as Tegra124, so they are probably wrong (the number
of pads has changed, so at the very list the diagram is incomplete).

The correlation of USB2 and USB3 ports is specifically done using this
nvidia,port property. The index in the USB3 port name represents the
hardware index of the port, which is used to index registers etc. Now I
suppose we could turn this around, and have the ports named after the
physical port, but then we'd still need an nvidia,port property to
relate that "physical port" to the pad controller hardware index. That
seems the wrong way around to me, since we're describing the XUSB pad
controller part of the port here.

> I wonder if we shouldn't represent USB (physical) ports at the lane-side of
> the XUSB pad controller rather than the IO controller side. That way, we
> could pack both USB2- and USB3-related information into a single node. For
> example, vbus-supply really applies equally to the companion USB2 and USB3
> ports, whereas this binding represents those two parts of the overall USB
> port separately, with the vbus-supply property appearing in only one of the
> two places.

My understanding is that the USB3 ports work on top of USB2 ports. USB3
ports don't work standalone, so they must always be associated with one
of the USB2 ports. If they aren't there is a hardware mechanism in place
that disables the USB3 port.

There are further some registers that control the electrical signalling
of each of these ports, so I think it makes sense to represent this from
the perspective of the pad controller rather than the I/O controller.

> I guess this boils down to what a "port" actually is; the IO
> controller <-> XUSB pad controller interface, or the XUSB pad controller <->
> SoC pins interface.
> 
> >+For Tegra124 and Tegra132, the XUSB pad controller exposes the following
> >+ports:
> >+- 3x UTMI: utmi-0, utmi-1, utmi-2
> >+- 1x ULPI: ulpi-0
> >+- 2x HSIC: hsic-0, hsic-1
> >+- 2x super-speed USB: usb3-0, usb3-1
> 
> I suspect that chunk would be better placed directly inside the "Port
> nodes:" section, since it describes valid values for the nodes that are
> subsequently described.
> 
> Do we need port nodes for PCIe or SATA at all? If not, should we
> s/ports/usb-ports/ in the container node name? I suppose it doesn't matter,
> but it feels slightly odd to only represent some of the ports.

I've seen references to "PCIe ports" and "SATA port" in documentation,
but I don't see any immediate need to include them. An early version of
the binding (and implementation) had them, but they ended up unused and
empty. I don't see any registers that would substantiate that we need
them. Then again, keeping the top-level node named "ports" will give us
the flexibility to add them if necessary with the benefit that the
parent name is still accurate, whereas naming it "usb-ports" would make
things somewhat inconsistent if we ever need PCIe and SATA ports.

> >+Examples:
> >+=========
> >+
> >+Tegra124 and Tegra132:
> >+----------------------
> 
> The example isn't valid for Tegra132 since the compatible values don't
> include any Tegra132 entry.
> 
> BTW, I've suggested a lot of phrasing changes. Once we've settled the other
> questions, just let me know if you want me to propose an updated version of
> the patch that contains all those phrasing changes so you don't have to do
> them all yourself.

Feel free to integrate whatever phrasing changes you think improve the
binding. It might be best for you to do that yourself to avoid churn due
to me misunderstanding what you were suggesting.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3958 bytes --]

On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote:
> On 11/04/2015 10:11 AM, Thierry Reding wrote:
> >From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> >Extend the binding to cover the set of feature found in Tegra210.
> 
> >diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
> 
> >+PCIe pad:
> >+---------
> >+
> >+Required properties:
> >+- clocks: Must contain an entry for each entry in clock-names.
> >+- clock-names: Must contain the following entries:
> >+  - "pll": phandle and specifier referring to the PLLE
> >+- resets: Must contain an entry for each entry in reset-names.
> >+- reset-names: Must contain the following entries:
> >+  - "phy": reset for the PCIe UPHY block
> 
> I don't recall any clocks or resets properties in the pads for Tegra124. Do
> we really not need any?

Tegra124 had two instances of what used to be called IOPHY, one for PCIe
and one for SATA. On Tegra210 these have been replaced by two instances
of what's called UPHY. The resets listed in the PCIe and SATA pad nodes
are wired to those UPHY instances, hence why they are new on Tegra210.

For Tegra124 no resets exist for the IOPHY instances.

> >+SATA pad:
> >+---------
> >+
> >+Required properties:
> >+- resets: Must contain an entry for each entry in reset-names.
> >+- reset-names: Must contain the following entries:
> >+  - "phy": reset for the SATA UPHY block
> >+
> >
> >  PHY nodes:
> 
> Nit: 2 blank lines there.

Those were intentional for additional spacing between sections.

> >+For Tegra210, the list of valid PHY nodes is given below:
> >+- utmi: utmi-0, utmi-1, utmi-2, utmi-3
> >+  - functions: "snps", "xusb", "uart"
> >+- hsic: hsic-0, hsic-1
> >+  - functions: "snps", "xusb"
> >+- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
> >+  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
> >+- sata: sata-0
> >+  - functions: "usb3-ss", "sata"
> 
> usb2-bias also needs to be present.

I'm not sure about this. All of the driver code that I've looked deals
with the usb2-bias pad internally. As far as I can tell, this pad needs
to be configured to whatever any of the other pads is configured for. I
think that means if any of the UTMI pads is configured for XUSB then the
usb2-bias pad must also be configured for XUSB. Which would also imply
that if one of the UTMI pads is configured for XUSB, all of them must be
configured for XUSB.

It's also not a pad in the sense that the others are pads. It doesn't
directly connect anywhere. It's also shared by all the UTMI pads. That
said, there are two registers that allow some of the parameters of the
pad to be set, so perhaps adding the node exclusively for
configurability might make sense.

It wouldn't really be a PHY node, though, so wouldn't fit into the above
group. Perhaps something like the following could be added:

  There is an additional pad that is used to support the bias voltages
  to the USB2/UTMI pads. This is not a PHY that can be consumed by any
  I/O controller, but the device tree node can be used to specify
  parameters needed for the programming of the pad.

I think the function shouldn't necessarily be exposed as a parameter,
because all that would do is add the possibility for a conflicting set
of mux options with the USB2/UTMI pads.

> >+
> >+
> >  Port nodes:
> >  ===========
> 
> Nit: 2 blank lines there.
> 
> >+For Tegra210, the XUSB pad controller exposes the following ports:
> >+- 4x UTMI: utmi-0, utmi-1, utmi-2, utmi-3
> >+- 2x HSIC: hsic-0, hsic-1
> >+- 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3
> >+
> >
> >  Examples:
> >  =========
> 
> Nit: 2 blank lines there.

Again, those were intentional for readability. I can remove them if you
don't think they fulfil that purpose.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Andrew Bresticker]

On Fri, Nov 13, 2015 at 8:32 AM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote:
>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>> >From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> >
>> >Extend the binding to cover the set of feature found in Tegra210.
>>
>> >diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
>>
>> >+For Tegra210, the list of valid PHY nodes is given below:
>> >+- utmi: utmi-0, utmi-1, utmi-2, utmi-3
>> >+  - functions: "snps", "xusb", "uart"
>> >+- hsic: hsic-0, hsic-1
>> >+  - functions: "snps", "xusb"
>> >+- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
>> >+  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
>> >+- sata: sata-0
>> >+  - functions: "usb3-ss", "sata"
>>
>> usb2-bias also needs to be present.
>
> I'm not sure about this. All of the driver code that I've looked deals
> with the usb2-bias pad internally. As far as I can tell, this pad needs
> to be configured to whatever any of the other pads is configured for. I
> think that means if any of the UTMI pads is configured for XUSB then the
> usb2-bias pad must also be configured for XUSB. Which would also imply
> that if one of the UTMI pads is configured for XUSB, all of them must be
> configured for XUSB.

I was told by hardware engineers at NVIDIA that (at least on
Tegra124/Tegra132) the usb2-bias pad must be configured in the
XUSB_PADCTL register space if UTMI pad 0 is muxed to XUSB.  If UTMI
pad 0 is muxed to SNPS, then the usb2-bias pad is configured in the
USB register space (base 0x7d000000).  You may want to follow up
internally to confirm this.  If it's true, that could make things here
a bit nastier, especially if we want to support configurations where
some pads are muxed to XUSB while others are muxed to SNPS.

-Andrew

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Martyn Welch]

On 13/11/15 16:11, Thierry Reding wrote:
> On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote:
>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> +- mboxes: Must contain an entry for each entry in mbox-names.
>>> +- mbox-names: Must include the following entries:
>>> +  - "xusb"
>> I thought we'd decided not to use any mbox binding or drivers in XUSB now?
>> See for example my proposed XUSB controller binding at:
>>
>> http://www.spinics.net/lists/linux-tegra/msg23922.html
>> [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding
>>
>> The idea is that the mailbox should be entirely internal to the XUSB
>> controller driver, and if the receipt of a mailbox message requires any
>> change in the XUSB pad controller programming, the XUSB controller driver
>> should simply call the XUSB pad controller driver to perform that operation.
> Okay, I think that should work, but it'll require a rather large rewrite
> of... well, everything. I think Martyn was going to look into that, so I
> guess I'll just drop this for now.
>
Unfortunately I've been shifted onto other things now,

Martyn

Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding

[Stephen Warren]

On 11/13/2015 09:11 AM, Thierry Reding wrote:
> On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote:
>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a
>>> set of lanes that are used for PCIe, SATA and USB.
>>
>>>   .../bindings/phy/nvidia,tegra-xusb-padctl.txt      | 359 +++++++++++++++++++++
>>
>> For Tegra bindings, we usually use the full compatible value as the
>> filename, so I'd expect the chip number in the filename too.
>
> I specifically didn't do that to avoid confusion. The XUSB pad
> controller was introduced on Tegra114, but patches weren't posted until
> Tegra124. So nvidia,tegra114-xusb-padctl.txt would be the proper name
> for this binding if we were following the conventions, but then we have
> never specified that binding (though I think it would look mostly the
> same as for Tegra124).
>
> I can of course still go for nvidia,tegra114-xusb-padctl.txt, the
> content would explicitly list valid compatible strings. It's just that
> none of them would match the filename.

I was asking that the file be named to match the compatible flag, not 
the SoC version that contains the HW module.

The first/oldest compatible value currently documented is Tegra124, so 
I'd expect that to appear in the filename.

Yes, it's theoretically possible for the binding to be enhanced in the 
future to cover Tegra114, and at that time we'd probably want to rename 
the file. However, I believe the likelihood of that happening is zero 
(or even negative, which is admittedly mathematically impossible, but 
I'm being hyperbolic).

>> I'd expect to see a patch in this series that edits the existing
>> Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
>> and mentions that the binding is deprecated.
>
> How about this:
>
> --- >8 ---
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> index 30676ded85bb..77dfba05ccfd 100644
> --- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> @@ -1,6 +1,11 @@
>   Device tree binding for NVIDIA Tegra XUSB pad controller
>   ========================================================
>
> +NOTE: It turns out that this binding isn't an accurate description of the XUSB
> +pad controller. While the description is good enough for the functional subset
> +required for PCIe and SATA, it lacks the flexibility to represent the features
> +needed for USB. For the new binding, see ../phy/nvidia,tegra-xusb-padctl.txt.
> +
>   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.
> --- >8 ---

That sounds good, although I'd suggest explicitly mentioning that the 
binding is deprecated. Perhaps add ", and so this binding is deprecated" 
at the end of the first sentence?

>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
>>> +- reg: Physical base address and length of the controller's registers.
>>> +- resets: Must contain an entry for each entry in reset-names.
>>> +- reset-names: Must include the following entries:
>>> +  - "padctl"
>>
>> Are there no clocks or power domains that affect XUSB padctl? I suppose we
>> can start off without any, and add them later if we need.
>
> I don't think there are any. The TRM specifies that it's in the ungated
> Vaux_soc power partition, and doesn't mention any clocks.

OK. Obviously the module must used some clock, since it contains clocked 
logic. However, equally that clock is obviously on even without the 
driver explicitly managing it since the HW works right now. So, I 
suppose we can defer adding any clock entries to the binding/DT until 
later, if the need arises. It'd still be nice if we could put the 
complete details into the binding from day one, but I understand that 
the documentation isn't exactly forthcoming on these topics.

>>> +- mboxes: Must contain an entry for each entry in mbox-names.
>>> +- mbox-names: Must include the following entries:
>>> +  - "xusb"
>>
>> I thought we'd decided not to use any mbox binding or drivers in XUSB now?
>> See for example my proposed XUSB controller binding at:
>>
>> http://www.spinics.net/lists/linux-tegra/msg23922.html
>> [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding
>>
>> The idea is that the mailbox should be entirely internal to the XUSB
>> controller driver, and if the receipt of a mailbox message requires any
>> change in the XUSB pad controller programming, the XUSB controller driver
>> should simply call the XUSB pad controller driver to perform that operation.
>
> Okay, I think that should work, but it'll require a rather large rewrite
> of... well, everything. I think Martyn was going to look into that, so I
> guess I'll just drop this for now.
>
> Instead I think we'll need a phandle to the XUSB pad controller, so that
> we can resolve it to the proper context. Something like this perhaps?
>
> 	- nvidia,padctl: phandle to the XUSB pad controller that is used
> 	  to configure the USB pads used by the XHCI controller.
>
> That should of course go into the XHCI controller binding. The nice
> thing about this would be that we get rid of the circular dependency
> XHCI -> padctl -> mailbox -> XHCI.

Yes, that sounds good.

>>> +Pad nodes:
>>> +==========
>>
>>> +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
>>> +and sata. No extra resources are required for operation of these pads.
>>
>> Judging by the diagram in the TRM (e.g. figure 41 in the Tegra124 TRM,
>> figure 36 in the Tegra210 TRM), there is not a single "utmi" pad, but rather
>> a separate pad per USB2 lane. However, the other types of pads are indeed
>> multi-lane. The TRM also refers to "USB2" pads rather than "UTMI" pads. I
>> don't see a ULPI pad in the diagram either. Assuming the diagram in the TRM
>> is consistent with the register layout, I think we should have the following
>> set of pad nodes:
>>
>> utmi-0
>> utmi-1
>> utmi-2
>> hsic
>> pcie
>> sata
>
> I think the diagram is incomplete. If you look at the register
> definitions there is clearly a ULPI lane that can be muxed to SNPS or
> XUSB. Further there are two registers that allow configuration of the
> ULPI link.

OK, I see ULPI-related fields/registers in the Tegra124 register specs. 
They've apparently been removed from Tegra210.

> Similarly I think the UTMI pads are innacurately represented. First I
> think naming them UTMI is more accurate, because USB2 could be either
> UTMI or HSIC. Also there is common logic shared between all UTMI pads
> which is why I think it makes sense to collect them in a separate top
> level utmi pad node.
 >
> With Tegra210 this will become more important because having the top-
> level utmi pad node provides for a good location to put shared resources
> in the device tree.

Can you please provide more details re: which shared resources you're 
referring to? Looking at the implementation I have of struct 
tegra_xusb_phy_ops .prepare()/.enable() I'm not seeing any common 
logic/resources. Note that I'm making the subtle (perhaps arbitrary) 
distinction between logic that's global to the entire of XUSB PADCTL 
(but naturally affects everything contained inside it, such as the 
clamping and reset bits) and logic that actually binds N USB2/UTMI lanes 
together into some common sub-block (such as registers 
XUSB_PADCTL_UPHY_PLL_P0_CTL* for PCIe), rather than those lanes being 
bound together only at the top level of XUSB PADCTL.

While I agree that "USB2" isn't good naming, it /is/ the naming used by 
the TRM and the register documentation. I'm not quite convinced that the 
DT binding is the correct place to fix this naming mistake. At the very 
least, can we add a note near the "utmi" names indicating that they're 
referred to as "USB2" in the HW documentation?

...
>>> +Super-speed USB ports:
>>> +----------------------
>>> +
>>> +Required properties:
>>> +- status: Defines the operation status of the port. Valid values are:
>>> +  - "disabled": the port is disabled
>>> +  - "okay": the port is enabled
>>> +- nvidia,port: A single cell that specifies the physical port number to map
>>> +  this super-speed USB port to. The range of valid port numbers varies with
>>> +  the SoC generation:
>>> +  - 0-2: for Tegra124 and Tegra132
>>
>> Wouldn't it be better to name the node after the physical port number, just
>> like we name the PHY/lane nodes after the PHY/lane identity? We don't seem
>> to have any such mapping information in the UTMI port nodes; how do we
>> correlate the USB2 and USB3 ports?
>
> The mapping of UTMI and HSIC ports seems to be static. ...
...
> The correlation of USB2 and USB3 ports is specifically done using this
> nvidia,port property.

OK, perhaps nvidia,usb2-companion-port-id would be a more descriptive 
property name then. I got the impression this property indicated which 
USB3 port this node contained configuration for.

> The index in the USB3 port name represents the
> hardware index of the port, which is used to index registers etc. Now I
> suppose we could turn this around, and have the ports named after the
> physical port, but then we'd still need an nvidia,port property to
> relate that "physical port" to the pad controller hardware index. That
> seems the wrong way around to me, since we're describing the XUSB pad
> controller part of the port here.
>
>> I wonder if we shouldn't represent USB (physical) ports at the lane-side of
>> the XUSB pad controller rather than the IO controller side. That way, we
>> could pack both USB2- and USB3-related information into a single node. For
>> example, vbus-supply really applies equally to the companion USB2 and USB3
>> ports, whereas this binding represents those two parts of the overall USB
>> port separately, with the vbus-supply property appearing in only one of the
>> two places.
>
> My understanding is that the USB3 ports work on top of USB2 ports. USB3
> ports don't work standalone, so they must always be associated with one
> of the USB2 ports. If they aren't there is a hardware mechanism in place
> that disables the USB3 port.

Yes, a USB3 port must be coupled with a "companion" USB2 port.

> There are further some registers that control the electrical signalling
> of each of these ports, so I think it makes sense to represent this from
> the perspective of the pad controller rather than the I/O controller.

But at the signal interface between the XUSB controller and XUSB PADCTL 
module, the two are the same thing.

By "port" are you referring to something other than the signals at the 
periphery of the XUSB PADCTL module (either directed internally for 
connection to the XUSB controller, or externally to the physical balls 
on the chip package).

>> I guess this boils down to what a "port" actually is; the IO
>> controller <-> XUSB pad controller interface, or the XUSB pad controller <->
>> SoC pins interface.
>>
>>> +For Tegra124 and Tegra132, the XUSB pad controller exposes the following
>>> +ports:
>>> +- 3x UTMI: utmi-0, utmi-1, utmi-2
>>> +- 1x ULPI: ulpi-0
>>> +- 2x HSIC: hsic-0, hsic-1
>>> +- 2x super-speed USB: usb3-0, usb3-1
>>
>> I suspect that chunk would be better placed directly inside the "Port
>> nodes:" section, since it describes valid values for the nodes that are
>> subsequently described.
>>
>> Do we need port nodes for PCIe or SATA at all? If not, should we
>> s/ports/usb-ports/ in the container node name? I suppose it doesn't matter,
>> but it feels slightly odd to only represent some of the ports.
>
> I've seen references to "PCIe ports" and "SATA port" in documentation,
> but I don't see any immediate need to include them. An early version of
> the binding (and implementation) had them, but they ended up unused and
> empty. I don't see any registers that would substantiate that we need
> them. Then again, keeping the top-level node named "ports" will give us
> the flexibility to add them if necessary with the benefit that the
> parent name is still accurate, whereas naming it "usb-ports" would make
> things somewhat inconsistent if we ever need PCIe and SATA ports.

OK, I was expecting to add top-level "sata-ports" and "pcie-ports" nodes 
in the future if required, but I supposed we can indeed just add 
everything under "ports" instead.

>>> +Examples:
>>> +=========
>>> +
>>> +Tegra124 and Tegra132:
>>> +----------------------
>>
>> The example isn't valid for Tegra132 since the compatible values don't
>> include any Tegra132 entry.
>>
>> BTW, I've suggested a lot of phrasing changes. Once we've settled the other
>> questions, just let me know if you want me to propose an updated version of
>> the patch that contains all those phrasing changes so you don't have to do
>> them all yourself.
>
> Feel free to integrate whatever phrasing changes you think improve the
> binding. It might be best for you to do that yourself to avoid churn due
> to me misunderstanding what you were suggesting.

I think I'd like to see a bit more clarity on the definition of "ports" 
and the description of the node structure before I suggest any more 
tweaks to the wording. I'm not 100% sure I understood the proposed 
structure fully yet.

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 11/13/2015 09:32 AM, Thierry Reding wrote:
> On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote:
>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> Extend the binding to cover the set of feature found in Tegra210.
>>
>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
>>
>>> +PCIe pad:
>>> +---------
>>> +
>>> +Required properties:
>>> +- clocks: Must contain an entry for each entry in clock-names.
>>> +- clock-names: Must contain the following entries:
>>> +  - "pll": phandle and specifier referring to the PLLE
>>> +- resets: Must contain an entry for each entry in reset-names.
>>> +- reset-names: Must contain the following entries:
>>> +  - "phy": reset for the PCIe UPHY block
>>
>> I don't recall any clocks or resets properties in the pads for Tegra124. Do
>> we really not need any?
>
> Tegra124 had two instances of what used to be called IOPHY, one for PCIe
> and one for SATA. On Tegra210 these have been replaced by two instances
> of what's called UPHY. The resets listed in the PCIe and SATA pad nodes
> are wired to those UPHY instances, hence why they are new on Tegra210.
>
> For Tegra124 no resets exist for the IOPHY instances.

OK.

I wonder if renaming the section title from "PCIe pad" to "Tegra210 PCIe 
pad" would be helpful; it'd certainly allow the reader to more quickly 
work out what part of the document they were looking at if jumping 
around in it.

>>> +SATA pad:
>>> +---------
>>> +
>>> +Required properties:
>>> +- resets: Must contain an entry for each entry in reset-names.
>>> +- reset-names: Must contain the following entries:
>>> +  - "phy": reset for the SATA UPHY block
>>> +
>>>
>>>   PHY nodes:
>>
>> Nit: 2 blank lines there.
>
> Those were intentional for additional spacing between sections.

That seems inconsistent, and not something I recall seeing before, so 
I'm not sure anyone would realize that. Better to do it with more 
explicit section names I think.

>>> +For Tegra210, the list of valid PHY nodes is given below:
>>> +- utmi: utmi-0, utmi-1, utmi-2, utmi-3
>>> +  - functions: "snps", "xusb", "uart"
>>> +- hsic: hsic-0, hsic-1
>>> +  - functions: "snps", "xusb"
>>> +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
>>> +  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
>>> +- sata: sata-0
>>> +  - functions: "usb3-ss", "sata"
>>
>> usb2-bias also needs to be present.
>
> I'm not sure about this. All of the driver code that I've looked deals
> with the usb2-bias pad internally. As far as I can tell, this pad needs
> to be configured to whatever any of the other pads is configured for. I
> think that means if any of the UTMI pads is configured for XUSB then the
> usb2-bias pad must also be configured for XUSB. Which would also imply
> that if one of the UTMI pads is configured for XUSB, all of them must be
> configured for XUSB.

I don't believe that's true; on Tegra210 I have successfully configured 
the (legacy) "USB2 controller" to drive the recovery/micro-USB 
board-level port, and the "XUSB controller" (USB2 and USB3 ports 
thereof) to drive a couple of other board-level ports.

> It's also not a pad in the sense that the others are pads. It doesn't
> directly connect anywhere. It's also shared by all the UTMI pads. That
> said, there are two registers that allow some of the parameters of the
> pad to be set, so perhaps adding the node exclusively for
> configurability might make sense.
>
> It wouldn't really be a PHY node, though, so wouldn't fit into the above
> group. Perhaps something like the following could be added:
>
>    There is an additional pad that is used to support the bias voltages
>    to the USB2/UTMI pads. This is not a PHY that can be consumed by any
>    I/O controller, but the device tree node can be used to specify
>    parameters needed for the programming of the pad.
>
> I think the function shouldn't necessarily be exposed as a parameter,
> because all that would do is add the possibility for a conflicting set
> of mux options with the USB2/UTMI pads.

OK, if we can come up with a well-described algorithm re: how/when to 
program/enable this pad, then we can probably represent this differently 
than the other pads. I might expect DT to contain values for 
HS_DISCON_LEVEL HS_SQUELCH_LEVEL, although I can't recall if those 
values are SoC- or board-specific off the top of my head.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 11/13/2015 10:58 AM, Andrew Bresticker wrote:
> On Fri, Nov 13, 2015 at 8:32 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote:
>>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> Extend the binding to cover the set of feature found in Tegra210.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
>>>
>>>> +For Tegra210, the list of valid PHY nodes is given below:
>>>> +- utmi: utmi-0, utmi-1, utmi-2, utmi-3
>>>> +  - functions: "snps", "xusb", "uart"
>>>> +- hsic: hsic-0, hsic-1
>>>> +  - functions: "snps", "xusb"
>>>> +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
>>>> +  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
>>>> +- sata: sata-0
>>>> +  - functions: "usb3-ss", "sata"
>>>
>>> usb2-bias also needs to be present.
>>
>> I'm not sure about this. All of the driver code that I've looked deals
>> with the usb2-bias pad internally. As far as I can tell, this pad needs
>> to be configured to whatever any of the other pads is configured for. I
>> think that means if any of the UTMI pads is configured for XUSB then the
>> usb2-bias pad must also be configured for XUSB. Which would also imply
>> that if one of the UTMI pads is configured for XUSB, all of them must be
>> configured for XUSB.
>
> I was told by hardware engineers at NVIDIA that (at least on
> Tegra124/Tegra132) the usb2-bias pad must be configured in the
> XUSB_PADCTL register space if UTMI pad 0 is muxed to XUSB.  If UTMI
> pad 0 is muxed to SNPS, then the usb2-bias pad is configured in the
> USB register space (base 0x7d000000).  You may want to follow up
> internally to confirm this.  If it's true, that could make things here
> a bit nastier, especially if we want to support configurations where
> some pads are muxed to XUSB while others are muxed to SNPS.

Hmm. I've certainly successfully tested a configuration where UTMI pad 0 
was handled by the SNPS controller and other pads by the XUSB controller 
*and* where I set the usb2-bias "pad"'s muxing and configuration via the 
XUSB PADCTL module. In that case, I /had/ to configure usb2-bias via 
XUSB PADCTL or the other XUSB pads didn't work. However, perhaps that 
was because the XUSB controller driver probed before the SNPS driver; 
perhaps if they'd probed the other way around and the SNPS driver 
configured the bias pad, then everything would have worked without 
configuring the bias pad via XUSB PADCTL.

I suppose I'll have to start another internal thread to get the full 
details, and differentiate between "recommended" and "supported" and 
"must" vs. "can"/"should".

Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Stephen Warren]

On 11/16/2015 01:30 PM, Stephen Warren wrote:
> On 11/13/2015 10:58 AM, Andrew Bresticker wrote:
>> On Fri, Nov 13, 2015 at 8:32 AM, Thierry Reding
>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote:
>>>> On 11/04/2015 10:11 AM, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> Extend the binding to cover the set of feature found in Tegra210.
>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
>>>>>
>>>>
>>>>> +For Tegra210, the list of valid PHY nodes is given below:
>>>>> +- utmi: utmi-0, utmi-1, utmi-2, utmi-3
>>>>> +  - functions: "snps", "xusb", "uart"
>>>>> +- hsic: hsic-0, hsic-1
>>>>> +  - functions: "snps", "xusb"
>>>>> +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6
>>>>> +  - functions: "pcie-x1", "usb3-ss", "pcie-x4"
>>>>> +- sata: sata-0
>>>>> +  - functions: "usb3-ss", "sata"
>>>>
>>>> usb2-bias also needs to be present.
>>>
>>> I'm not sure about this. All of the driver code that I've looked deals
>>> with the usb2-bias pad internally. As far as I can tell, this pad needs
>>> to be configured to whatever any of the other pads is configured for. I
>>> think that means if any of the UTMI pads is configured for XUSB then the
>>> usb2-bias pad must also be configured for XUSB. Which would also imply
>>> that if one of the UTMI pads is configured for XUSB, all of them must be
>>> configured for XUSB.
>>
>> I was told by hardware engineers at NVIDIA that (at least on
>> Tegra124/Tegra132) the usb2-bias pad must be configured in the
>> XUSB_PADCTL register space if UTMI pad 0 is muxed to XUSB.  If UTMI
>> pad 0 is muxed to SNPS, then the usb2-bias pad is configured in the
>> USB register space (base 0x7d000000).  You may want to follow up
>> internally to confirm this.  If it's true, that could make things here
>> a bit nastier, especially if we want to support configurations where
>> some pads are muxed to XUSB while others are muxed to SNPS.
>
> Hmm. I've certainly successfully tested a configuration where UTMI pad 0
> was handled by the SNPS controller and other pads by the XUSB controller
> *and* where I set the usb2-bias "pad"'s muxing and configuration via the
> XUSB PADCTL module. In that case, I /had/ to configure usb2-bias via
> XUSB PADCTL or the other XUSB pads didn't work. However, perhaps that
> was because the XUSB controller driver probed before the SNPS driver;
> perhaps if they'd probed the other way around and the SNPS driver
> configured the bias pad, then everything would have worked without
> configuring the bias pad via XUSB PADCTL.
>
> I suppose I'll have to start another internal thread to get the full
> details, and differentiate between "recommended" and "supported" and
> "must" vs. "can"/"should".

I've discussed this with a HW engineer, and apparently this rule is 
simply a recommendation, with the acknowledgement that everything will 
work perfectly fine if we ignore it.