Re: [PATCH 2/2] arm64: dts: qcom: ipq5210: Enable PCIe support

[sashiko-bot@kernel.org]

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