Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Fri, Aug 09, 2024 at 01:06:09PM +0530, Anand Moon wrote:
> Rockchip DWC PCIe driver currently waits for the combo PHY link
> (PCIe 3.0, PCIe 2.0, and SATA 3.0) to be established link training
> during boot, it also waits for the link to be up, which could consume
> several milliseconds during boot.
>
> To optimize boot time, this commit allows asynchronous probing.
> This change enables the PCIe link establishment to occur in the
> background while other devices are being probed.
>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v2: update the commit message to describe the changs.
> ---

Hello Anand,

I tried this patch.

It gives me the following splat on rock5b (rk3588):

[    1.412108] WARNING: CPU: 5 PID: 59 at kernel/module/kmod.c:143 __request_module+0x1c0/0x298
[    1.412853] Modules linked in:
[    1.413125] CPU: 5 UID: 0 PID: 59 Comm: kworker/u32:1 Not tainted 6.13.0-rc1+ #38
[    1.413781] Hardware name: Radxa ROCK 5B (DT)
[    1.414163] Workqueue: async async_run_entry_fn
[    1.414565] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.415175] pc : __request_module+0x1c0/0x298
[    1.415559] lr : __request_module+0x1bc/0x298
[    1.415943] sp : ffff8000804333f0
[    1.416234] x29: ffff800080433470 x28: ffff42bec2e40000 x27: ffff42bec2e400c8
[    1.416860] x26: ffff42bec1739000 x25: ffffb5bec9400e18 x24: 0000000000000000
[    1.417485] x23: ffffb5bec93e1a90 x22: 0000000000000001 x21: ffffb5bec74298f8
[    1.418111] x20: ffff800080433620 x19: ffff800080433410 x18: 0000000000000006
[    1.418736] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[    1.419360] x14: 0000000000000001 x13: 0000000000000000 x12: 0000000000000000
[    1.419985] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffb5bec750b834
[    1.420611] x8 : ffff800080433468 x7 : 0000000000000000 x6 : 0000000000000000
[    1.421235] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000030
[    1.421860] x2 : 0000000000000008 x1 : ffffb5bec750b708 x0 : 0000000000000001
[    1.422486] Call trace:
[    1.422701]  __request_module+0x1c0/0x298 (P)
[    1.423086]  __request_module+0x1bc/0x298 (L)
[    1.423471]  phy_request_driver_module+0x120/0x178
[    1.423895]  phy_device_create+0x230/0x250
[    1.424257]  get_phy_device+0x80/0x168
[    1.424588]  mdiobus_scan+0x20/0xa0
[    1.424896]  __mdiobus_register+0x21c/0x460
[    1.425265]  __devm_mdiobus_register+0x78/0xf8
[    1.425657]  rtl_init_one+0x7c8/0x1140
[    1.425989]  local_pci_probe+0x48/0xc0
[    1.426323]  pci_device_probe+0xcc/0x248
[    1.426671]  really_probe+0xc4/0x2d0
[    1.426989]  __driver_probe_device+0x80/0x130
[    1.427374]  driver_probe_device+0x44/0x168
[    1.427745]  __device_attach_driver+0xc0/0x148
[    1.428138]  bus_for_each_drv+0x90/0x100
[    1.428486]  __device_attach+0xa8/0x1a0
[    1.428826]  device_attach+0x1c/0x38
[    1.429143]  pci_bus_add_device+0xb4/0x1e0
[    1.429505]  pci_bus_add_devices+0x48/0xa0
[    1.429867]  pci_bus_add_devices+0x74/0xa0
[    1.430228]  pci_host_probe+0x94/0x100
[    1.430560]  dw_pcie_host_init+0x258/0x720
[    1.430923]  rockchip_pcie_probe+0x2ec/0x510
[    1.431300]  platform_probe+0x70/0xe8
[    1.431623]  really_probe+0xc4/0x2d0
[    1.431940]  __driver_probe_device+0x80/0x130
[    1.432326]  driver_probe_device+0x44/0x168
[    1.432696]  __device_attach_driver+0xc0/0x148
[    1.433089]  bus_for_each_drv+0x90/0x100
[    1.433436]  __device_attach_async_helper+0xbc/0xe8
[    1.433865]  async_run_entry_fn+0x3c/0xf0
[    1.434219]  process_one_work+0x158/0x3c8
[    1.434574]  worker_thread+0x2d4/0x3f8
[    1.434907]  kthread+0x118/0x128
[    1.435193]  ret_from_fork+0x10/0x20


Perhaps we should defer this patch until phylib core has been fixed?

For more info, see:
https://lore.kernel.org/netdev/Z3fJQEVV4ACpvP3L@ryzen/T/#u


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas,

On Fri, 3 Jan 2025 at 17:01, Niklas Cassel <cassel@kernel.org> wrote:
>
> On Fri, Aug 09, 2024 at 01:06:09PM +0530, Anand Moon wrote:
> > Rockchip DWC PCIe driver currently waits for the combo PHY link
> > (PCIe 3.0, PCIe 2.0, and SATA 3.0) to be established link training
> > during boot, it also waits for the link to be up, which could consume
> > several milliseconds during boot.
> >
> > To optimize boot time, this commit allows asynchronous probing.
> > This change enables the PCIe link establishment to occur in the
> > background while other devices are being probed.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v2: update the commit message to describe the changs.
> > ---
>
> Hello Anand,
>
> I tried this patch.
>
> It gives me the following splat on rock5b (rk3588):
>
> [    1.412108] WARNING: CPU: 5 PID: 59 at kernel/module/kmod.c:143 __request_module+0x1c0/0x298
> [    1.412853] Modules linked in:
> [    1.413125] CPU: 5 UID: 0 PID: 59 Comm: kworker/u32:1 Not tainted 6.13.0-rc1+ #38
> [    1.413781] Hardware name: Radxa ROCK 5B (DT)
> [    1.414163] Workqueue: async async_run_entry_fn
> [    1.414565] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    1.415175] pc : __request_module+0x1c0/0x298
> [    1.415559] lr : __request_module+0x1bc/0x298
> [    1.415943] sp : ffff8000804333f0
> [    1.416234] x29: ffff800080433470 x28: ffff42bec2e40000 x27: ffff42bec2e400c8
> [    1.416860] x26: ffff42bec1739000 x25: ffffb5bec9400e18 x24: 0000000000000000
> [    1.417485] x23: ffffb5bec93e1a90 x22: 0000000000000001 x21: ffffb5bec74298f8
> [    1.418111] x20: ffff800080433620 x19: ffff800080433410 x18: 0000000000000006
> [    1.418736] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [    1.419360] x14: 0000000000000001 x13: 0000000000000000 x12: 0000000000000000
> [    1.419985] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffb5bec750b834
> [    1.420611] x8 : ffff800080433468 x7 : 0000000000000000 x6 : 0000000000000000
> [    1.421235] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000030
> [    1.421860] x2 : 0000000000000008 x1 : ffffb5bec750b708 x0 : 0000000000000001
> [    1.422486] Call trace:
> [    1.422701]  __request_module+0x1c0/0x298 (P)
> [    1.423086]  __request_module+0x1bc/0x298 (L)
> [    1.423471]  phy_request_driver_module+0x120/0x178
> [    1.423895]  phy_device_create+0x230/0x250
> [    1.424257]  get_phy_device+0x80/0x168
> [    1.424588]  mdiobus_scan+0x20/0xa0
> [    1.424896]  __mdiobus_register+0x21c/0x460
> [    1.425265]  __devm_mdiobus_register+0x78/0xf8
> [    1.425657]  rtl_init_one+0x7c8/0x1140
> [    1.425989]  local_pci_probe+0x48/0xc0
> [    1.426323]  pci_device_probe+0xcc/0x248
> [    1.426671]  really_probe+0xc4/0x2d0
> [    1.426989]  __driver_probe_device+0x80/0x130
> [    1.427374]  driver_probe_device+0x44/0x168
> [    1.427745]  __device_attach_driver+0xc0/0x148
> [    1.428138]  bus_for_each_drv+0x90/0x100
> [    1.428486]  __device_attach+0xa8/0x1a0
> [    1.428826]  device_attach+0x1c/0x38
> [    1.429143]  pci_bus_add_device+0xb4/0x1e0
> [    1.429505]  pci_bus_add_devices+0x48/0xa0
> [    1.429867]  pci_bus_add_devices+0x74/0xa0
> [    1.430228]  pci_host_probe+0x94/0x100
> [    1.430560]  dw_pcie_host_init+0x258/0x720
> [    1.430923]  rockchip_pcie_probe+0x2ec/0x510
> [    1.431300]  platform_probe+0x70/0xe8
> [    1.431623]  really_probe+0xc4/0x2d0
> [    1.431940]  __driver_probe_device+0x80/0x130
> [    1.432326]  driver_probe_device+0x44/0x168
> [    1.432696]  __device_attach_driver+0xc0/0x148
> [    1.433089]  bus_for_each_drv+0x90/0x100
> [    1.433436]  __device_attach_async_helper+0xbc/0xe8
> [    1.433865]  async_run_entry_fn+0x3c/0xf0
> [    1.434219]  process_one_work+0x158/0x3c8
> [    1.434574]  worker_thread+0x2d4/0x3f8
> [    1.434907]  kthread+0x118/0x128
> [    1.435193]  ret_from_fork+0x10/0x20
>
>
> Perhaps we should defer this patch until phylib core has been fixed?
>
> For more info, see:
> https://lore.kernel.org/netdev/Z3fJQEVV4ACpvP3L@ryzen/T/#u
>

Thanks for testing this patch.

This patch should have been tested on hardware that includes all the
relevant controllers,
such as PCI 2.0, PCI 3.0, and the SATA controller.
I will test this patch again on all the Radxa devices I have.

This patch's dependency lies in deferring the probe until the PHY
controller initializes.

CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m

To my surprise, we have not enabled mdio on Rock-5B boards.
can you check if these changes work on your end?

-----8<----------8<----------8<----------8<----------8<----------8<-----
alarm@rock-5b:/media/nvme0/mainline/linux-rockchip-6.y-devel$ git diff
   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index c44d001da169..5008a05efd2a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -155,6 +155,19 @@ vcc_1v1_nldo_s3: regulator-vcc-1v1-nldo-s3 {
        };
 };

+&mdio1 {
+       rgmii_phy1: ethernet-phy@1 {
+               /* RTL8211F */
+               compatible = "ethernet-phy-id001c.c916";
+               reg = <0x1>;
+               pinctrl-names = "default";
+               pinctrl-0 = <&rtl8211f_rst>;
+               reset-assert-us = <20000>;
+               reset-deassert-us = <100000>;
+               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
+       };
+};
+
 &combphy0_ps {
        status = "okay";
 };
@@ -224,6 +237,21 @@ &hdptxphy_hdmi0 {
        status = "okay";
 };

+&gmac1 {
+       clock_in_out = "output";
+       phy-handle = <&rgmii_phy1>;
+       phy-mode = "rgmii";
+       pinctrl-0 = <&gmac1_miim
+                    &gmac1_tx_bus2
+                    &gmac1_rx_bus2
+                    &gmac1_rgmii_clk
+                    &gmac1_rgmii_bus>;
+       pinctrl-names = "default";
+       tx_delay = <0x3a>;
+       rx_delay = <0x3e>;
+       status = "okay";
+};
+
 &i2c0 {
        pinctrl-names = "default";
        pinctrl-0 = <&i2c0m2_xfer>;
@@ -419,6 +447,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
                };
        };

+       rtl8211f {
+               rtl8211f_rst: rtl8211f-rst {
+                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+               };
+       };
+
        usb {
                vcc5v0_host_en: vcc5v0-host-en {
                        rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
>
> Kind regards,
> Niklas

Can you check this on your end

alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
[sudo] password for alarm:
fc400000.usb    dwc3: failed to initialize core
alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

Hello Anand,

On Fri, Jan 03, 2025 at 07:24:07PM +0530, Anand Moon wrote:
> 
> Thanks for testing this patch.
> 
> This patch should have been tested on hardware that includes all the
> relevant controllers,
> such as PCI 2.0, PCI 3.0, and the SATA controller.
> I will test this patch again on all the Radxa devices I have.
> 
> This patch's dependency lies in deferring the probe until the PHY
> controller initializes.
> 
> CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m


Note that the splat, as reported in this thread, and in:
https://lore.kernel.org/netdev/20250101235122.704012-1-francesco@valla.it/T/

is related to the network PHY (CONFIG_REALTEK_PHY) on the RTL8125 NIC,
which is connected to one of the PCIe Gen2 controllers, not the PCIe PHY
on the PCIe controller (CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) itself.

For the record, I run with all the relevant drivers as built-in:
CONFIG_PCIE_ROCKCHIP_DW_HOST=y
CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y (for the PCIe Gen2 controllers)
CONFIG_PHY_ROCKCHIP_SNPS_PCIE3=y (for the PCIe Gen3 controllers)
CONFIG_R8169=y
CONFIG_REALTEK_PHY=y


> 
> To my surprise, we have not enabled mdio on Rock-5B boards.
> can you check if these changes work on your end?

I think these changes are wrong, at least for rock5b.

On rock5b the RTL8125 NIC is connected via PCI, and PCI devices should not
be specified in device tree, as PCI is a bus that can be enumerated.


> 
> -----8<----------8<----------8<----------8<----------8<----------8<-----
> alarm@rock-5b:/media/nvme0/mainline/linux-rockchip-6.y-devel$ git diff
>    arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index c44d001da169..5008a05efd2a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -155,6 +155,19 @@ vcc_1v1_nldo_s3: regulator-vcc-1v1-nldo-s3 {
>         };
>  };
> 
> +&mdio1 {
> +       rgmii_phy1: ethernet-phy@1 {
> +               /* RTL8211F */
> +               compatible = "ethernet-phy-id001c.c916";
> +               reg = <0x1>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&rtl8211f_rst>;
> +               reset-assert-us = <20000>;
> +               reset-deassert-us = <100000>;
> +               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> +       };
> +};
> +
>  &combphy0_ps {
>         status = "okay";
>  };
> @@ -224,6 +237,21 @@ &hdptxphy_hdmi0 {
>         status = "okay";
>  };
> 
> +&gmac1 {
> +       clock_in_out = "output";
> +       phy-handle = <&rgmii_phy1>;
> +       phy-mode = "rgmii";
> +       pinctrl-0 = <&gmac1_miim
> +                    &gmac1_tx_bus2
> +                    &gmac1_rx_bus2
> +                    &gmac1_rgmii_clk
> +                    &gmac1_rgmii_bus>;
> +       pinctrl-names = "default";
> +       tx_delay = <0x3a>;
> +       rx_delay = <0x3e>;
> +       status = "okay";
> +};
> +
>  &i2c0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&i2c0m2_xfer>;
> @@ -419,6 +447,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>                 };
>         };
> 
> +       rtl8211f {
> +               rtl8211f_rst: rtl8211f-rst {
> +                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> +               };
> +       };
> +
>         usb {
>                 vcc5v0_host_en: vcc5v0-host-en {
>                         rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> >
> > Kind regards,
> > Niklas
> 
> Can you check this on your end
> 
> alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
> [sudo] password for alarm:
> fc400000.usb    dwc3: failed to initialize core
> alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred

Sure:
# cat /sys/kernel/debug/devices_deferred
fc400000.usb    dwc3: failed to initialize core


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas

On Fri, 3 Jan 2025 at 19:55, Niklas Cassel <cassel@kernel.org> wrote:
>
> Hello Anand,
>
> On Fri, Jan 03, 2025 at 07:24:07PM +0530, Anand Moon wrote:
> >
> > Thanks for testing this patch.
> >
> > This patch should have been tested on hardware that includes all the
> > relevant controllers,
> > such as PCI 2.0, PCI 3.0, and the SATA controller.
> > I will test this patch again on all the Radxa devices I have.
> >
> > This patch's dependency lies in deferring the probe until the PHY
> > controller initializes.
> >
> > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m
>
>
> Note that the splat, as reported in this thread, and in:
> https://lore.kernel.org/netdev/20250101235122.704012-1-francesco@valla.it/T/
>
> is related to the network PHY (CONFIG_REALTEK_PHY) on the RTL8125 NIC,
> which is connected to one of the PCIe Gen2 controllers, not the PCIe PHY
> on the PCIe controller (CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) itself.
>
> For the record, I run with all the relevant drivers as built-in:
> CONFIG_PCIE_ROCKCHIP_DW_HOST=y
> CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y (for the PCIe Gen2 controllers)
> CONFIG_PHY_ROCKCHIP_SNPS_PCIE3=y (for the PCIe Gen3 controllers)
> CONFIG_R8169=y
> CONFIG_REALTEK_PHY=y
>
>
> >
> > To my surprise, we have not enabled mdio on Rock-5B boards.
> > can you check if these changes work on your end?
>
> I think these changes are wrong, at least for rock5b.

We need to enable the GMAC PHY and reset it using the proper GPIO pin
(PCIE_PERST_L).
Please refer to the schematic for more details.

These changes also apply to other device tree nodes for different boards.

Thanks
-Anand
>
> On rock5b the RTL8125 NIC is connected via PCI, and PCI devices should not
> be specified in device tree, as PCI is a bus that can be enumerated.
>
>
> >
> > -----8<----------8<----------8<----------8<----------8<----------8<-----
> > alarm@rock-5b:/media/nvme0/mainline/linux-rockchip-6.y-devel$ git diff
> >    arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index c44d001da169..5008a05efd2a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -155,6 +155,19 @@ vcc_1v1_nldo_s3: regulator-vcc-1v1-nldo-s3 {
> >         };
> >  };
> >
> > +&mdio1 {
> > +       rgmii_phy1: ethernet-phy@1 {
> > +               /* RTL8211F */
> > +               compatible = "ethernet-phy-id001c.c916";
> > +               reg = <0x1>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&rtl8211f_rst>;
> > +               reset-assert-us = <20000>;
> > +               reset-deassert-us = <100000>;
> > +               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> > +       };
> > +};
> > +
> >  &combphy0_ps {
> >         status = "okay";
> >  };
> > @@ -224,6 +237,21 @@ &hdptxphy_hdmi0 {
> >         status = "okay";
> >  };
> >
> > +&gmac1 {
> > +       clock_in_out = "output";
> > +       phy-handle = <&rgmii_phy1>;
> > +       phy-mode = "rgmii";
> > +       pinctrl-0 = <&gmac1_miim
> > +                    &gmac1_tx_bus2
> > +                    &gmac1_rx_bus2
> > +                    &gmac1_rgmii_clk
> > +                    &gmac1_rgmii_bus>;
> > +       pinctrl-names = "default";
> > +       tx_delay = <0x3a>;
> > +       rx_delay = <0x3e>;
> > +       status = "okay";
> > +};
> > +
> >  &i2c0 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&i2c0m2_xfer>;
> > @@ -419,6 +447,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >                 };
> >         };
> >
> > +       rtl8211f {
> > +               rtl8211f_rst: rtl8211f-rst {
> > +                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +               };
> > +       };
> > +
> >         usb {
> >                 vcc5v0_host_en: vcc5v0-host-en {
> >                         rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > >
> > > Kind regards,
> > > Niklas
> >
> > Can you check this on your end
> >
> > alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
> > [sudo] password for alarm:
> > fc400000.usb    dwc3: failed to initialize core
> > alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
>
> Sure:
> # cat /sys/kernel/debug/devices_deferred
> fc400000.usb    dwc3: failed to initialize core
>
>
> Kind regards,
> Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Fri, Jan 03, 2025 at 08:10:17PM +0530, Anand Moon wrote:
> Hi Niklas
> 
> On Fri, 3 Jan 2025 at 19:55, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > Hello Anand,
> >
> > On Fri, Jan 03, 2025 at 07:24:07PM +0530, Anand Moon wrote:
> > >
> > > Thanks for testing this patch.
> > >
> > > This patch should have been tested on hardware that includes all the
> > > relevant controllers,
> > > such as PCI 2.0, PCI 3.0, and the SATA controller.
> > > I will test this patch again on all the Radxa devices I have.
> > >
> > > This patch's dependency lies in deferring the probe until the PHY
> > > controller initializes.
> > >
> > > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m
> >
> >
> > Note that the splat, as reported in this thread, and in:
> > https://lore.kernel.org/netdev/20250101235122.704012-1-francesco@valla.it/T/
> >
> > is related to the network PHY (CONFIG_REALTEK_PHY) on the RTL8125 NIC,
> > which is connected to one of the PCIe Gen2 controllers, not the PCIe PHY
> > on the PCIe controller (CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) itself.
> >
> > For the record, I run with all the relevant drivers as built-in:
> > CONFIG_PCIE_ROCKCHIP_DW_HOST=y
> > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y (for the PCIe Gen2 controllers)
> > CONFIG_PHY_ROCKCHIP_SNPS_PCIE3=y (for the PCIe Gen3 controllers)
> > CONFIG_R8169=y
> > CONFIG_REALTEK_PHY=y
> >
> >
> > >
> > > To my surprise, we have not enabled mdio on Rock-5B boards.
> > > can you check if these changes work on your end?
> >
> > I think these changes are wrong, at least for rock5b.
> 
> We need to enable the GMAC PHY and reset it using the proper GPIO pin
> (PCIE_PERST_L).
> Please refer to the schematic for more details.

The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
(host) driver:
https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206

which will cause the endpoint device (a RTL8125 NIC in this case)
to be reset during bootup.


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas

On Fri, 3 Jan 2025 at 20:16, Niklas Cassel <cassel@kernel.org> wrote:
>
> On Fri, Jan 03, 2025 at 08:10:17PM +0530, Anand Moon wrote:
> > Hi Niklas
> >
> > On Fri, 3 Jan 2025 at 19:55, Niklas Cassel <cassel@kernel.org> wrote:
> > >
> > > Hello Anand,
> > >
> > > On Fri, Jan 03, 2025 at 07:24:07PM +0530, Anand Moon wrote:
> > > >
> > > > Thanks for testing this patch.
> > > >
> > > > This patch should have been tested on hardware that includes all the
> > > > relevant controllers,
> > > > such as PCI 2.0, PCI 3.0, and the SATA controller.
> > > > I will test this patch again on all the Radxa devices I have.
> > > >
> > > > This patch's dependency lies in deferring the probe until the PHY
> > > > controller initializes.
> > > >
> > > > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m
> > >
> > >
> > > Note that the splat, as reported in this thread, and in:
> > > https://lore.kernel.org/netdev/20250101235122.704012-1-francesco@valla.it/T/
> > >
> > > is related to the network PHY (CONFIG_REALTEK_PHY) on the RTL8125 NIC,
> > > which is connected to one of the PCIe Gen2 controllers, not the PCIe PHY
> > > on the PCIe controller (CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) itself.
> > >
> > > For the record, I run with all the relevant drivers as built-in:
> > > CONFIG_PCIE_ROCKCHIP_DW_HOST=y
> > > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y (for the PCIe Gen2 controllers)
> > > CONFIG_PHY_ROCKCHIP_SNPS_PCIE3=y (for the PCIe Gen3 controllers)
> > > CONFIG_R8169=y
> > > CONFIG_REALTEK_PHY=y
> > >
> > >
> > > >
> > > > To my surprise, we have not enabled mdio on Rock-5B boards.
> > > > can you check if these changes work on your end?
> > >
> > > I think these changes are wrong, at least for rock5b.
> >
> > We need to enable the GMAC PHY and reset it using the proper GPIO pin
> > (PCIE_PERST_L).
> > Please refer to the schematic for more details.
>
> The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
> (host) driver:
> https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206
>
> which will cause the endpoint device (a RTL8125 NIC in this case)
> to be reset during bootup.
>
Thanks for letting me know. It seems like a workaround.
I'll try to disable this and test it again.

My point is that we haven't enabled the GMAC PHY (device nodes)
and must properly reset the GMAC.

We're relying on the code above hack to do that job.

>
> Kind regards,
> Niklas
Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Fri, Jan 03, 2025 at 08:36:18PM +0530, Anand Moon wrote:
> > >
> > > We need to enable the GMAC PHY and reset it using the proper GPIO pin
> > > (PCIE_PERST_L).
> > > Please refer to the schematic for more details.
> >
> > The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
> > (host) driver:
> > https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206
> >
> > which will cause the endpoint device (a RTL8125 NIC in this case)
> > to be reset during bootup.
> >
> Thanks for letting me know. It seems like a workaround.
> I'll try to disable this and test it again.
> 
> My point is that we haven't enabled the GMAC PHY (device nodes)
> and must properly reset the GMAC.
> 
> We're relying on the code above hack to do that job.

I do not think it is a hack.

If you look in most PCIe controller drivers, they toggle PERST before
enumerating the bus:
$ git grep gpiod_set_value drivers/pci/controller/


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas

On Fri, 3 Jan 2025 at 20:40, Niklas Cassel <cassel@kernel.org> wrote:
>
> On Fri, Jan 03, 2025 at 08:36:18PM +0530, Anand Moon wrote:
> > > >
> > > > We need to enable the GMAC PHY and reset it using the proper GPIO pin
> > > > (PCIE_PERST_L).
> > > > Please refer to the schematic for more details.
> > >
> > > The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
> > > (host) driver:
> > > https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206
> > >
> > > which will cause the endpoint device (a RTL8125 NIC in this case)
> > > to be reset during bootup.
> > >
> > Thanks for letting me know. It seems like a workaround.
> > I'll try to disable this and test it again.
> >
> > My point is that we haven't enabled the GMAC PHY (device nodes)
> > and must properly reset the GMAC.
> >
> > We're relying on the code above hack to do that job.
>
> I do not think it is a hack.
>
> If you look in most PCIe controller drivers, they toggle PERST before
> enumerating the bus:
> $ git grep gpiod_set_value drivers/pci/controller/
>

Ok, understood. However, we have multiple reset lines per controller,
so the PCIe driver will reset these lines using gpiod_set_value.

PCIE30X4_PERSTn_M1_L
PCIE30x1_0_PERSTn_M1_L
PCIE_PERST_L
>
> Kind regards,
> Niklas

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Fri, Jan 03, 2025 at 08:59:51PM +0530, Anand Moon wrote:
> Hi Niklas
> 
> On Fri, 3 Jan 2025 at 20:40, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > On Fri, Jan 03, 2025 at 08:36:18PM +0530, Anand Moon wrote:
> > > > >
> > > > > We need to enable the GMAC PHY and reset it using the proper GPIO pin
> > > > > (PCIE_PERST_L).
> > > > > Please refer to the schematic for more details.
> > > >
> > > > The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
> > > > (host) driver:
> > > > https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206
> > > >
> > > > which will cause the endpoint device (a RTL8125 NIC in this case)
> > > > to be reset during bootup.
> > > >
> > > Thanks for letting me know. It seems like a workaround.
> > > I'll try to disable this and test it again.
> > >
> > > My point is that we haven't enabled the GMAC PHY (device nodes)
> > > and must properly reset the GMAC.
> > >
> > > We're relying on the code above hack to do that job.
> >
> > I do not think it is a hack.
> >
> > If you look in most PCIe controller drivers, they toggle PERST before
> > enumerating the bus:
> > $ git grep gpiod_set_value drivers/pci/controller/
> >
> 
> Ok, understood. However, we have multiple reset lines per controller,
> so the PCIe driver will reset these lines using gpiod_set_value.
> 
> PCIE30X4_PERSTn_M1_L
> PCIE30x1_0_PERSTn_M1_L
> PCIE_PERST_L

If you look in Documentation/devicetree/bindings/pci/pci.txt

You will see:
"""
- reset-gpios:
   If present this property specifies PERST# GPIO. Host drivers can parse the
   GPIO and apply fundamental reset to endpoints.
"""

For rock5b, reset-gpios/PERST# pins are defined in:
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts

$ git grep -p reset-gpio arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts=&pcie2x1l0 {
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts:        reset-gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts=&pcie2x1l2 {
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts:        reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts=&pcie3x4 {
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts:        reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;

So I think there is just one reset line per controller.


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Manivannan Sadhasivam]

On Fri, Jan 03, 2025 at 08:59:51PM +0530, Anand Moon wrote:
> Hi Niklas
> 
> On Fri, 3 Jan 2025 at 20:40, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > On Fri, Jan 03, 2025 at 08:36:18PM +0530, Anand Moon wrote:
> > > > >
> > > > > We need to enable the GMAC PHY and reset it using the proper GPIO pin
> > > > > (PCIE_PERST_L).
> > > > > Please refer to the schematic for more details.
> > > >
> > > > The PERST# GPIO is already asserted + deasserted from the PCIe Root Complex
> > > > (host) driver:
> > > > https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L191-L206
> > > >
> > > > which will cause the endpoint device (a RTL8125 NIC in this case)
> > > > to be reset during bootup.
> > > >
> > > Thanks for letting me know. It seems like a workaround.
> > > I'll try to disable this and test it again.
> > >
> > > My point is that we haven't enabled the GMAC PHY (device nodes)
> > > and must properly reset the GMAC.
> > >
> > > We're relying on the code above hack to do that job.
> >
> > I do not think it is a hack.
> >
> > If you look in most PCIe controller drivers, they toggle PERST before
> > enumerating the bus:
> > $ git grep gpiod_set_value drivers/pci/controller/
> >
> 
> Ok, understood. However, we have multiple reset lines per controller,
> so the PCIe driver will reset these lines using gpiod_set_value.
> 
> PCIE30X4_PERSTn_M1_L
> PCIE30x1_0_PERSTn_M1_L
> PCIE_PERST_L

PERST# gpio is unique per controller instance and will be asserted/deasserted
by the PCIe controller driver itself. Endpoint drivers should not touch these.

And most of the PCIe endpoint devices do not need to be described in devicetree
as PCIe is a discoverable bus. But we do define some of them if they require any
special board configuration.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas,

On Fri, 3 Jan 2025 at 19:55, Niklas Cassel <cassel@kernel.org> wrote:
>
> Hello Anand,
>
> On Fri, Jan 03, 2025 at 07:24:07PM +0530, Anand Moon wrote:
> >
> > Thanks for testing this patch.
> >
> > This patch should have been tested on hardware that includes all the
> > relevant controllers,
> > such as PCI 2.0, PCI 3.0, and the SATA controller.
> > I will test this patch again on all the Radxa devices I have.
> >
> > This patch's dependency lies in deferring the probe until the PHY
> > controller initializes.
> >
> > CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=m
>
>
> Note that the splat, as reported in this thread, and in:
> https://lore.kernel.org/netdev/20250101235122.704012-1-francesco@valla.it/T/
>
> is related to the network PHY (CONFIG_REALTEK_PHY) on the RTL8125 NIC,
> which is connected to one of the PCIe Gen2 controllers, not the PCIe PHY
> on the PCIe controller (CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) itself.
>
> For the record, I run with all the relevant drivers as built-in:
> CONFIG_PCIE_ROCKCHIP_DW_HOST=y
> CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y (for the PCIe Gen2 controllers)
> CONFIG_PHY_ROCKCHIP_SNPS_PCIE3=y (for the PCIe Gen3 controllers)
> CONFIG_R8169=y
> CONFIG_REALTEK_PHY=y
>
>
I am unable to reproduce this issue on my end. Could you share your
config file with me?
Additionally, if we build most of the ROCKCHIP components as modules..."

You will see this warning, which is the main reason for this patch

[   34.642365] platform fc400000.usb: deferred probe pending: dwc3:
failed to initialize core
[   34.642529] platform a41000000.pcie: deferred probe pending:
rockchip-dw-pcie: missing PHY
[   34.642604] platform a40800000.pcie: deferred probe pending:
rockchip-dw-pcie: missing PHY
[   34.642674] platform fcd00000.usb: deferred probe pending: dwc3:
failed to initialize core

> >
> > To my surprise, we have not enabled mdio on Rock-5B boards.
> > can you check if these changes work on your end?
>
> I think these changes are wrong, at least for rock5b.
>
> On rock5b the RTL8125 NIC is connected via PCI, and PCI devices should not
> be specified in device tree, as PCI is a bus that can be enumerated.
>

According to RK3588 (TRM) documentation specifies the requirement for
a dedicated GMAC controller
to effectively manage certain critical network features. In the
absence of this specialized controller,
the network interface card (NIC) may operate solely as a standard PCIe
NIC, potentially leading to
suboptimal performance and a lack of robust flow control mechanisms.

Log analysis indicates that Ethernet probing occurs before the
initialization of the PCIe PHY and PCIe hosts.
To investigate this issue, please test with the following configuration changes:

1 Set CONFIG_DWMAC_ROCKCHIP=m.
2 Enable the probe mode PROBE_PREFER_ASYNCHRONOUS for the DWMAC_ROCKCHIP driver.

Thanks
-Anand
>
> >
> > -----8<----------8<----------8<----------8<----------8<----------8<-----
> > alarm@rock-5b:/media/nvme0/mainline/linux-rockchip-6.y-devel$ git diff
> >    arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index c44d001da169..5008a05efd2a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -155,6 +155,19 @@ vcc_1v1_nldo_s3: regulator-vcc-1v1-nldo-s3 {
> >         };
> >  };
> >
> > +&mdio1 {
> > +       rgmii_phy1: ethernet-phy@1 {
> > +               /* RTL8211F */
> > +               compatible = "ethernet-phy-id001c.c916";
> > +               reg = <0x1>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&rtl8211f_rst>;
> > +               reset-assert-us = <20000>;
> > +               reset-deassert-us = <100000>;
> > +               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> > +       };
> > +};
> > +
> >  &combphy0_ps {
> >         status = "okay";
> >  };
> > @@ -224,6 +237,21 @@ &hdptxphy_hdmi0 {
> >         status = "okay";
> >  };
> >
> > +&gmac1 {
> > +       clock_in_out = "output";
> > +       phy-handle = <&rgmii_phy1>;
> > +       phy-mode = "rgmii";
> > +       pinctrl-0 = <&gmac1_miim
> > +                    &gmac1_tx_bus2
> > +                    &gmac1_rx_bus2
> > +                    &gmac1_rgmii_clk
> > +                    &gmac1_rgmii_bus>;
> > +       pinctrl-names = "default";
> > +       tx_delay = <0x3a>;
> > +       rx_delay = <0x3e>;
> > +       status = "okay";
> > +};
> > +
> >  &i2c0 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&i2c0m2_xfer>;
> > @@ -419,6 +447,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >                 };
> >         };
> >
> > +       rtl8211f {
> > +               rtl8211f_rst: rtl8211f-rst {
> > +                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +               };
> > +       };
> > +
> >         usb {
> >                 vcc5v0_host_en: vcc5v0-host-en {
> >                         rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > >
> > > Kind regards,
> > > Niklas
> >
> > Can you check this on your end
> >
> > alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
> > [sudo] password for alarm:
> > fc400000.usb    dwc3: failed to initialize core
> > alarm@rock-5b:~$ sudo cat /sys/kernel/debug/devices_deferred
>
> Sure:
> # cat /sys/kernel/debug/devices_deferred
> fc400000.usb    dwc3: failed to initialize core
>
>
> Kind regards,
> Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Andrew Lunn]

> +&gmac1 {
> +       clock_in_out = "output";
> +       phy-handle = <&rgmii_phy1>;
> +       phy-mode = "rgmii";

rgmii is wrong. Please search the archives about this topic, it comes
up every month. You need to remove the tx_delay and rx_delay
properties, and use rgmii-id.

	Andrew

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Andrew,

On Fri, 3 Jan 2025 at 21:34, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +&gmac1 {
> > +       clock_in_out = "output";
> > +       phy-handle = <&rgmii_phy1>;
> > +       phy-mode = "rgmii";
>
> rgmii is wrong. Please search the archives about this topic, it comes
> up every month. You need to remove the tx_delay and rx_delay
> properties, and use rgmii-id.
>
According to the RKRK3588 TRM-Part1 (section 25.6.11 Clock
Architecture), in RGMII mode,
the TX clock source is exclusively derived from the CRU (Clock Request Unit).
To dynamically adjust the timing alignment between TX/RX clocks and
data, delay lines are
integrated into both the TX and RX clock paths.

Register SYS_GRF_SOC_CON7[5:2] enables these delay lines,
while registers SYS_GRF_SOC_CON8[15:0] and SYS_GRF_SOC_CON9[15:0]
are used to configure the delay length for each path respectively.
Each delay line comprises 200 individual delay elements.

Therefore, it is necessary to configure both TX and RX delay values
appropriately
with phy-mode set as rgmii.

[1[ https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1889-L1914

I have gone through a few of the archives about this topic

[2] https://lore.kernel.org/linux-rockchip/4fdcb631-16cd-d5f1-e2be-19ecedb436eb@linaro.org/T/

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Andrew Lunn]

On Sun, Jan 05, 2025 at 11:16:21PM +0530, Anand Moon wrote:
> Hi Andrew,
> 
> On Fri, 3 Jan 2025 at 21:34, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +&gmac1 {
> > > +       clock_in_out = "output";
> > > +       phy-handle = <&rgmii_phy1>;
> > > +       phy-mode = "rgmii";
> >
> > rgmii is wrong. Please search the archives about this topic, it comes
> > up every month. You need to remove the tx_delay and rx_delay
> > properties, and use rgmii-id.
> >
> According to the RKRK3588 TRM-Part1 (section 25.6.11 Clock
> Architecture), in RGMII mode,
> the TX clock source is exclusively derived from the CRU (Clock Request Unit).
> To dynamically adjust the timing alignment between TX/RX clocks and
> data, delay lines are
> integrated into both the TX and RX clock paths.
> 
> Register SYS_GRF_SOC_CON7[5:2] enables these delay lines,
> while registers SYS_GRF_SOC_CON8[15:0] and SYS_GRF_SOC_CON9[15:0]
> are used to configure the delay length for each path respectively.
> Each delay line comprises 200 individual delay elements.
> 
> Therefore, it is necessary to configure both TX and RX delay values
> appropriately
> with phy-mode set as rgmii.
> 
> [1[ https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1889-L1914
> 
> I have gone through a few of the archives about this topic
> 
> [2] https://lore.kernel.org/linux-rockchip/4fdcb631-16cd-d5f1-e2be-19ecedb436eb@linaro.org/T/

O.K, let me repeat what i have said a number of times over the last
couple of years.

phy-mode = "rgmii" means the PCB has extra long clock lines on the
PCB, so the 2ns delay is provided by them.

phy-mode = "rgmii-id" means the MAC/PHY pair need to arrange to add
the 2ns delay. As far as the DT binding is concerned, it does not
matter which of the two does the delay. However, there is a convention
that the PHY adds the delay, if possible.

So, does your PCB have extra long clock lines?

Vendors often just hack until it works. But works does not mean
correct. I try to review as many .dts files as i can, but some do get
passed me, so there are plenty of bad examples in mainline.

	Andrew

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Andrew,

On Sun, 5 Jan 2025 at 23:27, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Jan 05, 2025 at 11:16:21PM +0530, Anand Moon wrote:
> > Hi Andrew,
> >
> > On Fri, 3 Jan 2025 at 21:34, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > +&gmac1 {
> > > > +       clock_in_out = "output";
> > > > +       phy-handle = <&rgmii_phy1>;
> > > > +       phy-mode = "rgmii";
> > >
> > > rgmii is wrong. Please search the archives about this topic, it comes
> > > up every month. You need to remove the tx_delay and rx_delay
> > > properties, and use rgmii-id.
> > >
> > According to the RKRK3588 TRM-Part1 (section 25.6.11 Clock
> > Architecture), in RGMII mode,
> > the TX clock source is exclusively derived from the CRU (Clock Request Unit).
> > To dynamically adjust the timing alignment between TX/RX clocks and
> > data, delay lines are
> > integrated into both the TX and RX clock paths.
> >
> > Register SYS_GRF_SOC_CON7[5:2] enables these delay lines,
> > while registers SYS_GRF_SOC_CON8[15:0] and SYS_GRF_SOC_CON9[15:0]
> > are used to configure the delay length for each path respectively.
> > Each delay line comprises 200 individual delay elements.
> >
> > Therefore, it is necessary to configure both TX and RX delay values
> > appropriately
> > with phy-mode set as rgmii.
> >
> > [1[ https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1889-L1914
> >
> > I have gone through a few of the archives about this topic
> >
> > [2] https://lore.kernel.org/linux-rockchip/4fdcb631-16cd-d5f1-e2be-19ecedb436eb@linaro.org/T/
>
> O.K, let me repeat what i have said a number of times over the last
> couple of years.
>
> phy-mode = "rgmii" means the PCB has extra long clock lines on the
> PCB, so the 2ns delay is provided by them.
>
> phy-mode = "rgmii-id" means the MAC/PHY pair need to arrange to add
> the 2ns delay. As far as the DT binding is concerned, it does not
> matter which of the two does the delay. However, there is a convention
> that the PHY adds the delay, if possible.
>
> So, does your PCB have extra long clock lines?
>
> Vendors often just hack until it works. But works does not mean
> correct. I try to review as many .dts files as i can, but some do get
> passed me, so there are plenty of bad examples in mainline.
>
>         Andrew

Thanks for this tip, I am no expert in hardware design.

Here is the schematic design of the board, it looks like RTL8125B page 24
is controlled by a PCIe 2.0 bus

[0] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock5b_v13_sch.pdf

PERSTB              ---<< PCIE_PERST_L (GPIO3_B0_u)
LANWAKER        --->> PCIE20_1_2_WAKEn_M1_L (GPIO3_D0_u)
LAN_CLKREQB  --->> PCIE20_1_2_CLKREQn_M1_L( GPIO3_C7_u)
IOLATEB             --->> +V3P3A

PCIE2.0 DATA Impedance 85 R

PCIE_WLAN_TX_C_DP  ----->PCIE20_0_TXP
PCIE_WLAN_TX_C_DN  ----->PCIE20_0_TXN

PCIE2.0 CLK Impedance 100 R

PCIE3_WLAN_REFCLK0_DP --> PCIE20_0_REFCLKP
PCIE3_WLAN_REFCLK0_DN--->PCIE20_0_REFCLKN

I have no idea about the grf clk and data path delay over here.

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Mon, Jan 06, 2025 at 01:28:27PM +0530, Anand Moon wrote:
> Hi Andrew,
> 
> On Sun, 5 Jan 2025 at 23:27, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sun, Jan 05, 2025 at 11:16:21PM +0530, Anand Moon wrote:
> > > Hi Andrew,
> > >
> > > On Fri, 3 Jan 2025 at 21:34, Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > +&gmac1 {
> > > > > +       clock_in_out = "output";
> > > > > +       phy-handle = <&rgmii_phy1>;
> > > > > +       phy-mode = "rgmii";
> > > >
> > > > rgmii is wrong. Please search the archives about this topic, it comes
> > > > up every month. You need to remove the tx_delay and rx_delay
> > > > properties, and use rgmii-id.
> > > >
> > > According to the RKRK3588 TRM-Part1 (section 25.6.11 Clock
> > > Architecture), in RGMII mode,
> > > the TX clock source is exclusively derived from the CRU (Clock Request Unit).
> > > To dynamically adjust the timing alignment between TX/RX clocks and
> > > data, delay lines are
> > > integrated into both the TX and RX clock paths.
> > >
> > > Register SYS_GRF_SOC_CON7[5:2] enables these delay lines,
> > > while registers SYS_GRF_SOC_CON8[15:0] and SYS_GRF_SOC_CON9[15:0]
> > > are used to configure the delay length for each path respectively.
> > > Each delay line comprises 200 individual delay elements.
> > >
> > > Therefore, it is necessary to configure both TX and RX delay values
> > > appropriately
> > > with phy-mode set as rgmii.
> > >
> > > [1[ https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1889-L1914
> > >
> > > I have gone through a few of the archives about this topic
> > >
> > > [2] https://lore.kernel.org/linux-rockchip/4fdcb631-16cd-d5f1-e2be-19ecedb436eb@linaro.org/T/
> >
> > O.K, let me repeat what i have said a number of times over the last
> > couple of years.
> >
> > phy-mode = "rgmii" means the PCB has extra long clock lines on the
> > PCB, so the 2ns delay is provided by them.
> >
> > phy-mode = "rgmii-id" means the MAC/PHY pair need to arrange to add
> > the 2ns delay. As far as the DT binding is concerned, it does not
> > matter which of the two does the delay. However, there is a convention
> > that the PHY adds the delay, if possible.
> >
> > So, does your PCB have extra long clock lines?
> >
> > Vendors often just hack until it works. But works does not mean
> > correct. I try to review as many .dts files as i can, but some do get
> > passed me, so there are plenty of bad examples in mainline.
> >
> >         Andrew
> 
> Thanks for this tip, I am no expert in hardware design.
> 
> Here is the schematic design of the board, it looks like RTL8125B page 24
> is controlled by a PCIe 2.0 bus

As both me an Manivannan said earlier in this thread,
PCIe endpoint devices should not be described in device tree
(the exception is an FPGA, and when you need to describe devices
within the FPGA).

So I think that adding a "ethernet-phy" device tree node in this case is
wrong (as the Ethernet PHY in this case is integrated in the PCIe connected
NIC, and not a discrete component on the SoC).


Kind regards,
Niklas

> 
> [0] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock5b_v13_sch.pdf
> 
> PERSTB              ---<< PCIE_PERST_L (GPIO3_B0_u)
> LANWAKER        --->> PCIE20_1_2_WAKEn_M1_L (GPIO3_D0_u)
> LAN_CLKREQB  --->> PCIE20_1_2_CLKREQn_M1_L( GPIO3_C7_u)
> IOLATEB             --->> +V3P3A
> 
> PCIE2.0 DATA Impedance 85 R
> 
> PCIE_WLAN_TX_C_DP  ----->PCIE20_0_TXP
> PCIE_WLAN_TX_C_DN  ----->PCIE20_0_TXN
> 
> PCIE2.0 CLK Impedance 100 R
> 
> PCIE3_WLAN_REFCLK0_DP --> PCIE20_0_REFCLKP
> PCIE3_WLAN_REFCLK0_DN--->PCIE20_0_REFCLKN
> 
> I have no idea about the grf clk and data path delay over here.
> 
> Thanks
> -Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Andrew Lunn]

> As both me an Manivannan said earlier in this thread,
> PCIe endpoint devices should not be described in device tree
> (the exception is an FPGA, and when you need to describe devices
> within the FPGA).
> 
> So I think that adding a "ethernet-phy" device tree node in this case is
> wrong (as the Ethernet PHY in this case is integrated in the PCIe connected
> NIC, and not a discrete component on the SoC).

There are other cases when PCIe devices need a DT node. One is when
you have an onboard ethernet switch connected to the Ethernet
device. The switch has to be described in DT, and it needs a phandle
to the ethernet interface. Hence you need a DT node the phandle points
to.

You are also making the assumption that the PCIe ethernet interface
has firmware driving all its subsystems. Which results in every PCIe
ethernet device manufacture re-inventing what Linux can already do for
SoC style Ethernet interfaces which do not have firmware, linux drives
it all. I personally would prefer Linux to drive the hardware, via a
DT node, since i then don't have to deal with firmware bugs i cannot
fix, its just Linux all the way down.

	Andrew

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Andrewm,

On Mon, 6 Jan 2025 at 19:14, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > As both me an Manivannan said earlier in this thread,
> > PCIe endpoint devices should not be described in device tree
> > (the exception is an FPGA, and when you need to describe devices
> > within the FPGA).
> >
> > So I think that adding a "ethernet-phy" device tree node in this case is
> > wrong (as the Ethernet PHY in this case is integrated in the PCIe connected
> > NIC, and not a discrete component on the SoC).
>
> There are other cases when PCIe devices need a DT node. One is when
> you have an onboard ethernet switch connected to the Ethernet
> device. The switch has to be described in DT, and it needs a phandle
> to the ethernet interface. Hence you need a DT node the phandle points
> to.
>
> You are also making the assumption that the PCIe ethernet interface
> has firmware driving all its subsystems. Which results in every PCIe
> ethernet device manufacture re-inventing what Linux can already do for
> SoC style Ethernet interfaces which do not have firmware, linux drives
> it all. I personally would prefer Linux to drive the hardware, via a
> DT node, since i then don't have to deal with firmware bugs i cannot
> fix, its just Linux all the way down.
>
>         Andrew
Ok Thanks for clarifying.

I was just trying to understand the call trace for mdio bus which got
me confused.

[0] https://lore.kernel.org/all/Z3fKkTSFFcU9gQLg@ryzen/

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Andrew Lunn]

> I was just trying to understand the call trace for mdio bus which got
> me confused.
> 
> [0] https://lore.kernel.org/all/Z3fKkTSFFcU9gQLg@ryzen/

There is nothing particularly unusual in there. We see PCI bus
enumeration has found a device and bound a driver to it. The driver
has instantiated an MDIO bus, which has scanned the MDIO bus and found
a PHY. The phylib core then tried to load the kernel module needed to
drive the PHY.

Just because it is a PCI device does not mean firmware has to control
all the hardware. Linux has no problems controlling all this, and it
saves reinventing the wheel in firmware, avoids firmware bugs, and
allows new features to be added etc.

	Andrew

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Andrew

On Tue, 7 Jan 2025 at 18:43, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I was just trying to understand the call trace for mdio bus which got
> > me confused.
> >
> > [0] https://lore.kernel.org/all/Z3fKkTSFFcU9gQLg@ryzen/
>
> There is nothing particularly unusual in there. We see PCI bus
> enumeration has found a device and bound a driver to it. The driver
> has instantiated an MDIO bus, which has scanned the MDIO bus and found
> a PHY. The phylib core then tried to load the kernel module needed to
> drive the PHY.
>
> Just because it is a PCI device does not mean firmware has to control
> all the hardware. Linux has no problems controlling all this, and it
> saves reinventing the wheel in firmware, avoids firmware bugs, and
> allows new features to be added etc.
>
>         Andrew

Thanks for clarifying.

-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Manivannan Sadhasivam]

On Tue, Jan 07, 2025 at 02:13:34PM +0100, Andrew Lunn wrote:
> > I was just trying to understand the call trace for mdio bus which got
> > me confused.
> > 
> > [0] https://lore.kernel.org/all/Z3fKkTSFFcU9gQLg@ryzen/
> 
> There is nothing particularly unusual in there. We see PCI bus
> enumeration has found a device and bound a driver to it. The driver
> has instantiated an MDIO bus, which has scanned the MDIO bus and found
> a PHY. The phylib core then tried to load the kernel module needed to
> drive the PHY.
> 
> Just because it is a PCI device does not mean firmware has to control
> all the hardware. Linux has no problems controlling all this, and it
> saves reinventing the wheel in firmware, avoids firmware bugs, and
> allows new features to be added etc.
> 

Most of the time, it would be hard to define the properties of the PCI device's
internal bus in devicetree. For instance, the pinctrl/clock properties which
linux expects are to be connected to the host SoC, and not to the PCI device's
SoC (unless the whole device's SoC is defined).

Not saying that it is not possible but all, but very rare.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Grimmauld]

Hello Anand,

I have tested this patch.
Hardware/Kernel information:
- radxa rock 5c lite
- rk3588s CPU, arm64
- defconfig NixOS kernel
- picked onto 6.18.7
- DT: rockchip/rk3588s-rock-5c.dtb
- tested both uboot (mainline) and edk2 (vendor)

On Fri, Aug 09, 2024 at 01:06:09PM +0530, Anand Moon wrote:
> Rockchip DWC PCIe driver currently waits for the combo PHY link
> (PCIe 3.0, PCIe 2.0, and SATA 3.0) to be established link training
> during boot, it also waits for the link to be up, which could consume
> several milliseconds during boot.

I found that without this patch, USB 3 ports as well as the PCIe connector seemingly stay uninitialized during boot on my hardware. 
This manifests in a bootable USB flash drive loading initrd from bootloader (both uboot and edk2) perfectly, but then fails to mount the rootfs from the drive.
In effect, boot is not just slower than it should be, it just does not boot all the way at all.
In that scenario, the devfs entries corresponding to the flash drive are simply missing, same for the sysfs where i would expect the USB device listed.
Replugging the USB flash drive during initrd does seem to fix that, but is tedious and not viable for a server.
Similarly, booting from m.2 SSD attached via PCIe fails the same way, with rootfs timing out despite the bootloader correctly reading initrd on the same drive.
Fwiw, replugging the SSD does not work like it does for USB flash drives, and is even worse of an idea.
USB 2 ports as well as SD card boots correctly, even without your patch.
Without your patch, i am seeing "deferred probe pending" in dmesg before boot gets stuck, which was the hint which made me find your patch.
I am not sure whether that is the actual cause or just a symptom for why drives are not recognized during boot, and am not quite sure how to debug this further.

> To optimize boot time, this commit allows asynchronous probing.
> This change enables the PCIe link establishment to occur in the
> background while other devices are being probed.

With this patch, booting from SSD or USB 3 port works flawlessly, and i have not seen any regressions with SD card or USB 2 boot, nor any other hardware component.
This setup has worked for multiple boots without fail, both with traditional ext4 and zfs rootfs being loaded from USB 3 and PCIe.
Because i require this patch to run my rock 5c from SSD, i am currently running a custom patched kernel, but would highly appreciate this patch making its way to mainline eventually.
There might well be something else going on here. The proposed patch may not be the "proper" fix to the issues i am seeing, but it does at least work.

I have NOT tested boot from eMMC (either with or without this patch), though i have no reason to believe it would be impacted.

I am happy to provide more info as needed. First time posting to the LKML, i hope i am doing this right...

Tested-by: Grimmauld <grimmauld@grimmauld.de>

Regards,
Grimmauld

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

Hello Grimmauld,

On Thu, Jan 29, 2026 at 03:06:59PM +0100, Grimmauld wrote:
> Hello Anand,
> 
> I have tested this patch.
> Hardware/Kernel information:
> - radxa rock 5c lite
> - rk3588s CPU, arm64
> - defconfig NixOS kernel
> - picked onto 6.18.7
> - DT: rockchip/rk3588s-rock-5c.dtb
> - tested both uboot (mainline) and edk2 (vendor)
> 
> On Fri, Aug 09, 2024 at 01:06:09PM +0530, Anand Moon wrote:
> > Rockchip DWC PCIe driver currently waits for the combo PHY link
> > (PCIe 3.0, PCIe 2.0, and SATA 3.0) to be established link training
> > during boot, it also waits for the link to be up, which could consume
> > several milliseconds during boot.
> 
> I found that without this patch, USB 3 ports as well as the PCIe connector seemingly stay uninitialized during boot on my hardware. 
> This manifests in a bootable USB flash drive loading initrd from bootloader (both uboot and edk2) perfectly, but then fails to mount the rootfs from the drive.
> In effect, boot is not just slower than it should be, it just does not boot all the way at all.
> In that scenario, the devfs entries corresponding to the flash drive are simply missing, same for the sysfs where i would expect the USB device listed.
> Replugging the USB flash drive during initrd does seem to fix that, but is tedious and not viable for a server.
> Similarly, booting from m.2 SSD attached via PCIe fails the same way, with rootfs timing out despite the bootloader correctly reading initrd on the same drive.
> Fwiw, replugging the SSD does not work like it does for USB flash drives, and is even worse of an idea.
> USB 2 ports as well as SD card boots correctly, even without your patch.
> Without your patch, i am seeing "deferred probe pending" in dmesg before boot gets stuck, which was the hint which made me find your patch.
> I am not sure whether that is the actual cause or just a symptom for why drives are not recognized during boot, and am not quite sure how to debug this further.
> 
> > To optimize boot time, this commit allows asynchronous probing.
> > This change enables the PCIe link establishment to occur in the
> > background while other devices are being probed.
> 
> With this patch, booting from SSD or USB 3 port works flawlessly, and i have not seen any regressions with SD card or USB 2 boot, nor any other hardware component.
> This setup has worked for multiple boots without fail, both with traditional ext4 and zfs rootfs being loaded from USB 3 and PCIe.
> Because i require this patch to run my rock 5c from SSD, i am currently running a custom patched kernel, but would highly appreciate this patch making its way to mainline eventually.
> There might well be something else going on here. The proposed patch may not be the "proper" fix to the issues i am seeing, but it does at least work.
> 
> I have NOT tested boot from eMMC (either with or without this patch), though i have no reason to believe it would be impacted.
> 
> I am happy to provide more info as needed. First time posting to the LKML, i hope i am doing this right...
> 
> Tested-by: Grimmauld <grimmauld@grimmauld.de>

I tested this patch again on the latest kernel, and it still results in
the "requesting loading a module with wait allowed while being called from
async context can result in a deadlock" warning from the modules code.
(With the calling code being phylib.)
See the phylib splat that I previously reported in this thread.

Note that I've built the network PHY driver that phylib wants to load
(CONFIG_REALTEK_PHY=y) as built-in. As long as the PHY driver is built
as built-in, I don't think that the problem the modules code is warning
about can happen. (But I also don't understand why it is trying to load
a module when the driver is built as built-in in the first place...)

Anyway, my networking is working perfectly fine even with the splat.


Having async probing for the Rockchip PCIe controller driver, which is
used a LOT of rockchip based SoCs, is a good thing, so I don't think it
is right to avoid enabling async probing just because it results in a
splat on a single rockchip based board (rock5b).


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas/ Grimmauld,

On Fri, 30 Jan 2026 at 15:55, Niklas Cassel <cassel@kernel.org> wrote:
>
> Hello Grimmauld,
>
> On Thu, Jan 29, 2026 at 03:06:59PM +0100, Grimmauld wrote:
> > Hello Anand,
> >
> > I have tested this patch.
> > Hardware/Kernel information:
> > - radxa rock 5c lite
> > - rk3588s CPU, arm64
> > - defconfig NixOS kernel
> > - picked onto 6.18.7
> > - DT: rockchip/rk3588s-rock-5c.dtb
> > - tested both uboot (mainline) and edk2 (vendor)
> >
> > On Fri, Aug 09, 2024 at 01:06:09PM +0530, Anand Moon wrote:
> > > Rockchip DWC PCIe driver currently waits for the combo PHY link
> > > (PCIe 3.0, PCIe 2.0, and SATA 3.0) to be established link training
> > > during boot, it also waits for the link to be up, which could consume
> > > several milliseconds during boot.
> >
> > I found that without this patch, USB 3 ports as well as the PCIe connector seemingly stay uninitialized during boot on my hardware.
> > This manifests in a bootable USB flash drive loading initrd from bootloader (both uboot and edk2) perfectly, but then fails to mount the rootfs from the drive.
> > In effect, boot is not just slower than it should be, it just does not boot all the way at all.
> > In that scenario, the devfs entries corresponding to the flash drive are simply missing, same for the sysfs where i would expect the USB device listed.
> > Replugging the USB flash drive during initrd does seem to fix that, but is tedious and not viable for a server.
> > Similarly, booting from m.2 SSD attached via PCIe fails the same way, with rootfs timing out despite the bootloader correctly reading initrd on the same drive.
> > Fwiw, replugging the SSD does not work like it does for USB flash drives, and is even worse of an idea.
> > USB 2 ports as well as SD card boots correctly, even without your patch.
> > Without your patch, i am seeing "deferred probe pending" in dmesg before boot gets stuck, which was the hint which made me find your patch.
> > I am not sure whether that is the actual cause or just a symptom for why drives are not recognized during boot, and am not quite sure how to debug this further.
> >
> > > To optimize boot time, this commit allows asynchronous probing.
> > > This change enables the PCIe link establishment to occur in the
> > > background while other devices are being probed.
> >
> > With this patch, booting from SSD or USB 3 port works flawlessly, and i have not seen any regressions with SD card or USB 2 boot, nor any other hardware component.
> > This setup has worked for multiple boots without fail, both with traditional ext4 and zfs rootfs being loaded from USB 3 and PCIe.
> > Because i require this patch to run my rock 5c from SSD, i am currently running a custom patched kernel, but would highly appreciate this patch making its way to mainline eventually.
> > There might well be something else going on here. The proposed patch may not be the "proper" fix to the issues i am seeing, but it does at least work.
> >
> > I have NOT tested boot from eMMC (either with or without this patch), though i have no reason to believe it would be impacted.
> >
> > I am happy to provide more info as needed. First time posting to the LKML, i hope i am doing this right...
> >
> > Tested-by: Grimmauld <grimmauld@grimmauld.de>
Thanks for testing this patch.
>
> I tested this patch again on the latest kernel, and it still results in
> the "requesting loading a module with wait allowed while being called from
> async context can result in a deadlock" warning from the modules code.
> (With the calling code being phylib.)
> See the phylib splat that I previously reported in this thread.
>
I’ve attempted to reproduce the warning but was unable to trigger it locally.

> Note that I've built the network PHY driver that phylib wants to load
> (CONFIG_REALTEK_PHY=y) as built-in. As long as the PHY driver is built
> as built-in, I don't think that the problem the modules code is warning
> about can happen. (But I also don't understand why it is trying to load
> a module when the driver is built as built-in in the first place...)
>
But both CONFIG_PHYLIB and CONFIG_REALTEK_PHY are selected as buildin
for R8169 module.

I have tested with the built-in module
CONFIG_R8169=y
CONFIG_R8169_LEDS=y

As well as the module
CONFIG_R8169=m
CONFIG_R8169_LEDS=y

> Anyway, my networking is working perfectly fine even with the splat.
>
>
> Having async probing for the Rockchip PCIe controller driver, which is
> used a LOT of rockchip based SoCs, is a good thing, so I don't think it
> is right to avoid enabling async probing just because it results in a
> splat on a single rockchip based board (rock5b).
>
Yes, this could help in the module probe pci module.

Earlier, I thought gmac will control the r8169 module, but I was wrong.
Could you please try these changes at your end? These changes are
related to MIMO..

$ git diff ./arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
index b3e76ad2d869..fb3a8ba4085a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
@@ -477,7 +477,6 @@ &pcie2x1l0 {
 &pcie2x1l2 {
        pinctrl-names = "default";
        pinctrl-0 = <&pcie2_2_rst>;
-       reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
        vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
        status = "okay";
 };
@@ -535,6 +534,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
                };
        };

+       rtl8211f {
+               rtl8211f_0_rst: rtl8211f-0-rst {
+                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+               };
+       };
+
        usb {
                usbc0_int: usbc0-int {
                        rockchip,pins = <3 RK_PB4 RK_FUNC_GPIO &pcfg_pull_none>;
@@ -550,6 +555,19 @@ &pwm1 {
        status = "okay";
 };

+&mdio0 {
+       rgmii_phy0: ethernet-phy@1 {
+               /* RTL8211F */
+               compatible = "ethernet-phy-id001c.c916";
+               reg = <0x1>;
+               pinctrl-names = "default";
+               pinctrl-0 = <&rtl8211f_0_rst>;
+               reset-assert-us = <20000>;
+               reset-deassert-us = <100000>;
+               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
+       };
+};
+
 &rknn_core_0 {
        npu-supply = <&vdd_npu_s0>;
        sram-supply = <&vdd_npu_s0>;

>
> Kind regards,
> Niklas

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Sat, Jan 31, 2026 at 03:08:42PM +0530, Anand Moon wrote:
> >
> I’ve attempted to reproduce the warning but was unable to trigger it locally.
> 
> I have tested with the built-in module
> CONFIG_R8169=y
> CONFIG_R8169_LEDS=y
> 
> As well as the module
> CONFIG_R8169=m
> CONFIG_R8169_LEDS=y
> 

I'm running with:
CONFIG_R8169=y
CONFIG_PHYLIB=y
CONFIG_REALTEK_PHY=y
CONFIG_REALTEK_PHY_HWMON=y

CONFIG_PCIE_ROCKCHIP_DW=y
CONFIG_PCIE_ROCKCHIP_DW_HOST=y
CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y
(PHY for the PCIe 2x)

$ cat /proc/cmdline
root=/dev/nfs nfsroot=192.168.1.10:/srv/nfs/rootfs_rc,nfsvers=4 ip=dhcp earlycon rootwait loglevel=8

Considering that all PHY drivers (for both Ethernet and PCIe),
and drivers (for both Ethernet and PCIe) are built as built-in,
having nfsroot= on the kernel command line should be no issue.

But, perhaps that is the reason why you cannot reproduce it?


> >
> > Having async probing for the Rockchip PCIe controller driver, which is
> > used a LOT of rockchip based SoCs, is a good thing, so I don't think it
> > is right to avoid enabling async probing just because it results in a
> > splat on a single rockchip based board (rock5b).
> >
> Yes, this could help in the module probe pci module.
> 
> Earlier, I thought gmac will control the r8169 module, but I was wrong.
> Could you please try these changes at your end? These changes are
> related to MIMO..
> 
> $ git diff ./arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> index b3e76ad2d869..fb3a8ba4085a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> @@ -477,7 +477,6 @@ &pcie2x1l0 {
>  &pcie2x1l2 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pcie2_2_rst>;
> -       reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
>         vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
>         status = "okay";
>  };
> @@ -535,6 +534,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>                 };
>         };
> 
> +       rtl8211f {
> +               rtl8211f_0_rst: rtl8211f-0-rst {
> +                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> +               };
> +       };
> +
>         usb {
>                 usbc0_int: usbc0-int {
>                         rockchip,pins = <3 RK_PB4 RK_FUNC_GPIO &pcfg_pull_none>;
> @@ -550,6 +555,19 @@ &pwm1 {
>         status = "okay";
>  };
> 
> +&mdio0 {
> +       rgmii_phy0: ethernet-phy@1 {
> +               /* RTL8211F */
> +               compatible = "ethernet-phy-id001c.c916";
> +               reg = <0x1>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&rtl8211f_0_rst>;
> +               reset-assert-us = <20000>;
> +               reset-deassert-us = <100000>;
> +               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> +       };
> +};
> +
>  &rknn_core_0 {
>         npu-supply = <&vdd_npu_s0>;
>         sram-supply = <&vdd_npu_s0>;
> 

I tried your patch above, but I still see the splat.

But as I said, I don't think the splat should be a showstopper.


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas,

On Mon, 2 Feb 2026 at 15:25, Niklas Cassel <cassel@kernel.org> wrote:
>
> On Sat, Jan 31, 2026 at 03:08:42PM +0530, Anand Moon wrote:
> > >
> > I’ve attempted to reproduce the warning but was unable to trigger it locally.
> >
> > I have tested with the built-in module
> > CONFIG_R8169=y
> > CONFIG_R8169_LEDS=y
> >
> > As well as the module
> > CONFIG_R8169=m
> > CONFIG_R8169_LEDS=y
> >
>
> I'm running with:
> CONFIG_R8169=y
> CONFIG_PHYLIB=y
> CONFIG_REALTEK_PHY=y
> CONFIG_REALTEK_PHY_HWMON=y
>
I feel CONFIG_R8169 should not be built into the kernel image.
Since the driver is registered via module_pci_driver(rtl8169_pci_driver),
it is intended to be loaded as a module. In addition, this driver
requires external firmware
during initialization, which could make a built‑in configuration problematic.
Keeping it modular ensures proper firmware loading and avoids
early‑boot failures.

> CONFIG_PCIE_ROCKCHIP_DW=y
> CONFIG_PCIE_ROCKCHIP_DW_HOST=y
> CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y
> (PHY for the PCIe 2x)
>
> $ cat /proc/cmdline
> root=/dev/nfs nfsroot=192.168.1.10:/srv/nfs/rootfs_rc,nfsvers=4 ip=dhcp earlycon rootwait loglevel=8
>
> Considering that all PHY drivers (for both Ethernet and PCIe),
> and drivers (for both Ethernet and PCIe) are built as built-in,
> having nfsroot= on the kernel command line should be no issue.
>
> But, perhaps that is the reason why you cannot reproduce it?
Thanks for sharing your setup.
>
>
> > >
> > > Having async probing for the Rockchip PCIe controller driver, which is
> > > used a LOT of rockchip based SoCs, is a good thing, so I don't think it
> > > is right to avoid enabling async probing just because it results in a
> > > splat on a single rockchip based board (rock5b).
> > >
> > Yes, this could help in the module probe pci module.
> >
> > Earlier, I thought gmac will control the r8169 module, but I was wrong.
> > Could you please try these changes at your end? These changes are
> > related to MIMO..
> >
> > $ git diff ./arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> > index b3e76ad2d869..fb3a8ba4085a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-5bp-5t.dtsi
> > @@ -477,7 +477,6 @@ &pcie2x1l0 {
> >  &pcie2x1l2 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pcie2_2_rst>;
> > -       reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
> >         vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
> >         status = "okay";
> >  };
> > @@ -535,6 +534,12 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >                 };
> >         };
> >
> > +       rtl8211f {
> > +               rtl8211f_0_rst: rtl8211f-0-rst {
> > +                       rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +               };
> > +       };
> > +
> >         usb {
> >                 usbc0_int: usbc0-int {
> >                         rockchip,pins = <3 RK_PB4 RK_FUNC_GPIO &pcfg_pull_none>;
> > @@ -550,6 +555,19 @@ &pwm1 {
> >         status = "okay";
> >  };
> >
> > +&mdio0 {
> > +       rgmii_phy0: ethernet-phy@1 {
> > +               /* RTL8211F */
> > +               compatible = "ethernet-phy-id001c.c916";
> > +               reg = <0x1>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&rtl8211f_0_rst>;
> > +               reset-assert-us = <20000>;
> > +               reset-deassert-us = <100000>;
> > +               reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> > +       };
> > +};
> > +
> >  &rknn_core_0 {
> >         npu-supply = <&vdd_npu_s0>;
> >         sram-supply = <&vdd_npu_s0>;
> >
>
> I tried your patch above, but I still see the splat.
>
> But as I said, I don't think the splat should be a showstopper.
Thanks for clarifying this issue. I will resubmit once I have
conducted further testing.
>
> Kind regards,
> Niklas

Thanks
-Anand

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Mon, Feb 02, 2026 at 11:35:48PM +0530, Anand Moon wrote:
> On Mon, 2 Feb 2026 at 15:25, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > On Sat, Jan 31, 2026 at 03:08:42PM +0530, Anand Moon wrote:
> > > >
> > > I’ve attempted to reproduce the warning but was unable to trigger it locally.
> > >
> > > I have tested with the built-in module
> > > CONFIG_R8169=y
> > > CONFIG_R8169_LEDS=y
> > >
> > > As well as the module
> > > CONFIG_R8169=m
> > > CONFIG_R8169_LEDS=y
> > >
> >
> > I'm running with:
> > CONFIG_R8169=y
> > CONFIG_PHYLIB=y
> > CONFIG_REALTEK_PHY=y
> > CONFIG_REALTEK_PHY_HWMON=y
> >
> I feel CONFIG_R8169 should not be built into the kernel image.
> Since the driver is registered via module_pci_driver(rtl8169_pci_driver),
> it is intended to be loaded as a module. In addition, this driver
> requires external firmware
> during initialization, which could make a built‑in configuration problematic.
> Keeping it modular ensures proper firmware loading and avoids
> early‑boot failures.

For what it is worth, I strongly disagree that you should not be allowed
to build something as built-in just because the driver is registered using
module_pci_driver().

In order to use NFS root, and to avoid the hassle of using an initramfs,
having the driver as built-in is a logical option.
Anyway, this is currently working fine with my setup today, and is
unrelated to the phylib splat that I have reported in this thread.

I know that the R8169 driver tries to load a firmware blob, I used to have
an initramfs just to be able to load this firmware blob, but I found out
that the driver probes and works perfectly fine even without the loading
any firmware, therefore I no longer use an initramfs.
Again, this is working perfectly fine today, and the R8169 loading or not
loading any firmware is unrealated to the phylib splat.

(Just for clarity, I don't think that the phylib splat should stop this
patch from being accepted.)


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Niklas Cassel]

On Fri, Jan 30, 2026 at 11:25:37AM +0100, Niklas Cassel wrote:
> Note that I've built the network PHY driver that phylib wants to load
> (CONFIG_REALTEK_PHY=y) as built-in. As long as the PHY driver is built
> as built-in, I don't think that the problem the modules code is warning
> about can happen. (But I also don't understand why it is trying to load
> a module when the driver is built as built-in in the first place...)

FWIW, the reason why PHYLIB tries to load the module even though it is built
as built-in (i.e. is already loaded) is explained by the following comment:
https://github.com/torvalds/linux/blob/v6.19-rc8/drivers/net/phy/phy_device.c#L852-L855


Kind regards,
Niklas

Re: [PATCH v2] PCI: dw-rockchip: Enable async probe by default

[Anand Moon]

Hi Niklas

On Mon, 2 Feb 2026 at 15:32, Niklas Cassel <cassel@kernel.org> wrote:
>
> On Fri, Jan 30, 2026 at 11:25:37AM +0100, Niklas Cassel wrote:
> > Note that I've built the network PHY driver that phylib wants to load
> > (CONFIG_REALTEK_PHY=y) as built-in. As long as the PHY driver is built
> > as built-in, I don't think that the problem the modules code is warning
> > about can happen. (But I also don't understand why it is trying to load
> > a module when the driver is built as built-in in the first place...)
>
> FWIW, the reason why PHYLIB tries to load the module even though it is built
> as built-in (i.e. is already loaded) is explained by the following comment:
> https://github.com/torvalds/linux/blob/v6.19-rc8/drivers/net/phy/phy_device.c#L852-L855
>
Yes, I have gone through the history of changes.

>
> Kind regards,
> Niklas
Thanks
-Anand