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

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

[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@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.)

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

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

[Heiko Stübner]

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

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.

Nobody is using the controller pinctrl entries so far anyway :-) .


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

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

[Sam Edwards]

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

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

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

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

[Heiko Stübner]

Hi Jonathan, Sam,

Am Mittwoch, 5. Juni 2024, 21:45:42 CEST schrieb Jonathan Bennett:
> On 12/8/23 11:27 AM, Sam Edwards wrote:
> > On 12/8/23 04:05, Heiko Stübner wrote:
> >> 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@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.

Can you possibly test if

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=28b8d7793b8573563b3d45321376f36168d77b1e

changes anything? In 6.11-rc1 now.

The PERST# toggling happening before that patch could've caused
issues with your PCIe device.


Heiko

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

[Sam Edwards]

On Mon, Jul 29, 2024 at 12:09 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Jonathan, Sam,
>
> Am Mittwoch, 5. Juni 2024, 21:45:42 CEST schrieb Jonathan Bennett:
> > On 12/8/23 11:27 AM, Sam Edwards wrote:
> > > On 12/8/23 04:05, Heiko Stübner wrote:
> > >> 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@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.
>
> Can you possibly test if
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=28b8d7793b8573563b3d45321376f36168d77b1e
>
> changes anything? In 6.11-rc1 now.
>
> The PERST# toggling happening before that patch could've caused
> issues with your PCIe device.
>
>
> Heiko
>
>

Good day Heiko,

Thanks for thinking of this! But I believe that this problem is not
with PERST# but rather CLKREQ#:

Evidently, the RK3588's PCIe 3.0 (and not 2.0?) PHYs depend on the bus
reference clock not just for external transfers, but for completion of
some/all PIPE transactions as well. This means that, without that
external clock, the RC block stalls indefinitely even for trivial
things like reading the version information from the DBI range. This
is the boot hang that Jonathan and I are seeing.

I would guess that it hasn't been a problem for most RK3588 boards,
which either do not support PCIe 3.0 (and thus have these blocks
disabled in the DT) or have a fixed clock generator feeding the
necessary pins. However, many Jetson SoM-type boards (Turing RK1,
Mixtile Core 3588E [1], ...) have the clock switchable, requiring that
CLKREQ# be asserted/low to provide that to the RK3588.

Fortunately, the PCIe 3.0 PHY knows when it needs that clock and
provides the necessary CLKREQ# signal. But unfortunately, the current
DT does not mux this out to the board, causing this boot hang for only
those RK3588 boards.

I can still give 6.11-rc1 a whirl now if you think it has a strong
chance of mitigating the issue, or if not I can take another stab at
the DT patch in a few weeks once my availability for kernel hacking
returns. :)

Cheers,
Sam

[1]: https://downloads.mixtile.com/core3588e/file/core3588e_schematic_drawing_v1_1_0_20230922.pdf
sheet 19 of 20