[PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Changes to the pad controller device tree binding have required that
each lane be associated with a separate PHY. Update the PCI host bridge
device tree binding to allow each root port to define the list of PHYs
required to drive the lanes associated with it.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/pci/nvidia,tegra20-pcie.txt    | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
index 75321ae23c08..033fe4b5afac 100644
--- a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
@@ -60,11 +60,14 @@ Required properties:
   - afi
   - pcie_x
 
-Required properties on Tegra124 and later:
+Required properties on Tegra124 and later (deprecated):
 - phys: Must contain an entry for each entry in phy-names.
 - phy-names: Must include the following entries:
   - pcie
 
+These properties are deprecated in favour of per-lane PHYs define in each of
+the root ports (see below).
+
 Power supplies for Tegra20:
 - avdd-pex-supply: Power supply for analog PCIe logic. Must supply 1.05 V.
 - vdd-pex-supply: Power supply for digital PCIe I/O. Must supply 1.05 V.
@@ -122,6 +125,13 @@ Required properties:
   - Root port 0 uses 4 lanes, root port 1 is unused.
   - Both root ports use 2 lanes.
 
+Required properties for Tegra124 and later:
+- phys: Must contain an phandle to a PHY for each entry in phy-names.
+- phy-names: Must include an entry for each active lane. Note that the number
+  of entries does not have to (though usually will) be equal to the specified
+  number of lanes in the nvidia,num-lanes property. Entries are of the form
+  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
+
 Example:
 
 SoC DTSI:
@@ -169,6 +179,9 @@ SoC DTSI:
 			ranges;
 
 			nvidia,num-lanes = <2>;
+
+			phys = <&{/padctl@0,7009f000/pads/pcie/pcie-4}>;
+			phy-names = "pcie-0";
 		};
 
 		pci@2,0 {
@@ -183,6 +196,9 @@ SoC DTSI:
 			ranges;
 
 			nvidia,num-lanes = <2>;
+
+			phys = <&{/padctl@0,7009f000/pads/pcie/pcie-2}>;
+			phy-names = "pcie-0";
 		};
 	};
 
-- 
2.7.1

[PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The current XUSB pad controller bindings are insufficient to describe
PHY devices attached to USB controllers. New bindings have been created
to overcome these restrictions. As a side-effect each root port now is
assigned a set of PHY devices, one for each lane associated with the
root port. This has the benefit of allowing fine-grained control of the
power management for each lane.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- cache result of check for new PHY bindings usage (Stephen Warren)

Changes in v2:
- rework commit message to more accurately describe this change

 drivers/pci/host/pci-tegra.c | 151 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 135 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 75c55265ca73..3b59e3162dfa 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -290,6 +290,7 @@ struct tegra_pcie {
 	struct reset_control *afi_rst;
 	struct reset_control *pcie_xrst;
 
+	bool legacy_phy;
 	struct phy *phy;
 
 	struct tegra_msi msi;
@@ -307,11 +308,14 @@ struct tegra_pcie {
 
 struct tegra_pcie_port {
 	struct tegra_pcie *pcie;
+	struct device_node *np;
 	struct list_head list;
 	struct resource regs;
 	void __iomem *base;
 	unsigned int index;
 	unsigned int lanes;
+
+	struct phy **phys;
 };
 
 struct tegra_pcie_bus {
@@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < port->lanes; i++) {
+		err = phy_power_on(port->phys[i]);
+		if (err < 0) {
+			dev_err(dev, "failed to power on PHY#%u: %d\n", i,
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 {
 	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
@@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 		afi_writel(pcie, value, AFI_FUSE);
 	}
 
-	if (!pcie->phy)
-		err = tegra_pcie_phy_enable(pcie);
-	else
-		err = phy_power_on(pcie->phy);
+	if (!pcie->legacy_phy) {
+		list_for_each_entry(port, &pcie->ports, list) {
+			err = tegra_pcie_port_phy_power_on(port);
+			if (err < 0) {
+				dev_err(pcie->dev,
+					"failed to power on PCIe port: %d\n",
+					err);
+				return err;
+			}
+		}
+	} else {
+		if (!pcie->phy)
+			err = tegra_pcie_phy_enable(pcie);
+		else
+			err = phy_power_on(pcie->phy);
 
-	if (err < 0) {
-		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
-		return err;
+		if (err < 0)
+			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
 	}
 
 	/* take the PCIe interface module out of reset */
@@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
+{
+	int err;
+
+	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
+	if (IS_ERR(pcie->phy)) {
+		err = PTR_ERR(pcie->phy);
+		dev_err(pcie->dev, "failed to get PHY: %d\n", err);
+		return err;
+	}
+
+	err = phy_init(pcie->phy);
+	if (err < 0) {
+		dev_err(pcie->dev, "failed to initialize PHY: %d\n", err);
+		return err;
+	}
+
+	pcie->legacy_phy = true;
+
+	return 0;
+}
+
+static struct phy *devm_of_phy_optional_get_index(struct device *dev,
+						  struct device_node *np,
+						  const char *consumer,
+						  unsigned int index)
+{
+	struct phy *phy;
+	char *name;
+
+	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	phy = devm_of_phy_get(dev, np, name);
+	kfree(name);
+
+	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+
+static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	struct phy *phy;
+	unsigned int i;
+	int err;
+
+	port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL);
+	if (!port->phys)
+		return -ENOMEM;
+
+	for (i = 0; i < port->lanes; i++) {
+		phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to get PHY#%u: %ld\n", i,
+				PTR_ERR(phy));
+			return PTR_ERR(phy);
+		}
+
+		err = phy_init(phy);
+		if (err < 0) {
+			dev_err(dev, "failed to initialize PHY#%u: %d\n", i,
+				err);
+			return err;
+		}
+
+		port->phys[i] = phy;
+	}
+
+	return 0;
+}
+
+static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
+{
+	struct tegra_pcie_port *port;
+	int err;
+
+	if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
+		return tegra_pcie_phys_get_legacy(pcie);
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		err = tegra_pcie_port_get_phys(port);
+		if (err < 0) {
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 {
 	struct platform_device *pdev = to_platform_device(pcie->dev);
@@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 		return err;
 	}
 
-	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
-	if (IS_ERR(pcie->phy)) {
-		err = PTR_ERR(pcie->phy);
-		dev_err(&pdev->dev, "failed to get PHY: %d\n", err);
-		return err;
-	}
-
-	err = phy_init(pcie->phy);
+	err = tegra_pcie_phys_get(pcie);
 	if (err < 0) {
-		dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err);
+		dev_err(&pdev->dev, "failed to get PHYs: %d\n", err);
 		return err;
 	}
 
@@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		rp->index = index;
 		rp->lanes = value;
 		rp->pcie = pcie;
+		rp->np = port;
 
 		rp->base = devm_ioremap_resource(pcie->dev, &rp->regs);
 		if (IS_ERR(rp->base))
-- 
2.7.1

[PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding]

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

The current XUSB pad controller bindings are insufficient to describe
PHY devices attached to USB controllers. New bindings have been created
to overcome these restrictions. As a side-effect each root port now is
assigned a set of PHY devices, one for each lane associated with the
root port. This has the benefit of allowing fine-grained control of the
power management for each lane.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- cache result of check for new PHY bindings usage (Stephen Warren)

Changes in v2:
- rework commit message to more accurately describe this change

 drivers/pci/host/pci-tegra.c | 151 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 135 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 75c55265ca73..3b59e3162dfa 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -290,6 +290,7 @@ struct tegra_pcie {
 	struct reset_control *afi_rst;
 	struct reset_control *pcie_xrst;
 
+	bool legacy_phy;
 	struct phy *phy;
 
 	struct tegra_msi msi;
@@ -307,11 +308,14 @@ struct tegra_pcie {
 
 struct tegra_pcie_port {
 	struct tegra_pcie *pcie;
+	struct device_node *np;
 	struct list_head list;
 	struct resource regs;
 	void __iomem *base;
 	unsigned int index;
 	unsigned int lanes;
+
+	struct phy **phys;
 };
 
 struct tegra_pcie_bus {
@@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < port->lanes; i++) {
+		err = phy_power_on(port->phys[i]);
+		if (err < 0) {
+			dev_err(dev, "failed to power on PHY#%u: %d\n", i,
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 {
 	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
@@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 		afi_writel(pcie, value, AFI_FUSE);
 	}
 
-	if (!pcie->phy)
-		err = tegra_pcie_phy_enable(pcie);
-	else
-		err = phy_power_on(pcie->phy);
+	if (!pcie->legacy_phy) {
+		list_for_each_entry(port, &pcie->ports, list) {
+			err = tegra_pcie_port_phy_power_on(port);
+			if (err < 0) {
+				dev_err(pcie->dev,
+					"failed to power on PCIe port: %d\n",
+					err);
+				return err;
+			}
+		}
+	} else {
+		if (!pcie->phy)
+			err = tegra_pcie_phy_enable(pcie);
+		else
+			err = phy_power_on(pcie->phy);
 
-	if (err < 0) {
-		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
-		return err;
+		if (err < 0)
+			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
 	}
 
 	/* take the PCIe interface module out of reset */
@@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
+{
+	int err;
+
+	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
+	if (IS_ERR(pcie->phy)) {
+		err = PTR_ERR(pcie->phy);
+		dev_err(pcie->dev, "failed to get PHY: %d\n", err);
+		return err;
+	}
+
+	err = phy_init(pcie->phy);
+	if (err < 0) {
+		dev_err(pcie->dev, "failed to initialize PHY: %d\n", err);
+		return err;
+	}
+
+	pcie->legacy_phy = true;
+
+	return 0;
+}
+
+static struct phy *devm_of_phy_optional_get_index(struct device *dev,
+						  struct device_node *np,
+						  const char *consumer,
+						  unsigned int index)
+{
+	struct phy *phy;
+	char *name;
+
+	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	phy = devm_of_phy_get(dev, np, name);
+	kfree(name);
+
+	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+
+static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	struct phy *phy;
+	unsigned int i;
+	int err;
+
+	port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL);
+	if (!port->phys)
+		return -ENOMEM;
+
+	for (i = 0; i < port->lanes; i++) {
+		phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to get PHY#%u: %ld\n", i,
+				PTR_ERR(phy));
+			return PTR_ERR(phy);
+		}
+
+		err = phy_init(phy);
+		if (err < 0) {
+			dev_err(dev, "failed to initialize PHY#%u: %d\n", i,
+				err);
+			return err;
+		}
+
+		port->phys[i] = phy;
+	}
+
+	return 0;
+}
+
+static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
+{
+	struct tegra_pcie_port *port;
+	int err;
+
+	if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
+		return tegra_pcie_phys_get_legacy(pcie);
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		err = tegra_pcie_port_get_phys(port);
+		if (err < 0) {
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 {
 	struct platform_device *pdev = to_platform_device(pcie->dev);
@@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 		return err;
 	}
 
-	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
-	if (IS_ERR(pcie->phy)) {
-		err = PTR_ERR(pcie->phy);
-		dev_err(&pdev->dev, "failed to get PHY: %d\n", err);
-		return err;
-	}
-
-	err = phy_init(pcie->phy);
+	err = tegra_pcie_phys_get(pcie);
 	if (err < 0) {
-		dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err);
+		dev_err(&pdev->dev, "failed to get PHYs: %d\n", err);
 		return err;
 	}
 
@@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		rp->index = index;
 		rp->lanes = value;
 		rp->pcie = pcie;
+		rp->np = port;
 
 		rp->base = devm_ioremap_resource(pcie->dev, &rp->regs);
 		if (IS_ERR(rp->base))
-- 
2.7.1

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Bjorn Helgaas]

Stephen, you had some comments on the previous version.  What do you
think about this one?

On Tue, Mar 08, 2016 at 04:48:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The current XUSB pad controller bindings are insufficient to describe
> PHY devices attached to USB controllers. New bindings have been created
> to overcome these restrictions. As a side-effect each root port now is
> assigned a set of PHY devices, one for each lane associated with the
> root port. This has the benefit of allowing fine-grained control of the
> power management for each lane.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ...

>  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  {
>  	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
> @@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  		afi_writel(pcie, value, AFI_FUSE);
>  	}
>  
> -	if (!pcie->phy)
> -		err = tegra_pcie_phy_enable(pcie);
> -	else
> -		err = phy_power_on(pcie->phy);
> +	if (!pcie->legacy_phy) {
> +		list_for_each_entry(port, &pcie->ports, list) {
> +			err = tegra_pcie_port_phy_power_on(port);
> +			if (err < 0) {
> +				dev_err(pcie->dev,
> +					"failed to power on PCIe port: %d\n",
> +					err);
> +				return err;
> +			}
> +		}
> +	} else {
> +		if (!pcie->phy)
> +			err = tegra_pcie_phy_enable(pcie);
> +		else
> +			err = phy_power_on(pcie->phy);
>  
> -	if (err < 0) {
> -		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
> -		return err;
> +		if (err < 0)
> +			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);

In the legacy_phy case, we used to bail out if tegra_pcie_phy_enable()
or phy_power_on() failed, but now we don't.  I assume this is an
unintentional change.

I would personally write this as follows because I hate reading
"if (!xxx) ... else ..." (this patch applies on top of the one you
posted).  Note that this restores the legacy_phy error path also.


diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 625db7d..139b9ca 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -882,6 +882,31 @@ static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
 	return 0;
 }
 
+static int tegra_pcie_port_phy_enable(struct tegra_pcie *pcie)
+{
+	if (pcie->legacy_phy) {
+		if (pcie->phy)
+			err = phy_power_on(pcie->phy);
+		else
+			err = tegra_pcie_phy_enable(pcie);
+
+		if (err < 0)
+			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
+
+		return err;
+	}
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		err = tegra_pcie_port_phy_power_on(port);
+		if (err < 0) {
+			dev_err(pcie->dev, "failed to power on PCIe port: %d\n",
+				err);
+			return err;
+		}
+	}
+	return 0;
+}
+
 static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 {
 	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
@@ -921,25 +946,9 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 		afi_writel(pcie, value, AFI_FUSE);
 	}
 
-	if (!pcie->legacy_phy) {
-		list_for_each_entry(port, &pcie->ports, list) {
-			err = tegra_pcie_port_phy_power_on(port);
-			if (err < 0) {
-				dev_err(pcie->dev,
-					"failed to power on PCIe port: %d\n",
-					err);
-				return err;
-			}
-		}
-	} else {
-		if (!pcie->phy)
-			err = tegra_pcie_phy_enable(pcie);
-		else
-			err = phy_power_on(pcie->phy);
-
-		if (err < 0)
-			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
-	}
+	err = tegra_pcie_port_phy_enable(pcie);
+	if (err < 0)
+		return err;
 
 	/* take the PCIe interface module out of reset */
 	reset_control_deassert(pcie->pcie_xrst);

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Stephen Warren]

On 03/08/2016 08:48 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The current XUSB pad controller bindings are insufficient to describe
> PHY devices attached to USB controllers. New bindings have been created
> to overcome these restrictions. As a side-effect each root port now is
> assigned a set of PHY devices, one for each lane associated with the
> root port. This has the benefit of allowing fine-grained control of the
> power management for each lane.

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		err = phy_power_on(port->phys[i]);

This assume the number of array entries is precisely the number of 
lanes. That seems to contradict the binding update which said the number 
might not match. Perhaps there's an expectation that phy_power_on() is a 
no-op for some "invalid" values like NULL or an error-pointer value? But...

> +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> +						  struct device_node *np,
> +						  const char *consumer,
> +						  unsigned int index)
> +{
> +	struct phy *phy;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = devm_of_phy_get(dev, np, name);
> +	kfree(name);
> +
> +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> +		phy = NULL;
> +
> +	return phy;
> +}

The error-handling there looks wrong. The function generally returns 
either a valid PHY or an error pointer. However, in the case of -ENODEV, 
NULL is returned. Subsystems are supposed to encode their handles as, 
and functions are supposed to return, either NULL or an error pointer 
for error cases, not both/either. Is the PHY API broken in this regard? 
If so, then this code is fine, but if not it might need a fix.

Aside from that, this patch looks fine.

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Stephen Warren]

On 03/08/2016 08:48 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> The current XUSB pad controller bindings are insufficient to describe
> PHY devices attached to USB controllers. New bindings have been created
> to overcome these restrictions. As a side-effect each root port now is
> assigned a set of PHY devices, one for each lane associated with the
> root port. This has the benefit of allowing fine-grained control of the
> power management for each lane.

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		err = phy_power_on(port->phys[i]);

This assume the number of array entries is precisely the number of 
lanes. That seems to contradict the binding update which said the number 
might not match. Perhaps there's an expectation that phy_power_on() is a 
no-op for some "invalid" values like NULL or an error-pointer value? But...

> +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> +						  struct device_node *np,
> +						  const char *consumer,
> +						  unsigned int index)
> +{
> +	struct phy *phy;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = devm_of_phy_get(dev, np, name);
> +	kfree(name);
> +
> +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> +		phy = NULL;
> +
> +	return phy;
> +}

The error-handling there looks wrong. The function generally returns 
either a valid PHY or an error pointer. However, in the case of -ENODEV, 
NULL is returned. Subsystems are supposed to encode their handles as, 
and functions are supposed to return, either NULL or an error pointer 
for error cases, not both/either. Is the PHY API broken in this regard? 
If so, then this code is fine, but if not it might need a fix.

Aside from that, this patch looks fine.

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding]

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

On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote:
> On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The current XUSB pad controller bindings are insufficient to describe
> > PHY devices attached to USB controllers. New bindings have been created
> > to overcome these restrictions. As a side-effect each root port now is
> > assigned a set of PHY devices, one for each lane associated with the
> > root port. This has the benefit of allowing fine-grained control of the
> > power management for each lane.
> 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> 
> > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> > +{
> > +	struct device *dev = port->pcie->dev;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	for (i = 0; i < port->lanes; i++) {
> > +		err = phy_power_on(port->phys[i]);
> 
> This assume the number of array entries is precisely the number of lanes.
> That seems to contradict the binding update which said the number might not
> match. Perhaps there's an expectation that phy_power_on() is a no-op for
> some "invalid" values like NULL or an error-pointer value? But...
> 
> > +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> > +						  struct device_node *np,
> > +						  const char *consumer,
> > +						  unsigned int index)
> > +{
> > +	struct phy *phy;
> > +	char *name;
> > +
> > +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> > +	if (!name)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	phy = devm_of_phy_get(dev, np, name);
> > +	kfree(name);
> > +
> > +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> > +		phy = NULL;
> > +
> > +	return phy;
> > +}
> 
> The error-handling there looks wrong. The function generally returns either
> a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is
> returned. Subsystems are supposed to encode their handles as, and functions
> are supposed to return, either NULL or an error pointer for error cases, not
> both/either. Is the PHY API broken in this regard? If so, then this code is
> fine, but if not it might need a fix.

This function mimics phy_optional_get() which similarily returns NULL
for -ENODEV. The remainder of the PHY API treats NULL pointers as
"dummy" PHYs and returns early. I think that's a sensible approach to
handling optional resources.

It might have been more obvious had I implemented this function within
phy-core.c, but I didn't think it universally useful because it uses a
rather uncommon lookup pattern. I did keep a generic name in case it's
ever deemed useful outside of this driver, at which point it could
simply be moved into phy-core.c without requiring this driver to
change.

Thierry

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

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding]

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

On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote:
> On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The current XUSB pad controller bindings are insufficient to describe
> > PHY devices attached to USB controllers. New bindings have been created
> > to overcome these restrictions. As a side-effect each root port now is
> > assigned a set of PHY devices, one for each lane associated with the
> > root port. This has the benefit of allowing fine-grained control of the
> > power management for each lane.
> 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> 
> > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> > +{
> > +	struct device *dev = port->pcie->dev;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	for (i = 0; i < port->lanes; i++) {
> > +		err = phy_power_on(port->phys[i]);
> 
> This assume the number of array entries is precisely the number of lanes.
> That seems to contradict the binding update which said the number might not
> match. Perhaps there's an expectation that phy_power_on() is a no-op for
> some "invalid" values like NULL or an error-pointer value? But...
> 
> > +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> > +						  struct device_node *np,
> > +						  const char *consumer,
> > +						  unsigned int index)
> > +{
> > +	struct phy *phy;
> > +	char *name;
> > +
> > +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> > +	if (!name)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	phy = devm_of_phy_get(dev, np, name);
> > +	kfree(name);
> > +
> > +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> > +		phy = NULL;
> > +
> > +	return phy;
> > +}
> 
> The error-handling there looks wrong. The function generally returns either
> a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is
> returned. Subsystems are supposed to encode their handles as, and functions
> are supposed to return, either NULL or an error pointer for error cases, not
> both/either. Is the PHY API broken in this regard? If so, then this code is
> fine, but if not it might need a fix.

This function mimics phy_optional_get() which similarily returns NULL
for -ENODEV. The remainder of the PHY API treats NULL pointers as
"dummy" PHYs and returns early. I think that's a sensible approach to
handling optional resources.

It might have been more obvious had I implemented this function within
phy-core.c, but I didn't think it universally useful because it uses a
rather uncommon lookup pattern. I did keep a generic name in case it's
ever deemed useful outside of this driver, at which point it could
simply be moved into phy-core.c without requiring this driver to
change.

Thierry

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

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Stephen Warren]

On 04/13/2016 10:01 AM, Thierry Reding wrote:
> On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote:
>> On 03/08/2016 08:48 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> The current XUSB pad controller bindings are insufficient to describe
>>> PHY devices attached to USB controllers. New bindings have been created
>>> to overcome these restrictions. As a side-effect each root port now is
>>> assigned a set of PHY devices, one for each lane associated with the
>>> root port. This has the benefit of allowing fine-grained control of the
>>> power management for each lane.
>>
>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>>
>>> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
>>> +{
>>> +	struct device *dev = port->pcie->dev;
>>> +	unsigned int i;
>>> +	int err;
>>> +
>>> +	for (i = 0; i < port->lanes; i++) {
>>> +		err = phy_power_on(port->phys[i]);
>>
>> This assume the number of array entries is precisely the number of lanes.
>> That seems to contradict the binding update which said the number might not
>> match. Perhaps there's an expectation that phy_power_on() is a no-op for
>> some "invalid" values like NULL or an error-pointer value? But...
>>
>>> +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
>>> +						  struct device_node *np,
>>> +						  const char *consumer,
>>> +						  unsigned int index)
>>> +{
>>> +	struct phy *phy;
>>> +	char *name;
>>> +
>>> +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
>>> +	if (!name)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	phy = devm_of_phy_get(dev, np, name);
>>> +	kfree(name);
>>> +
>>> +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
>>> +		phy = NULL;
>>> +
>>> +	return phy;
>>> +}
>>
>> The error-handling there looks wrong. The function generally returns either
>> a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is
>> returned. Subsystems are supposed to encode their handles as, and functions
>> are supposed to return, either NULL or an error pointer for error cases, not
>> both/either. Is the PHY API broken in this regard? If so, then this code is
>> fine, but if not it might need a fix.
>
> This function mimics phy_optional_get() which similarily returns NULL
> for -ENODEV. The remainder of the PHY API treats NULL pointers as
> "dummy" PHYs and returns early. I think that's a sensible approach to
> handling optional resources.
>
> It might have been more obvious had I implemented this function within
> phy-core.c, but I didn't think it universally useful because it uses a
> rather uncommon lookup pattern. I did keep a generic name in case it's
> ever deemed useful outside of this driver, at which point it could
> simply be moved into phy-core.c without requiring this driver to
> change.

Ah OK, so if a caller of this function is expected to only use IS_ERR(), 
and hence treat NULL as a perfectly valid PHY value, and all the PHY 
APIs deal with NULL correctly, then this is probably OK.

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Thierry Reding]

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

On Wed, Apr 13, 2016 at 11:01:58AM -0600, Stephen Warren wrote:
> On 04/13/2016 10:01 AM, Thierry Reding wrote:
> > On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote:
> > > On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > The current XUSB pad controller bindings are insufficient to describe
> > > > PHY devices attached to USB controllers. New bindings have been created
> > > > to overcome these restrictions. As a side-effect each root port now is
> > > > assigned a set of PHY devices, one for each lane associated with the
> > > > root port. This has the benefit of allowing fine-grained control of the
> > > > power management for each lane.
> > > 
> > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > 
> > > > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> > > > +{
> > > > +	struct device *dev = port->pcie->dev;
> > > > +	unsigned int i;
> > > > +	int err;
> > > > +
> > > > +	for (i = 0; i < port->lanes; i++) {
> > > > +		err = phy_power_on(port->phys[i]);
> > > 
> > > This assume the number of array entries is precisely the number of lanes.
> > > That seems to contradict the binding update which said the number might not
> > > match. Perhaps there's an expectation that phy_power_on() is a no-op for
> > > some "invalid" values like NULL or an error-pointer value? But...
> > > 
> > > > +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> > > > +						  struct device_node *np,
> > > > +						  const char *consumer,
> > > > +						  unsigned int index)
> > > > +{
> > > > +	struct phy *phy;
> > > > +	char *name;
> > > > +
> > > > +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> > > > +	if (!name)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	phy = devm_of_phy_get(dev, np, name);
> > > > +	kfree(name);
> > > > +
> > > > +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> > > > +		phy = NULL;
> > > > +
> > > > +	return phy;
> > > > +}
> > > 
> > > The error-handling there looks wrong. The function generally returns either
> > > a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is
> > > returned. Subsystems are supposed to encode their handles as, and functions
> > > are supposed to return, either NULL or an error pointer for error cases, not
> > > both/either. Is the PHY API broken in this regard? If so, then this code is
> > > fine, but if not it might need a fix.
> > 
> > This function mimics phy_optional_get() which similarily returns NULL
> > for -ENODEV. The remainder of the PHY API treats NULL pointers as
> > "dummy" PHYs and returns early. I think that's a sensible approach to
> > handling optional resources.
> > 
> > It might have been more obvious had I implemented this function within
> > phy-core.c, but I didn't think it universally useful because it uses a
> > rather uncommon lookup pattern. I did keep a generic name in case it's
> > ever deemed useful outside of this driver, at which point it could
> > simply be moved into phy-core.c without requiring this driver to
> > change.
> 
> Ah OK, so if a caller of this function is expected to only use IS_ERR(), and
> hence treat NULL as a perfectly valid PHY value, and all the PHY APIs deal
> with NULL correctly, then this is probably OK.

I think it's not completely consistently used, but at least the public
API that we do use has the necessary guards, so we should be fine.

Thierry

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

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Bjorn Helgaas]

Hi Thierry,

On Tue, Mar 08, 2016 at 04:48:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The current XUSB pad controller bindings are insufficient to describe
> PHY devices attached to USB controllers. New bindings have been created
> to overcome these restrictions. As a side-effect each root port now is
> assigned a set of PHY devices, one for each lane associated with the
> root port. This has the benefit of allowing fine-grained control of the
> power management for each lane.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

There are two open questions here (mine and Stephen's).  I'll drop
this and look for a v4.

> ---
> Changes in v3:
> - cache result of check for new PHY bindings usage (Stephen Warren)
> 
> Changes in v2:
> - rework commit message to more accurately describe this change
> 
>  drivers/pci/host/pci-tegra.c | 151 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 135 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 75c55265ca73..3b59e3162dfa 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -290,6 +290,7 @@ struct tegra_pcie {
>  	struct reset_control *afi_rst;
>  	struct reset_control *pcie_xrst;
>  
> +	bool legacy_phy;
>  	struct phy *phy;
>  
>  	struct tegra_msi msi;
> @@ -307,11 +308,14 @@ struct tegra_pcie {
>  
>  struct tegra_pcie_port {
>  	struct tegra_pcie *pcie;
> +	struct device_node *np;
>  	struct list_head list;
>  	struct resource regs;
>  	void __iomem *base;
>  	unsigned int index;
>  	unsigned int lanes;
> +
> +	struct phy **phys;
>  };
>  
>  struct tegra_pcie_bus {
> @@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		err = phy_power_on(port->phys[i]);
> +		if (err < 0) {
> +			dev_err(dev, "failed to power on PHY#%u: %d\n", i,
> +				err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  {
>  	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
> @@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  		afi_writel(pcie, value, AFI_FUSE);
>  	}
>  
> -	if (!pcie->phy)
> -		err = tegra_pcie_phy_enable(pcie);
> -	else
> -		err = phy_power_on(pcie->phy);
> +	if (!pcie->legacy_phy) {
> +		list_for_each_entry(port, &pcie->ports, list) {
> +			err = tegra_pcie_port_phy_power_on(port);
> +			if (err < 0) {
> +				dev_err(pcie->dev,
> +					"failed to power on PCIe port: %d\n",
> +					err);
> +				return err;
> +			}
> +		}
> +	} else {
> +		if (!pcie->phy)
> +			err = tegra_pcie_phy_enable(pcie);
> +		else
> +			err = phy_power_on(pcie->phy);
>  
> -	if (err < 0) {
> -		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
> -		return err;
> +		if (err < 0)
> +			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
>  	}
>  
>  	/* take the PCIe interface module out of reset */
> @@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
> +{
> +	int err;
> +
> +	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
> +	if (IS_ERR(pcie->phy)) {
> +		err = PTR_ERR(pcie->phy);
> +		dev_err(pcie->dev, "failed to get PHY: %d\n", err);
> +		return err;
> +	}
> +
> +	err = phy_init(pcie->phy);
> +	if (err < 0) {
> +		dev_err(pcie->dev, "failed to initialize PHY: %d\n", err);
> +		return err;
> +	}
> +
> +	pcie->legacy_phy = true;
> +
> +	return 0;
> +}
> +
> +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> +						  struct device_node *np,
> +						  const char *consumer,
> +						  unsigned int index)
> +{
> +	struct phy *phy;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = devm_of_phy_get(dev, np, name);
> +	kfree(name);
> +
> +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> +		phy = NULL;
> +
> +	return phy;
> +}
> +
> +static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct phy *phy;
> +	unsigned int i;
> +	int err;
> +
> +	port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL);
> +	if (!port->phys)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i);
> +		if (IS_ERR(phy)) {
> +			dev_err(dev, "failed to get PHY#%u: %ld\n", i,
> +				PTR_ERR(phy));
> +			return PTR_ERR(phy);
> +		}
> +
> +		err = phy_init(phy);
> +		if (err < 0) {
> +			dev_err(dev, "failed to initialize PHY#%u: %d\n", i,
> +				err);
> +			return err;
> +		}
> +
> +		port->phys[i] = phy;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
> +{
> +	struct tegra_pcie_port *port;
> +	int err;
> +
> +	if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
> +		return tegra_pcie_phys_get_legacy(pcie);
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		err = tegra_pcie_port_get_phys(port);
> +		if (err < 0) {
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  {
>  	struct platform_device *pdev = to_platform_device(pcie->dev);
> @@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  		return err;
>  	}
>  
> -	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
> -	if (IS_ERR(pcie->phy)) {
> -		err = PTR_ERR(pcie->phy);
> -		dev_err(&pdev->dev, "failed to get PHY: %d\n", err);
> -		return err;
> -	}
> -
> -	err = phy_init(pcie->phy);
> +	err = tegra_pcie_phys_get(pcie);
>  	if (err < 0) {
> -		dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err);
> +		dev_err(&pdev->dev, "failed to get PHYs: %d\n", err);
>  		return err;
>  	}
>  
> @@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		rp->index = index;
>  		rp->lanes = value;
>  		rp->pcie = pcie;
> +		rp->np = port;
>  
>  		rp->base = devm_ioremap_resource(pcie->dev, &rp->regs);
>  		if (IS_ERR(rp->base))
> -- 
> 2.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] PCI: tegra: Support per-lane PHYs

[Bjorn Helgaas]

Hi Thierry,

On Tue, Mar 08, 2016 at 04:48:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The current XUSB pad controller bindings are insufficient to describe
> PHY devices attached to USB controllers. New bindings have been created
> to overcome these restrictions. As a side-effect each root port now is
> assigned a set of PHY devices, one for each lane associated with the
> root port. This has the benefit of allowing fine-grained control of the
> power management for each lane.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

There are two open questions here (mine and Stephen's).  I'll drop
this and look for a v4.

> ---
> Changes in v3:
> - cache result of check for new PHY bindings usage (Stephen Warren)
> 
> Changes in v2:
> - rework commit message to more accurately describe this change
> 
>  drivers/pci/host/pci-tegra.c | 151 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 135 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 75c55265ca73..3b59e3162dfa 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -290,6 +290,7 @@ struct tegra_pcie {
>  	struct reset_control *afi_rst;
>  	struct reset_control *pcie_xrst;
>  
> +	bool legacy_phy;
>  	struct phy *phy;
>  
>  	struct tegra_msi msi;
> @@ -307,11 +308,14 @@ struct tegra_pcie {
>  
>  struct tegra_pcie_port {
>  	struct tegra_pcie *pcie;
> +	struct device_node *np;
>  	struct list_head list;
>  	struct resource regs;
>  	void __iomem *base;
>  	unsigned int index;
>  	unsigned int lanes;
> +
> +	struct phy **phys;
>  };
>  
>  struct tegra_pcie_bus {
> @@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		err = phy_power_on(port->phys[i]);
> +		if (err < 0) {
> +			dev_err(dev, "failed to power on PHY#%u: %d\n", i,
> +				err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  {
>  	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
> @@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  		afi_writel(pcie, value, AFI_FUSE);
>  	}
>  
> -	if (!pcie->phy)
> -		err = tegra_pcie_phy_enable(pcie);
> -	else
> -		err = phy_power_on(pcie->phy);
> +	if (!pcie->legacy_phy) {
> +		list_for_each_entry(port, &pcie->ports, list) {
> +			err = tegra_pcie_port_phy_power_on(port);
> +			if (err < 0) {
> +				dev_err(pcie->dev,
> +					"failed to power on PCIe port: %d\n",
> +					err);
> +				return err;
> +			}
> +		}
> +	} else {
> +		if (!pcie->phy)
> +			err = tegra_pcie_phy_enable(pcie);
> +		else
> +			err = phy_power_on(pcie->phy);
>  
> -	if (err < 0) {
> -		dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
> -		return err;
> +		if (err < 0)
> +			dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
>  	}
>  
>  	/* take the PCIe interface module out of reset */
> @@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie)
> +{
> +	int err;
> +
> +	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
> +	if (IS_ERR(pcie->phy)) {
> +		err = PTR_ERR(pcie->phy);
> +		dev_err(pcie->dev, "failed to get PHY: %d\n", err);
> +		return err;
> +	}
> +
> +	err = phy_init(pcie->phy);
> +	if (err < 0) {
> +		dev_err(pcie->dev, "failed to initialize PHY: %d\n", err);
> +		return err;
> +	}
> +
> +	pcie->legacy_phy = true;
> +
> +	return 0;
> +}
> +
> +static struct phy *devm_of_phy_optional_get_index(struct device *dev,
> +						  struct device_node *np,
> +						  const char *consumer,
> +						  unsigned int index)
> +{
> +	struct phy *phy;
> +	char *name;
> +
> +	name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = devm_of_phy_get(dev, np, name);
> +	kfree(name);
> +
> +	if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
> +		phy = NULL;
> +
> +	return phy;
> +}
> +
> +static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct phy *phy;
> +	unsigned int i;
> +	int err;
> +
> +	port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL);
> +	if (!port->phys)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < port->lanes; i++) {
> +		phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i);
> +		if (IS_ERR(phy)) {
> +			dev_err(dev, "failed to get PHY#%u: %ld\n", i,
> +				PTR_ERR(phy));
> +			return PTR_ERR(phy);
> +		}
> +
> +		err = phy_init(phy);
> +		if (err < 0) {
> +			dev_err(dev, "failed to initialize PHY#%u: %d\n", i,
> +				err);
> +			return err;
> +		}
> +
> +		port->phys[i] = phy;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
> +{
> +	struct tegra_pcie_port *port;
> +	int err;
> +
> +	if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
> +		return tegra_pcie_phys_get_legacy(pcie);
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		err = tegra_pcie_port_get_phys(port);
> +		if (err < 0) {
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  {
>  	struct platform_device *pdev = to_platform_device(pcie->dev);
> @@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>  		return err;
>  	}
>  
> -	pcie->phy = devm_phy_optional_get(pcie->dev, "pcie");
> -	if (IS_ERR(pcie->phy)) {
> -		err = PTR_ERR(pcie->phy);
> -		dev_err(&pdev->dev, "failed to get PHY: %d\n", err);
> -		return err;
> -	}
> -
> -	err = phy_init(pcie->phy);
> +	err = tegra_pcie_phys_get(pcie);
>  	if (err < 0) {
> -		dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err);
> +		dev_err(&pdev->dev, "failed to get PHYs: %d\n", err);
>  		return err;
>  	}
>  
> @@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		rp->index = index;
>  		rp->lanes = value;
>  		rp->pcie = pcie;
> +		rp->np = port;
>  
>  		rp->base = devm_ioremap_resource(pcie->dev, &rp->regs);
>  		if (IS_ERR(rp->base))
> -- 
> 2.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" 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: pci: tegra: Update for per-lane PHYs

[Stephen Warren]

On 03/08/2016 08:48 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Changes to the pad controller device tree binding have required that
> each lane be associated with a separate PHY.

I still don't think this has anything to do with DT bindings. Rather, 
the definition of a PHY (in HW and the Linux PHY subsystem) is a single 
lane. That fact then requires drivers to support a PHY per lane rather 
than a single multi-lane PHY, and equally means the DT bindings must be 
written according to the correct definition of a PHY.

Still, I suppose the commit description is fine as is.

> Update the PCI host bridge
> device tree binding to allow each root port to define the list of PHYs
> required to drive the lanes associated with it.

> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> +Required properties for Tegra124 and later:
> +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> +- phy-names: Must include an entry for each active lane. Note that the number
> +  of entries does not have to (though usually will) be equal to the specified
> +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.

When would the number of PHYs not equal the number of lanes? I thought 
the whole point of this patch was to switch to per-lane PHYs? Perhaps 
I'm just misremembering some exception, so there may be no need to 
change this.

>   Example:
>
>   SoC DTSI:
> @@ -169,6 +179,9 @@ SoC DTSI:
>   			ranges;
>
>   			nvidia,num-lanes = <2>;
> +
> +			phys = <&{/padctl@0,7009f000/pads/pcie/pcie-4}>;
> +			phy-names = "pcie-0";
>   		};

The example shows a Tegra20 PCIe controller, yet includes 
Tegra124-or-greater properties. That seems a bit odd. Should the changes 
to the example be dropped, or does "Required properties for Tegra124 and 
later" mean "Required for T124+, optional for earlier chips"?

Conceptually this change is fine by me though.

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Stephen Warren]

On 03/08/2016 08:48 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Changes to the pad controller device tree binding have required that
> each lane be associated with a separate PHY.

I still don't think this has anything to do with DT bindings. Rather, 
the definition of a PHY (in HW and the Linux PHY subsystem) is a single 
lane. That fact then requires drivers to support a PHY per lane rather 
than a single multi-lane PHY, and equally means the DT bindings must be 
written according to the correct definition of a PHY.

Still, I suppose the commit description is fine as is.

> Update the PCI host bridge
> device tree binding to allow each root port to define the list of PHYs
> required to drive the lanes associated with it.

> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> +Required properties for Tegra124 and later:
> +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> +- phy-names: Must include an entry for each active lane. Note that the number
> +  of entries does not have to (though usually will) be equal to the specified
> +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.

When would the number of PHYs not equal the number of lanes? I thought 
the whole point of this patch was to switch to per-lane PHYs? Perhaps 
I'm just misremembering some exception, so there may be no need to 
change this.

>   Example:
>
>   SoC DTSI:
> @@ -169,6 +179,9 @@ SoC DTSI:
>   			ranges;
>
>   			nvidia,num-lanes = <2>;
> +
> +			phys = <&{/padctl@0,7009f000/pads/pcie/pcie-4}>;
> +			phy-names = "pcie-0";
>   		};

The example shows a Tegra20 PCIe controller, yet includes 
Tegra124-or-greater properties. That seems a bit odd. Should the changes 
to the example be dropped, or does "Required properties for Tegra124 and 
later" mean "Required for T124+, optional for earlier chips"?

Conceptually this change is fine by me though.

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding]

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

On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote:
> On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Changes to the pad controller device tree binding have required that
> > each lane be associated with a separate PHY.
> 
> I still don't think this has anything to do with DT bindings. Rather, the
> definition of a PHY (in HW and the Linux PHY subsystem) is a single lane.
> That fact then requires drivers to support a PHY per lane rather than a
> single multi-lane PHY, and equally means the DT bindings must be written
> according to the correct definition of a PHY.
> 
> Still, I suppose the commit description is fine as is.

I've reworded the commit message to give a more accurate rationale for
the change. I'll be posting a v5 soon.

> > Update the PCI host bridge
> > device tree binding to allow each root port to define the list of PHYs
> > required to drive the lanes associated with it.
> 
> > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> 
> > +Required properties for Tegra124 and later:
> > +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> > +- phy-names: Must include an entry for each active lane. Note that the number
> > +  of entries does not have to (though usually will) be equal to the specified
> > +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> > +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
> 
> When would the number of PHYs not equal the number of lanes? I thought the
> whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just
> misremembering some exception, so there may be no need to change this.

This is useful to support the case where we want to connect a x1 or x2
device to a root port that is configured to drive more lanes. It's a
rather unusual configuration, but it would be possible for example to
have an onboard x1 ethernet card, but the board layout is such that it
runs in x1/x2 mode, with the ethernet card connected to the x2 port.

> >   Example:
> > 
> >   SoC DTSI:
> > @@ -169,6 +179,9 @@ SoC DTSI:
> >   			ranges;
> > 
> >   			nvidia,num-lanes = <2>;
> > +
> > +			phys = <&{/padctl@0,7009f000/pads/pcie/pcie-4}>;
> > +			phy-names = "pcie-0";
> >   		};
> 
> The example shows a Tegra20 PCIe controller, yet includes
> Tegra124-or-greater properties. That seems a bit odd. Should the changes to
> the example be dropped, or does "Required properties for Tegra124 and later"
> mean "Required for T124+, optional for earlier chips"?

I've annotated these properties with "for Tegra124 and later", hopefully
that clarifies that these properties are only valid on Tegra124 and
later chips. The reason is that earlier (Tegra114 didn't support PCIe,
Tegra30 and Tegra20 did but had PHY registers within the PCI host bridge
I/O memory).

Thierry

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

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Stephen Warren]

On 04/13/2016 10:22 AM, Thierry Reding wrote:
> On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote:
>> On 03/08/2016 08:48 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Changes to the pad controller device tree binding have required that
>>> each lane be associated with a separate PHY.
>>
>> I still don't think this has anything to do with DT bindings. Rather, the
>> definition of a PHY (in HW and the Linux PHY subsystem) is a single lane.
>> That fact then requires drivers to support a PHY per lane rather than a
>> single multi-lane PHY, and equally means the DT bindings must be written
>> according to the correct definition of a PHY.
>>
>> Still, I suppose the commit description is fine as is.
>
> I've reworded the commit message to give a more accurate rationale for
> the change. I'll be posting a v5 soon.
>
>>> Update the PCI host bridge
>>> device tree binding to allow each root port to define the list of PHYs
>>> required to drive the lanes associated with it.
>>
>>> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
>>
>>> +Required properties for Tegra124 and later:
>>> +- phys: Must contain an phandle to a PHY for each entry in phy-names.
>>> +- phy-names: Must include an entry for each active lane. Note that the number
>>> +  of entries does not have to (though usually will) be equal to the specified
>>> +  number of lanes in the nvidia,num-lanes property. Entries are of the form
>>> +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
>>
>> When would the number of PHYs not equal the number of lanes? I thought the
>> whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just
>> misremembering some exception, so there may be no need to change this.
>
> This is useful to support the case where we want to connect a x1 or x2
> device to a root port that is configured to drive more lanes. It's a
> rather unusual configuration, but it would be possible for example to
> have an onboard x1 ethernet card, but the board layout is such that it
> runs in x1/x2 mode, with the ethernet card connected to the x2 port.

Does the controller HW actually work correctly in such a mode?

Obviously a fully initialized x4 controller has to correctly handle 
being attached solely to a x1 device. However, that's a different case 
to simply not initializing 3 of the 4 PHYs. It's plausible the 
controller handles this just fine, or that it hangs up or otherwise 
misbehaves if some of the PHYs aren't enabled and hence it can't even 
detect whether something is attached to them or not. Either way, adding 
your explanation into the binding would be useful to highlight the 
reason for the special case.

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding]

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

On Wed, Apr 13, 2016 at 11:04:56AM -0600, Stephen Warren wrote:
> On 04/13/2016 10:22 AM, Thierry Reding wrote:
> > On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote:
> > > On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Changes to the pad controller device tree binding have required that
> > > > each lane be associated with a separate PHY.
> > > 
> > > I still don't think this has anything to do with DT bindings. Rather, the
> > > definition of a PHY (in HW and the Linux PHY subsystem) is a single lane.
> > > That fact then requires drivers to support a PHY per lane rather than a
> > > single multi-lane PHY, and equally means the DT bindings must be written
> > > according to the correct definition of a PHY.
> > > 
> > > Still, I suppose the commit description is fine as is.
> > 
> > I've reworded the commit message to give a more accurate rationale for
> > the change. I'll be posting a v5 soon.
> > 
> > > > Update the PCI host bridge
> > > > device tree binding to allow each root port to define the list of PHYs
> > > > required to drive the lanes associated with it.
> > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> > > 
> > > > +Required properties for Tegra124 and later:
> > > > +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> > > > +- phy-names: Must include an entry for each active lane. Note that the number
> > > > +  of entries does not have to (though usually will) be equal to the specified
> > > > +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> > > > +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
> > > 
> > > When would the number of PHYs not equal the number of lanes? I thought the
> > > whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just
> > > misremembering some exception, so there may be no need to change this.
> > 
> > This is useful to support the case where we want to connect a x1 or x2
> > device to a root port that is configured to drive more lanes. It's a
> > rather unusual configuration, but it would be possible for example to
> > have an onboard x1 ethernet card, but the board layout is such that it
> > runs in x1/x2 mode, with the ethernet card connected to the x2 port.
> 
> Does the controller HW actually work correctly in such a mode?

I think it does, and up until a few minutes ago I was even sure that I
had tested it once. But looking at the various boards that I have I
don't think I actually have test equipment that's wired the proper way
to test this.

> Obviously a fully initialized x4 controller has to correctly handle being
> attached solely to a x1 device. However, that's a different case to simply
> not initializing 3 of the 4 PHYs. It's plausible the controller handles this
> just fine, or that it hangs up or otherwise misbehaves if some of the PHYs
> aren't enabled and hence it can't even detect whether something is attached
> to them or not. Either way, adding your explanation into the binding would
> be useful to highlight the reason for the special case.

Perhaps for now it would be better to make the binding stricter. The
wording could be relaxed if we ever determine that it still works
correctly with a number of PHYs smaller than the number of lanes.

Thierry

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

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding]

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

On Thu, Apr 14, 2016 at 05:29:05PM +0200, Thierry Reding wrote:
> On Wed, Apr 13, 2016 at 11:04:56AM -0600, Stephen Warren wrote:
> > On 04/13/2016 10:22 AM, Thierry Reding wrote:
> > > On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote:
> > > > On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > Changes to the pad controller device tree binding have required that
> > > > > each lane be associated with a separate PHY.
> > > > 
> > > > I still don't think this has anything to do with DT bindings. Rather, the
> > > > definition of a PHY (in HW and the Linux PHY subsystem) is a single lane.
> > > > That fact then requires drivers to support a PHY per lane rather than a
> > > > single multi-lane PHY, and equally means the DT bindings must be written
> > > > according to the correct definition of a PHY.
> > > > 
> > > > Still, I suppose the commit description is fine as is.
> > > 
> > > I've reworded the commit message to give a more accurate rationale for
> > > the change. I'll be posting a v5 soon.
> > > 
> > > > > Update the PCI host bridge
> > > > > device tree binding to allow each root port to define the list of PHYs
> > > > > required to drive the lanes associated with it.
> > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> > > > 
> > > > > +Required properties for Tegra124 and later:
> > > > > +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> > > > > +- phy-names: Must include an entry for each active lane. Note that the number
> > > > > +  of entries does not have to (though usually will) be equal to the specified
> > > > > +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> > > > > +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
> > > > 
> > > > When would the number of PHYs not equal the number of lanes? I thought the
> > > > whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just
> > > > misremembering some exception, so there may be no need to change this.
> > > 
> > > This is useful to support the case where we want to connect a x1 or x2
> > > device to a root port that is configured to drive more lanes. It's a
> > > rather unusual configuration, but it would be possible for example to
> > > have an onboard x1 ethernet card, but the board layout is such that it
> > > runs in x1/x2 mode, with the ethernet card connected to the x2 port.
> > 
> > Does the controller HW actually work correctly in such a mode?
> 
> I think it does, and up until a few minutes ago I was even sure that I
> had tested it once. But looking at the various boards that I have I
> don't think I actually have test equipment that's wired the proper way
> to test this.
> 
> > Obviously a fully initialized x4 controller has to correctly handle being
> > attached solely to a x1 device. However, that's a different case to simply
> > not initializing 3 of the 4 PHYs. It's plausible the controller handles this
> > just fine, or that it hangs up or otherwise misbehaves if some of the PHYs
> > aren't enabled and hence it can't even detect whether something is attached
> > to them or not. Either way, adding your explanation into the binding would
> > be useful to highlight the reason for the special case.
> 
> Perhaps for now it would be better to make the binding stricter. The
> wording could be relaxed if we ever determine that it still works
> correctly with a number of PHYs smaller than the number of lanes.

Going over the patches again I realized that Jetson TK1 is actually one
such case. The PCI host bridge controller is configured to run root port
0 using two lanes, and root port 1 using one lane. However, only one
lane is connected to each port. Root port 0 goes to the miniPCIe slot,
which takes a single lane (PEX4), while root port 1 goes to the onboard
NIC, which takes a single lane (PEX2) as well. x1 + x1 is an unsupported
configuration for the root complex, though, hence why it is configured
to be x2 + x1.

I've tested that both the onboard NIC as well as a miniPCIe card work
correctly with the setup.

So I think the wording in the binding, as well as the example, are
correct. So I left the original wording in place, but instead added a
couple more examples so that we have one per SoC generation, which will
hopefully clarify what properties should and shouldn't be used.

Thierry

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

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Thierry Reding]

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

On Thu, Apr 14, 2016 at 05:29:05PM +0200, Thierry Reding wrote:
> On Wed, Apr 13, 2016 at 11:04:56AM -0600, Stephen Warren wrote:
> > On 04/13/2016 10:22 AM, Thierry Reding wrote:
> > > On Wed, Mar 16, 2016 at 10:51:58AM -0600, Stephen Warren wrote:
> > > > On 03/08/2016 08:48 AM, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > 
> > > > > Changes to the pad controller device tree binding have required that
> > > > > each lane be associated with a separate PHY.
> > > > 
> > > > I still don't think this has anything to do with DT bindings. Rather, the
> > > > definition of a PHY (in HW and the Linux PHY subsystem) is a single lane.
> > > > That fact then requires drivers to support a PHY per lane rather than a
> > > > single multi-lane PHY, and equally means the DT bindings must be written
> > > > according to the correct definition of a PHY.
> > > > 
> > > > Still, I suppose the commit description is fine as is.
> > > 
> > > I've reworded the commit message to give a more accurate rationale for
> > > the change. I'll be posting a v5 soon.
> > > 
> > > > > Update the PCI host bridge
> > > > > device tree binding to allow each root port to define the list of PHYs
> > > > > required to drive the lanes associated with it.
> > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> > > > 
> > > > > +Required properties for Tegra124 and later:
> > > > > +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> > > > > +- phy-names: Must include an entry for each active lane. Note that the number
> > > > > +  of entries does not have to (though usually will) be equal to the specified
> > > > > +  number of lanes in the nvidia,num-lanes property. Entries are of the form
> > > > > +  "pcie-N": where N ranges from 0 to the value specified in nvidia,num-lanes.
> > > > 
> > > > When would the number of PHYs not equal the number of lanes? I thought the
> > > > whole point of this patch was to switch to per-lane PHYs? Perhaps I'm just
> > > > misremembering some exception, so there may be no need to change this.
> > > 
> > > This is useful to support the case where we want to connect a x1 or x2
> > > device to a root port that is configured to drive more lanes. It's a
> > > rather unusual configuration, but it would be possible for example to
> > > have an onboard x1 ethernet card, but the board layout is such that it
> > > runs in x1/x2 mode, with the ethernet card connected to the x2 port.
> > 
> > Does the controller HW actually work correctly in such a mode?
> 
> I think it does, and up until a few minutes ago I was even sure that I
> had tested it once. But looking at the various boards that I have I
> don't think I actually have test equipment that's wired the proper way
> to test this.
> 
> > Obviously a fully initialized x4 controller has to correctly handle being
> > attached solely to a x1 device. However, that's a different case to simply
> > not initializing 3 of the 4 PHYs. It's plausible the controller handles this
> > just fine, or that it hangs up or otherwise misbehaves if some of the PHYs
> > aren't enabled and hence it can't even detect whether something is attached
> > to them or not. Either way, adding your explanation into the binding would
> > be useful to highlight the reason for the special case.
> 
> Perhaps for now it would be better to make the binding stricter. The
> wording could be relaxed if we ever determine that it still works
> correctly with a number of PHYs smaller than the number of lanes.

Going over the patches again I realized that Jetson TK1 is actually one
such case. The PCI host bridge controller is configured to run root port
0 using two lanes, and root port 1 using one lane. However, only one
lane is connected to each port. Root port 0 goes to the miniPCIe slot,
which takes a single lane (PEX4), while root port 1 goes to the onboard
NIC, which takes a single lane (PEX2) as well. x1 + x1 is an unsupported
configuration for the root complex, though, hence why it is configured
to be x2 + x1.

I've tested that both the onboard NIC as well as a miniPCIe card work
correctly with the setup.

So I think the wording in the binding, as well as the example, are
correct. So I left the original wording in place, but instead added a
couple more examples so that we have one per SoC generation, which will
hopefully clarify what properties should and shouldn't be used.

Thierry

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

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Rob Herring]

On Tue, Mar 08, 2016 at 04:48:13PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Changes to the pad controller device tree binding have required that
> each lane be associated with a separate PHY. Update the PCI host bridge
> device tree binding to allow each root port to define the list of PHYs
> required to drive the lanes associated with it.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/pci/nvidia,tegra20-pcie.txt    | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] dt-bindings: pci: tegra: Update for per-lane PHYs

[Rob Herring]

On Tue, Mar 08, 2016 at 04:48:13PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Changes to the pad controller device tree binding have required that
> each lane be associated with a separate PHY. Update the PCI host bridge
> device tree binding to allow each root port to define the list of PHYs
> required to drive the lanes associated with it.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/pci/nvidia,tegra20-pcie.txt    | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>