[PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280

[PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280

[Prasad Malisetty]

This series includes PCIe controller and phy binding updates
for SC7280 SoC and DT changes for SC7280 SoC and SC7280 IDP board.

Prasad Malisetty (3):
  dt-bindings: pci: qcom: Document PCIe bindings for SC720
  arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board

 .../devicetree/bindings/pci/qcom,pcie.txt          |  17 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |  13 ++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 138 +++++++++++++++++++++
 3 files changed, 168 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Prasad Malisetty]

Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
to the one used on SM8250. Add the compatible for SC7280.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 0da458a..e5245ed 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -12,6 +12,7 @@
 			- "qcom,pcie-ipq4019" for ipq4019
 			- "qcom,pcie-ipq8074" for ipq8074
 			- "qcom,pcie-qcs404" for qcs404
+			- "qcom,pcie-sc7280" for sc7280
 			- "qcom,pcie-sdm845" for sdm845
 			- "qcom,pcie-sm8250" for sm8250
 
@@ -133,6 +134,22 @@
 			- "slave_bus"	AXI Slave clock
 
 - clock-names:
+	Usage: required for sc7280
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "aux"         Auxiliary clock
+			- "cfg"         Configuration clock
+			- "bus_master"  Master AXI clock
+			- "bus_slave"   Slave AXI clock
+			- "slave_q2a"   Slave Q2A clock
+			- "tbu"         PCIe TBU clock
+			- "ddrss_sf_tbu" PCIe SF TBU clock
+			- "pipe"        PIPE clock
+			- "pipe_src"    PIPE MUX
+			- "pipe_ext"    PIPE output clock
+			- "ref"         REFERENCE clock
+
+- clock-names:
 	Usage: required for sdm845
 	Value type: <stringlist>
 	Definition: Should contain the following entries
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Prasad Malisetty]

Add PCIe controller and PHY nodes for sc7280 SOC.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 +++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 2cc4785..a9f25fc1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -12,6 +12,7 @@
 #include <dt-bindings/power/qcom-aoss-qmp.h>
 #include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -316,6 +317,118 @@
 			};
 		};
 
+	pcie1: pci@1c08000 {
+			compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie";
+			reg = <0 0x01c08000 0 0x3000>,
+			      <0 0x40000000 0 0xf1d>,
+			      <0 0x40000f20 0 0xa8>,
+			      <0 0x40001000 0 0x1000>,
+			      <0 0x40100000 0 0x100000>;
+
+			reg-names = "parf", "dbi", "elbi", "atu", "config";
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x01000000 0x0 0x40200000 0x0 0x40200000 0x0 0x100000>,
+				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;
+
+			interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 434 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 435 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
+				 <&pcie1_lane 0>,
+				 <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_PCIE_1_AUX_CLK>,
+				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
+				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
+				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
+				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
+				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
+
+			clock-names = "pipe",
+				      "pipe_src",
+				      "pipe_ext",
+				      "ref",
+				      "aux",
+				      "cfg",
+				      "bus_master",
+				      "bus_slave",
+				      "slave_q2a",
+				      "tbu",
+				      "ddrss_sf_tbu";
+
+			assigned-clocks = <&gcc GCC_PCIE_1_AUX_CLK>;
+			assigned-clock-rates = <19200000>;
+
+			resets = <&gcc GCC_PCIE_1_BCR>;
+			reset-names = "pci";
+
+			power-domains = <&gcc GCC_PCIE_1_GDSC>;
+
+			phys = <&pcie1_lane>;
+			phy-names = "pciephy";
+
+			perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pcie1_default_state>;
+
+			iommus = <&apps_smmu 0x1c80 0x1>;
+
+			iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,
+				    <0x100 &apps_smmu 0x1c81 0x1>;
+
+			status = "disabled";
+		};
+
+		pcie1_phy: phy@1c0e000 {
+			compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";
+			reg = <0 0x01c0e000 0 0x1c0>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
+				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+				 <&gcc GCC_PCIE_CLKREF_EN>,
+				 <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
+			clock-names = "aux", "cfg_ahb", "ref", "refgen";
+
+			resets = <&gcc GCC_PCIE_1_PHY_BCR>;
+			reset-names = "phy";
+
+			assigned-clocks = <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
+			assigned-clock-rates = <100000000>;
+
+			status = "disabled";
+
+			pcie1_lane: lanes@1c0e200 {
+				reg = <0 0x1c0e200 0 0x170>, /* tx0 */
+				      <0 0x1c0e400 0 0x200>, /* rx0 */
+				      <0 0x1c0ea00 0 0x1f0>, /* pcs */
+				      <0 0x1c0e600 0 0x170>, /* tx1 */
+				      <0 0x1c0e800 0 0x200>, /* rx1 */
+				      <0 0x1c0ee00 0 0xf4>; /* "pcs_com" same as pcs_misc? */
+				clocks = <&rpmhcc RPMH_CXO_CLK>;
+				clock-names = "pipe0";
+
+				#phy-cells = <0>;
+				#clock-cells = <1>;
+				clock-output-names = "pcie_1_pipe_clk";
+			};
+		};
+
 		stm@6002000 {
 			compatible = "arm,coresight-stm", "arm,primecell";
 			reg = <0 0x06002000 0 0x1000>,
@@ -871,6 +984,31 @@
 				pins = "gpio46", "gpio47";
 				function = "qup13";
 			};
+
+			pcie1_default_state: pcie1-default {
+				clkreq {
+					pins = "gpio79";
+					function = "pcie1_clkreqn";
+					bias-pull-up;
+				};
+
+				reset-n {
+					pins = "gpio2";
+					function = "gpio";
+
+					drive-strength = <16>;
+					output-low;
+					bias-disable;
+				};
+
+				wake-n {
+					pins = "gpio3";
+					function = "gpio";
+
+					drive-strength = <2>;
+					bias-pull-up;
+				};
+			};
 		};
 
 		apps_smmu: iommu@15000000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board

[Prasad Malisetty]

Add PCIe nodes for sc7280 IDP board.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 54d2cb3..9105a74 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -234,6 +234,19 @@
 	};
 };
 
+&pcie1 {
+	status = "okay";
+
+	vdda-supply = <&vreg_l10c_0p8>;
+};
+
+&pcie1_phy {
+	status = "okay";
+
+	vdda-phy-supply = <&vreg_l10c_0p8>;
+	vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Stephen Boyd]

Quoting Prasad Malisetty (2021-05-07 03:17:26)
> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> to the one used on SM8250. Add the compatible for SC7280.
>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 0da458a..e5245ed 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -12,6 +12,7 @@
>                         - "qcom,pcie-ipq4019" for ipq4019
>                         - "qcom,pcie-ipq8074" for ipq8074
>                         - "qcom,pcie-qcs404" for qcs404
> +                       - "qcom,pcie-sc7280" for sc7280
>                         - "qcom,pcie-sdm845" for sdm845
>                         - "qcom,pcie-sm8250" for sm8250
>
> @@ -133,6 +134,22 @@
>                         - "slave_bus"   AXI Slave clock
>
>  - clock-names:
> +       Usage: required for sc7280
> +       Value type: <stringlist>
> +       Definition: Should contain the following entries
> +                       - "aux"         Auxiliary clock
> +                       - "cfg"         Configuration clock
> +                       - "bus_master"  Master AXI clock
> +                       - "bus_slave"   Slave AXI clock
> +                       - "slave_q2a"   Slave Q2A clock
> +                       - "tbu"         PCIe TBU clock
> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
> +                       - "pipe"        PIPE clock
> +                       - "pipe_src"    PIPE MUX

Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
remove it and do whatever is necessary on the pipe clk instead of the
parent of the clk.

> +                       - "pipe_ext"    PIPE output clock

Is pipe output different from pipe?

> +                       - "ref"         REFERENCE clock
> +
>

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Stephen Boyd]

Quoting Prasad Malisetty (2021-05-07 03:17:27)
> Add PCIe controller and PHY nodes for sc7280 SOC.
>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 +++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 2cc4785..a9f25fc1 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -12,6 +12,7 @@
>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>  #include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +#include <dt-bindings/gpio/gpio.h>
>
>  / {
>         interrupt-parent = <&intc>;
> @@ -316,6 +317,118 @@
>                         };
>                 };
>
[...]
> +
> +               pcie1_phy: phy@1c0e000 {
> +                       compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";
> +                       reg = <0 0x01c0e000 0 0x1c0>;
> +                       #address-cells = <2>;
> +                       #size-cells = <2>;
> +                       ranges;
> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       clock-names = "aux", "cfg_ahb", "ref", "refgen";
> +
> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> +                       reset-names = "phy";
> +
> +                       assigned-clocks = <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       assigned-clock-rates = <100000000>;
> +
> +                       status = "disabled";

I think the style is to put status disabled close to the compatible?

> +
> +                       pcie1_lane: lanes@1c0e200 {
> +                               reg = <0 0x1c0e200 0 0x170>, /* tx0 */

Please pad reg addresses to 8 characters.

> +                                     <0 0x1c0e400 0 0x200>, /* rx0 */
> +                                     <0 0x1c0ea00 0 0x1f0>, /* pcs */
> +                                     <0 0x1c0e600 0 0x170>, /* tx1 */
> +                                     <0 0x1c0e800 0 0x200>, /* rx1 */
> +                                     <0 0x1c0ee00 0 0xf4>; /* "pcs_com" same as pcs_misc? */

Is this a TODO? I'd prefer all the comments on the reg properties to be
removed.

> +                               clocks = <&rpmhcc RPMH_CXO_CLK>;
> +                               clock-names = "pipe0";
> +
> +                               #phy-cells = <0>;
> +                               #clock-cells = <1>;
> +                               clock-output-names = "pcie_1_pipe_clk";
> +                       };
> +               };
> +
>                 stm@6002000 {
>                         compatible = "arm,coresight-stm", "arm,primecell";
>                         reg = <0 0x06002000 0 0x1000>,
> @@ -871,6 +984,31 @@
>                                 pins = "gpio46", "gpio47";
>                                 function = "qup13";
>                         };
> +
> +                       pcie1_default_state: pcie1-default {
> +                               clkreq {
> +                                       pins = "gpio79";
> +                                       function = "pcie1_clkreqn";
> +                                       bias-pull-up;

Move this bias-pull-up to the idp file?

> +                               };
> +
> +                               reset-n {
> +                                       pins = "gpio2";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <16>;
> +                                       output-low;
> +                                       bias-disable;
> +                               };
> +
> +                               wake-n {
> +                                       pins = "gpio3";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <2>;
> +                                       bias-pull-up;
> +                               };

These last two nodes with the pull-up and drive-strength settings should
be in the board files, like the idp one, instead of here in the SoC
file. That way board designers can take the SoC and connect the pcie to
an external device using these pins and set the configuration they want
on these pins, or choose not to connect them to the SoC at all and use
those pins for something else.

In addition, it looks like the reset could be a reset-gpios property
instead of an output-low config.

> +                       };
>                 };
>

Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Rob Herring]

On Fri, 07 May 2021 15:47:26 +0530, Prasad Malisetty wrote:
> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> to the one used on SM8250. Add the compatible for SC7280.
> 
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

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

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Prasad Malisetty]

On 2021-05-08 01:36, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-05-07 03:17:27)
>> Add PCIe controller and PHY nodes for sc7280 SOC.
>> 
>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 
>> +++++++++++++++++++++++++++++++++++
>>  1 file changed, 138 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 2cc4785..a9f25fc1 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -12,6 +12,7 @@
>>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>>  #include <dt-bindings/power/qcom-rpmpd.h>
>>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> 
>>  / {
>>         interrupt-parent = <&intc>;
>> @@ -316,6 +317,118 @@
>>                         };
>>                 };
>> 
> [...]
>> +
>> +               pcie1_phy: phy@1c0e000 {
>> +                       compatible = 
>> "qcom,sm8250-qmp-gen3x2-pcie-phy";
>> +                       reg = <0 0x01c0e000 0 0x1c0>;
>> +                       #address-cells = <2>;
>> +                       #size-cells = <2>;
>> +                       ranges;
>> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>> +                                <&gcc GCC_PCIE_CLKREF_EN>,
>> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
>> +                       clock-names = "aux", "cfg_ahb", "ref", 
>> "refgen";
>> +
>> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>> +                       reset-names = "phy";
>> +
>> +                       assigned-clocks = <&gcc 
>> GCC_PCIE1_PHY_RCHNG_CLK>;
>> +                       assigned-clock-rates = <100000000>;
>> +
>> +                       status = "disabled";
> 
> I think the style is to put status disabled close to the compatible?

Generally I have added status disabled in end as like many nodes. just 
curious to ask is there any specific reason to put close to compatible.
> 
>> +
>> +                       pcie1_lane: lanes@1c0e200 {
>> +                               reg = <0 0x1c0e200 0 0x170>, /* tx0 */
> 
> Please pad reg addresses to 8 characters.

Done
> 
>> +                                     <0 0x1c0e400 0 0x200>, /* rx0 */
>> +                                     <0 0x1c0ea00 0 0x1f0>, /* pcs */
>> +                                     <0 0x1c0e600 0 0x170>, /* tx1 */
>> +                                     <0 0x1c0e800 0 0x200>, /* rx1 */
>> +                                     <0 0x1c0ee00 0 0xf4>; /* 
>> "pcs_com" same as pcs_misc? */
> 
> Is this a TODO? I'd prefer all the comments on the reg properties to be
> removed.
> 
Done
>> +                               clocks = <&rpmhcc RPMH_CXO_CLK>;
>> +                               clock-names = "pipe0";
>> +
>> +                               #phy-cells = <0>;
>> +                               #clock-cells = <1>;
>> +                               clock-output-names = 
>> "pcie_1_pipe_clk";
>> +                       };
>> +               };
>> +
>>                 stm@6002000 {
>>                         compatible = "arm,coresight-stm", 
>> "arm,primecell";
>>                         reg = <0 0x06002000 0 0x1000>,
>> @@ -871,6 +984,31 @@
>>                                 pins = "gpio46", "gpio47";
>>                                 function = "qup13";
>>                         };
>> +
>> +                       pcie1_default_state: pcie1-default {
>> +                               clkreq {
>> +                                       pins = "gpio79";
>> +                                       function = "pcie1_clkreqn";
>> +                                       bias-pull-up;
> 
> Move this bias-pull-up to the idp file?

Done
> 
>> +                               };
>> +
>> +                               reset-n {
>> +                                       pins = "gpio2";
>> +                                       function = "gpio";
>> +
>> +                                       drive-strength = <16>;
>> +                                       output-low;
>> +                                       bias-disable;
>> +                               };
>> +
>> +                               wake-n {
>> +                                       pins = "gpio3";
>> +                                       function = "gpio";
>> +
>> +                                       drive-strength = <2>;
>> +                                       bias-pull-up;
>> +                               };
> 
> These last two nodes with the pull-up and drive-strength settings 
> should
> be in the board files, like the idp one, instead of here in the SoC
> file. That way board designers can take the SoC and connect the pcie to
> an external device using these pins and set the configuration they want
> on these pins, or choose not to connect them to the SoC at all and use
> those pins for something else.
> 
> In addition, it looks like the reset could be a reset-gpios property
> instead of an output-low config.
> 
we are using reset property as perst gpio in pcie node.
>> +                       };
>>                 };
>> 

Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Prasad Malisetty]

On 2021-05-08 01:29, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-05-07 03:17:26)
>> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
>> to the one used on SM8250. Add the compatible for SC7280.
>> 
>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 
>> +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt 
>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> index 0da458a..e5245ed 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> @@ -12,6 +12,7 @@
>>                         - "qcom,pcie-ipq4019" for ipq4019
>>                         - "qcom,pcie-ipq8074" for ipq8074
>>                         - "qcom,pcie-qcs404" for qcs404
>> +                       - "qcom,pcie-sc7280" for sc7280
>>                         - "qcom,pcie-sdm845" for sdm845
>>                         - "qcom,pcie-sm8250" for sm8250
>> 
>> @@ -133,6 +134,22 @@
>>                         - "slave_bus"   AXI Slave clock
>> 
>>  - clock-names:
>> +       Usage: required for sc7280
>> +       Value type: <stringlist>
>> +       Definition: Should contain the following entries
>> +                       - "aux"         Auxiliary clock
>> +                       - "cfg"         Configuration clock
>> +                       - "bus_master"  Master AXI clock
>> +                       - "bus_slave"   Slave AXI clock
>> +                       - "slave_q2a"   Slave Q2A clock
>> +                       - "tbu"         PCIe TBU clock
>> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
>> +                       - "pipe"        PIPE clock
>> +                       - "pipe_src"    PIPE MUX
> 
> Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
> remove it and do whatever is necessary on the pipe clk instead of the
> parent of the clk.

Here pipe_src is MUX. Newer targets require changing pipe-clk mux to 
switch between pipe_clk and XO for GDSC enable.
After PHY init, need to configure MUX.
> 
>> +                       - "pipe_ext"    PIPE output clock
> 
> Is pipe output different from pipe?
> 
Yes, pipe_ext clock will generate after PHY init.
>> +                       - "ref"         REFERENCE clock
>> +
>> 

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Stephen Boyd]

Quoting Prasad Malisetty (2021-05-21 02:57:00)
> On 2021-05-08 01:36, Stephen Boyd wrote:
> > Quoting Prasad Malisetty (2021-05-07 03:17:27)
> >> Add PCIe controller and PHY nodes for sc7280 SOC.
> >>
> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
> >> +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 138 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> index 2cc4785..a9f25fc1 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> @@ -12,6 +12,7 @@
> >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
> >>  #include <dt-bindings/power/qcom-rpmpd.h>
> >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> >> +#include <dt-bindings/gpio/gpio.h>
> >>
> >>  / {
> >>         interrupt-parent = <&intc>;
> >> @@ -316,6 +317,118 @@
> >>                         };
> >>                 };
> >>
> > [...]
> >> +
> >> +               pcie1_phy: phy@1c0e000 {
> >> +                       compatible =
> >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
> >> +                       reg = <0 0x01c0e000 0 0x1c0>;
> >> +                       #address-cells = <2>;
> >> +                       #size-cells = <2>;
> >> +                       ranges;
> >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> >> +                       clock-names = "aux", "cfg_ahb", "ref",
> >> "refgen";
> >> +
> >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> >> +                       reset-names = "phy";
> >> +
> >> +                       assigned-clocks = <&gcc
> >> GCC_PCIE1_PHY_RCHNG_CLK>;
> >> +                       assigned-clock-rates = <100000000>;
> >> +
> >> +                       status = "disabled";
> >
> > I think the style is to put status disabled close to the compatible?
>
> Generally I have added status disabled in end as like many nodes. just
> curious to ask is there any specific reason to put close to compatible.

It's really up to qcom maintainers, which I am not.

> >> +                               };
> >> +
> >> +                               reset-n {
> >> +                                       pins = "gpio2";
> >> +                                       function = "gpio";
> >> +
> >> +                                       drive-strength = <16>;
> >> +                                       output-low;
> >> +                                       bias-disable;
> >> +                               };
> >> +
> >> +                               wake-n {
> >> +                                       pins = "gpio3";
> >> +                                       function = "gpio";
> >> +
> >> +                                       drive-strength = <2>;
> >> +                                       bias-pull-up;
> >> +                               };
> >
> > These last two nodes with the pull-up and drive-strength settings
> > should
> > be in the board files, like the idp one, instead of here in the SoC
> > file. That way board designers can take the SoC and connect the pcie to
> > an external device using these pins and set the configuration they want
> > on these pins, or choose not to connect them to the SoC at all and use
> > those pins for something else.
> >
> > In addition, it looks like the reset could be a reset-gpios property
> > instead of an output-low config.
> >
> we are using reset property as perst gpio in pcie node.

Ok, perst-gpios should be fine. Presumably perst-gpios should be in the
board and not in the SoC because of what I wrote up above.

Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Stephen Boyd]

Quoting Prasad Malisetty (2021-06-04 04:26:57)
> On 2021-05-08 01:29, Stephen Boyd wrote:
> > Quoting Prasad Malisetty (2021-05-07 03:17:26)
> >> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> >> to the one used on SM8250. Add the compatible for SC7280.
> >>
> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> >> ---
> >>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17
> >> +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> index 0da458a..e5245ed 100644
> >> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> @@ -12,6 +12,7 @@
> >>                         - "qcom,pcie-ipq4019" for ipq4019
> >>                         - "qcom,pcie-ipq8074" for ipq8074
> >>                         - "qcom,pcie-qcs404" for qcs404
> >> +                       - "qcom,pcie-sc7280" for sc7280
> >>                         - "qcom,pcie-sdm845" for sdm845
> >>                         - "qcom,pcie-sm8250" for sm8250
> >>
> >> @@ -133,6 +134,22 @@
> >>                         - "slave_bus"   AXI Slave clock
> >>
> >>  - clock-names:
> >> +       Usage: required for sc7280
> >> +       Value type: <stringlist>
> >> +       Definition: Should contain the following entries
> >> +                       - "aux"         Auxiliary clock
> >> +                       - "cfg"         Configuration clock
> >> +                       - "bus_master"  Master AXI clock
> >> +                       - "bus_slave"   Slave AXI clock
> >> +                       - "slave_q2a"   Slave Q2A clock
> >> +                       - "tbu"         PCIe TBU clock
> >> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
> >> +                       - "pipe"        PIPE clock
> >> +                       - "pipe_src"    PIPE MUX
> >
> > Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
> > remove it and do whatever is necessary on the pipe clk instead of the
> > parent of the clk.
>
> Here pipe_src is MUX. Newer targets require changing pipe-clk mux to
> switch between pipe_clk and XO for GDSC enable.
> After PHY init, need to configure MUX.

Ok. I see, so we have to change the parent of the parent of the pipe
clk?

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Bjorn Andersson]

On Fri 04 Jun 16:43 CDT 2021, Stephen Boyd wrote:

> Quoting Prasad Malisetty (2021-05-21 02:57:00)
> > On 2021-05-08 01:36, Stephen Boyd wrote:
> > > Quoting Prasad Malisetty (2021-05-07 03:17:27)
> > >> Add PCIe controller and PHY nodes for sc7280 SOC.
> > >>
> > >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> > >> ---
> > >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
> > >> +++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 138 insertions(+)
> > >>
> > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> index 2cc4785..a9f25fc1 100644
> > >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> @@ -12,6 +12,7 @@
> > >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
> > >>  #include <dt-bindings/power/qcom-rpmpd.h>
> > >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> > >> +#include <dt-bindings/gpio/gpio.h>
> > >>
> > >>  / {
> > >>         interrupt-parent = <&intc>;
> > >> @@ -316,6 +317,118 @@
> > >>                         };
> > >>                 };
> > >>
> > > [...]
> > >> +
> > >> +               pcie1_phy: phy@1c0e000 {
> > >> +                       compatible =
> > >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
> > >> +                       reg = <0 0x01c0e000 0 0x1c0>;
> > >> +                       #address-cells = <2>;
> > >> +                       #size-cells = <2>;
> > >> +                       ranges;
> > >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> > >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> > >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> > >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> > >> +                       clock-names = "aux", "cfg_ahb", "ref",
> > >> "refgen";
> > >> +
> > >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> > >> +                       reset-names = "phy";
> > >> +
> > >> +                       assigned-clocks = <&gcc
> > >> GCC_PCIE1_PHY_RCHNG_CLK>;
> > >> +                       assigned-clock-rates = <100000000>;
> > >> +
> > >> +                       status = "disabled";
> > >
> > > I think the style is to put status disabled close to the compatible?
> >
> > Generally I have added status disabled in end as like many nodes. just
> > curious to ask is there any specific reason to put close to compatible.
> 
> It's really up to qcom maintainers, which I am not.
> 

I like when it's the last item, as it lends itself nicely to be
surrounded by empty lines and thereby easy to spot...

Regards,
Bjorn

> > >> +                               };
> > >> +
> > >> +                               reset-n {
> > >> +                                       pins = "gpio2";
> > >> +                                       function = "gpio";
> > >> +
> > >> +                                       drive-strength = <16>;
> > >> +                                       output-low;
> > >> +                                       bias-disable;
> > >> +                               };
> > >> +
> > >> +                               wake-n {
> > >> +                                       pins = "gpio3";
> > >> +                                       function = "gpio";
> > >> +
> > >> +                                       drive-strength = <2>;
> > >> +                                       bias-pull-up;
> > >> +                               };
> > >
> > > These last two nodes with the pull-up and drive-strength settings
> > > should
> > > be in the board files, like the idp one, instead of here in the SoC
> > > file. That way board designers can take the SoC and connect the pcie to
> > > an external device using these pins and set the configuration they want
> > > on these pins, or choose not to connect them to the SoC at all and use
> > > those pins for something else.
> > >
> > > In addition, it looks like the reset could be a reset-gpios property
> > > instead of an output-low config.
> > >
> > we are using reset property as perst gpio in pcie node.
> 
> Ok, perst-gpios should be fine. Presumably perst-gpios should be in the
> board and not in the SoC because of what I wrote up above.

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

[Prasad Malisetty]

On 2021-06-06 09:32, Bjorn Andersson wrote:
> On Fri 04 Jun 16:43 CDT 2021, Stephen Boyd wrote:
> 
>> Quoting Prasad Malisetty (2021-05-21 02:57:00)
>> > On 2021-05-08 01:36, Stephen Boyd wrote:
>> > > Quoting Prasad Malisetty (2021-05-07 03:17:27)
>> > >> Add PCIe controller and PHY nodes for sc7280 SOC.
>> > >>
>> > >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> > >> ---
>> > >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
>> > >> +++++++++++++++++++++++++++++++++++
>> > >>  1 file changed, 138 insertions(+)
>> > >>
>> > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> index 2cc4785..a9f25fc1 100644
>> > >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> @@ -12,6 +12,7 @@
>> > >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>> > >>  #include <dt-bindings/power/qcom-rpmpd.h>
>> > >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>> > >> +#include <dt-bindings/gpio/gpio.h>
>> > >>
>> > >>  / {
>> > >>         interrupt-parent = <&intc>;
>> > >> @@ -316,6 +317,118 @@
>> > >>                         };
>> > >>                 };
>> > >>
>> > > [...]
>> > >> +
>> > >> +               pcie1_phy: phy@1c0e000 {
>> > >> +                       compatible =
>> > >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
>> > >> +                       reg = <0 0x01c0e000 0 0x1c0>;
>> > >> +                       #address-cells = <2>;
>> > >> +                       #size-cells = <2>;
>> > >> +                       ranges;
>> > >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>> > >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>> > >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
>> > >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
>> > >> +                       clock-names = "aux", "cfg_ahb", "ref",
>> > >> "refgen";
>> > >> +
>> > >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>> > >> +                       reset-names = "phy";
>> > >> +
>> > >> +                       assigned-clocks = <&gcc
>> > >> GCC_PCIE1_PHY_RCHNG_CLK>;
>> > >> +                       assigned-clock-rates = <100000000>;
>> > >> +
>> > >> +                       status = "disabled";
>> > >
>> > > I think the style is to put status disabled close to the compatible?
>> >
>> > Generally I have added status disabled in end as like many nodes. just
>> > curious to ask is there any specific reason to put close to compatible.
>> 
>> It's really up to qcom maintainers, which I am not.
>> 
> 
> I like when it's the last item, as it lends itself nicely to be
> surrounded by empty lines and thereby easy to spot...
> 
>> Sure, I will change as like previous one.

> Regards,
> Bjorn
> 
>> > >> +                               };
>> > >> +
>> > >> +                               reset-n {
>> > >> +                                       pins = "gpio2";
>> > >> +                                       function = "gpio";
>> > >> +
>> > >> +                                       drive-strength = <16>;
>> > >> +                                       output-low;
>> > >> +                                       bias-disable;
>> > >> +                               };
>> > >> +
>> > >> +                               wake-n {
>> > >> +                                       pins = "gpio3";
>> > >> +                                       function = "gpio";
>> > >> +
>> > >> +                                       drive-strength = <2>;
>> > >> +                                       bias-pull-up;
>> > >> +                               };
>> > >
>> > > These last two nodes with the pull-up and drive-strength settings
>> > > should
>> > > be in the board files, like the idp one, instead of here in the SoC
>> > > file. That way board designers can take the SoC and connect the pcie to
>> > > an external device using these pins and set the configuration they want
>> > > on these pins, or choose not to connect them to the SoC at all and use
>> > > those pins for something else.
>> > >
>> > > In addition, it looks like the reset could be a reset-gpios property
>> > > instead of an output-low config.
>> > >
>> > we are using reset property as perst gpio in pcie node.
>> 
>> Ok, perst-gpios should be fine. Presumably perst-gpios should be in 
>> the
>> board and not in the SoC because of what I wrote up above.

>> Sure, I will move perst into board specific file