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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 5/14/26 6:13 AM, Varadarajan Narayanan wrote:
> Add DT entries to enable the PCIe controllers found in ipq5210.
> 
> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
> ---

[...]

>  &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;

You shouldn't need output-low in either of these definitions (+ sorting
by GPIO idx would be extra neat)

[...]

>  	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>;
> +		};

Why do these exist? Just pass the QMPPHY reference straight to GCC

[...]

> +		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>;

Is this clock supposed to be fixed at that rate, regardless of the link
speed? And is the default rate incorrect?

> +
> +			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";

Having a gcc_ prefix here smells fishy.. 

[...]

> +			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",

stray \n above, also in resets

[...]

> +			pcie1_rp: pcie@0 {

pcie1_port0 for consistency with other DTs, please

Same comments for the other port

Konrad