[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 at gmail.com>
>>> ---
>>>
>>> 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 at 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>;
>>>           };
>>>
>>
>>
>>
>>
>