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.

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
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 137d48451959..195559df81ad 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1587,6 +1587,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);
+
+	__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);
+
+	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);
+
+	return 0;
+}
+
+static void phy_sfp_module_remove(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->configure_mii)
+		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
+
+	if (phydev->n_ports == 1)
+		phydev->port = PORT_NONE;
+}
+
+static void phy_sfp_link_up(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->link_up)
+		port->ops->link_up(port);
+}
+
+static void phy_sfp_link_down(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->link_down)
+		port->ops->link_down(port);
+}
+
+static const struct sfp_upstream_ops sfp_phydev_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = phy_sfp_module_insert,
+	.module_remove = phy_sfp_module_remove,
+	.link_up = phy_sfp_link_up,
+	.link_down = phy_sfp_link_down,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
+};
+
 static int phy_add_port(struct phy_device *phydev, struct phy_port *port)
 {
 	int ret = 0;
@@ -3474,6 +3555,13 @@ static int phy_setup_ports(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Use generic SFP probing only if the driver didn't do so already */
+	if (!phydev->sfp_bus) {
+		ret = phy_sfp_probe(phydev, &sfp_phydev_ops);
+		if (ret)
+			goto out;
+	}
+
 	if (phydev->n_ports < phydev->max_n_ports) {
 		ret = phy_default_setup_single_port(phydev);
 		if (ret)
@@ -3509,6 +3597,25 @@ static int phy_setup_ports(struct phy_device *phydev)
 	return ret;
 }
 
+/**
+ * phy_get_sfp_port() - Returns the first valid SFP port of a PHY
+ * @phydev: pointer to the PHY device to get the SFP port from
+ *
+ * Returns: The first active SFP (serdes) port of a PHY device, NULL if none
+ * exist.
+ */
+struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
+{
+	struct phy_port *port;
+
+	list_for_each_entry(port, &phydev->ports, head)
+		if (port->active && port->is_mii)
+			return port;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(phy_get_sfp_port);
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
diff --git a/include/linux/phy.h b/include/linux/phy.h
index cb95f9008007..329603c802d1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2121,6 +2121,8 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config,
 		       struct netlink_ext_ack *extack);
 
+struct phy_port *phy_get_sfp_port(struct phy_device *phydev);
+
 extern const struct bus_type mdio_bus_type;
 extern const struct class mdio_bus_class;
 
-- 
2.49.0

On Tue, Jul 22, 2025 at 02:16:12PM +0200, Maxime Chevallier wrote:
> +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);
> +
> +	__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);
> +
> +	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);

I've been moving phylink away from using sfp_select_interface() because
it requires two stages of translation - one from the module capabilties
to linkmodes, and then linkmodes to interfaces.

sfp_parse_support() now provides the interfaces that the optical module
supports, and the possible interfaces that a copper module _might_
support (but we don't know for certain about that until we discover a
PHY.)

The only place in phylink where this function continues to be used is
when there's an optical module which supports multiple different
speeds, and we need to select it based on the advertising mask provided
by userspace. Everywhere else shouldn't use this function, but should
instead use the interfaces returned from sfp_parse_support().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Sun, 27 Jul 2025 10:56:52 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Jul 22, 2025 at 02:16:12PM +0200, Maxime Chevallier wrote:
> > +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);
> > +
> > +	__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);
> > +
> > +	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);  
> 
> I've been moving phylink away from using sfp_select_interface() because
> it requires two stages of translation - one from the module capabilties
> to linkmodes, and then linkmodes to interfaces.
> 
> sfp_parse_support() now provides the interfaces that the optical module
> supports, and the possible interfaces that a copper module _might_
> support (but we don't know for certain about that until we discover a
> PHY.)
> 
> The only place in phylink where this function continues to be used is
> when there's an optical module which supports multiple different
> speeds, and we need to select it based on the advertising mask provided
> by userspace. Everywhere else shouldn't use this function, but should
> instead use the interfaces returned from sfp_parse_support().

Thanks for the input, it'll make things even simpler so I'm all in for
that. I'll rework this for the next iteration, thanks a lot for taking
a look at this !

Maxime

>

Hello Russell,

I'm re-replying here even though a more recent version was sent, as I 
realise I forgot to fully address that.

On 27/07/2025 11:56, Russell King (Oracle) wrote:
> On Tue, Jul 22, 2025 at 02:16:12PM +0200, Maxime Chevallier wrote:
>> +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);
>> +
>> +	__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);
>> +
>> +	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);
> 
> I've been moving phylink away from using sfp_select_interface() because
> it requires two stages of translation - one from the module capabilties
> to linkmodes, and then linkmodes to interfaces.
> 
> sfp_parse_support() now provides the interfaces that the optical module
> supports, and the possible interfaces that a copper module _might_
> support (but we don't know for certain about that until we discover a
> PHY.)
> 
> The only place in phylink where this function continues to be used is
> when there's an optical module which supports multiple different
> speeds, and we need to select it based on the advertising mask provided
> by userspace. Everywhere else shouldn't use this function, but should
> instead use the interfaces returned from sfp_parse_support().
> 

In any case, we'll eventually have to select one of the interfaces if 
there are multiple matches from the sfp_parse_support. phylink maintains 
a sorted list of interfaces used as a preference, I think we should use 
the same list for phy-driver SFP. I'm thinking about moving 
phylink_choose_sfp_interface() in the sfp code, would you be OK with that ?

Maxime

On Tue, Jul 22, 2025 at 02:16:12PM +0200, 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

Too man n's.

> +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);

Strictly speeding, this is not allowed, reverse Christmas tree...

The assignment needs to move into the body of the function.

> +	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");

Maybe make this string different to the previous one, so somebody
debugging issues knows which happened?

> +/**
> + * phy_get_sfp_port() - Returns the first valid SFP port of a PHY
> + * @phydev: pointer to the PHY device to get the SFP port from
> + *
> + * Returns: The first active SFP (serdes) port of a PHY device, NULL if none
> + * exist.
> + */
> +struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
> +{
> +	struct phy_port *port;
> +
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active && port->is_mii)

Naming is hard, but this actually returns the first mii port.  Is
there a clear 1:1 mapping? I don't think i've ever seen it, but such a
SERDES port could be connected to a Ethernet switch? And when you get
further, add support for a MUX, could it be connected to a MUX?

	Andrew

On Sat, 26 Jul 2025 23:05:33 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Jul 22, 2025 at 02:16:12PM +0200, 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  
> 
> Too man n's.
> 
> > +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);  
> 
> Strictly speeding, this is not allowed, reverse Christmas tree...
> 
> The assignment needs to move into the body of the function.
> 
> > +	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");  
> 
> Maybe make this string different to the previous one, so somebody
> debugging issues knows which happened?
> 
> > +/**
> > + * phy_get_sfp_port() - Returns the first valid SFP port of a PHY
> > + * @phydev: pointer to the PHY device to get the SFP port from
> > + *
> > + * Returns: The first active SFP (serdes) port of a PHY device, NULL if none
> > + * exist.
> > + */
> > +struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
> > +{
> > +	struct phy_port *port;
> > +
> > +	list_for_each_entry(port, &phydev->ports, head)
> > +		if (port->active && port->is_mii)  
> 
> Naming is hard, but this actually returns the first mii port.  Is
> there a clear 1:1 mapping? I don't think i've ever seen it, but such a
> SERDES port could be connected to a Ethernet switch? And when you get
> further, add support for a MUX, could it be connected to a MUX?

Hmmm correct, that's a bit fragile. With a mux or a switch that could
be different indeed. There may even be PHYs with 2 MII interfaces one
day ?

So yes it's most of a time a 1:1 mapping but it doesn't have to, I'll
see how I can make that more reliable...

Thanks for the review,

Maxim