| Message ID | 20240401081302.942742-1-liujianfeng1994@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: remove startup-delay-us from vcc3v3_pcie2x1l0 on rock-5b | expand |
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
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
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
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.
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
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
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 >
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
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>; };
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(-) -- 2.34.1