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);
+
+	__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;
@@ -3351,6 +3432,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)
@@ -3384,6 +3472,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_serdes)
+			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 0180f4d4fd7d..aef13fab8882 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2091,6 +2091,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

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

> +	__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?

> +
> +	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.

I'd suggest something like "post_sfp_insert()", please let me know what you 
think.

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

This check is a bit confusing to me. Could you please explain why you're only 
setting the phydev's SFP port to PORT_NONE if the PHY device only has one 
registered port? Shouldn't this be done regardless?

> +}
> +
> +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;
> @@ -3351,6 +3432,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) {

Should the phy_sfp_probe() API be changed to something explicitely legacy? I 
feel like people writing new PHY drivers could be confused if they see some 
other drivers calling phy_sfp_probe() and others not doing anything and still 
getting SFP busses.

> +		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)
> @@ -3384,6 +3472,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)

I'd suggest "phy_get_active_sfp_port()".

> +{
> +	struct phy_port *port;
> +
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active && port->is_serdes)
> +			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 0180f4d4fd7d..aef13fab8882 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -2091,6 +2091,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;

Thanks!

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

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...

> 
> > +	__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.

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).

> 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 :)

One thing though is that this series uses a mix of "is_serdes" and
"configure_mii" to mean pretty-much the same thing, I'll make the names
a bit more homogenous.

> > +
> > +	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;  
> 
> This check is a bit confusing to me. Could you please explain why you're only 
> setting the phydev's SFP port to PORT_NONE if the PHY device only has one 
> registered port? Shouldn't this be done regardless?

So that we don't overwrite what the other port would have set :) but,
that's a bit fragile as I said and probably not correct anyways, let me
double-check that.

Maxime