| 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
Hello everyone, This is version five of my series that adds support for a Gigabit Ethernet controller featured in the Renesas r9a06g032 SoC, of the RZ/N1 family. This GMAC device is based on a Synopsys IP and is compatible with the stmmac driver. My former colleague Clément Léger originally sent a series for this driver, but an issue in bringing up the PCS clock had blocked the upstreaming process. This issue has since been resolved by the following series: https://lore.kernel.org/all/20240326-rxc_bugfix-v6-0-24a74e5c761f@bootlin.com/ This series consists of a devicetree binding describing the RZN1 GMAC controller IP, a node for the GMAC1 device in the r9a06g032 SoC device tree, and the GMAC driver itself which is a glue layer in stmmac. There are also two patches by Russell that improve pcs initialization handling in stmmac. Best Regards, Romain Gantois --- Changes in v5: - Refactored the stmmac_xpcs_setup() function to group together XPCS and PCS setup logic. - Added a stmmac_pcs_clean() function as a counterpart to stmmac_pcs_setup() - Link to v4: https://lore.kernel.org/r/20240424-rzn1-gmac1-v4-0-852a5f2ce0c0@bootlin.com Changes in v4: - Removed the second parameters of the new pcs_init/exit() callbacks - Removed unnecessary interrupt-parent reference in gmac1 device node - Link to v3: https://lore.kernel.org/r/20240415-rzn1-gmac1-v3-0-ab12f2c4401d@bootlin.com Changes in v3: - Fixed a typo in the socfpga patch - Link to v2: https://lore.kernel.org/r/20240409-rzn1-gmac1-v2-0-79ca45f2fc79@bootlin.com Changes in v2: - Add pcs_init/exit callbacks in stmmac to solve race condition - Use pcs_init/exit callbacks in dwmac_socfpga glue layer - Miscellaneous device tree binding corrections - Link to v1: https://lore.kernel.org/r/20240402-rzn1-gmac1-v1-0-5be2b2894d8c@bootlin.com --- Clément Léger (3): dt-bindings: net: renesas,rzn1-gmac: Document RZ/N1 GMAC support net: stmmac: add support for RZ/N1 GMAC ARM: dts: r9a06g032: describe GMAC1 Russell King (Oracle) (2): net: stmmac: introduce pcs_init/pcs_exit stmmac operations net: stmmac: dwmac-socfpga: use pcs_init/pcs_exit Serge Semin (2): net: stmmac: Add dedicated XPCS cleanup method net: stmmac: Make stmmac_xpcs_setup() generic to all PCS devices .../devicetree/bindings/net/renesas,rzn1-gmac.yaml | 66 +++++++++++++ MAINTAINERS | 6 ++ arch/arm/boot/dts/renesas/r9a06g032.dtsi | 18 ++++ drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 +++ drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c | 86 +++++++++++++++++ .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 107 ++++++++++----------- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +-- drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 48 ++++++--- include/linux/stmmac.h | 2 + 11 files changed, 287 insertions(+), 76 deletions(-) --- base-commit: dd1941f801bc958d2ee13f5be8b38db6b034b806 change-id: 20240402-rzn1-gmac1-685cf8793d0e Best regards,