[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 at 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 at 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