Message ID | 20240430-rzn1-gmac1-v5-0-62f65a84f418@bootlin.com |
---|---|
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 >
On Tue, Apr 30, 2024 at 09:29:44AM +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> > Co-developed-by: Romain Gantois <romain.gantois@bootlin.com> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> Reviewed-by: Serge Semin <fancer.lancer@gmail.com> -Serge(y) > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++++- > include/linux/stmmac.h | 2 ++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > index af8ad9768da10..1c788caea0cfb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > @@ -505,7 +505,10 @@ int stmmac_pcs_setup(struct net_device *ndev) > priv = netdev_priv(ndev); > mode = priv->plat->phy_interface; > > - if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { > + if (priv->plat->pcs_init) { > + ret = priv->plat->pcs_init(priv); > + } else if (priv->plat->mdio_bus_data && > + priv->plat->mdio_bus_data->has_xpcs) { > /* Try to probe the XPCS by scanning all addresses */ > for (addr = 0; addr < PHY_MAX_ADDR; addr++) { > xpcs = xpcs_create_mdiodev(priv->mii, addr, mode); > @@ -531,6 +534,9 @@ int stmmac_pcs_setup(struct net_device *ndev) > > void stmmac_pcs_clean(struct stmmac_priv *priv) > { > + if (priv->plat->pcs_exit) > + priv->plat->pcs_exit(priv); > + > if (!priv->hw->xpcs) > return; > > 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; > > -- > 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