Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Romain

On Wed, 28 May 2025 09:35:35 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> On Friday, 23 May 2025 14:54:57 CEST Maxime Chevallier wrote:
> > Hi Romain,
> > 
> > On Mon, 12 May 2025 10:38:52 +0200
> > 
> > Romain Gantois <romain.gantois@bootlin.com> wrote:  
> > > Hi Maxime,
> > > 
> > > On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:  
> > > > There are currently 4 PHY drivers that can drive downstream SFPs:
> > > > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > > > logic is boilerplate, either calling into generic phylib helpers (for
> > > > SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> > > > 
> > > > bit of validation :
> > > >  - Getting the module's expected interface mode
> > > >  - Making sure the PHY supports it
> > > >  - Optionnaly perform some configuration to make sure the PHY outputs
> > > >  
> > > >    the right mode
> > > > 
> > > > This can be made more generic by leveraging the phy_port, and its
> > > > configure_mii() callback which allows setting a port's interfaces when
> > > > the port is a serdes.
> > > > 
> > > > Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> > > > bus itself, but an SFP phandle is found in devicetree/firmware, then the
> > > > generic PHY SFP support will be used, relying on port ops.
> > > > 
> > > > PHY driver need to :
> > > >  - Register a .attach_port() callback
> > > >  - When a serdes port is registered to the PHY, drivers must set
> > > >  
> > > >    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
> > > >  
> > > >  - If the port has limitations regarding speed, duplex and aneg, the
> > > >  
> > > >    port can also fine-tune the final linkmodes that can be supported
> > > >  
> > > >  - The port may register a set of ops, including .configure_mii(), that
> > > >  
> > > >    will be called at module_insert time to adjust the interface based on
> > > >    the module detected.
> > > > 
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > > 
> > > >  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
> > > >  include/linux/phy.h          |   2 +
> > > >  2 files changed, 109 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index aaf0eccbefba..aca3a47cbb66 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct
> > > > sfp_bus
> > > > *bus) }
> > > > 
> > > >  EXPORT_SYMBOL(phy_sfp_detach);
> > > > 
> > > > +static int phy_sfp_module_insert(void *upstream, const struct
> > > > sfp_eeprom_id *id) +{
> > > > +	struct phy_device *phydev = upstream;
> > > > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > > > +  
> > > 
> > > RCT  
> > 
> > Can't be done here, it won't build if in the other order...
> >   
> 
> You could always separate the declaration from the assignment, I've seen that 
> done quite a lot to keep things in RCT.
> 
> > > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > > > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > > > +	phy_interface_t iface;
> > > > +
> > > > +	linkmode_zero(sfp_support);
> > > > +
> > > > +	if (!port)
> > > > +		return -EINVAL;
> > > > +
> > > > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> > > > +
> > > > +	if (phydev->n_ports == 1)
> > > > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,  
> > > 
> > > sfp_support);
> > > 
> > > As mentionned below, this check looks a bit strange to me. Why are we only
> > > parsing the SFP port if the PHY device only has one registered port?  
> > 
> > Because phydev->port is global to the PHY. If we have another port,
> > then phydev->port must be handled differently so that SFP insertion /
> > removal doesn't overwrite what the other port is.
> >   
> 
> Okay, I see, thanks for explaining.
> 
> > Handling of phydev->port is still fragile in this state of the series,
> > I'll try to improve on that for V7 and document it better.
> >   
> > > > +
> > > > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > > > +
> > > > +	if (linkmode_empty(sfp_support)) {
> > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module  
> > > 
> > > inserted\n");
> > >   
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > > > +
> > > > +	/* Check that this interface is supported */
> > > > +	if (!test_bit(iface, port->interfaces)) {
> > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module  
> > > 
> > > inserted\n");
> > >   
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (port->ops && port->ops->configure_mii)
> > > > +		return port->ops->configure_mii(port, true, iface);  
> > > 
> > > The name "configure_mii()" seems a bit narrow-scoped to me, as this
> > > callback might have to configure something else than a MII link. For
> > > example, if a DAC SFP module is inserted, the downstream side of the
> > > transciever will have to be configured to 1000Base-X or something
> > > similar.  
> > 
> > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > PHY_INTERFACE_MODE_1000BASEX).
> >   
> 
> Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes you're 
> right.
> 
> > > I'd suggest something like "post_sfp_insert()", please let me know what
> > > you
> > > think.  
> > 
> > That's not intended to be SFP-specific though. post_sfp_insert() sounds
> > lke the narrow-scoped name to me :) Here we are dealing with a PHy that
> > has a media-side port that isn't a MDI port, but an MII interface like
> > a MAC would usually export. There may be an SFP here, or something else
> > entirely :)
> >   
> 
> Is that callback really not meant to be SFP-specific? It's only called from 
> phy_sfp_module_insert() though.

Well for now yeah as this is the only user now, but ports are meant to
be about more than drving SFPs.

This series as of today is quite long but doesn't cover the other
classes of use-cases which are non-phy-driven ports.

Taking the example of the Turris Omnia, we have something like that :

      +------------------+ 
      | MUX  +--------+  |
      |    +-| port 1 | --- SGMII - PHY
      |    | +--------+  |
MAC -------+             |
      |    | +--------+  |
      |    +-| port 2 | ---- SGMII/1000BaseX - SFP
      |      +--------+  |
      +------------------+

Here we have 1 mac that drives 2 ports through a MUX. So the ports here
would be driven by the mux driver (which I have a framewrok for ready to
send once this series lands). The goal would be to use the same
phy_port representation for these 2 ports, the .configure_mii()
callback will be used to switch between ports (so, perform the muxing),
then the rest of the stack takes over through the usual means (phylink,
phylib and all that). So here, the .configure_mii() ops doesn't
necessarily drives an SFP :)

Maxime