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

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

[Prasad Malisetty]

This series includes PCIe support for qualcomm sc7280
which includes PCIe controller and PHY DT bindings.
The PCIe controller and PHYs are mostly comaptible with SM8250 SoC,
hence the existing pcie drivers are modified to add the support.

Changes in v2:
	* Moved pcie pin control settings into IDP file.
	* Replaced pipe_clk_src with pipe_clk_mux in pcie driver 
	* Included pipe clk mux setting change set in this series

Prasad Malisetty (4):
  arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
  arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  PCIe: qcom: Add support to control pipe clk mux
  dt-bindings: pci: qcom: Document PCIe bindings for SC720

 .../devicetree/bindings/pci/qcom,pcie.txt          |  17 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |  28 +++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 138 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c             |  22 ++++
 4 files changed, 205 insertions(+)

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

[PATCH v2 1/4] 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 | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 3900cfc..a58552b 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -268,6 +268,34 @@
 		};
 };
 
+&pcie1 {
+	status = "okay";
+
+	vdda-supply = <&vreg_l10c_0p8>;
+};
+
+&pcie1_phy {
+	status = "okay";
+
+	vdda-phy-supply = <&vreg_l10c_0p8>;
+	vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
+&pcie1_default_state {
+	clkreq {
+		bias-pull-up;
+	};
+
+	wake-n {
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
+	nvme-n {
+		bias-pull-up;
+	};
+};
+
 &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 v2 1/4] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board

[Bjorn Andersson]

On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> Add PCIe nodes for sc7280 IDP board.
> 

This looks good, but it can't come before patch 2 in the series, which
introduces these labels.

Regards,
Bjorn

> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 3900cfc..a58552b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -268,6 +268,34 @@
>  		};
>  };
>  
> +&pcie1 {
> +	status = "okay";
> +
> +	vdda-supply = <&vreg_l10c_0p8>;
> +};
> +
> +&pcie1_phy {
> +	status = "okay";
> +
> +	vdda-phy-supply = <&vreg_l10c_0p8>;
> +	vdda-pll-supply = <&vreg_l6b_1p2>;
> +};
> +
> +&pcie1_default_state {
> +	clkreq {
> +		bias-pull-up;
> +	};
> +
> +	wake-n {
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
> +	nvme-n {
> +		bias-pull-up;
> +	};
> +};
> +
>  &qupv3_id_0 {
>  	status = "okay";
>  };
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v2 2/4] 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 0b6f119..9e8414d 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -15,6 +15,7 @@
 #include <dt-bindings/reset/qcom,sdm845-pdc.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
 #include <dt-bindings/thermal/thermal.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -484,6 +485,117 @@
 			#power-domain-cells = <1>;
 		};
 
+		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";
+			status = "disabled";
+			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>;
+
+			pcie1_lane: lanes@1c0e200 {
+				reg = <0 0x01c0e200 0 0x170>,
+				      <0 0x01c0e400 0 0x200>,
+				      <0 0x01c0ea00 0 0x1f0>,
+				      <0 0x01c0e600 0 0x170>,
+				      <0 0x01c0e800 0 0x200>,
+				      <0 0x01c0ee00 0 0xf4>;
+				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>,
@@ -1102,6 +1214,32 @@
 				pins = "gpio46", "gpio47";
 				function = "qup13";
 			};
+
+			pcie1_default_state: pcie1-default {
+				clkreq {
+					pins = "gpio79";
+					function = "pcie1_clkreqn";
+				};
+
+				reset-n {
+					pins = "gpio2";
+					function = "gpio";
+
+					drive-strength = <16>;
+					output-low;
+					bias-disable;
+				};
+
+				wake-n {
+					pins = "gpio3";
+					function = "gpio";
+				};
+
+				nvme-n {
+					pins = "gpio19";
+					function = "gpio";
+				};
+			};
 		};
 
 		apps_smmu: iommu@15000000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Bjorn Andersson]

On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> 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 0b6f119..9e8414d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -15,6 +15,7 @@
>  #include <dt-bindings/reset/qcom,sdm845-pdc.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>  #include <dt-bindings/thermal/thermal.h>
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	interrupt-parent = <&intc>;
> @@ -484,6 +485,117 @@
>  			#power-domain-cells = <1>;
>  		};
>  
> +		pcie1: pci@1c08000 {

Does this name imply that you have a pcie0 as well? Please introduce it
while you're at it.

> +			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";

No, you don't have a sm8250-qmp-gen3x2-pcie-phy in your sc7280.

> +			status = "disabled";
> +			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>;
> +
> +			pcie1_lane: lanes@1c0e200 {
> +				reg = <0 0x01c0e200 0 0x170>,
> +				      <0 0x01c0e400 0 0x200>,
> +				      <0 0x01c0ea00 0 0x1f0>,
> +				      <0 0x01c0e600 0 0x170>,
> +				      <0 0x01c0e800 0 0x200>,
> +				      <0 0x01c0ee00 0 0xf4>;
> +				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>,
> @@ -1102,6 +1214,32 @@
>  				pins = "gpio46", "gpio47";
>  				function = "qup13";
>  			};
> +
> +			pcie1_default_state: pcie1-default {

Per the binding the name has to end with "-pins", although I would like
us to change that to "-state". Either way, this is not correct.

> +				clkreq {
> +					pins = "gpio79";
> +					function = "pcie1_clkreqn";
> +				};
> +
> +				reset-n {
> +					pins = "gpio2";
> +					function = "gpio";
> +
> +					drive-strength = <16>;
> +					output-low;
> +					bias-disable;
> +				};
> +
> +				wake-n {
> +					pins = "gpio3";
> +					function = "gpio";
> +				};
> +
> +				nvme-n {

This doesn't look like a standard PCIe pin, is it perhaps the enable pin
for the regulator powering your NVME, or something along those lines?

If so you should describe it as a fixed-regulator...and either way I
suspect it should be moved to the device specific file.

Regards,
Bjorn

> +					pins = "gpio19";
> +					function = "gpio";
> +				};
> +			};
>  		};
>  
>  		apps_smmu: iommu@15000000 {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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

[Prasad Malisetty]

On 2021-06-06 07:56, Bjorn Andersson wrote:
> On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:
> 
>> 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 0b6f119..9e8414d 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -15,6 +15,7 @@
>>  #include <dt-bindings/reset/qcom,sdm845-pdc.h>
>>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>>  #include <dt-bindings/thermal/thermal.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> 
>>  / {
>>  	interrupt-parent = <&intc>;
>> @@ -484,6 +485,117 @@
>>  			#power-domain-cells = <1>;
>>  		};
>> 
>> +		pcie1: pci@1c08000 {
> 
> Does this name imply that you have a pcie0 as well? Please introduce it
> while you're at it.
> 
>> We are not using pcie0 for HLOS.

>> +			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";
> 
> No, you don't have a sm8250-qmp-gen3x2-pcie-phy in your sc7280.
> 
>> Both are having same PHY sequence.

>> +			status = "disabled";
>> +			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>;
>> +
>> +			pcie1_lane: lanes@1c0e200 {
>> +				reg = <0 0x01c0e200 0 0x170>,
>> +				      <0 0x01c0e400 0 0x200>,
>> +				      <0 0x01c0ea00 0 0x1f0>,
>> +				      <0 0x01c0e600 0 0x170>,
>> +				      <0 0x01c0e800 0 0x200>,
>> +				      <0 0x01c0ee00 0 0xf4>;
>> +				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>,
>> @@ -1102,6 +1214,32 @@
>>  				pins = "gpio46", "gpio47";
>>  				function = "qup13";
>>  			};
>> +
>> +			pcie1_default_state: pcie1-default {
> 
> Per the binding the name has to end with "-pins", although I would like
> us to change that to "-state". Either way, this is not correct.
> 
>> +				clkreq {
>> +					pins = "gpio79";
>> +					function = "pcie1_clkreqn";
>> +				};
>> +
>> +				reset-n {
>> +					pins = "gpio2";
>> +					function = "gpio";
>> +
>> +					drive-strength = <16>;
>> +					output-low;
>> +					bias-disable;
>> +				};
>> +
>> +				wake-n {
>> +					pins = "gpio3";
>> +					function = "gpio";
>> +				};
>> +
>> +				nvme-n {
> 
> This doesn't look like a standard PCIe pin, is it perhaps the enable 
> pin
> for the regulator powering your NVME, or something along those lines?
> 
> If so you should describe it as a fixed-regulator...and either way I
> suspect it should be moved to the device specific file.
> 
> Regards,
> Bjorn
> 
> Agree, will move into board specific file.

>> +					pins = "gpio19";
>> +					function = "gpio";
>> +				};
>> +			};
>>  		};
>> 
>>  		apps_smmu: iommu@15000000 {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

[PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

In PCIe driver pipe-clk mux needs to switch between pipe_clk
and XO for GDSC enable. This is done by setting pipe_clk mux
as parent of pipe_clk after phy init.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8a7a300..5cbbea4 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
 	struct clk *pipe_clk;
+	struct clk *pipe_clk_mux;
+	struct clk *pipe_ext_src;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {
@@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
+	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
+		res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
+		if (IS_ERR(res->pipe_clk_mux))
+			return PTR_ERR(res->pipe_clk_mux);
+
+		res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
+		if (IS_ERR(res->pipe_ext_src))
+			return PTR_ERR(res->pipe_ext_src);
+
+		res->ref_clk_src = devm_clk_get(dev, "ref");
+		if (IS_ERR(res->ref_clk_src))
+			return PTR_ERR(res->ref_clk_src);
+	}
+
 	res->pipe_clk = devm_clk_get(dev, "pipe");
 	return PTR_ERR_OR_ZERO(res->pipe_clk);
 }
@@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+
+	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
+		clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
 
 	return clk_prepare_enable(res->pipe_clk);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Stephen Boyd]

Quoting Prasad Malisetty (2021-06-05 07:40:58)
> In PCIe driver pipe-clk mux needs to switch between pipe_clk
> and XO for GDSC enable. This is done by setting pipe_clk mux
> as parent of pipe_clk after phy init.

Just to confirm, we can't set this parent via assigned-clock-parents
property in DT?

>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300..5cbbea4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>         struct regulator_bulk_data supplies[2];
>         struct reset_control *pci_reset;
>         struct clk *pipe_clk;
> +       struct clk *pipe_clk_mux;
> +       struct clk *pipe_ext_src;
> +       struct clk *ref_clk_src;
>  };
>
>  union qcom_pcie_resources {
> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>         if (ret < 0)
>                 return ret;
>
> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
> +               res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
> +               if (IS_ERR(res->pipe_clk_mux))
> +                       return PTR_ERR(res->pipe_clk_mux);
> +
> +               res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
> +               if (IS_ERR(res->pipe_ext_src))
> +                       return PTR_ERR(res->pipe_ext_src);
> +
> +               res->ref_clk_src = devm_clk_get(dev, "ref");

Is this going to be used by any code?

> +               if (IS_ERR(res->ref_clk_src))
> +                       return PTR_ERR(res->ref_clk_src);
> +       }
> +
>         res->pipe_clk = devm_clk_get(dev, "pipe");
>         return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>         struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +       struct dw_pcie *pci = pcie->pci;
> +       struct device *dev = pci->dev;
> +
> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
> +               clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>
>         return clk_prepare_enable(res->pipe_clk);
>  }
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

On 2021-06-06 02:56, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-06-05 07:40:58)
>> In PCIe driver pipe-clk mux needs to switch between pipe_clk
>> and XO for GDSC enable. This is done by setting pipe_clk mux
>> as parent of pipe_clk after phy init.
> 
> Just to confirm, we can't set this parent via assigned-clock-parents
> property in DT?
> 
>> 
This clock setting need be done after phy init.

>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 8a7a300..5cbbea4 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>>         struct regulator_bulk_data supplies[2];
>>         struct reset_control *pci_reset;
>>         struct clk *pipe_clk;
>> +       struct clk *pipe_clk_mux;
>> +       struct clk *pipe_ext_src;
>> +       struct clk *ref_clk_src;
>>  };
>> 
>>  union qcom_pcie_resources {
>> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct 
>> qcom_pcie *pcie)
>>         if (ret < 0)
>>                 return ret;
>> 
>> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) 
>> {
>> +               res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
>> +               if (IS_ERR(res->pipe_clk_mux))
>> +                       return PTR_ERR(res->pipe_clk_mux);
>> +
>> +               res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
>> +               if (IS_ERR(res->pipe_ext_src))
>> +                       return PTR_ERR(res->pipe_ext_src);
>> +
>> +               res->ref_clk_src = devm_clk_get(dev, "ref");
> 
> Is this going to be used by any code?
> 
Yes, ref clock will be used in system suspend case. currently system 
suspend changes are in under validation.

>> +               if (IS_ERR(res->ref_clk_src))
>> +                       return PTR_ERR(res->ref_clk_src);
>> +       }
>> +
>>         res->pipe_clk = devm_clk_get(dev, "pipe");
>>         return PTR_ERR_OR_ZERO(res->pipe_clk);
>>  }
>> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct 
>> qcom_pcie *pcie)
>>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>>  {
>>         struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +       struct dw_pcie *pci = pcie->pci;
>> +       struct device *dev = pci->dev;
>> +
>> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
>> +               clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>> 
>>         return clk_prepare_enable(res->pipe_clk);
>>  }
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Bjorn Andersson]

On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> In PCIe driver pipe-clk mux needs to switch between pipe_clk
> and XO for GDSC enable. This is done by setting pipe_clk mux
> as parent of pipe_clk after phy init.

But you're not switching between pipe_clk and XO, you're only making
sure that the pipe_clk is parented by the PHY's pipe clock.

Also, can you please elaborate on how this relates to the GDSC?

> 
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300..5cbbea4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct regulator_bulk_data supplies[2];
>  	struct reset_control *pci_reset;
>  	struct clk *pipe_clk;
> +	struct clk *pipe_clk_mux;
> +	struct clk *pipe_ext_src;
> +	struct clk *ref_clk_src;
>  };
>  
>  union qcom_pcie_resources {
> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
> +		res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
> +		if (IS_ERR(res->pipe_clk_mux))
> +			return PTR_ERR(res->pipe_clk_mux);
> +
> +		res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
> +		if (IS_ERR(res->pipe_ext_src))
> +			return PTR_ERR(res->pipe_ext_src);
> +
> +		res->ref_clk_src = devm_clk_get(dev, "ref");
> +		if (IS_ERR(res->ref_clk_src))
> +			return PTR_ERR(res->ref_clk_src);
> +	}
> +
>  	res->pipe_clk = devm_clk_get(dev, "pipe");
>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))

If this is something only found on 7280, you need to document (in the
commit message at least) why this does not apply to other platforms with
this controller.

Thanks,
Bjorn

> +		clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>  
>  	return clk_prepare_enable(res->pipe_clk);
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 3/4] PCIe: qcom: Add support to control pipe clk mux

[Prasad Malisetty]

On 2021-06-06 07:45, Bjorn Andersson wrote:
> On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:
> 
>> In PCIe driver pipe-clk mux needs to switch between pipe_clk
>> and XO for GDSC enable. This is done by setting pipe_clk mux
>> as parent of pipe_clk after phy init.
> 
> But you're not switching between pipe_clk and XO, you're only making
> sure that the pipe_clk is parented by the PHY's pipe clock.
> 
> Also, can you please elaborate on how this relates to the GDSC?
> 
>> yes we are parenting the pipe clock by PHY's pipe clock. and also 
>> switching back to XO during suspend.

Below is the new requirement for SC7280 as part of LPM sequence.

In L1ss low power mode PHY turns the pipe clock off, so each access on 
slave AXI causes to exit from low power modes.
For completing the access, the pipe clock should be active from PHY.

In L23 mode, access on slave AXI doesn’t wake the core.
For accessing to DBI registers during L23, the SW should switch the pipe 
clock with 19.2MHz free-running clock (TCXO)
using GCC’s registers

>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 8a7a300..5cbbea4 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>>  	struct regulator_bulk_data supplies[2];
>>  	struct reset_control *pci_reset;
>>  	struct clk *pipe_clk;
>> +	struct clk *pipe_clk_mux;
>> +	struct clk *pipe_ext_src;
>> +	struct clk *ref_clk_src;
>>  };
>> 
>>  union qcom_pcie_resources {
>> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct 
>> qcom_pcie *pcie)
>>  	if (ret < 0)
>>  		return ret;
>> 
>> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
>> +		res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
>> +		if (IS_ERR(res->pipe_clk_mux))
>> +			return PTR_ERR(res->pipe_clk_mux);
>> +
>> +		res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
>> +		if (IS_ERR(res->pipe_ext_src))
>> +			return PTR_ERR(res->pipe_ext_src);
>> +
>> +		res->ref_clk_src = devm_clk_get(dev, "ref");
>> +		if (IS_ERR(res->ref_clk_src))
>> +			return PTR_ERR(res->ref_clk_src);
>> +	}
>> +
>>  	res->pipe_clk = devm_clk_get(dev, "pipe");
>>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>>  }
>> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct 
>> qcom_pcie *pcie)
>>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>>  {
>>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +	struct dw_pcie *pci = pcie->pci;
>> +	struct device *dev = pci->dev;
>> +
>> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
> 
> If this is something only found on 7280, you need to document (in the
> commit message at least) why this does not apply to other platforms 
> with
> this controller.
> 
> Thanks,
> Bjorn
> 
Sure, will add more info about the requirement.

>> +		clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>> 
>>  	return clk_prepare_enable(res->pipe_clk);
>>  }
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

[PATCH v2 4/4] 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>
Acked-by: Rob Herring <robh@kernel.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 25f4def..9c0908f 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
 			- "qcom,pcie-ipq6018" for ipq6018
@@ -144,6 +145,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

Re: [PATCH v2 4/4] dt-bindings: pci: qcom: Document PCIe bindings for SC720

[Bjorn Andersson]

On Sat 05 Jun 09:40 CDT 2021, 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>
> Acked-by: Rob Herring <robh@kernel.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 25f4def..9c0908f 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
>  			- "qcom,pcie-ipq6018" for ipq6018
> @@ -144,6 +145,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

If you describe it as pipe _mux_, then name it pipe_mux.

> +			- "pipe_ext"    PIPE output clock

Better to name this in a way that indicates that it is the pipe clock
coming out of the phy, perhaps "phy_pipe".

But that said, as Stephen points out ensuring that the pipe_clk_src's
parent is the phy's clock can be done in DT directly.

Regards,
Bjorn

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