Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH 0/2] Enable PCIe controllers present in Qualcomm ipq5210
@ 2026-05-14  4:13 Varadarajan Narayanan
  2026-05-14  4:13 ` [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ipq9574: Document the ipq5210 pcie controller Varadarajan Narayanan
  2026-05-14  4:13 ` [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support Varadarajan Narayanan
  0 siblings, 2 replies; 4+ messages in thread
From: Varadarajan Narayanan @ 2026-05-14  4:13 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pci, devicetree, linux-kernel,
	Varadarajan Narayanan

Update the relevant bindings and DT files to enable support
for the PCIe phy and controller.

Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
---
Varadarajan Narayanan (2):
      dt-bindings: PCI: qcom,pcie-ipq9574: Document the ipq5210 pcie controller
      arm64: dts: qcom: ipq5210: Enable PCIe support

 .../devicetree/bindings/pci/qcom,pcie-ipq9574.yaml |   1 +
 arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts        |  43 ++++
 arch/arm64/boot/dts/qcom/ipq5210.dtsi              | 261 ++++++++++++++++++++-
 3 files changed, 303 insertions(+), 2 deletions(-)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260514-pci-ipq5210-977672bcad62
prerequisite-change-id: 20260514-pci-phy-38ec9b1c5a90:v1
prerequisite-patch-id: aea0d856b804b879fd5f24ea02f809b4dec84a30
prerequisite-patch-id: 26fb9a62b5ba0fc6275c0b081c7a16348d5a4fd1
prerequisite-change-id: 20260514-icc-ipq5210-0ab03f3a3e83:v1
prerequisite-patch-id: 0b6145b6635b18fe79fbbff5815041b43778c5ed
prerequisite-patch-id: 924c6ff7baf4283ac7991ee94c803a00fc5cece4
prerequisite-patch-id: c2fe1800fe769dccd37f94c19860a07f979e3c4c

Best regards,
-- 
Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ipq9574: Document the ipq5210 pcie controller
  2026-05-14  4:13 [PATCH 0/2] Enable PCIe controllers present in Qualcomm ipq5210 Varadarajan Narayanan
@ 2026-05-14  4:13 ` Varadarajan Narayanan
  2026-05-14  4:13 ` [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support Varadarajan Narayanan
  1 sibling, 0 replies; 4+ messages in thread
From: Varadarajan Narayanan @ 2026-05-14  4:13 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pci, devicetree, linux-kernel,
	Varadarajan Narayanan

Document the ipq5210 PCIe controller using ipq9574 as fallback compatible.

Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/pci/qcom,pcie-ipq9574.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ipq9574.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ipq9574.yaml
index 4be342cc04e1..a75ff554c283 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-ipq9574.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ipq9574.yaml
@@ -17,6 +17,7 @@ properties:
           - qcom,pcie-ipq9574
       - items:
           - enum:
+              - qcom,pcie-ipq5210
               - qcom,pcie-ipq5332
               - qcom,pcie-ipq5424
           - const: qcom,pcie-ipq9574

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support
  2026-05-14  4:13 [PATCH 0/2] Enable PCIe controllers present in Qualcomm ipq5210 Varadarajan Narayanan
  2026-05-14  4:13 ` [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ipq9574: Document the ipq5210 pcie controller Varadarajan Narayanan
@ 2026-05-14  4:13 ` Varadarajan Narayanan
  2026-05-14 12:33   ` sashiko-bot
  1 sibling, 1 reply; 4+ messages in thread
From: Varadarajan Narayanan @ 2026-05-14  4:13 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pci, devicetree, linux-kernel,
	Varadarajan Narayanan

Add DT entries to enable the PCIe controllers found in ipq5210.

Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts |  43 +++++
 arch/arm64/boot/dts/qcom/ipq5210.dtsi       | 261 +++++++++++++++++++++++++++-
 2 files changed, 302 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
index 941f866ecfe9..5e599a1cea3f 100644
--- a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
@@ -5,6 +5,7 @@
 
 /dts-v1/;
 
+#include <dt-bindings/gpio/gpio.h>
 #include "ipq5210.dtsi"
 
 / {
@@ -20,6 +21,32 @@ chosen {
 	};
 };
 
+&pcie0_phy {
+	status = "okay";
+};
+
+&pcie0_rp {
+	reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+};
+
+&pcie0 {
+	pinctrl-0 = <&pcie0_default_state>;
+	status = "okay";
+};
+
+&pcie1_phy {
+	status = "okay";
+};
+
+&pcie1_rp {
+	reset-gpios = <&tlmm 29 GPIO_ACTIVE_LOW>;
+};
+
+&pcie1 {
+	pinctrl-0 = <&pcie1_default_state>;
+	status = "okay";
+};
+
 &sdhc {
 	max-frequency = <192000000>;
 	bus-width = <4>;
@@ -36,6 +63,22 @@ &sleep_clk {
 };
 
 &tlmm {
+	pcie0_default_state: pcie0-default-state {
+		pins = "gpio32";
+		function = "gpio";
+		drive-strength = <6>;
+		bias-pull-down;
+		output-low;
+	};
+
+	pcie1_default_state: pcie1-default-state {
+		pins = "gpio29";
+		function = "gpio";
+		drive-strength = <6>;
+		bias-pull-down;
+		output-low;
+	};
+
 	qup_uart1_default_state: qup-uart1-default-state {
 		pins = "gpio38", "gpio39";
 		function = "qup_se1";
diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
index 3761eb03ab24..ec1c9a8c08e0 100644
--- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
@@ -5,6 +5,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,ipq5210-gcc.h>
+#include <dt-bindings/interconnect/qcom,ipq5210.h>
 #include <dt-bindings/reset/qcom,ipq5210-gcc.h>
 
 / {
@@ -13,6 +14,18 @@ / {
 	interrupt-parent = <&intc>;
 
 	clocks {
+		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
+			compatible = "fixed-clock";
+			clock-frequency = <250000000>;
+			#clock-cells = <0>;
+		};
+
+		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
+			compatible = "fixed-clock";
+			clock-frequency = <250000000>;
+			#clock-cells = <0>;
+		};
+
 		sleep_clk: sleep-clk {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
@@ -130,6 +143,54 @@ soc@0 {
 		dma-ranges = <0 0 0 0 0x10 0>;
 		ranges = <0 0 0 0 0x10 0>;
 
+		pcie0_phy: phy@84000 {
+			compatible = "qcom,ipq5210-qmp-gen3x1-pcie-phy",
+				     "qcom,ipq9574-qmp-gen3x1-pcie-phy";
+			reg = <0x0 0x00084000 0x0 0x1000>;
+
+			clocks = <&gcc GCC_PCIE0_AUX_CLK>,
+				 <&gcc GCC_PCIE0_AHB_CLK>,
+				 <&gcc GCC_PCIE0_PIPE_CLK>;
+			clock-names = "aux", "cfg_ahb", "pipe";
+
+			assigned-clocks = <&gcc GCC_PCIE0_AUX_CLK>;
+			assigned-clock-rates = <20000000>;
+
+			resets = <&gcc GCC_PCIE0_PHY_BCR>,
+				 <&gcc GCC_PCIE0PHY_PHY_BCR>;
+			reset-names = "phy", "common";
+
+			#clock-cells = <0>;
+			clock-output-names = "gcc_pcie0_pipe_clk_src";
+
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
+		pcie1_phy: phy@f4000 {
+			compatible = "qcom,ipq5210-qmp-gen3x2-pcie-phy",
+				     "qcom,ipq9574-qmp-gen3x2-pcie-phy";
+			reg = <0x0 0x000f4000 0x0 0x2000>;
+
+			clocks = <&gcc GCC_PCIE1_AUX_CLK>,
+				 <&gcc GCC_PCIE1_AHB_CLK>,
+				 <&gcc GCC_PCIE1_PIPE_CLK>;
+			clock-names = "aux", "cfg_ahb", "pipe";
+
+			assigned-clocks = <&gcc GCC_PCIE1_AUX_CLK>, <&gcc GCC_PCIE1_AHB_CLK>;
+			assigned-clock-rates = <20000000>, <100000000>;
+
+			resets = <&gcc GCC_PCIE1_PHY_BCR>,
+				 <&gcc GCC_PCIE1PHY_PHY_BCR>;
+			reset-names = "phy", "common";
+
+			#clock-cells = <0>;
+			clock-output-names = "gcc_pcie1_pipe_clk_src";
+
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5210-tlmm";
 			reg = <0x0 0x01000000 0x0 0x300000>;
@@ -146,8 +207,8 @@ gcc: clock-controller@1800000 {
 			reg = <0x0 0x01800000 0x0 0x40000>;
 			clocks = <&xo_board>,
 				 <&sleep_clk>,
-				 <0>,
-				 <0>,
+				 <&pcie30_phy0_pipe_clk>,
+				 <&pcie30_phy1_pipe_clk>,
 				 <0>,
 				 <0>;
 			#clock-cells = <1>;
@@ -299,6 +360,202 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		pcie1: pcie@50000000 {
+			compatible = "qcom,pcie-ipq5210", "qcom,pcie-ipq9574";
+			reg = <0x0 0x50000000 0x0 0xf1c>,
+			      <0x0 0x50000f20 0x0 0xa8>,
+			      <0x0 0x50001000 0x0 0x1000>,
+			      <0x0 0x000f0000 0x0 0x3000>,
+			      <0x0 0x50100000 0x0 0x1000>,
+			      <0x0 0x000f6000 0x0 0x1000>;
+			reg-names = "dbi",
+				    "elbi",
+				    "atu",
+				    "parf",
+				    "config",
+				    "mhi";
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x81000000 0x0 0x50200000 0x0 0x50200000 0x0 0x00100000>,
+				 <0x82000000 0x0 0x50300000 0x0 0x50300000 0x0 0x0fd00000>;
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &intc 0 0 GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &intc 0 0 GIC_SPI 202 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &intc 0 0 GIC_SPI 203 IRQ_TYPE_LEVEL_HIGH>;
+
+			interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi0",
+					  "msi1",
+					  "msi2",
+					  "msi3",
+					  "msi4",
+					  "msi5",
+					  "msi6",
+					  "msi7";
+
+			clocks = <&gcc GCC_PCIE1_AXI_M_CLK>,
+				 <&gcc GCC_PCIE1_AXI_S_CLK>,
+				 <&gcc GCC_PCIE1_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE1_RCHNG_CLK>,
+				 <&gcc GCC_PCIE1_AHB_CLK>,
+				 <&gcc GCC_PCIE1_AUX_CLK>;
+
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng",
+				      "ahb",
+				      "aux";
+
+			resets = <&gcc GCC_PCIE1_PIPE_ARES>,
+				 <&gcc GCC_PCIE1_CORE_STICKY_RESET>,
+				 <&gcc GCC_PCIE1_AXI_S_STICKY_RESET>,
+				 <&gcc GCC_PCIE1_AXI_S_ARES>,
+				 <&gcc GCC_PCIE1_AXI_M_STICKY_RESET>,
+				 <&gcc GCC_PCIE1_AXI_M_ARES>,
+				 <&gcc GCC_PCIE1_AUX_ARES>,
+				 <&gcc GCC_PCIE1_AHB_ARES>;
+
+			reset-names = "pipe",
+				      "sticky",
+				      "axi_s_sticky",
+				      "axi_s",
+				      "axi_m_sticky",
+				      "axi_m",
+				      "aux",
+				      "ahb";
+
+			interconnects = <&gcc MASTER_CNOC_PCIE1 &gcc SLAVE_CNOC_PCIE1>,
+					<&gcc MASTER_SNOC_PCIE1 &gcc SLAVE_SNOC_PCIE1>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
+			status = "disabled";
+
+			pcie1_rp: pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				bus-range = <0x01 0xff>;
+				phys = <&pcie1_phy>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
+		};
+
+		pcie0: pcie@70000000 {
+			compatible = "qcom,pcie-ipq5210", "qcom,pcie-ipq9574";
+			reg = <0x0 0x70000000 0x0 0xf1c>,
+			      <0x0 0x70000f20 0x0 0xa8>,
+			      <0x0 0x70001000 0x0 0x1000>,
+			      <0x0 0x00080000 0x0 0x3000>,
+			      <0x0 0x70100000 0x0 0x1000>,
+			      <0x0 0x00086000 0x0 0x1000>;
+			reg-names = "dbi",
+				    "elbi",
+				    "atu",
+				    "parf",
+				    "config",
+				    "mhi";
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x81000000 0x0 0x70200000 0x0 0x70200000 0x0 0x00100000>,
+				 <0x82000000 0x0 0x70300000 0x0 0x70300000 0x0 0x0fd00000>;
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &intc 0 0 GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &intc 0 0 GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &intc 0 0 GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+
+			interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 182 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi0",
+					  "msi1",
+					  "msi2",
+					  "msi3",
+					  "msi4",
+					  "msi5",
+					  "msi6",
+					  "msi7";
+
+			clocks = <&gcc GCC_PCIE0_AXI_M_CLK>,
+				 <&gcc GCC_PCIE0_AXI_S_CLK>,
+				 <&gcc GCC_PCIE0_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE0_RCHNG_CLK>,
+				 <&gcc GCC_PCIE0_AHB_CLK>,
+				 <&gcc GCC_PCIE0_AUX_CLK>;
+
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng",
+				      "ahb",
+				      "aux";
+
+			resets = <&gcc GCC_PCIE0_PIPE_ARES>,
+				 <&gcc GCC_PCIE0_CORE_STICKY_RESET>,
+				 <&gcc GCC_PCIE0_AXI_S_STICKY_RESET>,
+				 <&gcc GCC_PCIE0_AXI_S_ARES>,
+				 <&gcc GCC_PCIE0_AXI_M_STICKY_RESET>,
+				 <&gcc GCC_PCIE0_AXI_M_ARES>,
+				 <&gcc GCC_PCIE0_AUX_ARES>,
+				 <&gcc GCC_PCIE0_AHB_ARES>;
+
+			reset-names = "pipe",
+				      "sticky",
+				      "axi_s_sticky",
+				      "axi_s",
+				      "axi_m_sticky",
+				      "axi_m",
+				      "aux",
+				      "ahb";
+
+			interconnects = <&gcc MASTER_CNOC_PCIE0 &gcc SLAVE_CNOC_PCIE0>,
+					<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
+			status = "disabled";
+
+			pcie0_rp: pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				bus-range = <0x01 0xff>;
+				phys = <&pcie0_phy>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
+		};
 	};
 
 	timer {

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support
  2026-05-14  4:13 ` [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support Varadarajan Narayanan
@ 2026-05-14 12:33   ` sashiko-bot
  0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-05-14 12:33 UTC (permalink / raw)
  To: Varadarajan Narayanan; +Cc: krzk+dt, devicetree, linux-pci, conor+dt, robh

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The 'phys' property is incorrectly placed in the PCIe Root Port child node rather than the main PCIe controller node.
- [Medium] Dummy fixed clocks are used for the PCIe pipe clocks instead of the actual PHY nodes.
- [Medium] The `ranges` property in `pcie0` and `pcie1` incorrectly sets the child PCI address for I/O space to match the CPU address.
- [Low] The `pcie0` and `pcie1` nodes define `pinctrl-0` but omit the `pinctrl-names` property.
--

commit 6a61b5c948b9e28a9b86554a5e888347a3c559ab
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

arm64: dts: qcom: ipq5210: Enable PCIe support

Add DT entries to enable the PCIe controllers found in ipq5210.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> index 941f866ecfe95..5e599a1cea3f4 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
[ ... ]
> @@ -20,6 +21,32 @@ chosen {
>  	};
>  };
>  
> +&pcie0_phy {
> +	status = "okay";
> +};
> +
> +&pcie0_rp {
> +	reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> +};
> +
> +&pcie0 {
> +	pinctrl-0 = <&pcie0_default_state>;

Should this node also include a pinctrl-names property?

Omitting pinctrl-names = "default" might trigger validation warnings, even
if the pinctrl core falls back to index 0.

> +	status = "okay";
> +};
> +
> +&pcie1_phy {
> +	status = "okay";
> +};
> +
> +&pcie1_rp {
> +	reset-gpios = <&tlmm 29 GPIO_ACTIVE_LOW>;
> +};
> +
> +&pcie1 {
> +	pinctrl-0 = <&pcie1_default_state>;

Similarly, should pinctrl-names = "default" be added here?

> +	status = "okay";
> +};
> +
>  &sdhc {
>  	max-frequency = <192000000>;
>  	bus-width = <4>;
> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 3761eb03ab246..ec1c9a8c08e0a 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
[ ... ]
> @@ -13,6 +14,18 @@ / {
>  	interrupt-parent = <&intc>;
>  
>  	clocks {
> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +

Is it intentional to use dummy fixed clocks for the PCIe pipe clocks here?

The PCIe PHY nodes below (pcie0_phy, pcie1_phy) are correctly configured as
clock providers (#clock-cells = <0> and clock-output-names).

Hardcoding these as fixed clocks severs the dependency graph, so the GCC might
not establish a parent-child relationship with the PHY. Could this allow the
clock to be enabled without the PHY being powered on?

[ ... ]
> +		pcie1: pcie@50000000 {
[ ... ]
> +			#size-cells = <2>;
> +
> +			ranges = <0x81000000 0x0 0x50200000 0x0 0x50200000 0x0 0x00100000>,

Does this I/O space mapping correctly set the PCI child address?

This maps the CPU address 0x50200000 to PCI I/O address 0x50200000, but standard
PCI architectures typically expect the I/O port address space to start at
address 0 on the PCI bus side.

Should it translate the CPU I/O base address to the PCI I/O address 0?

> +				 <0x82000000 0x0 0x50300000 0x0 0x50300000 0x0 0x0fd00000>;
[ ... ]
> +			pcie1_rp: pcie@0 {
> +				device_type = "pci";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +				phys = <&pcie1_phy>;

Should the phys property be placed in the main PCIe controller node instead
of the Root Port child node?

The Qualcomm PCIe controller driver expects to find the phys property in the
main controller node using devm_phy_optional_get(dev, "pciephy").

If it is placed in the child node, devm_phy_optional_get() might return NULL
and silently bypass PHY initialization.

> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};
> +
> +		pcie0: pcie@70000000 {
[ ... ]
> +			#size-cells = <2>;
> +
> +			ranges = <0x81000000 0x0 0x70200000 0x0 0x70200000 0x0 0x00100000>,

Similar to pcie1, should this I/O space mapping translate the CPU I/O base
address to the PCI I/O address 0 instead of matching the CPU address?

> +				 <0x82000000 0x0 0x70300000 0x0 0x70300000 0x0 0x0fd00000>;
[ ... ]
> +			pcie0_rp: pcie@0 {
> +				device_type = "pci";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +				phys = <&pcie0_phy>;

Also similarly here, should this phys property belong to the parent pcie0
node instead of the pcie0_rp child node?

> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-pci-ipq5210-v1-0-a09436200b35@oss.qualcomm.com?part=2

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-05-14 12:33 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-14  4:13 [PATCH 0/2] Enable PCIe controllers present in Qualcomm ipq5210 Varadarajan Narayanan
2026-05-14  4:13 ` [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ipq9574: Document the ipq5210 pcie controller Varadarajan Narayanan
2026-05-14  4:13 ` [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support Varadarajan Narayanan
2026-05-14 12:33   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox