| Message ID | 20240430-rzn1-gmac1-v5-2-62f65a84f418@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: Add support for RZN1 GMAC devices | expand |
Hi Romain On Tue, Apr 30, 2024 at 09:29:42AM +0200, Romain Gantois wrote: > From: Serge Semin <fancer.lancer@gmail.com> > > Currently the XPCS handler destruction is performed in the > stmmac_mdio_unregister() method. It doesn't look good because the handler > isn't originally created in the corresponding protagonist > stmmac_mdio_unregister(), but in the stmmac_xpcs_setup() function. In > order to have more coherent MDIO and XPCS setup/cleanup procedures, > let's move the DW XPCS destruction to the dedicated stmmac_pcs_clean() > method. > > This method will also be used to cleanup PCS hardware using the > pcs_exit() callback that will be introduced to stmmac in a subsequent > patch. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > Co-developed-by: Romain Gantois <romain.gantois@bootlin.com> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- > drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 12 +++++++++--- > 3 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index dddcaa9220cc3..7e0d727ed795b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -361,6 +361,7 @@ int stmmac_mdio_unregister(struct net_device *ndev); > int stmmac_mdio_register(struct net_device *ndev); > int stmmac_mdio_reset(struct mii_bus *mii); > int stmmac_xpcs_setup(struct mii_bus *mii); > +void stmmac_pcs_clean(struct stmmac_priv *priv); > void stmmac_set_ethtool_ops(struct net_device *netdev); > > int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags); > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 59bf83904b62d..2a55c5d07f6b8 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7789,8 +7789,9 @@ int stmmac_dvr_probe(struct device *device, > > error_netdev_register: > phylink_destroy(priv->phylink); > -error_xpcs_setup: > error_phy_setup: > + stmmac_pcs_clean(priv); > +error_xpcs_setup: > if (priv->hw->pcs != STMMAC_PCS_TBI && > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > @@ -7832,6 +7833,9 @@ void stmmac_dvr_remove(struct device *dev) > if (priv->plat->stmmac_rst) > reset_control_assert(priv->plat->stmmac_rst); > reset_control_assert(priv->plat->stmmac_ahb_rst); > + > + stmmac_pcs_clean(priv); > + > if (priv->hw->pcs != STMMAC_PCS_TBI && > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > index 0542cfd1817e6..508bd39cbe2b3 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > @@ -523,6 +523,15 @@ int stmmac_xpcs_setup(struct mii_bus *bus) > return 0; > } > > +void stmmac_pcs_clean(struct stmmac_priv *priv) Ideally it would have been great to have the entire driver fixed to accept the stmmac_priv pointer as the functions argument. But this would be too tiresome. Anyway seeing the PCS-setup protagonist method has the net_device pointer passed I would implement the same prototype for the antagonist even though it would require an additional local variable. That will make the MDIO and PCS local interface-functions looking alike and as if unified. That is the reason of why I made stmmac_xpcs_clean() accepting the net_device pointer. Alternatively both stmmac_pcs_setup() and stmmac_pcs_clean() could be converted to just accepting a pointer to the stmmac_priv instance. -Serge(y) > +{ > + if (!priv->hw->xpcs) > + return; > + > + xpcs_destroy(priv->hw->xpcs); > + priv->hw->xpcs = NULL; > +} > + > /** > * stmmac_mdio_register > * @ndev: net device structure > @@ -679,9 +688,6 @@ int stmmac_mdio_unregister(struct net_device *ndev) > if (!priv->mii) > return 0; > > - if (priv->hw->xpcs) > - xpcs_destroy(priv->hw->xpcs); > - > mdiobus_unregister(priv->mii); > priv->mii->priv = NULL; > mdiobus_free(priv->mii); > > -- > 2.44.0 >
Hi Serge, On Fri, 3 May 2024, Serge Semin wrote: > > > +void stmmac_pcs_clean(struct stmmac_priv *priv) > > Ideally it would have been great to have the entire driver fixed to > accept the stmmac_priv pointer as the functions argument. But this > would be too tiresome. Anyway seeing the PCS-setup protagonist method > has the net_device pointer passed I would implement the same prototype > for the antagonist even though it would require an additional local > variable. That will make the MDIO and PCS local interface-functions > looking alike and as if unified. That is the reason of why I made > stmmac_xpcs_clean() accepting the net_device pointer. > > Alternatively both stmmac_pcs_setup() and stmmac_pcs_clean() could be > converted to just accepting a pointer to the stmmac_priv instance. I think that adapting stmmac_pcs_clean() to take a net_device struct would be more appropriate since it's the simpler of the two methods. I'll implement this in the next version. Thanks,
On Mon, May 06, 2024 at 09:07:41AM +0200, Romain Gantois wrote: > Hi Serge, > > On Fri, 3 May 2024, Serge Semin wrote: > > > > > > +void stmmac_pcs_clean(struct stmmac_priv *priv) > > > > Ideally it would have been great to have the entire driver fixed to > > accept the stmmac_priv pointer as the functions argument. But this > > would be too tiresome. Anyway seeing the PCS-setup protagonist method > > has the net_device pointer passed I would implement the same prototype > > for the antagonist even though it would require an additional local > > variable. That will make the MDIO and PCS local interface-functions > > looking alike and as if unified. That is the reason of why I made > > stmmac_xpcs_clean() accepting the net_device pointer. > > > > Alternatively both stmmac_pcs_setup() and stmmac_pcs_clean() could be > > converted to just accepting a pointer to the stmmac_priv instance. > > I think that adapting stmmac_pcs_clean() to take a net_device struct would be > more appropriate since it's the simpler of the two methods. I'll implement this > in the next version. 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.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index dddcaa9220cc3..7e0d727ed795b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -361,6 +361,7 @@ int stmmac_mdio_unregister(struct net_device *ndev); int stmmac_mdio_register(struct net_device *ndev); int stmmac_mdio_reset(struct mii_bus *mii); int stmmac_xpcs_setup(struct mii_bus *mii); +void stmmac_pcs_clean(struct stmmac_priv *priv); void stmmac_set_ethtool_ops(struct net_device *netdev); int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 59bf83904b62d..2a55c5d07f6b8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7789,8 +7789,9 @@ int stmmac_dvr_probe(struct device *device, error_netdev_register: phylink_destroy(priv->phylink); -error_xpcs_setup: error_phy_setup: + stmmac_pcs_clean(priv); +error_xpcs_setup: if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); @@ -7832,6 +7833,9 @@ void stmmac_dvr_remove(struct device *dev) if (priv->plat->stmmac_rst) reset_control_assert(priv->plat->stmmac_rst); reset_control_assert(priv->plat->stmmac_ahb_rst); + + stmmac_pcs_clean(priv); + if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) stmmac_mdio_unregister(ndev); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index 0542cfd1817e6..508bd39cbe2b3 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -523,6 +523,15 @@ int stmmac_xpcs_setup(struct mii_bus *bus) return 0; } +void stmmac_pcs_clean(struct stmmac_priv *priv) +{ + if (!priv->hw->xpcs) + return; + + xpcs_destroy(priv->hw->xpcs); + priv->hw->xpcs = NULL; +} + /** * stmmac_mdio_register * @ndev: net device structure @@ -679,9 +688,6 @@ int stmmac_mdio_unregister(struct net_device *ndev) if (!priv->mii) return 0; - if (priv->hw->xpcs) - xpcs_destroy(priv->hw->xpcs); - mdiobus_unregister(priv->mii); priv->mii->priv = NULL; mdiobus_free(priv->mii);