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

[Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>]

On Fri, May 22, 2026 at 02:24:45PM +0200, Konrad Dybcio wrote:
> 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)

Ok.

> [...]
>
> >  	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

Ok.

> [...]
>
> > +		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?

Will drop this.

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

Followed what was used in ipq9574, ipq5424 etc. Will remove gcc_ & _src.

> [...]
>
> > +			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

Ok.

> [...]
>
> > +			pcie1_rp: pcie@0 {
>
> pcie1_port0 for consistency with other DTs, please
>
> Same comments for the other port

Ok.

Thanks
Varada