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 [off-list ref] wrote:

quoted

Hi Maxime,

On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:

quoted

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.

quoted

quoted

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

quoted

quoted

+
+	linkmode_and(sfp_support, port->supported, sfp_support);
+
+	if (linkmode_empty(sfp_support)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module

inserted\n");

quoted

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

quoted

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

quoted

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.

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.

Sure, sounds good.

quoted

quoted

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

All right, that makes sense given what you've already told me.

Thanks,

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