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

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 7761 bytes --]

On Wednesday, 28 May 2025 10:14:33 CEST Maxime Chevallier wrote:
> 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 :)

I see, thanks again for explaining.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]