| Message ID | 20240424-rzn1-gmac1-v4-2-852a5f2ce0c0@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: Add support for RZN1 GMAC devices | expand |
Hi Romain On Wed, Apr 24, 2024 at 11:06:20AM +0200, Romain Gantois wrote: > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > Introduce a mechanism whereby platforms can create their PCS instances > prior to the network device being published to userspace, but after > some of the core stmmac initialisation has been completed. This means > that the data structures that platforms need will be available. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > [rgantois: removed second parameters of new callbacks] > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ++++++++++++++ > include/linux/stmmac.h | 2 ++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 59bf83904b62d..bee9c9ab31a88 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7200,6 +7200,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > if (ret) > return ret; > > + if (priv->plat->pcs_init) { > + ret = priv->plat->pcs_init(priv); > + if (ret) > + return ret; > + } > + Once again. There is a ready-to-use stmmac_xpcs_setup() method. Which is currently intended for the XPCS setups. Let's collect all the PCS-related stuff in a single place there. That will make code cleaner and easier to read. This was discussed on v3: https://lore.kernel.org/netdev/42chuecdt7dpgm6fcrtt2crifvv5hflmtnmdrw5fvk3r7pwjgu@hlcv56dbeosf/ You agreed to do that, but just ignored in result. I'll repeat what I said in v3: On Tue, 16 Apr 2024 16:41:33 +0300, Serge Semin wrote: > I am currently working on my Memory-mapped DW XPCS patchset cooking: > https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@gmail.com/ > The changes in this series seems to intersect to what is/will be > introduced in my patchset. In particular as before I am going to > use the "pcs-handle" property for getting the XPCS node. If so what > about collecting PCS-related things in a single place. Like this: > > int stmmac_xpcs_setup(struct net_device *ndev) > { > ... > > if (priv->plat->pcs_init) { > return priv->plat->pcs_init(priv); /* Romain' part */ > } else if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) { > /* My DW XPCS part */ > } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { > /* Currently implemented procedure */ > } > > ... > } > > void stmmac_xpcs_clean(struct net_device *ndev) > { > ... > > if (priv->plat->pcs_exit) { > priv->plat->pcs_exit(priv); > return; > > } > > xpcs_destroy(priv->hw->xpcs); > priv->hw->xpcs = NULL; > } > > Please see the last two patches in my series: > https://lore.kernel.org/netdev/20231205103559.9605-16-fancer.lancer@gmail.com/ > https://lore.kernel.org/netdev/20231205103559.9605-17-fancer.lancer@gmail.com/ > as a reference of how the changes could be provided. You replied it was a good idea, but the function names should be renamed. That's not a problem. Just create a pre-requisite patch which does that. So the patch in the subject could be replaced with four subsequent patches: 1. Move the conditional XPCS-setup execution into the stmmac_xpcs_setup() method. This change is partly implemented here https://lore.kernel.org/netdev/20231205103559.9605-17-fancer.lancer@gmail.com/ 2. Rename stmmac_xpcs_setup() method to just stmmac_pcs_setup() as a preparation before adding the platform-specific PCS init()/exit() callbacks. 3. Introduce the PCS-cleanup method. You can pick it up from here, but use the stmmac_pcs_clean() name: https://lore.kernel.org/netdev/20231205103559.9605-16-fancer.lancer@gmail.com/ 4. Add pcc_init()/pcs_exit() callbacks as it's done in this patch but call them in the stmmac_pcs_setup()/stmmac_pcs_clean() methods instead of open-coding in the more generic stmmac_hw_init()/stmmac_hw_exit() functions. It doesn't look as that much hard thing to do, but will cause having a better readable code by providing a single coherent function for all PCS'es. -Serge(y) > /* Get the HW capability (new GMAC newer than 3.50a) */ > priv->hw_cap_support = stmmac_get_hw_features(priv); > if (priv->hw_cap_support) { > @@ -7282,6 +7288,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > return 0; > } > > +static void stmmac_hw_exit(struct stmmac_priv *priv) > +{ > + if (priv->plat->pcs_exit) > + priv->plat->pcs_exit(priv); > +} > + > > [...]
Hi Serge, On Wed, 24 Apr 2024, Serge Semin wrote: > Once again. There is a ready-to-use stmmac_xpcs_setup() method. Which > is currently intended for the XPCS setups. Let's collect all the > PCS-related stuff in a single place there. That will make code cleaner > and easier to read. This was discussed on v3: > > https://lore.kernel.org/netdev/42chuecdt7dpgm6fcrtt2crifvv5hflmtnmdrw5fvk3r7pwjgu@hlcv56dbeosf/ > > You agreed to do that, but just ignored in result. I'll repeat what I > said in v3: Yeah sorry I took a quick look at your merged patches and thought that stmmac_xpcs_setup() had been repurposed in the meantime, but it seems like I was just confused about that. > It doesn't look as that much hard thing to do, but will cause having a > better readable code by providing a single coherent function for all > PCS'es. Sure, I'll get to it in v5. Thanks,
On Wed, Apr 24, 2024 at 06:33:30PM +0200, Romain Gantois wrote: > Hi Serge, > > On Wed, 24 Apr 2024, Serge Semin wrote: > > > Once again. There is a ready-to-use stmmac_xpcs_setup() method. Which > > is currently intended for the XPCS setups. Let's collect all the > > PCS-related stuff in a single place there. That will make code cleaner > > and easier to read. This was discussed on v3: > > > > https://lore.kernel.org/netdev/42chuecdt7dpgm6fcrtt2crifvv5hflmtnmdrw5fvk3r7pwjgu@hlcv56dbeosf/ > > > > You agreed to do that, but just ignored in result. I'll repeat what I > > said in v3: > > Yeah sorry I took a quick look at your merged patches and thought that > stmmac_xpcs_setup() had been repurposed in the meantime, but it seems like I was > just confused about that. > > > It doesn't look as that much hard thing to do, but will cause having a > > better readable code by providing a single coherent function for all > > PCS'es. > > Sure, I'll get to it in v5. Awesome! Thanks. -Serge(y) > > Thanks, > > -- > Romain Gantois, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 59bf83904b62d..bee9c9ab31a88 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7200,6 +7200,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) if (ret) return ret; + if (priv->plat->pcs_init) { + ret = priv->plat->pcs_init(priv); + if (ret) + return ret; + } + /* Get the HW capability (new GMAC newer than 3.50a) */ priv->hw_cap_support = stmmac_get_hw_features(priv); if (priv->hw_cap_support) { @@ -7282,6 +7288,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) return 0; } +static void stmmac_hw_exit(struct stmmac_priv *priv) +{ + if (priv->plat->pcs_exit) + priv->plat->pcs_exit(priv); +} + static void stmmac_napi_add(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); @@ -7795,6 +7807,7 @@ int stmmac_dvr_probe(struct device *device, priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); error_mdio_register: + stmmac_hw_exit(priv); stmmac_napi_del(ndev); error_hw_init: destroy_workqueue(priv->wq); @@ -7835,6 +7848,7 @@ void stmmac_dvr_remove(struct device *dev) if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); + stmmac_hw_exit(priv); destroy_workqueue(priv->wq); mutex_destroy(&priv->lock); bitmap_free(priv->af_xdp_zc_qps); diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index dfa1828cd756a..4a24a246c617d 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -285,6 +285,8 @@ struct plat_stmmacenet_data { int (*crosststamp)(ktime_t *device, struct system_counterval_t *system, void *ctx); void (*dump_debug_regs)(void *priv); + int (*pcs_init)(struct stmmac_priv *priv); + void (*pcs_exit)(struct stmmac_priv *priv); void *bsp_priv; struct clk *stmmac_clk; struct clk *pclk;