| Message ID | 20240602143636.5839-11-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sun, Jun 02, 2024 at 05:36:24PM +0300, Serge Semin wrote: > Recently the DW XPCS DT-bindings have been introduced and the DW XPCS > driver has been altered to support the DW XPCS registered as a platform > device. In order to have the DW XPCS DT-device accessed from the STMMAC > driver let's alter the STMMAC PCS-setup procedure to support the > "pcs-handle" property containing the phandle reference to the DW XPCS > device DT-node. The respective fwnode will be then passed to the > xpcs_create_fwnode() function which in its turn will create the DW XPCS > descriptor utilized in the main driver for the PCS-related setups. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > index 807789d7309a..dc040051aa53 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c > @@ -497,15 +497,22 @@ int stmmac_mdio_reset(struct mii_bus *bus) > > int stmmac_pcs_setup(struct net_device *ndev) > { > + struct fwnode_handle *devnode, *pcsnode; > struct dw_xpcs *xpcs = NULL; > struct stmmac_priv *priv; > int addr, mode, ret; > > priv = netdev_priv(ndev); > mode = priv->plat->phy_interface; > + devnode = priv->plat->port_node; > > if (priv->plat->pcs_init) { > ret = priv->plat->pcs_init(priv); > + } else if (fwnode_property_present(devnode, "pcs-handle")) { > + pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0); > + xpcs = xpcs_create_fwnode(pcsnode, mode); > + fwnode_handle_put(pcsnode); > + ret = PTR_ERR_OR_ZERO(xpcs); Just figured, we might wish to be a bit more portable in the "pcs-handle" property semantics implementation seeing there can be at least three different PCS attached: DW XPCS Lynx PCS Renesas RZ/N1 MII Any suggestion of how to distinguish the passed handle? Perhaps named-property, phandle argument, by the compatible string or the node-name? -Serge(y) > } else if (priv->plat->mdio_bus_data && > priv->plat->mdio_bus_data->has_xpcs) { > addr = priv->plat->mdio_bus_data->xpcs_addr; > @@ -515,10 +522,8 @@ int stmmac_pcs_setup(struct net_device *ndev) > return 0; > } > > - if (ret) { > - dev_warn(priv->device, "No xPCS found\n"); > - return ret; > - } > + if (ret) > + return dev_err_probe(priv->device, ret, "No xPCS found\n"); > > priv->hw->xpcs = xpcs; > > -- > 2.43.0 >
On Mon, Jun 03, 2024 at 11:54:22AM +0300, Serge Semin wrote: > > if (priv->plat->pcs_init) { > > ret = priv->plat->pcs_init(priv); > > > + } else if (fwnode_property_present(devnode, "pcs-handle")) { > > + pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0); > > + xpcs = xpcs_create_fwnode(pcsnode, mode); > > + fwnode_handle_put(pcsnode); > > + ret = PTR_ERR_OR_ZERO(xpcs); > > Just figured, we might wish to be a bit more portable in the > "pcs-handle" property semantics implementation seeing there can be at > least three different PCS attached: > DW XPCS > Lynx PCS > Renesas RZ/N1 MII > > Any suggestion of how to distinguish the passed handle? Perhaps > named-property, phandle argument, by the compatible string or the > node-name? I can't think of a reasonable solution to this at the moment. One solution could be pushing this down into the platform code to deal with as an interim solution, via the new .pcs_init() method. We could also do that with the current XPCS code, since we know that only Intel mGBE uses xpcs. This would probably allow us to get rid of the has_xpcs flag.
On Mon, Jun 03, 2024 at 10:03:54AM +0100, Russell King (Oracle) wrote: > On Mon, Jun 03, 2024 at 11:54:22AM +0300, Serge Semin wrote: > > > if (priv->plat->pcs_init) { > > > ret = priv->plat->pcs_init(priv); > > > > > + } else if (fwnode_property_present(devnode, "pcs-handle")) { > > > + pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0); > > > + xpcs = xpcs_create_fwnode(pcsnode, mode); > > > + fwnode_handle_put(pcsnode); > > > + ret = PTR_ERR_OR_ZERO(xpcs); > > > > Just figured, we might wish to be a bit more portable in the > > "pcs-handle" property semantics implementation seeing there can be at > > least three different PCS attached: > > DW XPCS > > Lynx PCS > > Renesas RZ/N1 MII > > > > Any suggestion of how to distinguish the passed handle? Perhaps > > named-property, phandle argument, by the compatible string or the > > node-name? > > I can't think of a reasonable solution to this at the moment. One > solution could be pushing this down into the platform code to deal > with as an interim solution, via the new .pcs_init() method. > > We could also do that with the current XPCS code, since we know that > only Intel mGBE uses xpcs. This would probably allow us to get rid > of the has_xpcs flag. Basically you suggest to move the entire stmmac_pcs_setup() to the platforms, don't you? The patch 9 of this series indeed could have been converted to just moving the entire PCS-detection loop from stmmac_pcs_setup() to the Intel-specific pcs_init. But IMO some default/generic code would be still useful to preserve in the stmmac_pcs_setup() method. When it comes to the fwnode-based platform we at least could be falling back to the default DW XPCS device registration if no plat_stmmacenet_data::pcs_init() callback was specified and there was the "pcs-handle" property found, especially seeing DW *MAC and DW XPCS are of the same vendor. Based on that I can convert patch 9 of this series to introducing the pcs_init() callback in the Intel mGBE driver, but preserve the semantics of the rest of the series changes. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Jun 04, 2024 at 12:04:57PM +0300, Serge Semin wrote: > On Mon, Jun 03, 2024 at 10:03:54AM +0100, Russell King (Oracle) wrote: > > I can't think of a reasonable solution to this at the moment. One > > solution could be pushing this down into the platform code to deal > > with as an interim solution, via the new .pcs_init() method. > > > > We could also do that with the current XPCS code, since we know that > > only Intel mGBE uses xpcs. This would probably allow us to get rid > > of the has_xpcs flag. > > Basically you suggest to move the entire stmmac_pcs_setup() to the > platforms, don't you? The patch 9 of this series indeed could have > been converted to just moving the entire PCS-detection loop from > stmmac_pcs_setup() to the Intel-specific pcs_init. Yes, it's not like XPCS is used by more than one platform, it's only Intel mGBE. So I don't see why it should have a privileged position over any other PCS implementation that stmmac supports (there's now three different PCS.) If you don't want the code in the Intel driver, then what could be done is provide a core implementation that gets hooked into the .pcs_init() method. The same is probably true of other PCSes if they end up being shared across several different platforms.
On Tue, Jun 04, 2024 at 10:29:40AM +0100, Russell King (Oracle) wrote: > On Tue, Jun 04, 2024 at 12:04:57PM +0300, Serge Semin wrote: > > On Mon, Jun 03, 2024 at 10:03:54AM +0100, Russell King (Oracle) wrote: > > > I can't think of a reasonable solution to this at the moment. One > > > solution could be pushing this down into the platform code to deal > > > with as an interim solution, via the new .pcs_init() method. > > > > > > We could also do that with the current XPCS code, since we know that > > > only Intel mGBE uses xpcs. This would probably allow us to get rid > > > of the has_xpcs flag. > > > > Basically you suggest to move the entire stmmac_pcs_setup() to the > > platforms, don't you? The patch 9 of this series indeed could have > > been converted to just moving the entire PCS-detection loop from > > stmmac_pcs_setup() to the Intel-specific pcs_init. > > Yes, it's not like XPCS is used by more than one platform, it's only > Intel mGBE. So I don't see why it should have a privileged position > over any other PCS implementation that stmmac supports (there's now > three different PCS.) > Alas DW XPCS has already got a more privileged position. The STMMAC driver calls the XPCS driver methods here and there (supported ifaces, EEE or PHY setup). Unless these calls are converted to some standard/new phylink_pcs calls IMO it would be better to preserve the default DW XPCS init at least for the "pcs-handle" property to motivate the platform drivers developers to follow some pre-defined device description pattern (e.g. defining DW XPCS devices in device tree), but leave the .pcs_init() for some platform-specific PCS inits (including which have already been implemented). As I already mentioned DW XPCS is of Synopsys vendor. The IP-core has been invented to provide a bridge between the Synopsys MAC IP-cores and PMA (mainly Synopsys PMAs) for the 1G/10G links like 1000Base-X, and 10GBase-X/-R/-KX4/-KR. The reason we see just a single use-case of the XPCS in the driver is that even though the STMMAC driver has DW XGMAC support the driver is mainly utilized for the 1G MACs (I don't see any platform currently having DW XGMAC defined). Since DW GMAC/QoS Eth can be configured to have the standard PHY interfaces available there is no need in XPCS in these cases (except a weird Intel mGBE). But when it comes to DW XGMAC it can be synthesized with GMII and XGMII interfaces only. These're exactly interfaces which DW XPCS supports on upstream. Thus basically the DW XPCS IP-core has been mainly produced for been utilized in a couple with DW XGMAC providing a ready-to-use solution for the XFP/SFP(+) ports or backplane-based applications. So should we have more DW XGMACs supported in the kernel we would have met more DW XPCS defined in there too. > If you don't want the code in the Intel driver, then what could be > done is provide a core implementation that gets hooked into the > .pcs_init() method. I don't mind converting patch 9 to moving the XPCS registration in the Intel-specific .pcs_init() (especially seeing it's just a single xpcs_create_mdiodev() call), but having the "pcs-handle" property handled generically in the STMMAC core would be a useful thing to have (see my reasoning above). -Serge(y) > > The same is probably true of other PCSes if they end up being shared > across several different platforms. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index 807789d7309a..dc040051aa53 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -497,15 +497,22 @@ int stmmac_mdio_reset(struct mii_bus *bus) int stmmac_pcs_setup(struct net_device *ndev) { + struct fwnode_handle *devnode, *pcsnode; struct dw_xpcs *xpcs = NULL; struct stmmac_priv *priv; int addr, mode, ret; priv = netdev_priv(ndev); mode = priv->plat->phy_interface; + devnode = priv->plat->port_node; if (priv->plat->pcs_init) { ret = priv->plat->pcs_init(priv); + } else if (fwnode_property_present(devnode, "pcs-handle")) { + pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0); + xpcs = xpcs_create_fwnode(pcsnode, mode); + fwnode_handle_put(pcsnode); + ret = PTR_ERR_OR_ZERO(xpcs); } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { addr = priv->plat->mdio_bus_data->xpcs_addr; @@ -515,10 +522,8 @@ int stmmac_pcs_setup(struct net_device *ndev) return 0; } - if (ret) { - dev_warn(priv->device, "No xPCS found\n"); - return ret; - } + if (ret) + return dev_err_probe(priv->device, ret, "No xPCS found\n"); priv->hw->xpcs = xpcs;
Recently the DW XPCS DT-bindings have been introduced and the DW XPCS driver has been altered to support the DW XPCS registered as a platform device. In order to have the DW XPCS DT-device accessed from the STMMAC driver let's alter the STMMAC PCS-setup procedure to support the "pcs-handle" property containing the phandle reference to the DW XPCS device DT-node. The respective fwnode will be then passed to the xpcs_create_fwnode() function which in its turn will create the DW XPCS descriptor utilized in the main driver for the PCS-related setups. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)