Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> From: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> Add phy and controller nodes for pcie0_x1 and pcie1_x2.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v5: Add 'num-lanes' to "pcie1_phy: phy@4b1000"
>     Make ipq5332 as main and ipq9574 as fallback compatible
>     Move controller nodes per address
>     Having Konrad's Reviewed-By
> 
> v4: Remove 'reset-names' as driver uses bulk APIs
>     Remove 'clock-output-names' as driver uses bulk APIs
>     Add missing reset for pcie1_phy
>     Convert 'reg-names' to a vertical list
>     Move 'msi-map' before interrupts
> 
> v3: Fix compatible string for phy nodes
>     Use ipq9574 as backup compatible instead of new compatible for ipq5332
>     Fix mixed case hex addresses
>     Add "mhi" space
>     Removed unnecessary comments and stray blank lines
> 
> v2: Fix nodes' location per address
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 221 +++++++++++++++++++++++++-
>  1 file changed, 219 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index d3c3e215a15c..89daf955e4bd 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -186,6 +186,43 @@ rng: rng@e3000 {
>  			clock-names = "core";
>  		};
>  
> +		pcie0_phy: phy@4b0000 {
> +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> +			reg = <0x004b0000 0x800>;
> +
> +			clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>,
> +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK>;
> +
> +			resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>,
> +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>;
> +
> +			#clock-cells = <0>;
> +
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		pcie1_phy: phy@4b1000 {
> +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> +			reg = <0x004b1000 0x1000>;
> +
> +			clocks = <&gcc GCC_PCIE3X2_PIPE_CLK>,
> +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK>;
> +
> +			resets = <&gcc GCC_PCIE3X2_PHY_BCR>,
> +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2PHY_PHY_BCR>;
> +
> +			#clock-cells = <0>;
> +
> +			#phy-cells = <0>;
> +
> +			num-lanes = <2>;
> +
> +			status = "disabled";
> +		};
> +
>  		tlmm: pinctrl@1000000 {
>  			compatible = "qcom,ipq5332-tlmm";
>  			reg = <0x01000000 0x300000>;
> @@ -212,8 +249,8 @@ gcc: clock-controller@1800000 {
>  			#interconnect-cells = <1>;
>  			clocks = <&xo_board>,
>  				 <&sleep_clk>,
> -				 <0>,
> -				 <0>,
> +				 <&pcie1_phy>,
> +				 <&pcie0_phy>,
>  				 <0>;
>  		};
>  
> @@ -479,6 +516,186 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		pcie1: pcie@18000000 {

pcie@

> +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> +			reg = <0x00088000 0x3000>,
> +			      <0x18000000 0xf1d>,
> +			      <0x18000f20 0xa8>,
> +			      <0x18001000 0x1000>,
> +			      <0x18100000 0x1000>,
> +			      <0x0008b000 0x1000>;
> +			reg-names = "parf",
> +				    "dbi",
> +				    "elbi",
> +				    "atu",
> +				    "config",
> +				    "mhi";
> +			device_type = "pci";
> +			linux,pci-domain = <1>;
> +			bus-range = <0x00 0xff>;
> +			num-lanes = <2>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			ranges = <0x01000000 0 0x18200000 0x18200000 0 0x00100000>,

I/O address space should start from 0. Please refer other SoCs.

Also, use 0x0 for consistency.

> +				 <0x02000000 0 0x18300000 0x18300000 0 0x07d00000>;
> +
> +			msi-map = <0x0 &v2m0 0x0 0xffd>;
> +
> +			interrupts = <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "msi0",
> +					  "msi1",
> +					  "msi2",
> +					  "msi3",
> +					  "msi4",
> +					  "msi5",
> +					  "msi6",
> +					  "msi7";

Is there a 'global' interrupt? If so, please add it.

> +
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map = <0 0 0 1 &intc 0 0 412 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 2 &intc 0 0 413 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 3 &intc 0 0 414 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 4 &intc 0 0 415 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			clocks = <&gcc GCC_PCIE3X2_AXI_M_CLK>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_CLK>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_BRIDGE_CLK>,
> +				 <&gcc GCC_PCIE3X2_RCHG_CLK>,
> +				 <&gcc GCC_PCIE3X2_AHB_CLK>,
> +				 <&gcc GCC_PCIE3X2_AUX_CLK>;
> +			clock-names = "axi_m",
> +				      "axi_s",
> +				      "axi_bridge",
> +				      "rchng",
> +				      "ahb",
> +				      "aux";
> +
> +			resets = <&gcc GCC_PCIE3X2_PIPE_ARES>,
> +				 <&gcc GCC_PCIE3X2_CORE_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_M_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_M_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AUX_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AHB_CLK_ARES>;
> +			reset-names = "pipe",
> +				      "sticky",
> +				      "axi_s_sticky",
> +				      "axi_s",
> +				      "axi_m_sticky",
> +				      "axi_m",
> +				      "aux",
> +				      "ahb";
> +
> +			phys = <&pcie1_phy>;
> +			phy-names = "pciephy";
> +
> +			interconnects = <&gcc MASTER_SNOC_PCIE3_2_M &gcc SLAVE_SNOC_PCIE3_2_M>,
> +					<&gcc MASTER_ANOC_PCIE3_2_S &gcc SLAVE_ANOC_PCIE3_2_S>;
> +			interconnect-names = "pcie-mem", "cpu-pcie";

Can you check if the controller supports cache coherency? If so, you need to add
'dma-coherent'.

> +
> +			status = "disabled";

Please define the root port node as well.

All the above comments applies to 2nd controller node as well.

- Mani

-- 
மணிவண்ணன் சதாசிவம்