[PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Jianfeng Liu]

Property startup-delay-us is copied from vendor dts and it will
make kernel not detect pcie wifi device. If I run command:
"echo 1 > /sys/bus/pci/rescan", pcie wifi device is detected, but
my wifi device RTL8822CE failed to load driver. Another device
RTL8723BE can load driver but no wifi signal is detected.

Removing this property will fix issues above.

Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index d6bf2ee07..a9af654a0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -76,7 +76,6 @@ vcc3v3_pcie2x1l0: vcc3v3-pcie2x1l0-regulator {
 		regulator-boot-on;
 		regulator-min-microvolt = <3300000>;
 		regulator-max-microvolt = <3300000>;
-		startup-delay-us = <50000>;
 		vin-supply = <&vcc5v0_sys>;
 	};

--
2.34.1

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Heiko Stübner]

Am Montag, 1. April 2024, 10:13:02 CEST schrieb Jianfeng Liu:
> Property startup-delay-us is copied from vendor dts and it will
> make kernel not detect pcie wifi device. If I run command:
> "echo 1 > /sys/bus/pci/rescan", pcie wifi device is detected, but
> my wifi device RTL8822CE failed to load driver. Another device
> RTL8723BE can load driver but no wifi signal is detected.
> 
> Removing this property will fix issues above.
> 
> Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index d6bf2ee07..a9af654a0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -76,7 +76,6 @@ vcc3v3_pcie2x1l0: vcc3v3-pcie2x1l0-regulator {
>  		regulator-boot-on;
>  		regulator-min-microvolt = <3300000>;
>  		regulator-max-microvolt = <3300000>;
> -		startup-delay-us = <50000>;
>  		vin-supply = <&vcc5v0_sys>;
>  	};

this somehow sounds like a hack around a deeper issue.
Because regulator_enable just delays its return by that delay
so the pcie driver should just after this return do the scanning?

Does the pcie driver enable the regulator too late somehow?


Heiko

Re: Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Jianfeng Liu]

Hi Heiko,

Tue, 02 Apr 2024 12:39:17 +0200, Heiko Stübner wrote:
>Does the pcie driver enable the regulator too late somehow?
The pcie driver will enable the regulator imediately when it is probed.
I added log at when driver is probed and when regulator is enabled.
Here is the log with "startup-delay-us = <50000>":
```
[    1.572991] rockchip-dw-pcie a40800000.pcie: rockchip_pcie_probe start
[    1.573697] rockchip-dw-pcie a40800000.pcie: going to enable vpcie3v3 regulator
[    1.575194] rockchip-dw-pcie a40800000.pcie: enable vpcie3v3 regulator done
```

And here is the log without "startup-delay-us":
```
[    1.518490] rockchip-dw-pcie a40800000.pcie: rockchip_pcie_probe start
[    1.518603] rockchip-dw-pcie a40800000.pcie: going to enable vpcie3v3 regulator
[    1.518610] rockchip-dw-pcie a40800000.pcie: enable vpcie3v3 regulator done
```

We can see startup-delay-us will delay the driver probe.

I also take a look at rockchip's SDK kernel, their pci driver is probed
very late:
```
[    3.398682] dw-pcie fe170000.pcie: invalid resource
[    3.398686] dw-pcie fe170000.pcie: Failed to initialize host
[    3.398688] dw-pcie: probe of fe170000.pcie failed with error -22
[    3.399396] rk-pcie fe170000.pcie: invalid prsnt-gpios property in node
[    3.399410] rk-pcie fe170000.pcie: Looking up vpcie3v3-supply from device tree
[    3.405195] rk-pcie fe170000.pcie: host bridge /pcie@fe170000 ranges:
[    3.405253] rk-pcie fe170000.pcie:       IO 0x00f2100000..0x00f21fffff -> 0x00f2100000
[    3.405283] rk-pcie fe170000.pcie:      MEM 0x00f2200000..0x00f2ffffff -> 0x00f2200000
[    3.405310] rk-pcie fe170000.pcie:      MEM 0x0980000000..0x09bfffffff -> 0x0980000000
[    3.405372] rk-pcie fe170000.pcie: iATU unroll: enabled
[    3.405381] rk-pcie fe170000.pcie: iATU regions: 8 ob, 8 ib, align 64K, limit 8G
[    3.666917] rk-pcie fe170000.pcie: PCIe Link up, LTSSM is 0x30011
[    3.666932] rk-pcie fe170000.pcie: PCIe Gen.1 x1 link up
[    3.667139] rk-pcie fe170000.pcie: PCI host bridge to bus 0002:20
```

And it is reported that startup-delay-us is necessary in rockchip's SDK
kernel. But in mainline kernel it is different.

Jianfeng

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Heiko Stuebner]

Am Mittwoch, 3. April 2024, 09:59:16 CEST schrieb Jianfeng Liu:
> Hi Heiko,
> 
> Tue, 02 Apr 2024 12:39:17 +0200, Heiko Stübner wrote:
> >Does the pcie driver enable the regulator too late somehow?
> The pcie driver will enable the regulator imediately when it is probed.
> I added log at when driver is probed and when regulator is enabled.
> Here is the log with "startup-delay-us = <50000>":
> ```
> [    1.572991] rockchip-dw-pcie a40800000.pcie: rockchip_pcie_probe start
> [    1.573697] rockchip-dw-pcie a40800000.pcie: going to enable vpcie3v3 regulator
> [    1.575194] rockchip-dw-pcie a40800000.pcie: enable vpcie3v3 regulator done
> ```
> 
> And here is the log without "startup-delay-us":
> ```
> [    1.518490] rockchip-dw-pcie a40800000.pcie: rockchip_pcie_probe start
> [    1.518603] rockchip-dw-pcie a40800000.pcie: going to enable vpcie3v3 regulator
> [    1.518610] rockchip-dw-pcie a40800000.pcie: enable vpcie3v3 regulator done
> ```
> 
> We can see startup-delay-us will delay the driver probe.
> 
> I also take a look at rockchip's SDK kernel, their pci driver is probed
> very late:
> ```
> [    3.398682] dw-pcie fe170000.pcie: invalid resource
> [    3.398686] dw-pcie fe170000.pcie: Failed to initialize host
> [    3.398688] dw-pcie: probe of fe170000.pcie failed with error -22
> [    3.399396] rk-pcie fe170000.pcie: invalid prsnt-gpios property in node
> [    3.399410] rk-pcie fe170000.pcie: Looking up vpcie3v3-supply from device tree
> [    3.405195] rk-pcie fe170000.pcie: host bridge /pcie@fe170000 ranges:
> [    3.405253] rk-pcie fe170000.pcie:       IO 0x00f2100000..0x00f21fffff -> 0x00f2100000
> [    3.405283] rk-pcie fe170000.pcie:      MEM 0x00f2200000..0x00f2ffffff -> 0x00f2200000
> [    3.405310] rk-pcie fe170000.pcie:      MEM 0x0980000000..0x09bfffffff -> 0x0980000000
> [    3.405372] rk-pcie fe170000.pcie: iATU unroll: enabled
> [    3.405381] rk-pcie fe170000.pcie: iATU regions: 8 ob, 8 ib, align 64K, limit 8G
> [    3.666917] rk-pcie fe170000.pcie: PCIe Link up, LTSSM is 0x30011
> [    3.666932] rk-pcie fe170000.pcie: PCIe Gen.1 x1 link up
> [    3.667139] rk-pcie fe170000.pcie: PCI host bridge to bus 0002:20
> ```
> 
> And it is reported that startup-delay-us is necessary in rockchip's SDK
> kernel. But in mainline kernel it is different.

that's not directly what I meant.

I.e. if the behaviour changes with arbitary delay changes, it points
very much to some sort of timing issue in the pcie driver itself.

That's why I asked about the regulator turning on, because if the enable
call returns 50ms earlier or later should never influence the behaviour
of the driver.

For example other threads could also just hinder the kernel from
continuing the pcie probe even after the regulator is on - again
leading to undefined behaviour, as you seem to be experiencing as
described in your mail from yesterday.

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Jianfeng Liu]

After removing this property I still meet the wifi card missing issue
today. So this fix doesn't hit the root cause. But this property from
rockchip's sdk kernel will definitely make the situation worse.

Jianfeng

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Shawn Lin]

Hi Jianfeng,

On 2024/4/1 16:13, Jianfeng Liu wrote:
> Property startup-delay-us is copied from vendor dts and it will
> make kernel not detect pcie wifi device. If I run command:
> "echo 1 > /sys/bus/pci/rescan", pcie wifi device is detected, but
> my wifi device RTL8822CE failed to load driver. Another device
> RTL8723BE can load driver but no wifi signal is detected.
> 
> Removing this property will fix issues above.
> 
> Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>

startup-delay-us just make sure the power rail is stable before
any action is taken to start the link, preventing the device from
unable to work stably. So it shouldn't be the root cause I think.

Could you help try this patch to checkout if it works for you?

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c 
b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index d684214..df30127 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -167,7 +167,7 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
         struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);

         /* Reset device */
-       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
+       //gpiod_set_value_cansleep(rockchip->rst_gpio, 0);

         rockchip_pcie_enable_ltssm(rockchip);

@@ -180,7 +180,7 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
          * We need more extra time as before, rather than setting just
          * 100us as we don't know how long should the device need to reset.
          */
-       msleep(100);
+       msleep(300);
         gpiod_set_value_cansleep(rockchip->rst_gpio, 1);

         return 0;
@@ -311,6 +311,8 @@ static int rockchip_pcie_probe(struct 
platform_device *pdev)
         if (ret)
                 return ret;

+       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
+
         /* DON'T MOVE ME: must be enable before PHY init */
         rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");



> ---
>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index d6bf2ee07..a9af654a0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -76,7 +76,6 @@ vcc3v3_pcie2x1l0: vcc3v3-pcie2x1l0-regulator {
>   		regulator-boot-on;
>   		regulator-min-microvolt = <3300000>;
>   		regulator-max-microvolt = <3300000>;
> -		startup-delay-us = <50000>;
>   		vin-supply = <&vcc5v0_sys>;
>   	};
> 
> --
> 2.34.1
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Jianfeng Liu]

Hi Shawn,

Wed, 10 Apr 2024 14:30:16 +0800, Shawn Lin wrote:
>Could you help try this patch to checkout if it works for you?

This patch works for me. Thank you for pointing me to the right path!
Looking forward to your fix.

Jianfeng

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]

Hi,

On Wed, Apr 10, 2024 at 02:30:16PM +0800, Shawn Lin wrote:
> Hi Jianfeng,
> 
> On 2024/4/1 16:13, Jianfeng Liu wrote:
> > Property startup-delay-us is copied from vendor dts and it will
> > make kernel not detect pcie wifi device. If I run command:
> > "echo 1 > /sys/bus/pci/rescan", pcie wifi device is detected, but
> > my wifi device RTL8822CE failed to load driver. Another device
> > RTL8723BE can load driver but no wifi signal is detected.
> > 
> > Removing this property will fix issues above.
> > 
> > Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> 
> startup-delay-us just make sure the power rail is stable before
> any action is taken to start the link, preventing the device from
> unable to work stably. So it shouldn't be the root cause I think.
> 
> Could you help try this patch to checkout if it works for you?
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index d684214..df30127 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -167,7 +167,7 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
>         struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> 
>         /* Reset device */
> -       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> +       //gpiod_set_value_cansleep(rockchip->rst_gpio, 0);

Is this removal actually needed?

> 
>         rockchip_pcie_enable_ltssm(rockchip);
> 
> @@ -180,7 +180,7 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
>          * We need more extra time as before, rather than setting just
>          * 100us as we don't know how long should the device need to reset.
>          */
> -       msleep(100);
> +       msleep(300);
>         gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> 
>         return 0;
> @@ -311,6 +311,8 @@ static int rockchip_pcie_probe(struct platform_device
> *pdev)
>         if (ret)
>                 return ret;
> 
> +       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);

I suppose it makes sense to use GPIOD_OUT_LOW in
rockchip_pcie_resource_get(), so that the GPIO is requested low from
the start instead of being high for a very short amount of time.

Greetings,

-- Sebastian

> +
>         /* DON'T MOVE ME: must be enable before PHY init */
>         rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> 
> 
> 
> > ---
> >   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index d6bf2ee07..a9af654a0 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -76,7 +76,6 @@ vcc3v3_pcie2x1l0: vcc3v3-pcie2x1l0-regulator {
> >   		regulator-boot-on;
> >   		regulator-min-microvolt = <3300000>;
> >   		regulator-max-microvolt = <3300000>;
> > -		startup-delay-us = <50000>;
> >   		vin-supply = <&vcc5v0_sys>;
> >   	};
> > 
> > --
> > 2.34.1
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b

[Marc Giger]

Hi,

On Fri, 12 Apr 2024 18:09:13 +0200
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:

> Hi,
> 
> On Wed, Apr 10, 2024 at 02:30:16PM +0800, Shawn Lin wrote:
> > Hi Jianfeng,
> > 
> > On 2024/4/1 16:13, Jianfeng Liu wrote:
> > > Property startup-delay-us is copied from vendor dts and it will
> > > make kernel not detect pcie wifi device. If I run command:
> > > "echo 1 > /sys/bus/pci/rescan", pcie wifi device is detected, but
> > > my wifi device RTL8822CE failed to load driver. Another device
> > > RTL8723BE can load driver but no wifi signal is detected.
> > > 
> > > Removing this property will fix issues above.
> > > 
> > > Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> > 
> > startup-delay-us just make sure the power rail is stable before
> > any action is taken to start the link, preventing the device from
> > unable to work stably. So it shouldn't be the root cause I think.
> > 
> > Could you help try this patch to checkout if it works for you?
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index d684214..df30127 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -167,7 +167,7 @@ static int rockchip_pcie_start_link(struct
> > dw_pcie *pci) struct rockchip_pcie *rockchip =
> > to_rockchip_pcie(pci);
> > 
> >         /* Reset device */
> > -       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> > +       //gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> 
> Is this removal actually needed?
> 
> > 
> >         rockchip_pcie_enable_ltssm(rockchip);
> > 
> > @@ -180,7 +180,7 @@ static int rockchip_pcie_start_link(struct
> > dw_pcie *pci)
> >          * We need more extra time as before, rather than setting
> > just
> >          * 100us as we don't know how long should the device need
> > to reset. */
> > -       msleep(100);
> > +       msleep(300);
> >         gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> > 
> >         return 0;
> > @@ -311,6 +311,8 @@ static int rockchip_pcie_probe(struct
> > platform_device *pdev)
> >         if (ret)
> >                 return ret;
> > 
> > +       gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> 
> I suppose it makes sense to use GPIOD_OUT_LOW in
> rockchip_pcie_resource_get(), so that the GPIO is requested low from
> the start instead of being high for a very short amount of time.
> 

I see the very same issue as the original reporter on a Orange Pi 5
(rk3588s) and a Orange Pi 5 plus (rk3588). In my case the onboard
ethernet interfaces and/or the nvme drive are randomly not
initialized properly. This is with Linux 6.8.9. Funny enough it seemed
to work all the time on 6.8.4 without any issued (having the bifurcation
patches applied).

After applying the following patch (with the incorporated suggestions
from Sebastian) everything seems to work on the Opi5 plus but on the
Opi5 the nvme drive still wan't to properly initialize.

In dmesg the only difference in repect to pcie/nvme I can see is that on
a working system the following to lines are shown whereby they are
missing when the nvme was not initialized properly:

nvme nvme0: Shutdown timeout set to 10 seconds
nvme nvme0: 8/0/0 default/read/poll queues



---
debian-linux.orig/drivers/pci/controller/dwc/pcie-dw-rockchip.c
2024-05-02 14:35:35.000000000 +0000 +++
debian-linux/drivers/pci/controller/dwc/pcie-dw-rockchip.c
2024-05-14 19:25:18.519434456 +0000 @@ -180,7 +180,7 @@
 	 * We need more extra time as before, rather than setting just
 	 * 100us as we don't know how long should the device need to
reset. */
-	msleep(100);
+	msleep(300);
 	gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
 
 	return 0;
@@ -240,7 +240,7 @@
 		return PTR_ERR(rockchip->apb_base);
 
 	rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev,
"reset",
-						     GPIOD_OUT_HIGH);
+						     GPIOD_OUT_LOW);
 	if (IS_ERR(rockchip->rst_gpio))
 		return PTR_ERR(rockchip->rst_gpio);
 
@@ -311,6 +311,8 @@
 	if (ret)
 		return ret;
 
+	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
+
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev,
"vpcie3v3"); if (IS_ERR(rockchip->vpcie3v3)) {


Thanks,

Marc