Skip to contentSkip to conversation

Add DT bindings and DT nodes for PCIe and PHY in SC7280

PATCH Superseded Prasad Malisetty 3 participants 10 messages Sep 28, 2021 – Oct 1, 2021

Subsystems (8)
Device tree bindings USB Arm Qualcomm mailing list PCI +4 more subsystems

v9 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Prasad Malisetty

author
#0 lore raw

v9 cover Add DT bindings and DT nodes for PCIe and PHY in SC7280

Changes added in v9:
* Added fixed regulator entry for nvme.suggested by Stephen Boyd
* Added NULL pointer check before accessing ops in pcie probe Suggested by Stephen Boyd

Changes added in v8:

* Added seperate pinctrl state for NVMe LDO enable pin [v8 P3/4]
* Removed pointer initialization for pcie_cfg [v8 P4/4]
* Replaced bool pcie_pipe_clk_src with unsigned int:1 [v8 P4/4]
* Changed gcc_pcie_1_pipe_clk_src to pipe_clk_src

Changes added in v7:

* Removed two fallbacks qcom,pcie-sm8250 and snps,dw-pcie.
* Replaced compatible method in get_resources_2_7_0 with flag approach suggested by Bjorn Helgaas .
* Setting gcc_pcie_1_clk_src as XO in init_2_7_0 for gdsc enable.
* Added specific NVMe GPIO entries for SKU1 and SKU2 support in idp.dts and idp2.dts respectively.
* Moved pcie_1 and pcie_1_phy board specific entries into common board file sc7280-idp.dtsi file.

Changes in v6:

* Removed platform check while setting gcc_pcie_1_pipe_clk_src as clk_set_parent will return 0 with nop if platform doesn't need to switch pipe clk source.
* Moved wake-n gpio to board specific file sc7280-idp.dtsi
* Sorted gpio.h header entry in sc7280.dtsi file

Changes in v5:

* Re ordered PCIe, PHY nodes in Soc and board specific dtsi files.
* Removed ref_clk entry in current patch [PATCH v4 P4/4].
* I will add ref clk entry in suspend/ resume commits.
* Added boolean flag in Soc specific dtsi file to differentiate SM8250 and SC7280 platforms. based on boolean flag, platforms will handle the pipe clk handling.

Changes in v4 as suggested by Bjorn:

* Changed pipe clk mux name as gcc_pcie_1_pipe_clk_src.
* Changed pipe_ext_src as phy_pipe_clk.
* Updated commit message for [PATCH v4 4/4].

Changes in v3:
* Changed pipe clock names in dt bindings as pipe_mux and phy_pipe.
* Moved reset and NVMe GPIO pin configs into board specific file.
* Updated pipe clk mux commit message.

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):
dt-bindings: pci: qcom: Document PCIe bindings for SC7280
arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280

 .../devicetree/bindings/pci/qcom,pcie.txt          |  17 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |   9 ++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |  54 ++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts           |   9 ++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 119 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c             |  94 ++++++++++++++--
 6 files changed, 291 insertions(+), 11 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v9 1/4 dt-bindings: pci: qcom: Document PCIe bindings for SC7280

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 <[email protected]>
Acked-by Rob Herring <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +17 -0
index 3f64687..ff423cd 100644
@@ -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_mux"    PIPE MUX
+			- "phy_pipe"    PIPE output clock
+			- "ref"         REFERENCE clock
+
+- clock-names:
 	Usage: required for sdm845
 	Value type: <stringlist>
 	Definition: Should contain the following entries
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#2 lore raw

PATCH v9 2/4 arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

In reply to Prasad Malisetty

Add PCIe controller and PHY nodes for sc7280 SOC.

Signed-off-by Prasad Malisetty <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 119 +++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)
arch/arm64/boot/dts/qcom/sc7280.dtsi +119 -0
index 92b6b65..26f6c04 100644
@@ -1530,6 +1530,117 @@
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
+		pcie1: pci@1c08000 {
+			compatible = "qcom,pcie-sc7280";
+			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>,
+					<0 0 0 2 &intc 0 435 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &intc 0 438 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &intc 0 439 IRQ_TYPE_LEVEL_HIGH>;
+
+			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_mux",
+				      "phy_pipe",
+				      "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";
+
+			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 0x01c0e200 0 0x170>,
+				      <0 0x01c0e400 0 0x200>,
+				      <0 0x01c0ea00 0 0x1f0>,
+				      <0 0x01c0e600 0 0x170>,
+				      <0 0x01c0e800 0 0x200>,
+				      <0 0x01c0ee00 0 0xf4>;
+				clocks = <&gcc GCC_PCIE_1_PIPE_CLK>;
+				clock-names = "pipe0";
+
+				#phy-cells = <0>;
+				#clock-cells = <1>;
+				clock-output-names = "pcie_1_pipe_clk";
+			};
+		};
+
 		ipa: ipa@1e40000 {
 			compatible = "qcom,sc7280-ipa";
 

@@ -2586,6 +2697,14 @@
 			gpio-ranges = <&tlmm 0 0 175>;
 			wakeup-parent = <&pdc>;
 
+			pcie1_default_state: pcie1-default-state {
+				clkreq {
+					pins = "gpio79";
+					function = "pcie1_clkreqn";
+					bias-pull-up;
+				};
+			};
+
 			qspi_clk: qspi-clk {
 				pins = "gpio14";
 				function = "qspi_clk";
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#3 lore raw

PATCH v9 3/4 arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board

In reply to Prasad Malisetty

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 ++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 54 ++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 ++++++
 3 files changed, 72 insertions(+)
arch/arm64/boot/dts/qcom/sc7280-idp.dts +9 -0
index 64fc22a..1562386 100644
@@ -61,6 +61,15 @@
 	modem-init;
 };
 
+&nvme_pwren_pin {
+	pins = "gpio19";
+};
+
+&nvme_3v3_regulators {
+	gpio = <&tlmm 19 GPIO_ACTIVE_HIGH>;
+	enable-active-high;
+};
+
 &pmk8350_vadc {
 	pmr735a_die_temp {
 		reg = <PMR735A_ADC7_DIE_TEMP>;
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +54 -0

69 lines not on this page · raw message

arch/arm64/boot/dts/qcom/sc7280-idp2.dts +9 -0
index 1fc2add..0548cb6 100644
@@ -21,3 +21,12 @@
 		stdout-path = "serial0:115200n8";
 	};
 };
+
+&nvme_pwren_pin {
+	pins = "gpio51";
+};
+
+&nvme_3v3_regulators {
+	gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+	enable-active-high;
+};
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#4 lore raw

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

Quoting Prasad Malisetty (2021-09-28 06:55:49)

Quoted 15 lines

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 ++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 54 ++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 ++++++
 3 files changed, 72 insertions(+)
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0 -0
index 64fc22a..1562386 100644

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 66

Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +3 -0
@@ -61,6 +61,15 @@
        modem-init;
 };
 
+&nvme_pwren_pin {
+       pins = "gpio19";
+};

This should move to the bottom in the "pinctrl" section.

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 70

Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +4 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 19 GPIO_ACTIVE_HIGH>;
+       enable-active-high;

The enable-active-high can be in the idp.dtsi file? That doesn't seem to change.

Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +2 -0
+};
+
 &pmk8350_vadc {
        pmr735a_die_temp {
                reg = <PMR735A_ADC7_DIE_TEMP>;
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +0 -0
index def22ff..5b5505f 100644

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 35

Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +2 -0
@@ -31,6 +31,17 @@
                        linux,can-disable;
                };
        };
+
+       nvme_3v3_regulators: nvme-3v3-regulators {

Why plural? Isn't it a single regulator?

Quoted 24 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +17 -0
+               compatible = "regulator-fixed";
+               regulator-name = "VLDO_3V3";
+
+               regulator-min-microvolt = <3300000>;
+               regulator-max-microvolt = <3300000>;
+
+               pinctrl-names = "default";
+               pinctrl-0 = <&nvme_pwren_pin>;
+       };
 };
 
 &apps_rsc {

@@ -220,6 +231,42 @@
        modem-init;
 };
 
+&pcie1 {
+       status = "okay";
+       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
+
+       vddpe-3v3-supply = <&nvme_3v3_regulators>;
+
+       pinctrl-names = "default";
+       pinctrl-0 = <&pcie1_default_state>;

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 251

Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +10 -0
+};
+
+&pcie1_phy {
+       status = "okay";
+
+       vdda-phy-supply = <&vreg_l10c_0p8>;
+       vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
+&pcie1_default_state {

I thought the node would be split into a reset config node and a wake config node. Is that not being done for some reason? The pinctrl-0 would look like

	pinctrl-0 = <&pcie1_default_state>, <&pcie1_reset_n>, <&pcie1_wake_n>;
Quoted 37 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +25 -0
+       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;
+       };
+};
+
 &pmk8350_vadc {
        pmk8350_die_temp {
                reg = <PMK8350_ADC7_DIE_TEMP>;

@@ -489,3 +536,10 @@
                bias-pull-up;
        };
 };
+
+&tlmm {
+       nvme_pwren_pin: nvme-pwren-pin {
+               function = "gpio";
+               bias-pull-up;
+       };
+};
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +0 -0
index 1fc2add..0548cb6 100644

arch/arm64/boot/dts/qcom/sc7280-idp2.dts line 27

Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +4 -0
@@ -21,3 +21,12 @@
                stdout-path = "serial0:115200n8";
        };
 };
+
+&nvme_pwren_pin {
+       pins = "gpio51";
+};

The pin config can go to a pinctrl section at the bottom of this file?

Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +5 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+       enable-active-high;
+};
author
#5 lore raw

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

Quoted 131 lines · Stephen Boyd

Quoting Prasad Malisetty (2021-09-28 06:55:49)

Quoted 16 lines

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 ++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 54 
++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 ++++++
 3 files changed, 72 insertions(+)
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0 -0
index 64fc22a..1562386 100644

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 66

Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +3 -0
@@ -61,6 +61,15 @@
        modem-init;
 };
 
+&nvme_pwren_pin {
+       pins = "gpio19";
+};

This should move to the bottom in the "pinctrl" section.

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 70

Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +4 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 19 GPIO_ACTIVE_HIGH>;
+       enable-active-high;

The enable-active-high can be in the idp.dtsi file? That doesn't seem to change.

Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +2 -0
+};
+
 &pmk8350_vadc {
        pmr735a_die_temp {
                reg = <PMR735A_ADC7_DIE_TEMP>;
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +0 -0
index def22ff..5b5505f 100644

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 35

Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +2 -0
@@ -31,6 +31,17 @@
                        linux,can-disable;
                };
        };
+
+       nvme_3v3_regulators: nvme-3v3-regulators {

Why plural? Isn't it a single regulator?

Quoted 24 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +17 -0
+               compatible = "regulator-fixed";
+               regulator-name = "VLDO_3V3";
+
+               regulator-min-microvolt = <3300000>;
+               regulator-max-microvolt = <3300000>;
+
+               pinctrl-names = "default";
+               pinctrl-0 = <&nvme_pwren_pin>;
+       };
 };
 
 &apps_rsc {

@@ -220,6 +231,42 @@
        modem-init;
 };
 
+&pcie1 {
+       status = "okay";
+       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
+
+       vddpe-3v3-supply = <&nvme_3v3_regulators>;
+
+       pinctrl-names = "default";
+       pinctrl-0 = <&pcie1_default_state>;

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 251

Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +10 -0
+};
+
+&pcie1_phy {
+       status = "okay";
+
+       vdda-phy-supply = <&vreg_l10c_0p8>;
+       vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
+&pcie1_default_state {

I thought the node would be split into a reset config node and a wake config node. Is that not being done for some reason? The pinctrl-0 would look like

	pinctrl-0 = <&pcie1_default_state>, <&pcie1_reset_n>, <&pcie1_wake_n>;
Quoted 38 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +25 -0
+       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;
+       };
+};
+
 &pmk8350_vadc {
        pmk8350_die_temp {
                reg = <PMK8350_ADC7_DIE_TEMP>;

@@ -489,3 +536,10 @@
                bias-pull-up;
        };
 };
+
+&tlmm {
+       nvme_pwren_pin: nvme-pwren-pin {
+               function = "gpio";
+               bias-pull-up;
+       };
+};
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +0 -0
index 1fc2add..0548cb6 100644
@@ -21,3 +21,12 @@
                stdout-path = "serial0:115200n8";
Quoted 9 lines

arch/arm64/boot/dts/qcom/sc7280-idp2.dts line 27

Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +4 -0
        };
 };
+
+&nvme_pwren_pin {
+       pins = "gpio51";
+};

The pin config can go to a pinctrl section at the bottom of this file?

Hi Stephen,

Thanks for the review and comments.

Sure, I will address all the comments and incorporate the changes in next version series.

Thanks
-Prasad

Quoted 5 lines
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +5 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+       enable-active-high;
+};
author
#6 lore raw

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

In reply to Stephen Boyd
Quoted 20 lines · Stephen Boyd

Quoting Prasad Malisetty (2021-09-28 06:55:49)

Quoted 19 lines

Enable PCIe controller and PHY for sc7280 IDP board. Add specific NVMe GPIO entries for SKU1 and SKU2 support.

Signed-off-by Prasad Malisetty <[email protected]>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  9 ++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 54 
++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  9 ++++++
 3 files changed, 72 insertions(+)
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0 -0
index 64fc22a..1562386 100644
@@ -61,6 +61,15 @@
        modem-init;
 };
Quoted 6 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 66

Quoted 3 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +3 -0
+&nvme_pwren_pin {
+       pins = "gpio19";
+};

This should move to the bottom in the "pinctrl" section.

Hi Stephen,

There is no pinctrl section in this file. we defined nvme_pwren_pin in common IDP file(sc7280-idp.dtsi) and using the nvme_pwren_pin reference to define SKU specific gpio pin for SKU1 and SKU2 support.

Thanks
-Prasad

Quoted 17 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dts line 70

Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +4 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 19 GPIO_ACTIVE_HIGH>;
+       enable-active-high;

The enable-active-high can be in the idp.dtsi file? That doesn't seem to change.

Quoted 9 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +2 -0
+};
+
 &pmk8350_vadc {
        pmr735a_die_temp {
                reg = <PMR735A_ADC7_DIE_TEMP>;
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +0 -0
index def22ff..5b5505f 100644
Quoted 9 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 35

Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +2 -0
@@ -31,6 +31,17 @@
                        linux,can-disable;
                };
        };
+
+       nvme_3v3_regulators: nvme-3v3-regulators {

Why plural? Isn't it a single regulator?

Sure, I will update in next version

Thanks
-Prasad

Quoted 30 lines
Quoted 30 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +23 -0
+               compatible = "regulator-fixed";
+               regulator-name = "VLDO_3V3";
+
+               regulator-min-microvolt = <3300000>;
+               regulator-max-microvolt = <3300000>;
+
+               pinctrl-names = "default";
+               pinctrl-0 = <&nvme_pwren_pin>;
+       };
 };
 
 &apps_rsc {

@@ -220,6 +231,42 @@
        modem-init;
 };
 
+&pcie1 {
+       status = "okay";
+       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
+
+       vddpe-3v3-supply = <&nvme_3v3_regulators>;
+
+       pinctrl-names = "default";
+       pinctrl-0 = <&pcie1_default_state>;
+};
+
+&pcie1_phy {
+       status = "okay";
+
+       vdda-phy-supply = <&vreg_l10c_0p8>;
Quoted 9 lines

arch/arm64/boot/dts/qcom/sc7280-idp.dtsi line 251

Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +4 -0
+       vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
+&pcie1_default_state {

I thought the node would be split into a reset config node and a wake config node. Is that not being done for some reason? The pinctrl-0 would look like

Agree, I will incorporate the changes in next version.

Quoted 40 lines
	pinctrl-0 = <&pcie1_default_state>, <&pcie1_reset_n>, <&pcie1_wake_n>;
Quoted 38 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +25 -0
+       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;
+       };
+};
+
 &pmk8350_vadc {
        pmk8350_die_temp {
                reg = <PMK8350_ADC7_DIE_TEMP>;

@@ -489,3 +536,10 @@
                bias-pull-up;
        };
 };
+
+&tlmm {
+       nvme_pwren_pin: nvme-pwren-pin {
+               function = "gpio";
+               bias-pull-up;
+       };
+};
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +0 -0
index 1fc2add..0548cb6 100644
@@ -21,3 +21,12 @@
                stdout-path = "serial0:115200n8";
Quoted 9 lines

arch/arm64/boot/dts/qcom/sc7280-idp2.dts line 27

Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +4 -0
        };
 };
+
+&nvme_pwren_pin {
+       pins = "gpio51";
+};

The pin config can go to a pinctrl section at the bottom of this file?

Same as a like SKU1 (sc7280-idp.dts)

Quoted 5 lines
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280-idp2.dts +5 -0
+
+&nvme_3v3_regulators {
+       gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+       enable-active-high;
+};
author
#7 lore raw

PATCH v9 4/4 PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280

In reply to Prasad Malisetty

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

Signed-off-by Prasad Malisetty <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 94 ++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +83 -11
index 8a7a300..7896a86 100644
@@ -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_src;
+	struct clk *phy_pipe_clk;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {

@@ -189,6 +192,11 @@ struct qcom_pcie_ops {
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
 
+struct qcom_pcie_cfg {
+	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */

@@ -197,6 +205,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

@@ -1167,6 +1176,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
+	if (pcie->pipe_clk_need_muxing) {
+		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
+		if (IS_ERR(res->pipe_clk_src))
+			return PTR_ERR(res->pipe_clk_src);
+
+		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
+		if (IS_ERR(res->phy_pipe_clk))
+			return PTR_ERR(res->phy_pipe_clk);
+
+		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);
 }

@@ -1185,6 +1208,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Set TCXO as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;

@@ -1256,6 +1283,10 @@ 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;
 
+	/* Set pipe clock as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
+
 	return clk_prepare_enable(res->pipe_clk);
 }
 

@@ -1456,6 +1487,39 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+static const struct qcom_pcie_cfg apq8084_cfg = {
+	.ops = &ops_1_0_0,
+};
+
+static const struct qcom_pcie_cfg ipq8064_cfg = {
+	.ops = &ops_2_1_0,
+};
+
+static const struct qcom_pcie_cfg msm8996_cfg = {
+	.ops = &ops_2_3_2,
+};
+
+static const struct qcom_pcie_cfg ipq8074_cfg = {
+	.ops = &ops_2_3_3,
+};
+
+static const struct qcom_pcie_cfg ipq4019_cfg = {
+	.ops = &ops_2_4_0,
+};
+
+static const struct qcom_pcie_cfg sdm845_cfg = {
+	.ops = &ops_2_7_0,
+};
+
+static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+};
+
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.pipe_clk_need_muxing = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,

@@ -1467,6 +1531,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
+	const struct qcom_pcie_cfg *pcie_cfg;
 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	if (!pcie->ops) {
+		dev_err(dev, "Invalid platform data\n");
+		return -EINVAL;
+	}
+	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
 
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {

@@ -1545,16 +1616,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_match[] = {
-	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
-	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
-	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
-	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-msm8996", .data = &msm8996_cfg },
+	{ .compatible = "qcom,pcie-ipq8074", .data = &ipq8074_cfg },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
+	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#8 lore raw

Re: [PATCH v9 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280

[+cc linux-pci; please cc drivers/pci changes there]

Quoted 3 lines · Prasad Malisetty

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

This looks good. Ideally the commit log would say what the patch *does*. I know it's in the subject, but it's nice to have it both places so the body is complete in itself.

Again in an ideal world, I would split this into two patches:

1) Do the boilerplate conversions to struct qcom_pcie_cfg and updates of qcom_pcie_match[]. This patch would have no functional change.

2) Add the code to deal with pcie_1_pipe_clk_src, which should be a much smaller patch.

This makes it easier to see the important part of dealing with pcie_1_pipe_clk_src and makes any future bisect much more specific.

Quoted 178 lines
Signed-off-by Prasad Malisetty <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 94 ++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +83 -11
index 8a7a300..7896a86 100644
@@ -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_src;
+	struct clk *phy_pipe_clk;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {

@@ -189,6 +192,11 @@ struct qcom_pcie_ops {
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
 
+struct qcom_pcie_cfg {
+	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */

@@ -197,6 +205,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

@@ -1167,6 +1176,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
+	if (pcie->pipe_clk_need_muxing) {
+		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
+		if (IS_ERR(res->pipe_clk_src))
+			return PTR_ERR(res->pipe_clk_src);
+
+		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
+		if (IS_ERR(res->phy_pipe_clk))
+			return PTR_ERR(res->phy_pipe_clk);
+
+		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);
 }

@@ -1185,6 +1208,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Set TCXO as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;

@@ -1256,6 +1283,10 @@ 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;
 
+	/* Set pipe clock as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
+
 	return clk_prepare_enable(res->pipe_clk);
 }
 

@@ -1456,6 +1487,39 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+static const struct qcom_pcie_cfg apq8084_cfg = {
+	.ops = &ops_1_0_0,
+};
+
+static const struct qcom_pcie_cfg ipq8064_cfg = {
+	.ops = &ops_2_1_0,
+};
+
+static const struct qcom_pcie_cfg msm8996_cfg = {
+	.ops = &ops_2_3_2,
+};
+
+static const struct qcom_pcie_cfg ipq8074_cfg = {
+	.ops = &ops_2_3_3,
+};
+
+static const struct qcom_pcie_cfg ipq4019_cfg = {
+	.ops = &ops_2_4_0,
+};
+
+static const struct qcom_pcie_cfg sdm845_cfg = {
+	.ops = &ops_2_7_0,
+};
+
+static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+};
+
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.pipe_clk_need_muxing = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,

@@ -1467,6 +1531,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
+	const struct qcom_pcie_cfg *pcie_cfg;
 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	if (!pcie->ops) {
+		dev_err(dev, "Invalid platform data\n");
+		return -EINVAL;
+	}
+	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
 
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset)) {

@@ -1545,16 +1616,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_match[] = {
-	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
-	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
-	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
-	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-msm8996", .data = &msm8996_cfg },
+	{ .compatible = "qcom,pcie-ipq8074", .data = &ipq8074_cfg },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
+	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#9 lore raw

Re: [PATCH v9 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280

Quoted 1 line · Bjorn Helgaas

[+cc linux-pci; please cc drivers/pci changes there]

Sure Bjorn, I missed to add the lists. I will cc from next release.

Quoted 16 lines · Prasad Malisetty
Quoted 3 lines

On the SC7280, the clock source for gcc_pcie_1_pipe_clk_src must be the TCXO while gdsc is enabled. After PHY init successful clock source should switch to pipe clock for gcc_pcie_1_pipe_clk_src.

This looks good. Ideally the commit log would say what the patch *does*. I know it's in the subject, but it's nice to have it both places so the body is complete in itself.

Again in an ideal world, I would split this into two patches:

1) Do the boilerplate conversions to struct qcom_pcie_cfg and updates of qcom_pcie_match[]. This patch would have no functional change.

Quoted 6 lines

2) Add the code to deal with pcie_1_pipe_clk_src, which should be a much smaller patch.

This makes it easier to see the important part of dealing with pcie_1_pipe_clk_src and makes any future bisect much more specific.

Hi Bjorn,

Thanks for the suggestion.

I will split [Patch v9 4/4] patch into two separate changes and update in next version series.

Thanks
-Prasad

Quoted 185 lines
Quoted 185 lines
Signed-off-by Prasad Malisetty <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 94 
++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +23 -0
index 8a7a300..7896a86 100644
@@ -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_src;
+	struct clk *phy_pipe_clk;
+	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {

@@ -189,6 +192,11 @@ struct qcom_pcie_ops {
 	int (*config_sid)(struct qcom_pcie *pcie);
 };
 
+struct qcom_pcie_cfg {
+	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
+};
+
 struct qcom_pcie {
 	struct dw_pcie *pci;
 	void __iomem *parf;			/* DT parf */

@@ -197,6 +205,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	unsigned int pipe_clk_need_muxing:1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

@@ -1167,6 +1176,20 @@ static int qcom_pcie_get_resources_2_7_0(struct 
 qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
+	if (pcie->pipe_clk_need_muxing) {
+		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
+		if (IS_ERR(res->pipe_clk_src))
+			return PTR_ERR(res->pipe_clk_src);
+
+		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
+		if (IS_ERR(res->phy_pipe_clk))
+			return PTR_ERR(res->phy_pipe_clk);
+
+		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);

}

drivers/pci/controller/dwc/pcie-qcom.c +4 -0
@@ -1185,6 +1208,10 @@ static int qcom_pcie_init_2_7_0(struct 
 qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Set TCXO as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
drivers/pci/controller/dwc/pcie-qcom.c +38 -0
 		goto err_disable_regulators;

@@ -1256,6 +1283,10 @@ 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;
 
+	/* Set pipe clock as clock source for pcie_pipe_clk_src */
+	if (pcie->pipe_clk_need_muxing)
+		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
+
 	return clk_prepare_enable(res->pipe_clk);
 }

@@ -1456,6 +1487,39 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+static const struct qcom_pcie_cfg apq8084_cfg = {
+	.ops = &ops_1_0_0,
+};
+
+static const struct qcom_pcie_cfg ipq8064_cfg = {
+	.ops = &ops_2_1_0,
+};
+
+static const struct qcom_pcie_cfg msm8996_cfg = {
+	.ops = &ops_2_3_2,
+};
+
+static const struct qcom_pcie_cfg ipq8074_cfg = {
+	.ops = &ops_2_3_3,
+};
+
+static const struct qcom_pcie_cfg ipq4019_cfg = {
+	.ops = &ops_2_4_0,
+};
+
+static const struct qcom_pcie_cfg sdm845_cfg = {
+	.ops = &ops_2_7_0,
+};
+
+static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+};
+
+static const struct qcom_pcie_cfg sc7280_cfg = {
+	.ops = &ops_1_9_0,
+	.pipe_clk_need_muxing = true,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,

@@ -1467,6 +1531,7 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
+	const struct qcom_pcie_cfg *pcie_cfg;
 	int ret;
drivers/pci/controller/dwc/pcie-qcom.c +7 -1
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);

@@ -1488,7 +1553,13 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie->ops = of_device_get_match_data(dev);
+	pcie_cfg = of_device_get_match_data(dev);
+	pcie->ops = pcie_cfg->ops;
+	if (!pcie->ops) {
+		dev_err(dev, "Invalid platform data\n");
+		return -EINVAL;
+	}
+	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
 
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
drivers/pci/controller/dwc/pcie-qcom.c +11 -11
 	if (IS_ERR(pcie->reset)) {

@@ -1545,16 +1616,17 @@ static int qcom_pcie_probe(struct 
 platform_device *pdev)
 }
 
 static const struct of_device_id qcom_pcie_match[] = {
-	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
-	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
-	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
-	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
-	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
-	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ipq8064_cfg },
+	{ .compatible = "qcom,pcie-msm8996", .data = &msm8996_cfg },
+	{ .compatible = "qcom,pcie-ipq8074", .data = &ipq8074_cfg },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ipq4019_cfg },
+	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
+	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
 
--

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