Message ID | 20240809073610.2517-1-linux.amoon@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] PCI: dw-rockchip: Enable async probe by default | expand |
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
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
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
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
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
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
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
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
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
> +&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
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
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
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
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
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
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
> 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
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
> 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
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
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
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, };
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(+) base-commit: ee9a43b7cfe2d8a3520335fea7d8ce71b8cabd9d