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: 7286 bytes --]

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

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