Re: [PATCH v2] arm64: dts: rockchip: Add PCIe pinctrls to Turing RK1

[Jonathan Bennett]

On 12/8/23 11:27 AM, Sam Edwards wrote:
> On 12/8/23 04:05, Heiko Stübner wrote:
> > Hi Sam,
> >
> > Am Freitag, 8. Dezember 2023, 07:25:10 CET schrieb Sam Edwards:
> > > The RK3588 PCIe 3.0 controller seems to have unpredictable behavior 
> > > when
> > > no CLKREQ/PERST/WAKE pins are configured in the pinmux. In 
> > > particular, it
> > > will sometimes (varying between specific RK3588 chips, not over 
> > > time) shut
> > > off the DBI block, and reads to this range will instead stall
> > > indefinitely.
> > >
> > > When this happens, it will prevent Linux from booting altogether. The
> > > PCIe driver will stall the CPU core once it attempts to read the 
> > > version
> > > information from the DBI range.
> > >
> > > Fix this boot hang by adding the correct pinctrl configuration to the
> > > PCIe 3.0 device node, which is the proper thing to do anyway. While
> > > we're at it, also add the necessary configuration to the PCIe 2.0 node,
> > > which may or may not fix the equivalent problem over there -- but is 
> > > the
> > > proper thing to do anyway. :)
> > >
> > > Fixes: 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM 
> > > support")
> > > Signed-off-by: Sam Edwards <CFSworks@xxxxxxxxx>
> > > ---
> > >
> > > Hi list,
> > >
> > > Compared to v1, v2 removes the `reset-gpios` properties as well -- 
> > > this should
> > > give control of the PCIe resets exclusively to the PCIe cores. (And 
> > > even if the
> > > `reset-gpios` props had no effect in v1, it'd be confusing to have 
> > > them there.)
> >
> > Hmm, I'd think this could result in differing behaviour.
> >
> > I.e. I tried the same on a different board with a nvme drive on the 
> > pci30x4
> > controller. But moving the reset from the gpio-way to "just" setting the
> > perstn pinctrl, simply hung the controller when probing the device.
>
> Ah, I'm guessing it died in:
> ver = dw_pcie_readl_dbi(pci, PCIE_VERSION_NUMBER);
>
> If so, that's the same hang that this patch is aiming to solve. I'm 
> starting to wonder if having muxed pins != 1 for a given signal upsets 
> the RC(s). Is your board (in an earlier boot stage: 
> reset/maskrom/bootloader) muxing a different perstn pin option to that 
> controller (and Linux doesn't know to clear it away)? Then when you 
> add the perstn pinctrl the total number of perstns muxed to the 
> controller comes to 2, and without a reset-gpios = <...>; to take it 
> away again, that number stays at 2 to cause the hang upon the DBI read?
>
> If so, that'd mean the change resolves the hang for me, because it 
> brings the total up to 1 (from 0), but also causes the hang for you, 
> because it brings the total up to 2 (away from 1).
>
> > So I guess I'd think the best way would be to split the pinctrl up 
> > into the
> > 3 separate functions (clkreqn, perstn, waken) so that boards can include
> > them individually.
>
> Mm, maybe. Though that might be more appropriate if a board comes 
> along that doesn't connect all of those signals to the same group of 
> pins. I worry that attempting to solve this hang by doing that will 
> instead just mask the real problem.
>
> > Nobody is using the controller pinctrl entries so far anyway :-) .
>
> Then it's interesting that this board requires them in order to avoid 
> a hang on boot. I'll see if there's anything else that I can find out.

I've just finished testing the latest iteration of this patch with 
6.10-rc2 on my RK1 on a Turing Pi 2 carrier board. I can confirm that 
unpatched mainline fails to boot with the same hang described here, and 
the patch does make the board boot again.


--Jonathan Bennett

> Happy Friday,
> Sam
>
> > Heiko
> >
> >
> > > Note that it is OK for the pcie2x1l1 node to refer to 
> > > pcie30x1m1_pins. The
> > > pcie2x1l1 device is *in fact* a PCIe 3.0 controller, and the 
> > > pcie30x1m1_pins
> > > pinmux setting is so-named to reflect this. The pcie2x1l1 node is 
> > > so-named
> > > because Linux does not (currently) support routing it to a PCIe 3.0 
> > > PHY; so in
> > > practice it is effectively a PCIe 2.0 controller, for the time being.
> > >
> > > Cheers and thank you for your time,
> > > Sam
> > >
> > > ---
> > >   .../boot/dts/rockchip/rk3588-turing-rk1.dtsi     | 16 
> > > ++--------------
> > >   1 file changed, 2 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi 
> > > b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
> > > index 9570b34aca2e..875446fdb67e 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
> > > @@ -214,8 +214,7 @@ rgmii_phy: ethernet-phy@1 {
> > >   &pcie2x1l1 {
> > >       linux,pci-domain = <1>;
> > >       pinctrl-names = "default";
> > > -    pinctrl-0 = <&pcie2_reset>;
> > > -    reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
> > > +    pinctrl-0 = <&pcie30x1m1_pins>;
> > >       status = "okay";
> > >   };
> > >   @@ -226,8 +225,7 @@ &pcie30phy {
> > >   &pcie3x4 {
> > >       linux,pci-domain = <0>;
> > >       pinctrl-names = "default";
> > > -    pinctrl-0 = <&pcie3_reset>;
> > > -    reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> > > +    pinctrl-0 = <&pcie30x4m1_pins>;
> > >       vpcie3v3-supply = <&vcc3v3_pcie30>;
> > >       status = "okay";
> > >   };
> > > @@ -245,17 +243,7 @@ hym8563_int: hym8563-int {
> > >           };
> > >       };
> > >   -    pcie2 {
> > > -        pcie2_reset: pcie2-reset {
> > > -            rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
> > > -        };
> > > -    };
> > > -
> > >       pcie3 {
> > > -        pcie3_reset: pcie3-reset {
> > > -            rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> > > -        };
> > > -
> > >           vcc3v3_pcie30_en: pcie3-reg {
> > >               rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
> > >           };