Add DT bindings and DT nodes for PCIe and PHY in SC7280
Subsystems (8)
v4 cover Add DT bindings and DT nodes for PCIe and PHY in SC7280
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 SC720
arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
PCIe: qcom: Add support to control pipe clk src
.../devicetree/bindings/pci/qcom,pcie.txt | 17 +++ arch/arm64/boot/dts/qcom/sc7280-idp.dts | 38 +++++++ arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++ drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++ 4 files changed, 202 insertions(+)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v4 1/4 dt-bindings: pci: qcom: Document PCIe bindings for SC720
Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar to the one used on SM8250. Add the compatible for SC7280.
--- Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +17 -0
@@ -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
Re: [PATCH v4 1/4] dt-bindings: pci: qcom: Document PCIe bindings for SC720
Quoting Prasad Malisetty (2021-07-16 06:58:44)
Quoted 6 lines
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]>---
Any chance this file can be converted to YAML?
Re: [PATCH v4 1/4] dt-bindings: pci: qcom: Document PCIe bindings for SC720
Quoted 11 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-07-16 06:58:44)
Quoted 6 lines
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]>Any chance this file can be converted to YAML?
Yes, we can convert into YAML. we will incorporate the change in next version.
PATCH v4 2/4 arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
In reply to Prasad MalisettyAdd PCIe controller and PHY nodes for sc7280 SOC.
--- arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
arch/arm64/boot/dts/qcom/sc7280.dtsi +125 -0
@@ -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>;
@@ -546,6 +547,118 @@ #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>, + <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"; + + 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 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>,
@@ -1185,6 +1298,18 @@ pins = "gpio46", "gpio47"; function = "qup13"; }; + + pcie1_default_state: pcie1-default-state { + clkreq { + pins = "gpio79"; + function = "pcie1_clkreqn"; + }; + + wake-n { + pins = "gpio3"; + function = "gpio"; + }; + }; }; apps_smmu: iommu@15000000 {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoting Prasad Malisetty (2021-07-16 06:58:45)
Quoted 13 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +1
-0@@ -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>;
arch/arm64/boot/dts/qcom/sc7280.dtsi line 547
Quoted 1 line
arch/arm64/boot/dts/qcom/sc7280.dtsi +0
-0@@ -546,6 +547,118 @@ #power-domain-cells = <1>;
Is this the gpucc node? At address 3d90000? Please sort the nodes based on their address, so this would be 1c08000 which comes before gpucc and some others in this file.
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +8
-0}; + 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>; +
Re: [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoted 11 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-07-16 06:58:45)
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +1
-0@@ -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>
Quoted 9 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi line 547
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +0
-0/ { interrupt-parent = <&intc>;@@ -546,6 +547,118 @@ #power-domain-cells = <1>;Is this the gpucc node? At address 3d90000? Please sort the nodes based on their address, so this would be 1c08000 which comes before gpucc and some others in this file.
sure, I will move it accordingly.
Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +8
-0}; + 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>; +
Re: [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
In reply to Prasad MalisettyQuoted 143 lines · Prasad Malisetty
Add PCIe controller and PHY nodes for sc7280 SOC.
Signed-off-by Prasad Malisetty <[email protected]>--- arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)arch/arm64/boot/dts/qcom/sc7280.dtsi +114
-0@@ -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>;@@ -546,6 +547,118 @@ #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>, + <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"; + + 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 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>,@@ -1185,6 +1298,18 @@ pins = "gpio46", "gpio47"; function = "qup13"; }; +
arch/arm64/boot/dts/qcom/sc7280.dtsi line 1311
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +10
-0+ pcie1_default_state: pcie1-default-state { + clkreq { + pins = "gpio79"; + function = "pcie1_clkreqn"; + }; + + wake-n { + pins = "gpio3"; + function = "gpio"; + };
This could be essentially any GPIO, right? Does it really make sense to have this node in the SoC file? I would say it belongs in the board file.
Re: [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoted 151 lines · Matthias Kaehlcke
Quoted 150 lines · Prasad Malisetty
Add PCIe controller and PHY nodes for sc7280 SOC.
Signed-off-by Prasad Malisetty <[email protected]>--- arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)arch/arm64/boot/dts/qcom/sc7280.dtsi +113
-0@@ -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>;@@ -546,6 +547,118 @@ #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>, + <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"; + + 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 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";arch/arm64/boot/dts/qcom/sc7280.dtsi +6
-0reg = <0 0x06002000 0 0x1000>,@@ -1185,6 +1298,18 @@ pins = "gpio46", "gpio47"; function = "qup13"; }; + + pcie1_default_state: pcie1-default-state { + clkreq { + pins = "gpio79"; + function = "pcie1_clkreqn"; + };
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi line 1311
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +5
-0+ + wake-n { + pins = "gpio3"; + function = "gpio"; + };This could be essentially any GPIO, right? Does it really make sense to have this node in the SoC file? I would say it belongs in the board file.
Hi Matthias,
Thanks for your review and comments.
Sorry for the delay in acknowledging. I would move this entry in IDP file in next version.
Thanks
-Prasad
PATCH v4 3/4 arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
In reply to Prasad MalisettyAdd PCIe nodes for sc7280 IDP board.
--- arch/arm64/boot/dts/qcom/sc7280-idp.dts | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
arch/arm64/boot/dts/qcom/sc7280-idp.dts +38 -0
@@ -268,6 +268,44 @@ }; }; +&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; + }; + + reset-n { + pins = "gpio2"; + function = "gpio"; + + drive-strength = <16>; + output-low; + bias-disable; + }; + + wake-n { + drive-strength = <2>; + bias-pull-up; + }; + + nvme-n { + pins = "gpio19"; + bias-pull-up; + }; +}; + &qupv3_id_0 { status = "okay"; };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 3/4] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
Quoting Prasad Malisetty (2021-07-16 06:58:46)
arch/arm64/boot/dts/qcom/sc7280-idp.dts line 268
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0
-0@@ -268,6 +268,44 @@ };
Is this pmk8350_vadc? 'pc' comes before 'pm' so please sort this section alphabetically on node name.
Quoted 14 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +12
-0}; +&pcie1 { + status = "okay"; + + vdda-supply = <&vreg_l10c_0p8>; +}; + +&pcie1_phy { + status = "okay"; + + vdda-phy-supply = <&vreg_l10c_0p8>; + vdda-pll-supply = <&vreg_l6b_1p2>; +};
Re: [PATCH v4 3/4] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
Quoted 12 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-07-16 06:58:46)
arch/arm64/boot/dts/qcom/sc7280-idp.dts line 268
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0
-0@@ -268,6 +268,44 @@ };Is this
pmk8350_vadc? 'pc' comes before 'pm' so please sort this section alphabetically on node name.Quoted 1 line
arch/arm64/boot/dts/qcom/sc7280-idp.dts +0
-0};
Agree, we will incorporate the change in next version.
Quoted 13 lines
Quoted 13 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +12
-0+&pcie1 { + status = "okay"; + + vdda-supply = <&vreg_l10c_0p8>; +}; + +&pcie1_phy { + status = "okay"; + + vdda-phy-supply = <&vreg_l10c_0p8>; + vdda-pll-supply = <&vreg_l6b_1p2>; +};
PATCH v4 4/4 PCIe: qcom: Add support to control pipe clk src
In reply to Prasad MalisettyThis is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO. after PHY initialization gcc_pcie_1_pipe_clk_src needs to switch from TCXO to gcc_pcie_1_pipe_clk.
--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +22 -0
@@ -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 *gcc_pcie_1_pipe_clk_src; +struct clk *phy_pipe_clk; +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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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); }
@@ -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->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk); }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
and make your subject match the style and structure (in particular, s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init
Quoted 4 lines · Prasad Malisetty
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO. after PHY initializationgcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.
This says what *needs* to happen, but it doesn't actually say what this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO while gdsc is enabled. But after the PHY is initialized, the clock source must be switched to gcc_pcie_1_pipe_clk.
On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to gcc_pcie_1_pipe_clk after the PHY has been initialized.
Nits: Rewrap to fill 75 columns or so. Add blank lines between paragraphs. Start sentences with capital letter.
Quoted 25 lines
Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -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 *gcc_pcie_1_pipe_clk_src; +struct clk *phy_pipe_clk; +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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" );
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1184
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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);
Not clear why ref_clk_src is here, since it's not used anywhere. If it's not necessary here, drop it and add it in a future patch that uses it.
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1278
Quoted 13 lines
drivers/pci/controller/dwc/pcie-qcom.c +6
-0+ } + 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" ))
Using of_device_is_compatible() follows existing style in the driver, which is good. But I'm not sure that's good style in general because it's a little repetitious and wasteful.
qcom_pcie_probe() already calls of_device_get_match_data(), which does basically the same thing as of_device_is_compatible(), so I think we could take better advantage of that by augmenting struct qcom_pcie_ops with these device-specific details.
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +1
-0+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk); }Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Quoted 77 lines · Bjorn Helgaas
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.cand make your subject match the style and structure (in particular, s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280
gcc_pcie_1_pipe_clk_srcafter PHY initQuoted 4 lines · Prasad Malisetty
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO. after PHY initializationgcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.This says what *needs* to happen, but it doesn't actually say what this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for
pcie_1_pipemust be the TCXO while gdsc is enabled. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clk.On sc7280, switch
gcc_pcie_1_pipe_clk_srcfrom TCXO togcc_pcie_1_pipe_clkafter the PHY has been initialized.Nits: Rewrap to fill 75 columns or so. Add blank lines between paragraphs. Start sentences with capital letter.
Quoted 25 lines
Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -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 *gcc_pcie_1_pipe_clk_src; +struct clk *phy_pipe_clk; +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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" );drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1184
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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);Not clear why
ref_clk_srcis here, since it's not used anywhere. If it's not necessary here, drop it and add it in a future patch that uses it.Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +2
-0+ } + 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;
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1278
Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +4
-0+struct dw_pcie *pci = pcie->pci; +struct device *dev = pci->dev; + +if (of_device_is_compatible (dev->of_node,"qcom,pcie-sc7280" ))Using
of_device_is_compatible()follows existing style in the driver, which is good. But I'm not sure that's good style in general because it's a little repetitious and wasteful.
Following the style is good, but up until the recent sm8250 addition it was just a hack to deal with legacy platforms that we don't know the exact details about.
But, all platforms I know of has the pipe_clk from the PHY fed into the pipe_clk_src mux in the gcc block and then ends up in the PCIe controller. As such, I suspect that the pipe_clk handling should be moved to the common code path of the driver and there's definitely no harm in making sure that the pipe_clk_src mux is explicitly configured on existing platforms (at least all 2.7.0 based ones).
Quoted 5 lines
qcom_pcie_probe()already callsof_device_get_match_data(), which does basically the same thing asof_device_is_compatible(), so I think we could take better advantage of that by augmenting structqcom_pcie_opswith these device-specific details.
I agree.
Regards,
Bjorn
Quoted 19 lines
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.cQuoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +1
-0+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk); }Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Quoted 24 lines · Bjorn Andersson
Quoted 22 lines · Bjorn Helgaas
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.cand make your subject match the style and structure (in particular, s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280
gcc_pcie_1_pipe_clk_srcafter PHY initQuoted 4 lines · Prasad Malisetty
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO. after PHY initializationgcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.This says what *needs* to happen, but it doesn't actually say what this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for
pcie_1_pipemust be the TCXO while gdsc is enabled. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clk.
Quoted 6 lines
Quoted 6 lines
On sc7280, switch
gcc_pcie_1_pipe_clk_srcfrom TCXO togcc_pcie_1_pipe_clkafter the PHY has been initialized.Nits: Rewrap to fill 75 columns or so. Add blank lines between paragraphs. Start sentences with capital letter.
Agree, looks good. will add more details and update the commit message in next version.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +12
-0@@ -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 *gcc_pcie_1_pipe_clk_src; +struct clk *phy_pipe_clk; +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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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); +
Quoted 8 lines
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1184
Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +3
-0+ res->ref_clk_src =devm_clk_get (dev,"ref" ); +if (IS_ERR (res->ref_clk_src)) +return PTR_ERR (res->ref_clk_src);Not clear why
ref_clk_srcis here, since it's not used anywhere. If it's not necessary here, drop it and add it in a future patch that uses it.
Its more useful in suspend /resume patch set. as of now we will move to suspend/resume patch set.
Quoted 52 lines
Quoted 9 lines
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +2
-0+ } + 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;Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1278
Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +4
-0+struct dw_pcie *pci = pcie->pci; +struct device *dev = pci->dev; + +if (of_device_is_compatible (dev->of_node,"qcom,pcie-sc7280" ))Using
of_device_is_compatible()follows existing style in the driver, which is good. But I'm not sure that's good style in general because it's a little repetitious and wasteful.Following the style is good, but up until the recent sm8250 addition it was just a hack to deal with legacy platforms that we don't know the exact details about.
But, all platforms I know of has the
pipe_clkfrom the PHY fed into thepipe_clk_srcmux in the gcc block and then ends up in the PCIe controller. As such, I suspect that thepipe_clkhandling should be moved to the common code path of the driver and there's definitely no harm in making sure that thepipe_clk_srcmux is explicitly configured on existing platforms (at least all 2.7.0 based ones).Quoted 5 lines
qcom_pcie_probe()already callsof_device_get_match_data(), which does basically the same thing asof_device_is_compatible(), so I think we could take better advantage of that by augmenting structqcom_pcie_opswith these device-specific details.I agree.
Regards,
BjornQuoted 11 lines
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1282
Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +1
-0+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk); }
Sure, we will make use of struct qcom_pcie_ops and add a new callback to configure pipe clk src. In coming platforms, if the platform doesn't need to configure pipe clk src, it will return as callback not defined.
We will incorporate the changes in next release.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
In reply to Bjorn AnderssonQuoted 122 lines · Bjorn Andersson
Quoted 77 lines · Bjorn Helgaas
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.cand make your subject match the style and structure (in particular, s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280
gcc_pcie_1_pipe_clk_srcafter PHY initQuoted 4 lines · Prasad Malisetty
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO. after PHY initializationgcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.This says what *needs* to happen, but it doesn't actually say what this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for
pcie_1_pipemust be the TCXO while gdsc is enabled. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clk.On sc7280, switch
gcc_pcie_1_pipe_clk_srcfrom TCXO togcc_pcie_1_pipe_clkafter the PHY has been initialized.Nits: Rewrap to fill 75 columns or so. Add blank lines between paragraphs. Start sentences with capital letter.
Quoted 25 lines
Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -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 *gcc_pcie_1_pipe_clk_src; +struct clk *phy_pipe_clk; +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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" );drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1184
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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);Not clear why
ref_clk_srcis here, since it's not used anywhere. If it's not necessary here, drop it and add it in a future patch that uses it.Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +2
-0+ } + 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;Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1278
Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +4
-0+struct dw_pcie *pci = pcie->pci; +struct device *dev = pci->dev; + +if (of_device_is_compatible (dev->of_node,"qcom,pcie-sc7280" ))Using
of_device_is_compatible()follows existing style in the driver, which is good. But I'm not sure that's good style in general because it's a little repetitious and wasteful.Following the style is good, but up until the recent sm8250 addition it was just a hack to deal with legacy platforms that we don't know the exact details about.
But, all platforms I know of has the
pipe_clkfrom the PHY fed into thepipe_clk_srcmux in the gcc block and then ends up in the PCIe controller. As such, I suspect that thepipe_clkhandling should be moved to the common code path of the driver and there's definitely no harm in making sure that thepipe_clk_srcmux is explicitly configured on existing platforms (at least all 2.7.0 based ones).Quoted 5 lines
qcom_pcie_probe()already callsof_device_get_match_data(), which does basically the same thing asof_device_is_compatible(), so I think we could take better advantage of that by augmenting structqcom_pcie_opswith these device-specific details.I agree.
Regards,
BjornQuoted 11 lines
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
Quoted 5 lines
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +1
-1+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk); } --
Hi Bjorn,
Thanks for your review and inputs.
I would like to add more details on this requirement. The hardware ver. of SM8250 & SC7280 platforms is the same but where as only for SC7280, we need to switch gcc_pcie_1_pipe_clk source between TXCO and gcc_pcie_1_pipe_clk hence this is SoC level specific requirement. all existing platforms doesn't need this pipe clk handling.
I will use boolean flag entry in SoC dtsi such as "pipe-clk-source-switch" to differentiate between SM8250 and SC7280 instead of compatible method.
Please provide any other better approach for the above case.
Thanks
-Prasad
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
In reply to Prasad MalisettyQuoting Prasad Malisetty (2021-07-16 06:58:47)
Quoted 2 lines
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO.
Why? Can you add that detail here? Presumably it's something like the GDSC needs a running clk to send a reset through the flops or something like that.
Quoted 34 lines
after PHY initialization
gcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +14
-0@@ -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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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 static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1279
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -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->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
Is anything wrong if we call clk_set_parent() here when this driver is running on previous SoCs where the parent is assigned via DT? Also, shouldn't we make sure the parent is XO at driver probe time so that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that requires the clock to be running at XO and we're working around that in the pcie driver instead of sticking that logic into the GDSC. What do we do if the GDSC is already enabled out of boot instead of being the power on reset (POR) configuration?
Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0return clk_prepare_enable (res->pipe_clk); }
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Quoted 8 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-07-16 06:58:47)
Quoted 2 lines
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO.Why? Can you add that detail here? Presumably it's something like the GDSC needs a running clk to send a reset through the flops or something like that.
Which presumably means that we need to "park" gcc_pcie_N_pipe_clk_src whenever the PHY pipe is paused due to a suspend or runtime suspend.
I find this part of the commit message to primarily describing the next patch (that is yet to be posted).
Quoted 37 lines
Quoted 37 lines
after PHY initialization
gcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +14
-0@@ -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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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); }@@ -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;
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1279
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-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->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);Is anything wrong if we call
clk_set_parent()here when this driver is running on previous SoCs where the parent is assigned via DT?
We don't assign the parent on previous platforms, we apparently just rely on the reset value (afaict).
So I think it makes sense for all platforms to explicitly mux pipe_clk_src to phy::pipe_clk, one the PHY is up and running.
But I was under the impression that we have the BRANCH_HALT_SKIP on the pipe_clk because there was some sort of feedback loop to the PHY's calibration... What this patch indicates is that we should park pipe_clk_src onto XO at boot time, then after the PHY starts ticking we should enable and reparent the clk_src - at which point I don't see why we need the HALT_SKIP.
Quoted 9 lines
Also, shouldn't we make sure the parent is XO at driver probe time so that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that requires the clock to be running at XO and we're working around that in the pcie driver instead of sticking that logic into the GDSC. What do we do if the GDSC is already enabled out of boot instead of being the power on reset (POR) configuration?
What happens if we boot the device out of NVME...
PS. Are we certain that it's the PCIe driver and not the PHY that should do this dance? I really would like to see the continuation of this patch to see the full picture...
Regards,
Bjorn
Quoted 3 lines
Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0return clk_prepare_enable (res->pipe_clk); }
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Quoting Bjorn Andersson (2021-07-16 13:31:55)
Quoted 11 lines · Stephen Boyd
Quoted 9 lines
Quoting Prasad Malisetty (2021-07-16 06:58:47)
Quoted 2 lines
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO.Why? Can you add that detail here? Presumably it's something like the GDSC needs a running clk to send a reset through the flops or something like that.
Quoted 5 lines
Which presumably means that we need to "park"
gcc_pcie_N_pipe_clk_srcwhenever the PHY pipe is paused due to a suspend or runtime suspend.I find this part of the commit message to primarily describing the next patch (that is yet to be posted).
Ah I see. So there will be another patch to do the park and unpark over suspend/resume?
Quoted 47 lines
Quoted 38 lines
Quoted 38 lines
after PHY initialization
gcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +14
-0@@ -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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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); }@@ -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;Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1279
Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-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->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);Is anything wrong if we call
clk_set_parent()here when this driver is running on previous SoCs where the parent is assigned via DT?
Quoted 2 lines
We don't assign the parent on previous platforms, we apparently just rely on the reset value (afaict).
Oh sheesh. I thought that was being done already. It looks like at least on sdm845 that there is only one parent for this clk so we don't need to call clk_set_parent to set it there.
Quoted 3 lines
So I think it makes sense for all platforms to explicitly mux
pipe_clk_srctophy::pipe_clk, one the PHY is up and running.
Sure, except some platforms don't have a mux?
Quoted 7 lines
But I was under the impression that we have the
BRANCH_HALT_SKIPon thepipe_clkbecause there was some sort of feedback loop to the PHY's calibration... What this patch indicates is that we should parkpipe_clk_srconto XO at boot time, then after the PHY starts ticking we should enable and reparent theclk_src- at which point I don't see why we need theHALT_SKIP.
I recall that qcom folks kept saying they needed to enable the pipe_clk_src clk branch in GCC before enabling the phy. So they required the halt skip flag so that the clk_prepare_enable() call would effectively set the enable bit in GCC and move on without caring. Then they could enable the upstream clk source in the phy without having to stop halfway through to enable the branch in GCC. The whole design here is pretty insane.
In fact, I think we discussed this whole topic in late 2019[1] and we concluded that we could just slam the clk on forever and deal with the clk_set_parent() when the clk became a mux+gate instead of a pure gate.
Quoted 4 lines
Quoted 4 lines
Also, shouldn't we make sure the parent is XO at driver probe time so that powering on the GDSC works properly?
Quoted 8 lines
Quoted 6 lines
It all feels like a kludge though given that the GDSC is the one that requires the clock to be running at XO and we're working around that in the pcie driver instead of sticking that logic into the GDSC. What do we do if the GDSC is already enabled out of boot instead of being the power on reset (POR) configuration?
What happens if we boot the device out of NVME...
I guess it's fine? The GDSC will be on and the parent clk will already be set so things are a no-op.
Quoted 6 lines
PS. Are we certain that it's the PCIe driver and not the PHY that should do this dance? I really would like to see the continuation of this patch to see the full picture...
Re: [PATCH v4 4/4] PCIe: qcom: Add support to control pipe clk src
Quoted 12 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-07-16 13:31:55)
Quoted 11 lines · Stephen Boyd
Quoted 9 lines
Quoting Prasad Malisetty (2021-07-16 06:58:47)
Quoted 2 lines
This is a new requirement for sc7280 SoC.
To enable gdscgcc_pcie_1_pipe_clk_srcshould be TCXO.Why? Can you add that detail here? Presumably it's something like the GDSC needs a running clk to send a reset through the flops or something like that.
Quoted 9 lines
Quoted 5 lines
Which presumably means that we need to "park"
gcc_pcie_N_pipe_clk_srcwhenever the PHY pipe is paused due to a suspend or runtime suspend.I find this part of the commit message to primarily describing the next patch (that is yet to be posted).
Ah I see. So there will be another patch to do the park and unpark over suspend/resume?
That's my understanding.
Quoted 47 lines
Quoted 47 lines
Quoted 46 lines
Quoted 35 lines
after PHY initialization
gcc_pcie_1_pipe_clk_srcneeds to switch from TCXO togcc_pcie_1_pipe_clk.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +14
-0@@ -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->gcc_pcie_1_pipe_clk_src =devm_clk_get (dev,"pipe_mux" ); +if (IS_ERR (res->gcc_pcie_1_pipe_clk_src)) +return PTR_ERR (res->gcc_pcie_1_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 static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1279
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -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->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);Is anything wrong if we call
clk_set_parent()here when this driver is running on previous SoCs where the parent is assigned via DT?
Quoted 7 lines
Quoted 2 lines
We don't assign the parent on previous platforms, we apparently just rely on the reset value (afaict).
Oh sheesh. I thought that was being done already. It looks like at least on sdm845 that there is only one parent for this clk so we don't need to call
clk_set_parentto set it there.
I'll have to check the documentation on that...
Quoted 22 lines
Quoted 3 lines
So I think it makes sense for all platforms to explicitly mux
pipe_clk_srctophy::pipe_clk, one the PHY is up and running.Sure, except some platforms don't have a mux?
Quoted 7 lines
But I was under the impression that we have the
BRANCH_HALT_SKIPon thepipe_clkbecause there was some sort of feedback loop to the PHY's calibration... What this patch indicates is that we should parkpipe_clk_srconto XO at boot time, then after the PHY starts ticking we should enable and reparent theclk_src- at which point I don't see why we need theHALT_SKIP.I recall that qcom folks kept saying they needed to enable the
pipe_clk_srcclk branch in GCC before enabling the phy. So they required the halt skip flag so that theclk_prepare_enable()call would effectively set the enable bit in GCC and move on without caring. Then they could enable the upstream clk source in the phy without having to stop halfway through to enable the branch in GCC. The whole design here is pretty insane.
Quoted 4 lines
In fact, I think we discussed this whole topic in late 2019[1] and we concluded that we could just slam the clk on forever and deal with the
clk_set_parent()when the clk became a mux+gate instead of a pure gate.
That's exactly what I asked Prasad about, because per the description and content of this patch the parent pipe_clk_src will remain XO until the PHY is initialized. So either the PHY no longer need gcc in the loop to calibrate the pipe clock or it used to, but no longer does.
Thanks for the link, we definitely should clean that up, but I think at this point it might be worth waiting a little bit longer to see what actually going to happen in the suspend/resume (system and runtime) paths...
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Also, shouldn't we make sure the parent is XO at driver probe time so that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that requires the clock to be running at XO and we're working around that in the pcie driver instead of sticking that logic into the GDSC. What do we do if the GDSC is already enabled out of boot instead of being the power on reset (POR) configuration?
Quoted 5 lines
Quoted 1 line
What happens if we boot the device out of NVME...
I guess it's fine? The GDSC will be on and the parent clk will already be set so things are a no-op.
Yes, if the pipe_clk_src is parked nicely in late initcall, so that when the pd late init cuts the GDSC things will end up in a clean state.
Regards,
Bjorn
Quoted 8 lines
Quoted 6 lines
PS. Are we certain that it's the PCIe driver and not the PHY that should do this dance? I really would like to see the continuation of this patch to see the full picture...
[+cc linux-pci]
Quoted 23 lines · Prasad Malisetty
Changes in v4 as suggested by Bjorn:
* Changed pipe clk mux name as
gcc_pcie_1_pipe_clk_src.
* Changedpipe_ext_srcasphy_pipe_clk.
* Updated commit message for [PATCH v4 4/4].Changes in v3:
* Changed pipe clock names in dt bindings aspipe_muxandphy_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.
* Replacedpipe_clk_srcwithpipe_clk_muxin pcie driver
* Included pipe clk mux setting change set in this seriesPrasad Malisetty (4):
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
PCIe: qcom: Add support to control pipe clk src
Quoted 4 lines
.../devicetree/bindings/pci/qcom,pcie.txt | 17 +++ arch/arm64/boot/dts/qcom/sc7280-idp.dts | 38 +++++++ arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++ drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++
$ ./scripts/get_maintainer.pl -f drivers/pci/controller/dwc/pcie-qcom.c
tells you that linux-pci should be included. One reason that's important is because patchwork watches linux-pci for incoming patches, and I use patchwork as my to-do list.