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

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

[Anand Moon]

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.
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 1170e1107508..7a895b66e4e4 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -616,6 +616,7 @@ static struct platform_driver rockchip_pcie_driver = {
 		.name	= "rockchip-dw-pcie",
 		.of_match_table = rockchip_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
 	.probe = rockchip_pcie_probe,
 };

base-commit: ee9a43b7cfe2d8a3520335fea7d8ce71b8cabd9d
-- 
2.44.0

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

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