| Message ID | 1690540760-20191-1-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Rob Herring |
| Headers | show |
| Series | [v1] arm64: dts: qcom: sc7280: Add PCIe0 node | expand |
On 28/07/2023 12:39, Krishna chaitanya chundru wrote: > Add PCIe dtsi node for PCIe0 controller on sc7280 platform. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> Thank you for your patch. There is something to discuss/improve. > + pcie0_phy: phy@1c06000 { > + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; > + reg = <0 0x01c06000 0 0x1c0>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, > + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, > + <&gcc GCC_PCIE_CLKREF_EN>, > + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; > + clock-names = "aux", "cfg_ahb", "ref", "refgen"; > + > + resets = <&gcc GCC_PCIE_0_PHY_BCR>; > + reset-names = "phy"; > + > + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; > + assigned-clock-rates = <100000000>; > + > + status = "disabled"; > + > + pcie0_lane: phy@1c0e6200 { Isn't this old-style of bindings? Wasn't there a change? On what tree did you base it? > + > + pcie0_wake_n: pcie0-wake-n { It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). Nodes end with 'state'. > + pins = "gpio89"; > + function = "gpio"; > + > + drive-strength = <2>; > + bias-pull-up; > + }; Best regards, Krzysztof
On 7/28/2023 5:33 PM, Krzysztof Kozlowski wrote: > On 28/07/2023 12:39, Krishna chaitanya chundru wrote: >> Add PCIe dtsi node for PCIe0 controller on sc7280 platform. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > Thank you for your patch. There is something to discuss/improve. > > >> + pcie0_phy: phy@1c06000 { >> + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; >> + reg = <0 0x01c06000 0 0x1c0>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, >> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, >> + <&gcc GCC_PCIE_CLKREF_EN>, >> + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >> + clock-names = "aux", "cfg_ahb", "ref", "refgen"; >> + >> + resets = <&gcc GCC_PCIE_0_PHY_BCR>; >> + reset-names = "phy"; >> + >> + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >> + assigned-clock-rates = <100000000>; >> + >> + status = "disabled"; >> + >> + pcie0_lane: phy@1c0e6200 { > Isn't this old-style of bindings? Wasn't there a change? On what tree > did you base it? Let me rebase and send it again. >> + >> + pcie0_wake_n: pcie0-wake-n { > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check` (see > Documentation/devicetree/bindings/writing-schema.rst or > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > for instructions). > > Nodes end with 'state'. I will run it send it again. Thanks, KC > >> + pins = "gpio89"; >> + function = "gpio"; >> + >> + drive-strength = <2>; >> + bias-pull-up; >> + }; > Best regards, > Krzysztof >
On 28/07/2023 17:10, Krishna Chaitanya Chundru wrote: > > On 7/28/2023 5:33 PM, Krzysztof Kozlowski wrote: >> On 28/07/2023 12:39, Krishna chaitanya chundru wrote: >>> Add PCIe dtsi node for PCIe0 controller on sc7280 platform. >>> >>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> Thank you for your patch. There is something to discuss/improve. >> >> >>> + pcie0_phy: phy@1c06000 { >>> + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; >>> + reg = <0 0x01c06000 0 0x1c0>; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + ranges; >>> + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, >>> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, >>> + <&gcc GCC_PCIE_CLKREF_EN>, >>> + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>> + clock-names = "aux", "cfg_ahb", "ref", "refgen"; >>> + >>> + resets = <&gcc GCC_PCIE_0_PHY_BCR>; >>> + reset-names = "phy"; >>> + >>> + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>> + assigned-clock-rates = <100000000>; >>> + >>> + status = "disabled"; >>> + >>> + pcie0_lane: phy@1c0e6200 { >> Isn't this old-style of bindings? Wasn't there a change? On what tree >> did you base it? The work was here: https://lore.kernel.org/all/20230324022514.1800382-5-dmitry.baryshkov@linaro.org/ But I don't remember the status. > Let me rebase and send it again. This anyway looks like wrong compatible. You used sm8250. Best regards, Krzysztof
On 7/28/2023 9:27 PM, Krzysztof Kozlowski wrote: > On 28/07/2023 17:10, Krishna Chaitanya Chundru wrote: >> On 7/28/2023 5:33 PM, Krzysztof Kozlowski wrote: >>> On 28/07/2023 12:39, Krishna chaitanya chundru wrote: >>>> Add PCIe dtsi node for PCIe0 controller on sc7280 platform. >>>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>> Thank you for your patch. There is something to discuss/improve. >>> >>> >>>> + pcie0_phy: phy@1c06000 { >>>> + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; >>>> + reg = <0 0x01c06000 0 0x1c0>; >>>> + #address-cells = <2>; >>>> + #size-cells = <2>; >>>> + ranges; >>>> + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, >>>> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, >>>> + <&gcc GCC_PCIE_CLKREF_EN>, >>>> + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>> + clock-names = "aux", "cfg_ahb", "ref", "refgen"; >>>> + >>>> + resets = <&gcc GCC_PCIE_0_PHY_BCR>; >>>> + reset-names = "phy"; >>>> + >>>> + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>> + assigned-clock-rates = <100000000>; >>>> + >>>> + status = "disabled"; >>>> + >>>> + pcie0_lane: phy@1c0e6200 { >>> Isn't this old-style of bindings? Wasn't there a change? On what tree >>> did you base it? > The work was here: > https://lore.kernel.org/all/20230324022514.1800382-5-dmitry.baryshkov@linaro.org/ > > But I don't remember the status. > >> Let me rebase and send it again. > This anyway looks like wrong compatible. You used sm8250. The patch was send on latest linux-next only and the above change is not merged yet. We are using the same compatible string as sm8250 because the phy is same both from hardware and software perspective for sm8250. that is why we are using the same compatible string. Can you let me know if we want create a separate compatible string for this even though we are using same phy? - KC. > > Best regards, > Krzysztof >
On 31/07/2023 07:29, Krishna Chaitanya Chundru wrote: > > On 7/28/2023 9:27 PM, Krzysztof Kozlowski wrote: >> On 28/07/2023 17:10, Krishna Chaitanya Chundru wrote: >>> On 7/28/2023 5:33 PM, Krzysztof Kozlowski wrote: >>>> On 28/07/2023 12:39, Krishna chaitanya chundru wrote: >>>>> Add PCIe dtsi node for PCIe0 controller on sc7280 platform. >>>>> >>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> Thank you for your patch. There is something to discuss/improve. >>>> >>>> >>>>> + pcie0_phy: phy@1c06000 { >>>>> + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; >>>>> + reg = <0 0x01c06000 0 0x1c0>; >>>>> + #address-cells = <2>; >>>>> + #size-cells = <2>; >>>>> + ranges; >>>>> + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, >>>>> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, >>>>> + <&gcc GCC_PCIE_CLKREF_EN>, >>>>> + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>>> + clock-names = "aux", "cfg_ahb", "ref", "refgen"; >>>>> + >>>>> + resets = <&gcc GCC_PCIE_0_PHY_BCR>; >>>>> + reset-names = "phy"; >>>>> + >>>>> + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>>> + assigned-clock-rates = <100000000>; >>>>> + >>>>> + status = "disabled"; >>>>> + >>>>> + pcie0_lane: phy@1c0e6200 { >>>> Isn't this old-style of bindings? Wasn't there a change? On what tree >>>> did you base it? >> The work was here: >> https://lore.kernel.org/all/20230324022514.1800382-5-dmitry.baryshkov@linaro.org/ >> >> But I don't remember the status. >> >>> Let me rebase and send it again. >> This anyway looks like wrong compatible. You used sm8250. > > The patch was send on latest linux-next only and the above change is not > merged yet. I don't think we will want old DTS syntax... but this actually depends on the status of Dmitry's patchset. > > We are using the same compatible string as sm8250 because the phy is > same both from hardware and software perspective for sm8250. > > that is why we are using the same compatible string. > > Can you let me know if we want create a separate compatible string for > this even though we are using same phy? https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#42 Best regards, Krzysztof
On 7/31/2023 12:24 PM, Krzysztof Kozlowski wrote: > On 31/07/2023 07:29, Krishna Chaitanya Chundru wrote: >> On 7/28/2023 9:27 PM, Krzysztof Kozlowski wrote: >>> On 28/07/2023 17:10, Krishna Chaitanya Chundru wrote: >>>> On 7/28/2023 5:33 PM, Krzysztof Kozlowski wrote: >>>>> On 28/07/2023 12:39, Krishna chaitanya chundru wrote: >>>>>> Add PCIe dtsi node for PCIe0 controller on sc7280 platform. >>>>>> >>>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>>> Thank you for your patch. There is something to discuss/improve. >>>>> >>>>> >>>>>> + pcie0_phy: phy@1c06000 { >>>>>> + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; >>>>>> + reg = <0 0x01c06000 0 0x1c0>; >>>>>> + #address-cells = <2>; >>>>>> + #size-cells = <2>; >>>>>> + ranges; >>>>>> + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, >>>>>> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, >>>>>> + <&gcc GCC_PCIE_CLKREF_EN>, >>>>>> + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>>>> + clock-names = "aux", "cfg_ahb", "ref", "refgen"; >>>>>> + >>>>>> + resets = <&gcc GCC_PCIE_0_PHY_BCR>; >>>>>> + reset-names = "phy"; >>>>>> + >>>>>> + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; >>>>>> + assigned-clock-rates = <100000000>; >>>>>> + >>>>>> + status = "disabled"; >>>>>> + >>>>>> + pcie0_lane: phy@1c0e6200 { >>>>> Isn't this old-style of bindings? Wasn't there a change? On what tree >>>>> did you base it? >>> The work was here: >>> https://lore.kernel.org/all/20230324022514.1800382-5-dmitry.baryshkov@linaro.org/ >>> >>> But I don't remember the status. >>> >>>> Let me rebase and send it again. >>> This anyway looks like wrong compatible. You used sm8250. >> The patch was send on latest linux-next only and the above change is not >> merged yet. > I don't think we will want old DTS syntax... but this actually depends > on the status of Dmitry's patchset. Kryzysztof, as the bindings also not yet merged shall we use old DTS syntax or shall I rebase on dmitry's patch and send it. - KC > >> We are using the same compatible string as sm8250 because the phy is >> same both from hardware and software perspective for sm8250. >> >> that is why we are using the same compatible string. >> >> Can you let me know if we want create a separate compatible string for >> this even though we are using same phy? > https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#42 > > > Best regards, > Krzysztof >
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index a0e8db8..8629575 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -868,7 +868,7 @@ reg = <0 0x00100000 0 0x1f0000>; clocks = <&rpmhcc RPMH_CXO_CLK>, <&rpmhcc RPMH_CXO_CLK_A>, <&sleep_clk>, - <0>, <&pcie1_lane>, + <&pcie0_lane>, <&pcie1_lane>, <0>, <0>, <0>, <0>; clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk", "pcie_0_pipe_clk", "pcie_1_pipe_clk", @@ -2088,6 +2088,123 @@ qcom,smem-state-names = "wlan-smp2p-out"; }; + pcie0: pci@1c00000 { + compatible = "qcom,pcie-sc7280"; + reg = <0 0x01c00000 0 0x3000>, + <0 0x60000000 0 0xf1d>, + <0 0x60000f20 0 0xa8>, + <0 0x60001000 0 0x1000>, + <0 0x60100000 0 0x100000>; + + reg-names = "parf", "dbi", "elbi", "atu", "config"; + device_type = "pci"; + linux,pci-domain = <0>; + bus-range = <0x00 0xff>; + num-lanes = <1>; + + #address-cells = <3>; + #size-cells = <2>; + + ranges = <0x01000000 0x0 0x00000000 0x0 0x60200000 0x0 0x100000>, + <0x02000000 0x0 0x60300000 0x0 0x60300000 0x0 0x3d00000>; + + interrupts = <GIC_SPI 141 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 0 0 149 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 2 &intc 0 0 0 150 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>, + <&gcc GCC_PCIE_0_PIPE_CLK_SRC>, + <&pcie0_lane>, + <&rpmhcc RPMH_CXO_CLK>, + <&gcc GCC_PCIE_0_AUX_CLK>, + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>, + <&gcc GCC_PCIE_0_SLV_AXI_CLK>, + <&gcc GCC_PCIE_0_SLV_Q2A_AXI_CLK>, + <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>, + <&gcc GCC_DDRSS_PCIE_SF_CLK>, + <&gcc GCC_AGGRE_NOC_PCIE_CENTER_SF_AXI_CLK>, + <&gcc GCC_AGGRE_NOC_PCIE_0_AXI_CLK>; + + clock-names = "pipe", + "pipe_mux", + "phy_pipe", + "ref", + "aux", + "cfg", + "bus_master", + "bus_slave", + "slave_q2a", + "tbu", + "ddrss_sf_tbu", + "aggre0", + "aggre1"; + + assigned-clocks = <&gcc GCC_PCIE_0_AUX_CLK>; + assigned-clock-rates = <19200000>; + + resets = <&gcc GCC_PCIE_0_BCR>; + reset-names = "pci"; + + power-domains = <&gcc GCC_PCIE_0_GDSC>; + + phys = <&pcie0_lane>; + phy-names = "pciephy"; + + pinctrl-names = "default"; + pinctrl-0 = <&pcie0_clkreq_n>, <&pcie0_reset_n>, <&pcie0_wake_n>; + + dma-coherent; + + interconnects = <&aggre1_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>, + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_PCIE_0 0>; + interconnect-names = "pcie-mem", "cpu-pcie"; + + iommu-map = <0x0 &apps_smmu 0x1c00 0x1>, + <0x100 &apps_smmu 0x1c01 0x1>; + + status = "disabled"; + }; + + pcie0_phy: phy@1c06000 { + compatible = "qcom,sm8250-qmp-gen3x1-pcie-phy"; + reg = <0 0x01c06000 0 0x1c0>; + #address-cells = <2>; + #size-cells = <2>; + ranges; + clocks = <&gcc GCC_PCIE_0_AUX_CLK>, + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, + <&gcc GCC_PCIE_CLKREF_EN>, + <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; + clock-names = "aux", "cfg_ahb", "ref", "refgen"; + + resets = <&gcc GCC_PCIE_0_PHY_BCR>; + reset-names = "phy"; + + assigned-clocks = <&gcc GCC_PCIE0_PHY_RCHNG_CLK>; + assigned-clock-rates = <100000000>; + + status = "disabled"; + + pcie0_lane: phy@1c0e6200 { + reg = <0 0x01c06200 0 0x200>, /* tx */ + <0 0x01c06400 0 0x200>, /* rx */ + <0 0x01c06800 0 0x200>, /* pcs */ + <0 0x01c06c00 0 0x200>; /* pcs_pcie */ + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>; + clock-names = "pipe0"; + + #phy-cells = <0>; + #clock-cells = <0>; + clock-output-names = "pcie_0_pipe_clk"; + }; + }; + pcie1: pci@1c08000 { compatible = "qcom,pcie-sc7280"; reg = <0 0x01c08000 0 0x3000>, @@ -4341,6 +4458,31 @@ function = "mi2s1_ws"; }; + pcie0_reset_n: pcie0-reset-n { + pins = "gpio87"; + function = "gpio"; + + drive-strength = <16>; + output-low; + bias-disable; + }; + + pcie0_wake_n: pcie0-wake-n { + pins = "gpio89"; + function = "gpio"; + + drive-strength = <2>; + bias-pull-up; + }; + + pcie0_clkreq_n: pcie0-clkreq-n-state { + pins = "gpio88"; + function = "pcie0_clkreqn"; + + bias-pull-up; + drive-strength = <2>; + }; + pcie1_clkreq_n: pcie1-clkreq-n-state { pins = "gpio79"; function = "pcie1_clkreqn";
Add PCIe dtsi node for PCIe0 controller on sc7280 platform. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- arch/arm64/boot/dts/qcom/sc7280.dtsi | 144 ++++++++++++++++++++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-)