net: stmmac: socfpga: fix init ordering and cleanups
Subsystems (5)
v2 cover 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.
Before we swap the order of socfpga_dwmac_init() and stmmac_dvr_probe(), we need to change the way the interface is obtained, as that uses driver data and the struct net_device which haven't been initialised. Save a pointer to plat_dat in the socfpga private data, and use that to get the interface mode. We can then swap the order of the init and probe functions.
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.
v2: fix oops when calling set_phy_mode() early.
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 79 +++++----------------- 1 file changed, 16 insertions(+), 63 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
PATCH v2 1/5 net: stmmac: socfpga: init dwmac->stmmac_rst before registration
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(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +6 -11
@@ -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;
Signature · 1 line
2.30.2
PATCH v2 2/5 net: stmmac: socfpga: provide init function
In reply to Russell King (Oracle)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(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +9 -2
@@ -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); }
Signature · 1 line
2.30.2
PATCH v2 3/5 net: stmmac: socfpga: convert to stmmac_pltfr_pm_ops
In reply to Russell King (Oracle)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(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +2 -37
@@ -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, }, };
Signature · 1 line
2.30.2
PATCH v2 4/5 net: stmmac: socfpga: call set_phy_mode() before registration
In reply to Russell King (Oracle)Initialisation/setup after registration is a bug. This is the second of two patches fixing this in socfpga.
The set_phy_mode() functions do various hardware setup that would interfere with a netdev that has been published, and thus available to be opened by the kernel/userspace.
However, set_phy_mode() relies upon the netdev having been initialised to get at the plat_stmmacenet_data structure, which is probably why it was placed after stmmac_drv_probe(). We can remove that need by storing a pointer to struct plat_stmmacenet_data in struct socfpga_dwmac.
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.
--- .../ethernet/stmicro/stmmac/dwmac-socfpga.c | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +5 -15
@@ -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;
@@ -501,20 +500,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 = {
Signature · 1 line
2.30.2
PATCH v2 5/5 net: stmmac: socfpga: convert to devm_stmmac_pltfr_probe()
In reply to Russell King (Oracle)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(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +1 -6
@@ -500,11 +500,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 = {
@@ -524,7 +520,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,
Signature · 1 line
2.30.2
Hello Russell,
Quoted 27 lines · On Wed, 16 Apr 2025 10:31:44 +0100 "Russell King (Oracle)"…
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_rstalong with all the other dwmac initialisers - there's no reason for this to be late asplat_dat->stmmac_rsthas already been populated.Next, replace the call to
ops->set_phy_mode()with an init functionsocfpga_dwmac_init()which will then be linked in toplat_dat->init.Then, add this to
plat_dat->init, and switch tostmmac_pltfr_pm_opsfrom the private ops. The runtime suspend/resume socfpga implementations are identical to the platform ones, but misses the noirq versions which this will add.Before we swap the order of
socfpga_dwmac_init()andstmmac_dvr_probe(), we need to change the way the interface is obtained, as that uses driver data and the structnet_devicewhich haven't been initialised. Save a pointer toplat_datin the socfpga private data, and use that to get the interface mode. We can then swap the order of the init and probe functions.
Quoted 8 lines
Finally, convert to
devm_stmmac_pltfr_probe()by moving the call toops->set_phy_mode()into an init function appropriately populatingplat_dat->init.v2: fix oops when calling
set_phy_mode()early..../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 79 +++++----------------- 1 file changed, 16 insertions(+), 63 deletions(-)
Feel free to CC: me for dwmac-socfpga stuff, I have some HW to test this on :)
Thanks for this work, it's working fine :)
For the series,
Maxime