Add DT bindings and DT nodes for PCIe and PHY in SC7280
Subsystems (8)
v5 cover Add DT bindings and DT nodes for PCIe and PHY in SC7280
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 | 38 +++++++ arch/arm64/boot/dts/qcom/sc7280.dtsi | 126 +++++++++++++++++++++ drivers/pci/controller/dwc/pcie-qcom.c | 18 +++ 4 files changed, 199 insertions(+)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v5 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.
--- 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
PATCH v5 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 | 126 +++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)
arch/arm64/boot/dts/qcom/sc7280.dtsi +126 -0
@@ -16,6 +16,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>;
@@ -586,6 +587,119 @@ qcom,bcm-voters = <&apps_bcm_voter>; }; + 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>; + pipe-clk-source-switch; + + #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"; + }; + }; + ipa: ipa@1e40000 { compatible = "qcom,sc7280-ipa";
@@ -1598,6 +1712,18 @@ bias-bus-hold; }; }; + + 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 v5 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoting Prasad Malisetty (2021-08-09 21:08:34)
Quoted 12 lines
Add PCIe controller and PHY nodes for sc7280 SOC.
Signed-off-by Prasad Malisetty <[email protected]>--- arch/arm64/boot/dts/qcom/sc7280.dtsi | 126 +++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)arch/arm64/boot/dts/qcom/sc7280.dtsi +0
-0
arch/arm64/boot/dts/qcom/sc7280.dtsi line 19
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +1
-0@@ -16,6 +16,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>
Please sort this alphabetically, gpio comes before reset at the least.
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +4
-0/ { interrupt-parent = <&intc>;@@ -586,6 +587,119 @@ qcom,bcm-voters = <&apps_bcm_voter>; }; + pcie1: pci@1c08000 { + compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie"; + reg = <0 0x01c08000 0 0x3000>, + <0 0x40000000 0 0xf1d>,
arch/arm64/boot/dts/qcom/sc7280.dtsi line 603
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +10
-0+ <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>; + pipe-clk-source-switch;
I'd rather not have this DT property, but key it off the compatible string.
Quoted 3 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +3
-0+ + #address-cells = <3>; + #size-cells = <2>;
Re: [PATCH v5 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
In reply to Prasad MalisettyQuoting Prasad Malisetty (2021-08-09 21:08:34)
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +0
-0@@ -1598,6 +1712,18 @@ bias-bus-hold; }; };
arch/arm64/boot/dts/qcom/sc7280.dtsi line 1724
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 is function gpio, so presumably board designers could decide to change the wake gpio to something else, right? I'd prefer we move wake-n to the board level (idp) as well. gpio79 looks fine as it is muxed to be the pcie1_clkreqn function, not gpio, so it seems to be a dedicated pin for this purpose.
Re: [PATCH v5 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoted 17 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-09 21:08:34)
Quoted 16 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +8
-0@@ -1598,6 +1712,18 @@ bias-bus-hold; }; }; + + pcie1_default_state: pcie1-default-state { + clkreq { + pins = "gpio79"; + function = "pcie1_clkreqn"; + }; + + wake-n {
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi line 1724
Quoted 2 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +2
-0+ pins = "gpio3"; + function = "gpio";This is function gpio, so presumably board designers could decide to change the wake gpio to something else, right? I'd prefer we move wake-n to the board level (idp) as well. gpio79 looks fine as it is muxed to be the
pcie1_clkreqnfunction, not gpio, so it seems to be a dedicated pin for this purpose.
Sure, I will move it IDP file in next version.
Re: [PATCH v5 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
In reply to Prasad MalisettyQuoted 20 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 | 126 +++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)arch/arm64/boot/dts/qcom/sc7280.dtsi +1
-0@@ -16,6 +16,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 591
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +2
-0@@ -586,6 +587,119 @@ qcom,bcm-voters = <&apps_bcm_voter>; }; + pcie1: pci@1c08000 { + compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie";
Why 2 fallbacks? Fallbacks are meant to be used when the "fallback" compatible driver can fully support the hw. In this case, neither "qcom,pcie-sm8250" nor "snps,dw-pcie" can work properly, right?
I did the same mistake for SM8250 though... But please get rid of them.
arch/arm64/boot/dts/qcom/sc7280.dtsi line 603
Quoted 12 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +12
-0+ 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>; + pipe-clk-source-switch;
Did you document this property in binding? You need to add "qcom" prefix since this is a qcom specific one and not a generic PCI property.
Thanks,
Mani
Quoted 125 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +111
-0+ + #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"; + }; + }; + ipa: ipa@1e40000 { compatible = "qcom,sc7280-ipa";@@ -1598,6 +1712,18 @@ bias-bus-hold; }; }; + + 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 v5 2/4] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Quoted 34 lines · Manivannan Sadhasivam
Quoted 21 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 | 126 +++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)arch/arm64/boot/dts/qcom/sc7280.dtsi +1
-0@@ -16,6 +16,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 592
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +2
-0@@ -586,6 +587,119 @@ qcom,bcm-voters = <&apps_bcm_voter>; }; + pcie1: pci@1c08000 { + compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie";Why 2 fallbacks? Fallbacks are meant to be used when the "fallback" compatible driver can fully support the hw. In this case, neither "qcom,pcie-sm8250" nor "snps,dw-pcie"
can work properly, right?
Quoted 2 lines
I did the same mistake for SM8250 though... But please get rid of them.
Hi Mani,
Thanks for your review and comments.
Sure I agree, will remove one of the fallbacks and update the changes in next version.
Thanks
-Prasad
Quoted 16 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi line 603
Quoted 12 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +12
-0+ 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>; + pipe-clk-source-switch;Did you document this property in binding? You need to add "qcom" prefix since this is a qcom specific one and not a generic PCI property.
Quoted 3 lines
Thanks,
Mani
I haven't document this property, missed it in v5. once this change is finalized I will update it next version.
Quoted 125 lines
Quoted 125 lines
arch/arm64/boot/dts/qcom/sc7280.dtsi +111
-1+ + #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"; + }; + }; + ipa: ipa@1e40000 { compatible = "qcom,sc7280-ipa";@@ -1598,6 +1712,18 @@ bias-bus-hold; }; }; + + pcie1_default_state: pcie1-default-state { + clkreq { + pins = "gpio79"; + function = "pcie1_clkreqn"; + }; + + wake-n { + pins = "gpio3"; + function = "gpio"; + }; + }; }; apps_smmu: iommu@15000000 { --The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v5 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
@@ -61,6 +61,44 @@ modem-init; }; +&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; + }; +}; + &pmk8350_vadc { pmr735a_die_temp { reg = <PMR735A_ADC7_DIE_TEMP>;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v5 3/4] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
Quoting Prasad Malisetty (2021-08-09 21:08:35)
Quoted 33 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +25
-0@@ -61,6 +61,44 @@ modem-init; }; +&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;
arch/arm64/boot/dts/qcom/sc7280-idp.dts line 98
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +10
-0+ }; + + wake-n { + drive-strength = <2>; + bias-pull-up; + }; + + nvme-n { + pins = "gpio19"; + bias-pull-up;
function = "gpio"?
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7280-idp.dts +3
-0+ }; +}; + &pmk8350_vadc { pmr735a_die_temp { reg = <PMR735A_ADC7_DIE_TEMP>;
PATCH v5 4/4 PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
In reply to Prasad MalisettyOn the SC7280, By default the clock source for pcie_1_pipe is TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched to gcc_pcie_1_pipe_clk from TCXO.
--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +18 -0
@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {
@@ -1167,6 +1169,16 @@ 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->pipe_clk =devm_clk_get (dev,"pipe" );return PTR_ERR_OR_ZERO (res->pipe_clk); }
@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" )) +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 v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoting Prasad Malisetty (2021-08-09 21:08:36)
Quoted 31 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +10
-0@@ -1167,6 +1169,16 @@ 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->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 1274
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" ))
This can be straightline code. If gcc_pcie_1_pipe_clk_src is NULL, calling clk_set_parent() on it is a nop, return 0, so drop the property check and only assign the clk pointer if it needs to be done.
drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie) · line 1275
Quoted 1 line
drivers/pci/controller/dwc/pcie-qcom.c +1
-0+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
Please check the return value and fail if it fails to set the parent. I'd also prefer a comment indicating that we have to set the parent because the GDSC must be enabled with the clk at XO speed. The DT should probably also have an assigned clock parent of XO so when the driver probes it is set to XO parent for gdsc enable and then this driver code can change the parent to the phy pipe clk.
Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0return clk_prepare_enable (res->pipe_clk); }
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 52 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-09 21:08:36)
Quoted 34 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +10
-0@@ -1167,6 +1169,16 @@ 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->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 · line 1275
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" ))This can be straightline code. If
gcc_pcie_1_pipe_clk_srcis NULL, callingclk_set_parent()on it is a nop, return 0, so drop the property check and only assign the clk pointer if it needs to be done.drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_deinit_2_7_0(struct · line 1277
Quoted 2 lines
drivers/pci/controller/dwc/pcie-qcom.c +1
-0+clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
Quoted 10 lines
Please check the return value and fail if it fails to set the parent. I'd also prefer a comment indicating that we have to set the parent because the GDSC must be enabled with the clk at XO speed. The DT should probably also have an assigned clock parent of XO so when the driver probes it is set to XO parent for gdsc enable and then this driver code can change the parent to the phy pipe clk.
Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0return clk_prepare_enable (res->pipe_clk);}
Hi Stephen,
Thanks for your review and inputs.
Yes, clk_set_parent function returning NULL if src pointer is NULL. we can call clk_set_parent function without any check.
I will validate and incorporate the changes in next version.
Thanks
-Prasad
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
In reply to Prasad MalisettyQuoted 40 lines · Prasad Malisetty
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +12
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ 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->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 1274
Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" ))
Wondering why you didn't use the compatible here as well. This will break if the property exist but the clocks are not.
Thanks,
Mani
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 v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 55 lines · Manivannan Sadhasivam
Quoted 41 lines · Prasad Malisetty
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +12
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ 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->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 · line 1275
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +5
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" ))Wondering why you didn't use the compatible here as well. This will break if the property exist but the clocks are not.
Quoted 3 lines
Thanks,
Mani
Hi Mani,
In earlier versions we used compatible method here as well, but in v5 replaced compatible with new boolean flag.
In recent comments as Stephen suggested, its straight forward approach. if src pointer is NULL, clk_set_parent return 0 and nop I will remove both compatible and property read approach and update the change in next version.
Quoted 8 lines
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);}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
In reply to Prasad MalisettyQuoted 28 lines · Prasad Malisetty
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1182
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+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); + } +
Hi All,
Greetings!
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
Thanks
-Prasad
Quoted 17 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0res->pipe_clk =devm_clk_get (dev,"pipe" );return PTR_ERR_OR_ZERO (res->pipe_clk);}
drivers/pci/controller/dwc/pcie-qcom.c +6
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" )) +clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);return clk_prepare_enable (res->pipe_clk);}
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 65 lines · Prasad Malisetty
Quoted 28 lines · Prasad Malisetty
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1182
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+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); + } +Hi All,
Greetings!
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
Thanks
-PrasadQuoted 15 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0res->pipe_clk =devm_clk_get (dev,"pipe" );return PTR_ERR_OR_ZERO (res->pipe_clk);}
drivers/pci/controller/dwc/pcie-qcom.c +6
-0@@ -1255,6 +1267,12 @@ 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; +struct device_node *node = dev->of_node; + +if (of_property_read_bool (node,"pipe-clk-source-switch" )) +clk_set_parent (res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
Quoted 2 lines
Quoted 2 lines
drivers/pci/controller/dwc/pcie-qcom.c +0
-0return clk_prepare_enable (res->pipe_clk);}
Hi,
Kindly provide your inputs and confirmation on latest queries, I will share new patch version.
Thanks
-Prasad
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 43 lines · Prasad Malisetty
Quoted 42 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1182
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+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); + } +Hi All,
Quoted 5 lines
Quoted 5 lines
Greetings!
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
I'd prefer the compatible method. If nobody is responding then it's best to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
[+cc linux-pci; patches to drivers/pci/ should always be cc'd there]
Quoted 44 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1180
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
Quoted 3 lines
I'd prefer the compatible method. If nobody is responding then it's best to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use of_device_is_compatible() if they've already called of_device_get_match_data() (as qcom_pcie_probe() has).
of_device_is_compatible() does basically the same work of looking for a match in qcom_pcie_match[] that of_device_get_match_data() does, so it seems pointless to repeat it.
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Bjorn
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 56 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 44 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1180
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
Quoted 3 lines
I'd prefer the compatible method. If nobody is responding then it's best to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).
Quoted 9 lines
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Bjorn
Hi Bjorn,
I agree on your point, but the main reason is to use compatible in get_resources_2_7_0 is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method in get_resources_2_7_0 to differentiate SM8250 and SC7280.
Thanks
-Prasad
Quoted 2 lines
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 54 lines · Prasad Malisetty
Quoted 52 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 48 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) · line 1180
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
I'd prefer the compatible method. If nobody is responding then it's best
to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
Quoted 8 lines
Quoted 8 lines
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.
+1
Quoted 2 lines
Quoted 2 lines
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Either that's missing or there's a fallback to 8250 that's not documented.
Quoted 10 lines
Quoted 3 lines
Bjorn
Hi Bjorn,
I agree on your point, but the main reason is to use compatible in
get_resources_2_7_0is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method inget_resources_2_7_0to differentiate SM8250 and SC7280.
Then fix the match data to be not just ops, but ops and the flag you need here.
Rob
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 82 lines · Rob Herring
Quoted 54 lines · Prasad Malisetty
Quoted 52 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 48 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1181
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
I'd prefer the compatible method. If nobody is responding then it's best
to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.Quoted 8 lines
Quoted 8 lines
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.+1
Quoted 2 lines
Quoted 2 lines
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Either that's missing or there's a fallback to 8250 that's not documented.
Quoted 10 lines
Quoted 3 lines
Bjorn
Hi Bjorn,
I agree on your point, but the main reason is to use compatible in
get_resources_2_7_0is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method inget_resources_2_7_0to differentiate SM8250 and SC7280.
Quoted 3 lines
Then fix the match data to be not just ops, but ops and the flag you need here.
Rob
Hi Rob,
Thanks for your review comments and inputs .
This difference is not universal across all the platforms but instead this is specific to SC7280. Hence it make sense to use compatible other than going for a flag.
Thanks
-Prasad
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 80 lines · Prasad Malisetty
Quoted 79 lines · Rob Herring
Quoted 54 lines · Prasad Malisetty
Quoted 52 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 48 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1181
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
I'd prefer the compatible method. If nobody is responding then it's best
to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.Quoted 8 lines
Quoted 8 lines
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.+1
Quoted 2 lines
Quoted 2 lines
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Either that's missing or there's a fallback to 8250 that's not documented.
Quoted 5 lines
I agree on your point, but the main reason is to use compatible in
get_resources_2_7_0is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method inget_resources_2_7_0to differentiate SM8250 and SC7280.
Quoted 6 lines
Quoted 2 lines
Then fix the match data to be not just ops, but ops and the flag you need here.
This difference is not universal across all the platforms but instead this is specific to SC7280.
Hence it make sense to use compatible other than going for a flag.
There's no reason your qcom_pcie_match[].data pointers need to be strictly based on the hardware version.
You can do something like what pcie-brcmstb.c does, e.g.,
struct pcie_cfg_data {
struct qcom_pcie_ops *ops;
unsigned int pipe_mux:1;
};
static const struct pcie_cfg_data sm8250_cfg = {
.ops = &ops_1_9_0,
};
static const struct pcie_cfg_data sc7280_cfg = {
.ops = &ops_1_9_0,
.pipe_mux = 1,
}; static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },};
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 107 lines · Bjorn Helgaas
Quoted 80 lines · Prasad Malisetty
Quoted 79 lines · Rob Herring
Quoted 54 lines · Prasad Malisetty
Quoted 52 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 48 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 12 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1181
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 2 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
I'd prefer the compatible method. If nobody is responding then it's best
to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.Quoted 8 lines
Quoted 8 lines
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.+1
Quoted 2 lines
Quoted 2 lines
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Either that's missing or there's a fallback to 8250 that's not documented.
Quoted 5 lines
I agree on your point, but the main reason is to use compatible in
get_resources_2_7_0is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method inget_resources_2_7_0to differentiate SM8250 and SC7280.Quoted 6 lines
Quoted 2 lines
Then fix the match data to be not just ops, but ops and the flag you need here.
This difference is not universal across all the platforms but instead this is specific to SC7280.
Hence it make sense to use compatible other than going for a flag.There's no reason your qcom_pcie_match[].data pointers need to be strictly based on the hardware version.
You can do something like what
pcie-brcmstb.cdoes, e.g.,struct pcie_cfg_data { struct qcom_pcie_ops *ops; unsigned int pipe_mux:1; }; static const struct pcie_cfg_data sm8250_cfg = { .ops = &ops_1_9_0, }; static const struct pcie_cfg_data sc7280_cfg = { .ops = &ops_1_9_0, .pipe_mux = 1, };
Quoted 4 lines
static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },};
Hi Bjorn,
Thanks for the detailed example.
I have one quick query, If we use above approach, we should change platform data reading in PCIe probe to differentiate remaining platforms right. expect SM8250 and SC7280 all other platforms are using same qcom_pcie_ops structure pointer as data.
Kindly correct me if my understanding is wrong.
Just posted v6 patch series with same compatible approach as of now. I will go through your example and update further.
Thanks
-Prasad
Re: [PATCH v5 4/4] PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
Quoted 116 lines · Prasad Malisetty
Quoted 110 lines · Bjorn Helgaas
Quoted 83 lines · Prasad Malisetty
Quoted 82 lines · Rob Herring
Quoted 61 lines · Prasad Malisetty
Quoted 59 lines · Bjorn Helgaas
[+cc linux-pci; patches to
drivers/pci/should always be cc'd there]Quoted 45 lines · Stephen Boyd
Quoting Prasad Malisetty (2021-08-24 01:10:48)
Quoted 30 lines · Prasad Malisetty
Quoted 29 lines · Prasad Malisetty
Quoted 28 lines
On the SC7280, By default the clock source for
pcie_1_pipeis TCXO for gdsc enable. But after the PHY is initialized, the clock source must be switched togcc_pcie_1_pipe_clkfrom TCXO.Signed-off-by Prasad Malisetty <[email protected]>--- drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/pci/controller/dwc/pcie-qcom.c +2
-0@@ -166,6 +166,8 @@ 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; };union qcom_pcie_resources {@@ -1167,6 +1169,16 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)if (ret <0 )return ret;Quoted 13 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_7_0(struct · line 1181
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +9
-0+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 3 lines
I would like to check is there any other better approach instead of compatible method here as well or is it fine to use compatible method.
Quoted 4 lines
I'd prefer the compatible method. If nobody is responding then it's best to just resend the patches with the approach you prefer instead of waiting for someone to respond to a review comment.
I'm missing some context here, so I'm not exactly sure what your question is, Prasad, but IMO drivers generally should not need to use
of_device_is_compatible()if they've already calledof_device_get_match_data()(asqcom_pcie_probe()has).Quoted 4 lines
Quoted 4 lines
of_device_is_compatible()does basically the same work of looking for a match in qcom_pcie_match[] thatof_device_get_match_data()does, so it seems pointless to repeat it.+1
Quoted 2 lines
Quoted 2 lines
I am a little confused because while [1] adds "qcom,pcie-sc7280" to qcom,pcie.txt, I don't see a patch that adds it to qcom_pcie_match[].
Either that's missing or there's a fallback to 8250 that's not documented.
Quoted 5 lines
I agree on your point, but the main reason is to use compatible in
get_resources_2_7_0is same hardware version. For SM8250 & SC7280 platforms, the hw version is same. Since we can't have a separate ops for SC7280, we are using compatible method inget_resources_2_7_0to differentiate SM8250 and SC7280.Quoted 6 lines
Quoted 2 lines
Then fix the match data to be not just ops, but ops and the flag you need here.
This difference is not universal across all the platforms but instead this is specific to SC7280. Hence it make sense to use compatible other than going for a flag.
There's no reason your qcom_pcie_match[].data pointers need to be strictly based on the hardware version.
You can do something like what
pcie-brcmstb.cdoes, e.g.,struct pcie_cfg_data { struct qcom_pcie_ops *ops; unsigned int pipe_mux:1; }; static const struct pcie_cfg_data sm8250_cfg = { .ops = &ops_1_9_0, }; static const struct pcie_cfg_data sc7280_cfg = { .ops = &ops_1_9_0, .pipe_mux = 1, };Quoted 4 lines
static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },};
Quoted 4 lines
I have one quick query, If we use above approach, we should change platform data reading in PCIe probe to differentiate remaining platforms right. expect SM8250 and SC7280 all other platforms are using same
qcom_pcie_opsstructure pointer as data.
Yes. of_device_get_match_data() must return the same type of pointer (in the example above, "struct pcie_cfg_data *") for all platforms. So you would have to add a struct for each of them, and each struct would contain the ops pointer (&ops_1_0_0, &ops_2_1_0, etc).
Thanks for working on this!
Bjorn