diff mbox

[RFC,6/6] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

Message ID 1500004366-241633-5-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin July 14, 2017, 3:52 a.m. UTC
Deprecate legacy PHY model and encourage to use per-lane PHY
model.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

 .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Brian Norris July 14, 2017, 8:47 p.m. UTC | #1
Hi Shawn,

On Fri, Jul 14, 2017 at 11:52:46AM +0800, Shawn Lin wrote:
> Deprecate legacy PHY model and encourage to use per-lane PHY
> model.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index 1453a73..78d4469 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -19,8 +19,6 @@ Required properties:
>  	- "pm"
>  - msi-map: Maps a Requester ID to an MSI controller and associated
>  	msi-specifier data. See ./pci-msi.txt
> -- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
> -- phy-names:  MUST be "pcie-phy".
>  - interrupts: Three interrupt entries must be specified.
>  - interrupt-names: Must include the following names
>  	- "sys"
> @@ -42,6 +40,18 @@ Required properties:
>  	interrupt source. The value must be 1.
>  - interrupt-map-mask and interrupt-map: standard PCI properties
>  
> +Required properties for legacy PHY model (deprecated):
> +- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
> +- phy-names:  MUST be "pcie-phy".
> +
> +Required properties for per-lane PHY model:
> +- 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 num-lanes property. Entries are of the form
> +  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
> +  (see example below)

So (for the non-legacy case) are you requiring listing all 4 PHY lanes,
or not? If you aren't, then you need to make the PHY driver handle the
case were lanes {X..3} were never "powered on", but we want them idled.
IIUC, your current (broken) implementation is trying (incorrectly) to
only idle a lane once it has been powered on and then back off. But that
won't ever happen if we only request (for example) PHY lane 0.

It's also misleading that it's possible to specify only some subset of
the PHY lanes, but the driver might still try to use them all.

Brian

> +
>  Optional Property:
>  - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
>  	using 24MHz OSC for RC's PHY.
> @@ -95,6 +105,7 @@ pcie0: pcie@f8000000 {
>  		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
>  	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
>  		      "pm", "pclk", "aclk";
> +	/* deprecated legacy PHY model */
>  	phys = <&pcie_phy>;
>  	phy-names = "pcie-phy";
>  	pinctrl-names = "default";
> @@ -111,3 +122,13 @@ pcie0: pcie@f8000000 {
>  		#interrupt-cells = <1>;
>  	};
>  };
> +
> +pcie0: pcie@f8000000 {
> +	...
> +
> +	/* preferred per-lane PHY model */
> +	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> +	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
> +
> +	...
> +};
> -- 
> 1.9.1
> 
>
Shawn Lin July 17, 2017, 3:25 a.m. UTC | #2
Hi Brian,

On 2017/7/15 4:47, Brian Norris wrote:
> Hi Shawn,
> 
> On Fri, Jul 14, 2017 at 11:52:46AM +0800, Shawn Lin wrote:
>> Deprecate legacy PHY model and encourage to use per-lane PHY
>> model.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>>   .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
>>   1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> index 1453a73..78d4469 100644
>> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> @@ -19,8 +19,6 @@ Required properties:
>>   	- "pm"
>>   - msi-map: Maps a Requester ID to an MSI controller and associated
>>   	msi-specifier data. See ./pci-msi.txt
>> -- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
>> -- phy-names:  MUST be "pcie-phy".
>>   - interrupts: Three interrupt entries must be specified.
>>   - interrupt-names: Must include the following names
>>   	- "sys"
>> @@ -42,6 +40,18 @@ Required properties:
>>   	interrupt source. The value must be 1.
>>   - interrupt-map-mask and interrupt-map: standard PCI properties
>>   
>> +Required properties for legacy PHY model (deprecated):
>> +- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
>> +- phy-names:  MUST be "pcie-phy".
>> +
>> +Required properties for per-lane PHY model:
>> +- 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 num-lanes property. Entries are of the form
>> +  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
>> +  (see example below)
> 
> So (for the non-legacy case) are you requiring listing all 4 PHY lanes,
> or not? 

I intended to do that however that makes the code more complex and I
didn't see any gains here. So I would say "list 4 of them here is
mandatory".

Will update this Doc to reflect the fact.

Thanks.

If you aren't, then you need to make the PHY driver handle the
> case were lanes {X..3} were never "powered on", but we want them idled.
> IIUC, your current (broken) implementation is trying (incorrectly) to
> only idle a lane once it has been powered on and then back off. But that
> won't ever happen if we only request (for example) PHY lane 0.
> 
> It's also misleading that it's possible to specify only some subset of
> the PHY lanes, but the driver might still try to use them al
> 
> Brian
> 
>> +
>>   Optional Property:
>>   - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
>>   	using 24MHz OSC for RC's PHY.
>> @@ -95,6 +105,7 @@ pcie0: pcie@f8000000 {
>>   		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
>>   	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
>>   		      "pm", "pclk", "aclk";
>> +	/* deprecated legacy PHY model */
>>   	phys = <&pcie_phy>;
>>   	phy-names = "pcie-phy";
>>   	pinctrl-names = "default";
>> @@ -111,3 +122,13 @@ pcie0: pcie@f8000000 {
>>   		#interrupt-cells = <1>;
>>   	};
>>   };
>> +
>> +pcie0: pcie@f8000000 {
>> +	...
>> +
>> +	/* preferred per-lane PHY model */
>> +	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
>> +	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
>> +
>> +	...
>> +};
>> -- 
>> 1.9.1
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 1453a73..78d4469 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -19,8 +19,6 @@  Required properties:
 	- "pm"
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
-- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
-- phy-names:  MUST be "pcie-phy".
 - interrupts: Three interrupt entries must be specified.
 - interrupt-names: Must include the following names
 	- "sys"
@@ -42,6 +40,18 @@  Required properties:
 	interrupt source. The value must be 1.
 - interrupt-map-mask and interrupt-map: standard PCI properties
 
+Required properties for legacy PHY model (deprecated):
+- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
+- phy-names:  MUST be "pcie-phy".
+
+Required properties for per-lane PHY model:
+- 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 num-lanes property. Entries are of the form
+  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
+  (see example below)
+
 Optional Property:
 - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
 	using 24MHz OSC for RC's PHY.
@@ -95,6 +105,7 @@  pcie0: pcie@f8000000 {
 		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
 	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
 		      "pm", "pclk", "aclk";
+	/* deprecated legacy PHY model */
 	phys = <&pcie_phy>;
 	phy-names = "pcie-phy";
 	pinctrl-names = "default";
@@ -111,3 +122,13 @@  pcie0: pcie@f8000000 {
 		#interrupt-cells = <1>;
 	};
 };
+
+pcie0: pcie@f8000000 {
+	...
+
+	/* preferred per-lane PHY model */
+	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
+	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
+
+	...
+};