mbox series

[net-next,v6,00/14] Introduce an ethernet port representation

Message ID 20250507135331.76021-1-maxime.chevallier@bootlin.com
Headers show
Series Introduce an ethernet port representation | expand

Message

Maxime Chevallier May 7, 2025, 1:53 p.m. UTC
Hi everyone,

Here's a V6 for the ongoing support for Ethernet media-side port
support.

This V6 addresses Jakub's and Simon's comments, but could use some
review on the overall approach, any comment is welcome :)

Before going into the details, a few important notes :

 - This is only a first phase. It instantiates the port, and leverage
   that to make the MAC <-> PHY <-> SFP usecase simpler.

 - Next phase will deal with controlling the port state, as well as the
   netlink uAPI for that.

 - The end-goal is to enable support for complex port MUX. This
   preliminary work focuses on PHY-driven ports, but this will be
   extended to support muxing at the MII level (Multi-phy, or compo PHY
   + SFP as found on Turris Omnia for example).

 - The naming is definitely not set in stone. I named that "phy_port",
   but this may convey the false sense that this is phylib-specific.
   Even the word "port" is not that great, as it already has several
   different meanings in the net world (switch port, devlink port,
   etc.). I used the term "connector" in the binding.

A bit of history on that work :

The end goal that I personnaly want to achieve is :

            + PHY - RJ45
            | 
 MAC - MUX -+ PHY - RJ45

After many discussions here on netdev@, but also at netdevconf[1] and
LPC[2], there appears to be several analoguous designs that exist out
there.

[1] : https://netdevconf.info/0x17/sessions/talk/improving-multi-phy-and-multi-port-interfaces.html
[2] : https://lpc.events/event/18/contributions/1964/ (video isn't the
right one)

Take the MAchiatobin, it has 2 interfaces that looks like this :

 MAC - PHY -+ RJ45
            |
	    + SFP - Whatever the module does

Now, looking at the Turris Omnia, we have :


 MAC - MUX -+ PHY - RJ45
            |
	    + SFP - Whatever the module does

We can find more example of this kind of designs, the common part is
that we expose multiple front-facing media ports. This is what this
current work aims at supporting. As of right now, it does'nt add any
support for muxing, but this will come later on.

This first phase focuses on phy-driven ports only, but there are already
quite some challenges already. For one, we can't really autodetect how
many ports are sitting behind a PHY. That's why this series introduces a
new binding. Describing ports in DT should however be a last-resort
thing when we need to clear some ambiguity about the PHY media-side.

The only use-cases that we have today for multi-port PHYs are combo PHYs
that drive both a Copper port and an SFP (the Macchiatobin case). This
in itself is challenging and this series only addresses part of this
support, by registering a phy_port for the PHY <-> SFP connection. The
SFP module should in the end be considered as a port as well, but that's
not yet the case.

However, because now PHYs can register phy_ports for every media-side
interface they have, they can register the capabilities of their ports,
which allows making the PHY-driver SFP case much more generic.

Let me know what you think, I'm all in for discussions :)

Regards,

Changes in V6:

 - Fixed kdoc on patch 3
 - Addressed a missing port-ops registration for the Marvell 88x2222
   driver
 - Addressed a warning reported by Simon on the DP83822 when building
   without CONFIG_OF_MDIO

Changes in V5 :

 - renamed the bindings to use the term "connector" instead of "port"
 - Rebased, and fixed some issues reported on the 83822 driver
 - Use phy_caps

Changes in V4 :

 - Introduced a kernel doc
 - Reworked the mediums definitions in patch 2
 - QCA807x now uses the generic SFP support
 - Fixed some implementation bugs to build the support list based on the
   interfaces supported on a port

V5: https://lore.kernel.org/netdev/20250425141511.182537-1-maxime.chevallier@bootlin.com/
V4: https://lore.kernel.org/netdev/20250213101606.1154014-1-maxime.chevallier@bootlin.com/
V3: https://lore.kernel.org/netdev/20250207223634.600218-1-maxime.chevallier@bootlin.com/
RFC V2: https://lore.kernel.org/netdev/20250122174252.82730-1-maxime.chevallier@bootlin.com/
RFC V1: https://lore.kernel.org/netdev/20241220201506.2791940-1-maxime.chevallier@bootlin.com/

Maxime



Maxime Chevallier (14):
  dt-bindings: net: Introduce the ethernet-connector description
  net: ethtool: Introduce ETHTOOL_LINK_MEDIUM_* values
  net: phy: Introduce PHY ports representation
  net: phy: dp83822: Add support for phy_port representation
  net: phy: Create a phy_port for PHY-driven SFPs
  net: phy: Introduce generic SFP handling for PHY drivers
  net: phy: marvell-88x2222: Support SFP through phy_port interface
  net: phy: marvell: Support SFP through phy_port interface
  net: phy: marvell10g: Support SFP through phy_port
  net: phy: at803x: Support SFP through phy_port interface
  net: phy: qca807x: Support SFP through phy_port interface
  net: phy: Only rely on phy_port for PHY-driven SFP
  net: phy: dp83822: Add SFP support through the phy_port interface
  Documentation: networking: Document the phy_port infrastructure

 .../bindings/net/ethernet-connector.yaml      |  47 +++
 .../devicetree/bindings/net/ethernet-phy.yaml |  18 +
 Documentation/networking/index.rst            |   1 +
 Documentation/networking/phy-port.rst         | 111 +++++++
 MAINTAINERS                                   |   3 +
 drivers/net/phy/Makefile                      |   3 +-
 drivers/net/phy/dp83822.c                     |  78 +++--
 drivers/net/phy/marvell-88x2222.c             |  98 +++---
 drivers/net/phy/marvell.c                     | 100 +++---
 drivers/net/phy/marvell10g.c                  |  37 +--
 drivers/net/phy/phy_device.c                  | 312 +++++++++++++++++-
 drivers/net/phy/phy_port.c                    | 188 +++++++++++
 drivers/net/phy/qcom/at803x.c                 |  64 +---
 drivers/net/phy/qcom/qca807x.c                |  75 ++---
 include/linux/ethtool.h                       |  33 +-
 include/linux/phy.h                           |  38 ++-
 include/linux/phy_port.h                      |  93 ++++++
 include/uapi/linux/ethtool.h                  |  20 ++
 net/ethtool/common.c                          | 265 +++++++++------
 19 files changed, 1197 insertions(+), 387 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet-connector.yaml
 create mode 100644 Documentation/networking/phy-port.rst
 create mode 100644 drivers/net/phy/phy_port.c
 create mode 100644 include/linux/phy_port.h

Comments

Romain Gantois May 12, 2025, 7:53 a.m. UTC | #1
Hi Maxime,

On Wednesday, 7 May 2025 15:53:19 CEST Maxime Chevallier wrote:
> Ethernet provides a wide variety of layer 1 protocols and standards for
> data transmission. The front-facing ports of an interface have their own
> complexity and configurability.
> 
...
> +
> +static int phy_default_setup_single_port(struct phy_device *phydev)
> +{
> +	struct phy_port *port = phy_port_alloc();
> +
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->parent_type = PHY_PORT_PHY;
> +	port->phy = phydev;
> +	linkmode_copy(port->supported, phydev->supported);
> +
> +	phy_add_port(phydev, port);
> +
> +	/* default medium is copper */
> +	if (!port->mediums)
> +		port->mediums |= BIT(ETHTOOL_LINK_MEDIUM_BASET);

Could this be moved to phy_port_alloc() instead? That way, you'd avoid the 
extra conditional and the "default medium == baseT" rule would be enforced as 
early as possible.

> +
> +	return 0;
> +}
> +
> +static int of_phy_ports(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *mdi;
> +	struct phy_port *port;
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return 0;
> +
> +	if (!node)
> +		return 0;
> +
> +	mdi = of_get_child_by_name(node, "mdi");
> +	if (!mdi)
> +		return 0;

There are a lot of different possible failure paths here, so some specific error 
messages would be relevant IMO.

> +
> +	for_each_available_child_of_node_scoped(mdi, port_node) {
> +		port = phy_of_parse_port(port_node);
> +		if (IS_ERR(port)) {
> +			err = PTR_ERR(port);
> +			goto out_err;
> +		}
> +
> +		port->parent_type = PHY_PORT_PHY;
> +		port->phy = phydev;
> +		err = phy_add_port(phydev, port);
> +		if (err)
> +			goto out_err;
> +	}
> +	of_node_put(mdi);
> +
> +	return 0;
> +
> +out_err:
> +	phy_cleanup_ports(phydev);
> +	of_node_put(mdi);
> +	return err;
> +}
> +
> +static int phy_setup_ports(struct phy_device *phydev)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(ports_supported);
> +	struct phy_port *port;
> +	int ret;
> +
> +	ret = of_phy_ports(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->n_ports < phydev->max_n_ports) {
> +		ret = phy_default_setup_single_port(phydev);
> +		if (ret)
> +			goto out;
> +	}

Couldn't the function return at this point if phydev->n_ports is 0? That 
would eliminate the need for the linkmode_empty() check later.

> +
> +	linkmode_zero(ports_supported);
> +
> +	/* Aggregate the supported modes, which are made-up of :
> +	 *  - What the PHY itself supports
> +	 *  - What the sum of all ports support
> +	 */
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active)
> +			linkmode_or(ports_supported, ports_supported,
> +				    port->supported);
> +
> +	if (!linkmode_empty(ports_supported))
> +		linkmode_and(phydev->supported, phydev->supported,
> +			     ports_supported);
> +
> +	/* For now, the phy->port field is set as the first active port's type 
*/
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active)
> +			phydev->port = phy_port_get_type(port);
> +
> +	return 0;
> +
> +out:
> +	phy_cleanup_ports(phydev);
> +	return ret;
> +}
> +
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode
> @@ -3339,6 +3500,11 @@ static int phy_probe(struct device *dev)
>  		phydev->is_gigabit_capable = 1;
> 
>  	of_set_phy_supported(phydev);
> +
> +	err = phy_setup_ports(phydev);
> +	if (err)
> +		goto out;
> +
>  	phy_advertise_supported(phydev);
> 
>  	/* Get PHY default EEE advertising modes and handle them as 
potentially
> @@ -3414,6 +3580,8 @@ static int phy_remove(struct device *dev)
> 
>  	phydev->state = PHY_DOWN;
> 
> +	phy_cleanup_ports(phydev);
> +
>  	sfp_bus_del_upstream(phydev->sfp_bus);
>  	phydev->sfp_bus = NULL;
> 
> diff --git a/drivers/net/phy/phy_port.c b/drivers/net/phy/phy_port.c
> new file mode 100644
> index 000000000000..c69ba0d9afba
> --- /dev/null
> +++ b/drivers/net/phy/phy_port.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Framework to drive Ethernet ports
> + *
> + * Copyright (c) 2024 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/linkmode.h>
> +#include <linux/of.h>
> +#include <linux/phy_port.h>
> +
> +/**
> + * phy_port_alloc() - Allocate a new phy_port
> + *
> + * Returns: a newly allocated struct phy_port, or NULL.
> + */
> +struct phy_port *phy_port_alloc(void)
> +{
> +	struct phy_port *port;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return NULL;
> +
> +	linkmode_zero(port->supported);
> +	INIT_LIST_HEAD(&port->head);
> +
> +	return port;
> +}
> +EXPORT_SYMBOL_GPL(phy_port_alloc);
> +
> +/**
> + * phy_port_destroy() - Free a struct phy_port
> + * @port: The port to destroy
> + */
> +void phy_port_destroy(struct phy_port *port)
> +{
> +	kfree(port);
> +}
> +EXPORT_SYMBOL_GPL(phy_port_destroy);
> +
> +static void ethtool_medium_get_supported(unsigned long *supported,
> +					 enum ethtool_link_medium medium,
> +					 int lanes)
> +{
> +	int i;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
> +		/* Special bits such as Autoneg, Pause, Asym_pause, etc. are
> +		 * set and will be masked away by the port parent.
> +		 */
> +		if (link_mode_params[i].mediums == 
BIT(ETHTOOL_LINK_MEDIUM_NONE)) {
> +			linkmode_set_bit(i, supported);
> +			continue;

If you change the subsequent "if" into an "else if", you'll avoid having to 
use "continue" here, which IMO would make things a bit clearer.

> +		}
> +
> +		/* For most cases, min_lanes == lanes, except for 10/100BaseT 
that work
> +		 * on 2 lanes but are compatible with 4 lanes mediums
> +		 */
> +		if (link_mode_params[i].mediums & BIT(medium) &&
> +		    link_mode_params[i].lanes >= lanes &&
> +		    link_mode_params[i].min_lanes <= lanes) {
> +			linkmode_set_bit(i, supported);
> +		}
> +	}
> +}
> +
> +static enum ethtool_link_medium ethtool_str_to_medium(const char *str)
> +{
> +	int i;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MEDIUM_LAST; i++)
> +		if (!strcmp(phy_mediums(i), str))
> +			return i;
> +
> +	return ETHTOOL_LINK_MEDIUM_NONE;
> +}
> +
> +/**
> + * phy_of_parse_port() - Create a phy_port from a firmware representation
> + * @dn: device_node representation of the port, following the
> + *	ethernet-connector.yaml binding
> + *
> + * Returns: a newly allocated and initialized phy_port pointer, or an
> ERR_PTR. + */
> +struct phy_port *phy_of_parse_port(struct device_node *dn)
> +{
> +	struct fwnode_handle *fwnode = of_fwnode_handle(dn);
> +	enum ethtool_link_medium medium;
> +	struct phy_port *port;
> +	struct property *prop;
> +	const char *med_str;
> +	u32 lanes, mediums = 0;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "lanes", &lanes);
> +	if (ret)
> +		lanes = 0;

Checking if this property exists before calling fwnode_property_read_u32() 
would avoid masking potential error conditions where the property exists, but 
something goes wrong while reading it.

> +
> +	ret = fwnode_property_read_string(fwnode, "media", &med_str);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	of_property_for_each_string(to_of_node(fwnode), "media", prop, 
med_str) {
> +		medium = ethtool_str_to_medium(med_str);
> +		if (medium == ETHTOOL_LINK_MEDIUM_NONE)
> +			return ERR_PTR(-EINVAL);
> +
> +		mediums |= BIT(medium);
> +	}
> +
> +	if (!mediums)
> +		return ERR_PTR(-EINVAL);
> +
> +	port = phy_port_alloc();
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	port->lanes = lanes;
> +	port->mediums = mediums;
> +
> +	return port;
> +}
> +EXPORT_SYMBOL_GPL(phy_of_parse_port);
> +
> +/**
> + * phy_port_update_supported() - Setup the port->supported field
> + * @port: the port to update
> + *
> + * Once the port's medium list and number of lanes has been configured
> based + * on firmware, straps and vendor-specific properties, this function
> may be + * called to update the port's supported linkmodes list.
> + *
> + * Any mode that was manually set in the port's supported list remains set.
> + */
> +void phy_port_update_supported(struct phy_port *port)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +	int i, lanes = 1;
> +
> +	/* If there's no lanes specified, we grab the default number of
> +	 * lanes as the max of the default lanes for each medium
> +	 */
> +	if (!port->lanes)
> +		for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
> +			lanes = max_t(int, lanes, phy_medium_default_lanes(i));
> +
> +	for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST) {
> +		linkmode_zero(supported);

ethtool_medium_get_supported() can only set bits in "supported", so you could 
just do:

```
for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
	ethtool_medium_get_supported(port->supported, i, port->lanes);
```

unless you're wary of someone modifying ethtool_medium_get_supported() in a 
way that breaks this in the future?

> +		ethtool_medium_get_supported(supported, i, port->lanes);
> +		linkmode_or(port->supported, port->supported, supported);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phy_port_update_supported);
> +
> +/**
> + * phy_port_get_type() - get the PORT_* attribut for that port.
> + * @port: The port we want the information from
> + *
> + * Returns: A PORT_XXX value.
> + */
> +int phy_port_get_type(struct phy_port *port)
> +{
> +	if (port->mediums & ETHTOOL_LINK_MEDIUM_BASET)
> +		return PORT_TP;
> +
> +	if (phy_port_is_fiber(port))
> +		return PORT_FIBRE;
> +
> +	return PORT_OTHER;
> +}
> +EXPORT_SYMBOL_GPL(phy_port_get_type);
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c1d805d3e02f..0d3063af5905 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -226,6 +226,10 @@ extern const struct link_mode_info link_mode_params[];
> 
>  extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
> 
> +#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
> +				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
> +				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
> +
>  static inline const char *phy_mediums(enum ethtool_link_medium medium)
>  {
>  	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
> @@ -234,6 +238,17 @@ static inline const char *phy_mediums(enum
> ethtool_link_medium medium) return ethtool_link_medium_names[medium];
>  }
> 
> +static inline int phy_medium_default_lanes(enum ethtool_link_medium medium)
> +{
> +	/* Let's consider that the default BaseT ethernet is BaseT4, i.e.
> +	 * Gigabit Ethernet.
> +	 */
> +	if (medium == ETHTOOL_LINK_MEDIUM_BASET)
> +		return 4;
> +
> +	return 1;
> +}
> +
>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index d62d292024bc..0180f4d4fd7d 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -299,6 +299,7 @@ static inline long rgmii_clock(int speed)
>  struct device;
>  struct kernel_hwtstamp_config;
>  struct phylink;
> +struct phy_port;
>  struct sfp_bus;
>  struct sfp_upstream_ops;
>  struct sk_buff;
> @@ -590,6 +591,9 @@ struct macsec_ops;
>   * @master_slave_state: Current master/slave configuration
>   * @mii_ts: Pointer to time stamper callbacks
>   * @psec: Pointer to Power Sourcing Equipment control struct
> + * @ports: List of PHY ports structures
> + * @n_ports: Number of ports currently attached to the PHY
> + * @max_n_ports: Max number of ports this PHY can expose
>   * @lock:  Mutex for serialization access to PHY
>   * @state_queue: Work queue for state machine
>   * @link_down_events: Number of times link was lost
> @@ -724,6 +728,10 @@ struct phy_device {
>  	struct mii_timestamper *mii_ts;
>  	struct pse_control *psec;
> 
> +	struct list_head ports;
> +	int n_ports;
> +	int max_n_ports;
> +
>  	u8 mdix;
>  	u8 mdix_ctrl;
> 
> @@ -1242,6 +1250,27 @@ struct phy_driver {
>  	 * Returns the time in jiffies until the next update event.
>  	 */
>  	unsigned int (*get_next_update_time)(struct phy_device *dev);
> +
> +	/**
> +	 * @attach_port: Indicates to the PHY driver that a port is detected
> +	 * @dev: PHY device to notify
> +	 * @port: The port being added
> +	 *
> +	 * Called when a port that needs to be driven by the PHY is found. The
> +	 * number of time this will be called depends on phydev->max_n_ports,
> +	 * which the driver can change in .probe().
> +	 *
> +	 * The port that is being passed may or may not be initialized. If it 
is
> +	 * already initialized, it is by the generic port representation from
> +	 * devicetree, which superseeds any strapping or vendor-specific
> +	 * properties.
> +	 *
> +	 * If the port isn't initialized, the port->mediums and port->lanes
> +	 * fields must be set, possibly according to stapping information.
> +	 *
> +	 * Returns 0, or an error code.
> +	 */
> +	int (*attach_port)(struct phy_device *dev, struct phy_port *port);
>  };
>  #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		
\
>  				      struct phy_driver, mdiodrv)
> @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
>  void phy_mac_interrupt(struct phy_device *phydev);
>  void phy_start_machine(struct phy_device *phydev);
>  void phy_stop_machine(struct phy_device *phydev);
> +
>  void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  			       struct ethtool_link_ksettings *cmd);
>  int phy_ethtool_ksettings_set(struct phy_device *phydev,
> diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
> new file mode 100644
> index 000000000000..70aa75f93096
> --- /dev/null
> +++ b/include/linux/phy_port.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __PHY_PORT_H
> +#define __PHY_PORT_H
> +
> +#include <linux/ethtool.h>
> +#include <linux/types.h>
> +#include <linux/phy.h>
> +
> +struct phy_port;
> +
> +/**
> + * enum phy_port_parent - The device this port is attached to
> + *
> + * @PHY_PORT_PHY: Indicates that the port is driven by a PHY device
> + */
> +enum phy_port_parent {
> +	PHY_PORT_PHY,
> +};
> +
> +struct phy_port_ops {
> +	/* Sometimes, the link state can be retrieved from physical,
> +	 * out-of-band channels such as the LOS signal on SFP. These
> +	 * callbacks allows notifying the port about state changes
> +	 */
> +	void (*link_up)(struct phy_port *port);
> +	void (*link_down)(struct phy_port *port);
> +
> +	/* If the port acts as a Media Independent Interface (Serdes port),
> +	 * configures the port with the relevant state and mode. When enable is
> +	 * not set, interface should be ignored
> +	 */
> +	int (*configure_mii)(struct phy_port *port, bool enable, 
phy_interface_t
> interface); +};
> +
> +/**
> + * struct phy_port - A representation of a network device physical
> interface + *
> + * @head: Used by the port's parent to list ports
> + * @parent_type: The type of device this port is directly connected to
> + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> + * @ops: Callback ops implemented by the port controller
> + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> applicable + * @mediums: Bitmask of the physical mediums this port provides
> access to + * @supported: The link modes this port can expose, if this port
> is MDI (not MII) + * @interfaces: The MII interfaces this port supports, if
> this port is MII + * @active: Indicates if the port is currently part of
> the active link. + * @is_serdes: Indicates if this port is Serialised MII
> (Media Independent + *	       Interface), or an MDI (Media Dependent
> Interface).
> + */
> +struct phy_port {
> +	struct list_head head;
> +	enum phy_port_parent parent_type;
> +	union {
> +		struct phy_device *phy;
> +	};
> +
> +	const struct phy_port_ops *ops;
> +
> +	int lanes;
> +	unsigned long mediums;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> +
> +	unsigned int active:1;
> +	unsigned int is_serdes:1;
> +};
> +
> +struct phy_port *phy_port_alloc(void);
> +void phy_port_destroy(struct phy_port *port);
> +
> +static inline struct phy_device *port_phydev(struct phy_port *port)
> +{
> +	return port->phy;
> +}
> +
> +struct phy_port *phy_of_parse_port(struct device_node *dn);
> +
> +static inline bool phy_port_is_copper(struct phy_port *port)
> +{
> +	return port->mediums == BIT(ETHTOOL_LINK_MEDIUM_BASET);

BaseC is also "copper" right? Maybe this should be renamed to 
"phy_port_is_tp"?

> +}
> +
> +static inline bool phy_port_is_fiber(struct phy_port *port)
> +{
> +	return !!(port->mediums & ETHTOOL_MEDIUM_FIBER_BITS);
> +}
> +
> +void phy_port_update_supported(struct phy_port *port);
> +
> +int phy_port_get_type(struct phy_port *port);
> +
> +#endif

Thanks!
Kory Maincent May 13, 2025, 1:53 p.m. UTC | #2
On Wed,  7 May 2025 15:53:19 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Ethernet provides a wide variety of layer 1 protocols and standards for
> data transmission. The front-facing ports of an interface have their own
> complexity and configurability.
> 
> Introduce a representation of these front-facing ports. The current code
> is minimalistic and only support ports controlled by PHY devices, but
> the plan is to extend that to SFP as well as raw Ethernet MACs that
> don't use PHY devices.
> 
> This minimal port representation allows describing the media and number
> of lanes of a port. From that information, we can derive the linkmodes
> usable on the port, which can be used to limit the capabilities of an
> interface.
> 
> For now, the port lanes and medium is derived from devicetree, defined
> by the PHY driver, or populated with default values (as we assume that
> all PHYs expose at least one port).
> 
> The typical example is 100M ethernet. 100BaseT can work using only 2
> lanes on a Cat 5 cables. However, in the situation where a 10/100/1000
> capable PHY is wired to its RJ45 port through 2 lanes only, we have no
> way of detecting that. The "max-speed" DT property can be used, but a
> more accurate representation can be used :
> 
> mdi {
> 	connector-0 {
> 		media = "BaseT";
> 		lanes = <2>;
> 	};
> };
> 
> From that information, we can derive the max speed reachable on the
> port.
> 
> Another benefit of having that is to avoid vendor-specific DT properties
> (micrel,fiber-mode or ti,fiber-mode).
> 
> This basic representation is meant to be expanded, by the introduction
> of port ops, userspace listing of ports, and support for multi-port
> devices.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

...

> +	for_each_available_child_of_node_scoped(mdi, port_node) {
> +		port = phy_of_parse_port(port_node);
> +		if (IS_ERR(port)) {
> +			err = PTR_ERR(port);
> +			goto out_err;
> +		}
> +
> +		port->parent_type = PHY_PORT_PHY;
> +		port->phy = phydev;
> +		err = phy_add_port(phydev, port);
> +		if (err)
> +			goto out_err;

I think of_node_put(port_node) is missing here.

...

> @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
>  void phy_mac_interrupt(struct phy_device *phydev);
>  void phy_start_machine(struct phy_device *phydev);
>  void phy_stop_machine(struct phy_device *phydev);
> +

New empty line here?

> +/**
> + * struct phy_port - A representation of a network device physical interface
> + *
> + * @head: Used by the port's parent to list ports
> + * @parent_type: The type of device this port is directly connected to
> + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> + * @ops: Callback ops implemented by the port controller
> + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> applicable
> + * @mediums: Bitmask of the physical mediums this port provides access to
> + * @supported: The link modes this port can expose, if this port is MDI (not
> MII)
> + * @interfaces: The MII interfaces this port supports, if this port is MII
> + * @active: Indicates if the port is currently part of the active link.
> + * @is_serdes: Indicates if this port is Serialised MII (Media Independent
> + *	       Interface), or an MDI (Media Dependent Interface).
> + */
> +struct phy_port {
> +	struct list_head head;
> +	enum phy_port_parent parent_type;
> +	union {
> +		struct phy_device *phy;
> +	};

The union is useless here?

Regards,