diff mbox

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

Message ID 1460131994-24493-1-git-send-email-thierry.reding@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thierry Reding April 8, 2016, 4:13 p.m. UTC
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.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- add additional lanes subnode when dereferencing PHYs from the XUSB pad
  controller to reflect changes in its binding

 .../devicetree/bindings/pci/nvidia,tegra20-pcie.txt    | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas April 8, 2016, 4:48 p.m. UTC | #1
Hi Thierry,

On Fri, Apr 08, 2016 at 06:13:13PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Changes to the pad controller device tree binding 

Is there a commit we can reference for this pad controller binding
change?  If so, I can fold it in.

Is there a benefit to keeping this binding patch separate from the
corresponding driver patch?  It seems like it'd nice to have a
connection between them.  (Sorry, I'm sure I've asked this before,
but I can't remember the answer.)

Trivial typos below that I've already fixed, but you can fold them in
if there's any reason to repost this.

> 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.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v4:
> - add additional lanes subnode when dereferencing PHYs from the XUSB pad
>   controller to reflect changes in its binding
> 
>  .../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..f5364084b494 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

s/define/defined/

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

s/an phandle/a phandle/

> +- 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/lanes/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/lanes/pcie-2}>;
> +			phy-names = "pcie-0";
>  		};
>  	};
>  
> -- 
> 2.8.0
> 
> --
> 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
--
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
Bjorn Helgaas April 8, 2016, 7:11 p.m. UTC | #2
Hi Thierry,

I have a couple more questions, probably just because I'm
DT-illiterate.

On Fri, Apr 08, 2016 at 06:13:13PM +0200, 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.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v4:
> - add additional lanes subnode when dereferencing PHYs from the XUSB pad
>   controller to reflect changes in its binding
> 
>  .../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..f5364084b494 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:

I had a little trouble disambiguating this from the "Required
properties on Tegra124 and later (deprecated)" line above.  It might
help if they said:

  Required PCIe controller properties on Tegra124 and later (deprecated):
  Required PCIe Root Port properties for Tegra124 and later:

I'm not sure how to interpret the "deprecated" part.  Assume I'm
writing a DTS.  What am I supposed to include?

  - "phys" and "phy-names" under the PCIe controller *and*
    "phys" and "phy-names" under the Root Port?

  - "phys" and "phy-names" under the PCIe controller only if I don't
    supply "phys" and "phy-names" under the Root Port?

My guess is that a board with more than one PHY for PCIe should omit
"phys" and "phy-names" under the PCIe controller and include them
under each Root Port.  And a board with only one PHY could conceivably
supply these properties either under the controller or the Root Port
or both.

> +- 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/lanes/pcie-4}>;
> +			phy-names = "pcie-0";

I'm also a little confused here because it looks like this root port
supports two lanes, but there's only one entry in phy-names.  I
thought you needed one entry for each lane.

>  		};
>  
>  		pci@2,0 {
> @@ -183,6 +196,9 @@ SoC DTSI:
>  			ranges;
>  
>  			nvidia,num-lanes = <2>;
> +
> +			phys = <&{/padctl@0,7009f000/pads/pcie/lanes/pcie-2}>;
> +			phy-names = "pcie-0";
>  		};
>  	};
>  
> -- 
> 2.8.0
> 
> --
> 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
--
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
Thierry Reding April 11, 2016, 10:23 a.m. UTC | #3
On Fri, Apr 08, 2016 at 11:48:14AM -0500, Bjorn Helgaas wrote:
> Hi Thierry,
> 
> On Fri, Apr 08, 2016 at 06:13:13PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Changes to the pad controller device tree binding 
> 
> Is there a commit we can reference for this pad controller binding
> change?  If so, I can fold it in.

It hasn't been merged yet. Technically this patch is a prerequisite for
getting the new binding merged. And there's a specific order that needs
to be observed for this not to regress (albeit only temporarily).

The correct sequence for merging this would be:

	1) XUSB pad controller binding & driver updates
	2) PCI driver updates
	3) DT changes that implement the new binding

Because of the above I'd prefer to merge everything through the Tegra
tree. If that's okay with you, I can provide stable branches once its
all in place for you to merge back into the PCI tree.

> Is there a benefit to keeping this binding patch separate from the
> corresponding driver patch?  It seems like it'd nice to have a
> connection between them.  (Sorry, I'm sure I've asked this before,
> but I can't remember the answer.)

I've seen both approaches applied. I think separate patches is (or was?)
preferred because eventually the DT bindings were supposed to get moved
to a separate repository. Having separate patches would make the
transition easier by making it possible to cherry-pick between trees.

Thierry
Thierry Reding April 11, 2016, 11:26 a.m. UTC | #4
On Fri, Apr 08, 2016 at 02:11:42PM -0500, Bjorn Helgaas wrote:
> Hi Thierry,
> 
> I have a couple more questions, probably just because I'm
> DT-illiterate.
> 
> On Fri, Apr 08, 2016 at 06:13:13PM +0200, 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.
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v4:
> > - add additional lanes subnode when dereferencing PHYs from the XUSB pad
> >   controller to reflect changes in its binding
> > 
> >  .../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..f5364084b494 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:
> 
> I had a little trouble disambiguating this from the "Required
> properties on Tegra124 and later (deprecated)" line above.  It might
> help if they said:
> 
>   Required PCIe controller properties on Tegra124 and later (deprecated):
>   Required PCIe Root Port properties for Tegra124 and later:

That's kind of implied by the structure of the binding. The first is in
the section dedicated to the description of the host bridge controller,
whereas the second in in the section for root port properties. Granted,
the format for device tree bindings isn't very rich, so this isn't very
easy to spot. Perhaps something like this could help:

	NVIDIA Tegra PCIe controller

	The top-level device tree node describes the host bridge
	controller and properties listed therein apply to the controller
	as a whole.

	Required properties:
	...

	Root Ports:
	-----------

	Root ports are defined as subnodes of the PCIe controller node.

	Required properties:
	...

Would that help clarify?

> I'm not sure how to interpret the "deprecated" part.  Assume I'm
> writing a DTS.  What am I supposed to include?
> 
>   - "phys" and "phy-names" under the PCIe controller *and*
>     "phys" and "phy-names" under the Root Port?
> 
>   - "phys" and "phy-names" under the PCIe controller only if I don't
>     supply "phys" and "phy-names" under the Root Port?
> 
> My guess is that a board with more than one PHY for PCIe should omit
> "phys" and "phy-names" under the PCIe controller and include them
> under each Root Port.  And a board with only one PHY could conceivably
> supply these properties either under the controller or the Root Port
> or both.

Whatever is marked as deprecated should not be used in new bindings at
all. That is, new board files should move to per-lane PHYs, no matter
how many lanes they enable.

The XUSB pad controller that provides these PHYs can be driven by two
different drivers: a pinctrl driver that exposes a single PHY for all
PCI lanes (it uses per-lane nvidia,iddq properties to enable or disable
each of the lanes) or a PHY driver that exposes one PHY per lane. Each
PHY can separately be powered on.

Perhaps a better approach would be to simply remove the deprecated
properties to avoid any possible confusion. It does have the drawback
that old device trees won't have a documentation counterpart.

> > +- 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/lanes/pcie-4}>;
> > +			phy-names = "pcie-0";
> 
> I'm also a little confused here because it looks like this root port
> supports two lanes, but there's only one entry in phy-names.  I
> thought you needed one entry for each lane.

Not necessarily. I'm not sure if the wording in the description makes it
clear, but there are a limited number of configurations that the root
ports support. However the ports may not want (or need) to drive all of
the lanes that they support. Consider for example one possible
configuration on Tegra124, which is to have one root port operate in x1
mode and the other in x2 mode. The x2 port may have a fixed card wired
to it (onboard) that is x1. Enabling the second lane would be wasting
power because it is never used.

Thierry
Stephen Warren April 11, 2016, 4:55 p.m. UTC | #5
On 04/11/2016 05:26 AM, Thierry Reding wrote:
> On Fri, Apr 08, 2016 at 02:11:42PM -0500, Bjorn Helgaas wrote:
>> Hi Thierry,
>>
>> I have a couple more questions, probably just because I'm
>> DT-illiterate.
>>
>> On Fri, Apr 08, 2016 at 06:13:13PM +0200, 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.

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

>>> -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:
>>
>> I had a little trouble disambiguating this from the "Required
>> properties on Tegra124 and later (deprecated)" line above.  It might
>> help if they said:
>>
>>    Required PCIe controller properties on Tegra124 and later (deprecated):
>>    Required PCIe Root Port properties for Tegra124 and later:
...
>> I'm not sure how to interpret the "deprecated" part.  Assume I'm
>> writing a DTS.  What am I supposed to include?
>>
>>    - "phys" and "phy-names" under the PCIe controller *and*
>>      "phys" and "phy-names" under the Root Port?
>>
>>    - "phys" and "phy-names" under the PCIe controller only if I don't
>>      supply "phys" and "phy-names" under the Root Port?
>>
>> My guess is that a board with more than one PHY for PCIe should omit
>> "phys" and "phy-names" under the PCIe controller and include them
>> under each Root Port.  And a board with only one PHY could conceivably
>> supply these properties either under the controller or the Root Port
>> or both.
>
> Whatever is marked as deprecated should not be used in new bindings at
> all. That is, new board files should move to per-lane PHYs, no matter
> how many lanes they enable.
>
> The XUSB pad controller that provides these PHYs can be driven by two
> different drivers: a pinctrl driver that exposes a single PHY for all
> PCI lanes (it uses per-lane nvidia,iddq properties to enable or disable
> each of the lanes) or a PHY driver that exposes one PHY per lane. Each
> PHY can separately be powered on.
>
> Perhaps a better approach would be to simply remove the deprecated
> properties to avoid any possible confusion. It does have the drawback
> that old device trees won't have a documentation counterpart.

I recall requests during past DT binding reviews that deprecated 
properties still be documented. That allows legacy bindings and driver 
support for them to be understood, since they're still documented.

--
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
Stephen Warren April 11, 2016, 5:38 p.m. UTC | #6
On 04/08/2016 10:13 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. 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.

I think the feedback I gave on v3 still applies here (I'm talking about 
the comments on the patch, not the commit description).

http://www.spinics.net/lists/linux-pci/msg49718.html

--
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
Thierry Reding April 13, 2016, 4:23 p.m. UTC | #7
On Mon, Apr 11, 2016 at 11:38:49AM -0600, Stephen Warren wrote:
> On 04/08/2016 10:13 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. 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.
> 
> I think the feedback I gave on v3 still applies here (I'm talking about the
> comments on the patch, not the commit description).
> 
> http://www.spinics.net/lists/linux-pci/msg49718.html

Apologies, I had missed those when going over review comments. I think
I've addressed all of them now, or provided rationale for why the code
and binding are the way they are.

I'll be sending out a v5 soon, hopefully that will have addressed any
outstanding concerns.

Thierry
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
index 75321ae23c08..f5364084b494 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/lanes/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/lanes/pcie-2}>;
+			phy-names = "pcie-0";
 		};
 	};