net: stmmac: socfpga: fix init ordering and cleanups

Hi,

This series fixes the init ordering of the socfpga probe function. The standard rule is to do all setup before publishing any device, and socfpga violates that. I can see no reason for this, but these patches have not been tested on hardware.

Address this by moving the initialisation of dwmac->stmmac_rst along with all the other dwmac initialisers - there's no reason for this to be late as plat_dat->stmmac_rst has already been populated.

Next, replace the call to ops->set_phy_mode() with an init function socfpga_dwmac_init() which will then be linked in to plat_dat->init.

Then, add this to plat_dat->init, and switch to stmmac_pltfr_pm_ops from the private ops. The runtime suspend/resume socfpga implementations are identical to the platform ones, but misses the noirq versions which this will add.

Next, swap the order of socfpga_dwmac_init() and stmmac_dvr_probe().

Finally, convert to devm_stmmac_pltfr_probe() by moving the call to ops->set_phy_mode() into an init function appropriately populating plat_dat->init.

 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c    | 79 +++++-----------------
 1 file changed, 16 insertions(+), 63 deletions(-)

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Initialisation/setup after registration is a bug. This is the first of two patches fixing this in socfpga.

dwmac->stmmac_rst is initialised from the stmmac plat_dat's stmmac_rst member, which is itself initialised by devm_stmmac_probe_config_dt(). Therefore, this can be initialised before we call stmmac_dvr_probe(). Move it there.

dwmac->stmmac_rst is used by the set_phy_mode() method.

---
 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

index 116855658559..bcdb25ee2a33 100644
@@ -442,8 +442,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	int			ret;
 	struct socfpga_dwmac	*dwmac;
-	struct net_device	*ndev;
-	struct stmmac_priv	*stpriv;
 	const struct socfpga_dwmac_ops *ops;
 
 	ops = device_get_match_data(&pdev->dev);

@@ -479,7 +477,13 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* The socfpga driver needs to control the stmmac reset to set the phy
+	 * mode. Create a copy of the core reset handle so it can be used by
+	 * the driver later.
+	 */
+	dwmac->stmmac_rst = plat_dat->stmmac_rst;
 	dwmac->ops = ops;
+
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;
 	plat_dat->pcs_init = socfpga_dwmac_pcs_init;

@@ -493,15 +497,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ndev = platform_get_drvdata(pdev);
-	stpriv = netdev_priv(ndev);
-
-	/* The socfpga driver needs to control the stmmac reset to set the phy
-	 * mode. Create a copy of the core reset handle so it can be used by
-	 * the driver later.
-	 */
-	dwmac->stmmac_rst = stpriv->plat->stmmac_rst;
-
 	ret = ops->set_phy_mode(dwmac);
 	if (ret)
 		goto err_dvr_remove;

2.30.2

Both the resume and probe path needs to configure the phy mode, so provide a common function to do this which can later be hooked into plat_dat->init.

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index bcdb25ee2a33..000d349a6d4c 100644
@@ -435,6 +435,13 @@ static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,
 	return priv->hw->phylink_pcs;
 }
 
+static int socfpga_dwmac_init(struct platform_device *pdev, void *bsp_priv)
+{
+	struct socfpga_dwmac *dwmac = bsp_priv;
+
+	return dwmac->ops->set_phy_mode(dwmac);
+}
+
 static int socfpga_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;

@@ -497,7 +504,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = ops->set_phy_mode(dwmac);
+	ret = socfpga_dwmac_init(pdev, dwmac);
 	if (ret)
 		goto err_dvr_remove;
 

@@ -516,7 +523,7 @@ static int socfpga_dwmac_resume(struct device *dev)
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	struct socfpga_dwmac *dwmac_priv = get_stmmac_bsp_priv(dev);
 
-	dwmac_priv->ops->set_phy_mode(priv->plat->bsp_priv);
+	socfpga_dwmac_init(to_platform_device(dev), dwmac_priv);
 
 	return stmmac_resume(dev);
 }

2.30.2

Convert socfpga to use the generic stmmac_pltfr_pm_ops, which can be achieved by adding an appropriate plat_dat->init function to do the setup.

---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 39 +------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

index 000d349a6d4c..69ffc52c0275 100644
@@ -493,6 +493,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;
+	plat_dat->init = socfpga_dwmac_init;
 	plat_dat->pcs_init = socfpga_dwmac_pcs_init;
 	plat_dat->pcs_exit = socfpga_dwmac_pcs_exit;
 	plat_dat->select_pcs = socfpga_dwmac_select_pcs;

@@ -516,42 +517,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	return ret;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int socfpga_dwmac_resume(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-	struct stmmac_priv *priv = netdev_priv(ndev);
-	struct socfpga_dwmac *dwmac_priv = get_stmmac_bsp_priv(dev);
-
-	socfpga_dwmac_init(to_platform_device(dev), dwmac_priv);
-
-	return stmmac_resume(dev);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-static int __maybe_unused socfpga_dwmac_runtime_suspend(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-	struct stmmac_priv *priv = netdev_priv(ndev);
-
-	stmmac_bus_clks_config(priv, false);
-
-	return 0;
-}
-
-static int __maybe_unused socfpga_dwmac_runtime_resume(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-	struct stmmac_priv *priv = netdev_priv(ndev);
-
-	return stmmac_bus_clks_config(priv, true);
-}
-
-static const struct dev_pm_ops socfpga_dwmac_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(stmmac_suspend, socfpga_dwmac_resume)
-	SET_RUNTIME_PM_OPS(socfpga_dwmac_runtime_suspend, socfpga_dwmac_runtime_resume, NULL)
-};
-
 static const struct socfpga_dwmac_ops socfpga_gen5_ops = {
 	.set_phy_mode = socfpga_gen5_set_phy_mode,
 };

@@ -572,7 +537,7 @@ static struct platform_driver socfpga_dwmac_driver = {
 	.remove = stmmac_pltfr_remove,
 	.driver = {
 		.name           = "socfpga-dwmac",
-		.pm		= &socfpga_dwmac_pm_ops,
+		.pm		= &stmmac_pltfr_pm_ops,
 		.of_match_table = socfpga_dwmac_match,
 	},
 };

2.30.2

Initialisation/setup after registration is a bug. This is the second of two patches fixing this in socfpga.

The set_phy_mode() does various hardware setup that would interfere with a netdev that has been published, and thus available to be opened by the kernel/userspace.

Move the call to set_phy_mode() before calling stmmac_dvr_probe(). This also simplifies the probe function as there is no need to unregister the netdev if set_phy_mode() fails.

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

index 69ffc52c0275..251d8b72bed5 100644
@@ -501,20 +501,11 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 
 	plat_dat->riwt_off = 1;
 
-	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
-	if (ret)
-		return ret;
-
 	ret = socfpga_dwmac_init(pdev, dwmac);
 	if (ret)
-		goto err_dvr_remove;
-
-	return 0;
-
-err_dvr_remove:
-	stmmac_dvr_remove(&pdev->dev);
+		return ret;
 
-	return ret;
+	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 }
 
 static const struct socfpga_dwmac_ops socfpga_gen5_ops = {

2.30.2

Convert socfpga to use devm_stmmac_pltfr_probe() to further simplify the probe function, wrapping the call to the set_phy_mode() method into socfpga_dwmac_init() which can be called from the plat_dat->init() method. Also call this from socfpga_dwmac_resume() thereby simplifying that function.

Using the devm variant also means we can remove the call to stmmac_pltfr_remove().

Unfortunately, we can't also convert to stmmac_pltfr_pm_ops as there is extra work done in socfpga_dwmac_resume().

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

index 251d8b72bed5..8e6d780669b9 100644
@@ -501,11 +501,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 
 	plat_dat->riwt_off = 1;
 
-	ret = socfpga_dwmac_init(pdev, dwmac);
-	if (ret)
-		return ret;
-
-	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
 }
 
 static const struct socfpga_dwmac_ops socfpga_gen5_ops = {

@@ -525,7 +521,6 @@ MODULE_DEVICE_TABLE(of, socfpga_dwmac_match);
 
 static struct platform_driver socfpga_dwmac_driver = {
 	.probe  = socfpga_dwmac_probe,
-	.remove = stmmac_pltfr_remove,
 	.driver = {
 		.name           = "socfpga-dwmac",
 		.pm		= &stmmac_pltfr_pm_ops,

2.30.2

Hello Russell,

I've given this a try and unfortunately :

[    1.005290] 8<--- cut here ---
[    1.008378] Unable to handle kernel paging request at virtual address 00003c40 when read
[    1.016449] [00003c40] *pgd=00000000
[    1.020057] Internal error: Oops: 5 [#1] SMP ARM
[    1.024668] Modules linked in:
[    1.027726] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-rc1-00379-g4552d2fc04f5-dirty #304 PREEMPT 
[    1.037790] Hardware name: Altera SOCFPGA
[    1.041790] PC is at socfpga_gen5_set_phy_mode+0x44/0x338
[    1.047189] LR is at stmmac_pltfr_probe+0x24/0x6c
[    1.051893] pc : [<c010b82c>]    lr : [<c0611680>]    psr: a0000013
[    1.058142] sp : f0831d58  ip : 00000001  fp : 00000000
[    1.063353] r10: c0b4a508  r9 : 00000000  r8 : c093a170
[    1.068563] r7 : 00000060  r6 : c1880c00  r5 : c191a040  r4 : c203e9c0
[    1.075071] r3 : 00003000  r2 : 00000119  r1 : c0939ed0  r0 : c0a45f70
[    1.081579] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    1.088697] Control: 10c5387d  Table: 0000404a  DAC: 00000051
[    1.094424] Register r0 information: non-slab/vmalloc memory
[    1.100075] Register r1 information: non-slab/vmalloc memory
[    1.105723] Register r2 information: non-paged memory
[    1.110765] Register r3 information: non-paged memory
[    1.115807] Register r4 information: slab kmalloc-128 start c203e980 pointer offset 64 size 128
[    1.124508] Register r5 information: slab kmalloc-1k start c191a000 pointer offset 64 size 1024
[    1.133207] Register r6 information: slab kmalloc-512 start c1880c00 pointer offset 0 size 512
[    1.141816] Register r7 information: non-paged memory
[    1.146858] Register r8 information: non-slab/vmalloc memory
[    1.152506] Register r9 information: NULL pointer
[    1.157202] Register r10 information: non-slab/vmalloc memory
[    1.162937] Register r11 information: NULL pointer
[    1.167719] Register r12 information: non-paged memory
[    1.172848] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    1.178840] Stack: (0xf0831d58 to 0xf0832000)
[    1.183192] 1d40:                                                       c0508cfc c050a250
[    1.191348] 1d60: c203e9c0 fb79b606 c18f6010 00000000 c191a040 f0831db0 c18f6000 c093a170
[    1.199504] 1d80: c203e9c0 c0611680 00000000 c18f6000 c18f6010 c191a040 c093a170 c0611918
[    1.207658] 1da0: 00000000 c18f6000 c18f6010 c061309c f0960000 00000000 00000000 0000001e
[    1.215811] 1dc0: fffffffa 0000001e fffffffa 00000000 00000000 00000000 00000000 00000000
[    1.223963] 1de0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.232117] 1e00: 00000000 00000000 00000000 00000000 00000000 fb79b606 c18f6010 00000000
[    1.240270] 1e20: c18f6010 c173b5b4 00000000 c203a1b8 c0b3f858 c0551410 c18f6010 00000000
[    1.248425] 1e40: c173b5b4 c054ed84 c18f6010 c173b5b4 c18f6010 00000000 c203a1b8 c054f008
[    1.256580] 1e60: 60000013 c0b3f858 c176b75c c173b5b4 c18f6010 00000000 c203a1b8 c054f1e8
[    1.264733] 1e80: c18f6010 c173b5b4 c18f6054 c1833600 c203a1b8 c054f3cc 00000000 c173b5b4
[    1.272888] 1ea0: c054f33c c054cdc0 c174b000 c1833658 c19b9f34 fb79b606 c1833600 c173b5b4
[    1.281041] 1ec0: c203a180 00000000 c1833600 c054e164 c0a46214 00000006 c173b5b4 c18b3600
[    1.289193] 1ee0: 00000006 00000000 c174b000 c05500c8 c0b25bd4 c18b3600 00000006 c010d244
[    1.297346] 1f00: c19a8bc6 c19a8bcc 00000000 00000000 000000e4 c19a8bcc c19a8be3 c014a2a0
[    1.305499] 1f20: c0aa4490 c0a46d70 00000000 00000000 00000006 00000006 00000064 c19a8bcc
[    1.313653] 1f40: c19a8bd1 fb79b606 c0b4a064 fb79b606 00000064 c19a8b80 00000064 00000006
[    1.321806] 1f60: c0b3f834 c0a62760 c0b3f858 c0b01340 00000006 00000006 00000000 c0b004d0
[    1.329958] 1f80: c1703e40 c08c3d08 00000000 00000000 00000000 00000000 00000000 c08c3d24
[    1.338113] 1fa0: 00000000 c08c3d08 00000000 c010014c 00000000 00000000 00000000 00000000
[    1.346266] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.354420] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    1.362568] Call trace: 
[    1.362581]  socfpga_gen5_set_phy_mode from stmmac_pltfr_probe+0x24/0x6c
[    1.371804]  stmmac_pltfr_probe from devm_stmmac_pltfr_probe+0xc/0x78
[    1.378243]  devm_stmmac_pltfr_probe from socfpga_dwmac_probe+0x144/0x19c
[    1.385027]  socfpga_dwmac_probe from platform_probe+0x5c/0xb0
[    1.390868]  platform_probe from really_probe+0xb8/0x2b4
[    1.396184]  really_probe from __driver_probe_device+0x88/0x1a0
[    1.402100]  __driver_probe_device from driver_probe_device+0x30/0xc0
[    1.408536]  driver_probe_device from __driver_attach+0x90/0x13c
[    1.414539]  __driver_attach from bus_for_each_dev+0x70/0xc0
[    1.420194]  bus_for_each_dev from bus_add_driver+0xcc/0x1ec
[    1.425848]  bus_add_driver from driver_register+0x7c/0x114
[    1.431418]  driver_register from do_one_initcall+0x44/0x270
[    1.437080]  do_one_initcall from kernel_init_freeable+0x208/0x260
[    1.443258]  kernel_init_freeable from kernel_init+0x1c/0x12c
[    1.449000]  kernel_init from ret_from_fork+0x14/0x28

From what I see the issue is :

socfpga_dwmac_init()
socfpga_gen5_set_phy_mode()

    socfpga_get_plat_phymode() {
      struct net_device *ndev = dev_get_drvdata(dwmac->dev);
      [...]
    }

At that moment, the netdev doesn't exist yet as the ->init() is called before the netdev is created.

This is only to get the phymode, maybe we should do like dwmac_imx and store a pointer to plat_dat into struct dwmac_socfpga, so that we can get it back in dwmac_init ? I've tried with the patch below and it does solve the issue, but maybe you have a better approach.

Maxime

---

From 4cad86cec2023030ae86126b436e4b08b9b99212 Mon Sep 17 00:00:00 2001
From: Maxime Chevallier <[email protected]>
Date: Wed, 16 Apr 2025 09:49:50 +0200
Subject: [PATCH] net: stmmac: socfpga: Don't grab plat_data from netdev_priv
in init path

---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index 8e6d780669b9..b866054808d6 100644
@@ -43,7 +43,7 @@
 
 struct socfpga_dwmac;
 struct socfpga_dwmac_ops {
-	int (*set_phy_mode)(struct socfpga_dwmac *dwmac_priv);
+	int (*set_phy_mode)(struct socfpga_dwmac *dwmac_priv, int phymode);
 };
 
 struct socfpga_dwmac {

@@ -59,6 +59,7 @@ struct socfpga_dwmac {
 	bool f2h_ptp_ref_clk;
 	const struct socfpga_dwmac_ops *ops;
 	struct mdio_device *pcs_mdiodev;
+	struct plat_stmmacenet_data *plat_dat;
 };
 
 static void socfpga_dwmac_fix_mac_speed(void *priv, int speed, unsigned int mode)

@@ -269,10 +270,9 @@ static int socfpga_set_phy_mode_common(int phymode, u32 *val)
 	return 0;
 }
 
-static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
+static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac, int phymode)
 {
 	struct regmap *sys_mgr_base_addr = dwmac->sys_mgr_base_addr;
-	int phymode = socfpga_get_plat_phymode(dwmac);
 	u32 reg_offset = dwmac->reg_offset;
 	u32 reg_shift = dwmac->reg_shift;
 	u32 ctrl, val, module;

@@ -327,10 +327,9 @@ static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
 	return 0;
 }
 
-static int socfpga_gen10_set_phy_mode(struct socfpga_dwmac *dwmac)
+static int socfpga_gen10_set_phy_mode(struct socfpga_dwmac *dwmac, int phymode)
 {
 	struct regmap *sys_mgr_base_addr = dwmac->sys_mgr_base_addr;
-	int phymode = socfpga_get_plat_phymode(dwmac);
 	u32 reg_offset = dwmac->reg_offset;
 	u32 reg_shift = dwmac->reg_shift;
 	u32 ctrl, val, module;

@@ -438,8 +437,9 @@ static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,
 static int socfpga_dwmac_init(struct platform_device *pdev, void *bsp_priv)
 {
 	struct socfpga_dwmac *dwmac = bsp_priv;
+	struct plat_stmmacenet_data *plat_dat = dwmac->plat_dat;
 
-	return dwmac->ops->set_phy_mode(dwmac);
+	return dwmac->ops->set_phy_mode(dwmac, plat_dat->mac_interface);
 }
 
 static int socfpga_dwmac_probe(struct platform_device *pdev)

@@ -490,6 +490,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	 */
 	dwmac->stmmac_rst = plat_dat->stmmac_rst;
 	dwmac->ops = ops;
+	dwmac->plat_dat = plat_dat;
 
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;

2.49.0

Great, someone with hardware, and who responds to patches! :)

Yes, but I don't think we need such a big patch:

 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index 8e6d780669b9..59f90b123c5b 100644
@@ -50,6 +50,7 @@ struct socfpga_dwmac {
 	u32	reg_offset;
 	u32	reg_shift;
 	struct	device *dev;
+	struct plat_stmmacenet_data *plat_dat;
 	struct regmap *sys_mgr_base_addr;
 	struct reset_control *stmmac_rst;
 	struct reset_control *stmmac_ocp_rst;

@@ -233,10 +234,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
 
 static int socfpga_get_plat_phymode(struct socfpga_dwmac *dwmac)
 {
-	struct net_device *ndev = dev_get_drvdata(dwmac->dev);
-	struct stmmac_priv *priv = netdev_priv(ndev);
-
-	return priv->plat->mac_interface;
+	return dwmac->plat_dat->mac_interface;
 }
 
 static void socfpga_sgmii_config(struct socfpga_dwmac *dwmac, bool enable)

@@ -490,6 +488,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	 */
 	dwmac->stmmac_rst = plat_dat->stmmac_rst;
 	dwmac->ops = ops;
+	dwmac->plat_dat = plat_dat;
 
 	plat_dat->bsp_priv = dwmac;
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!