Message ID | 20250507135331.76021-1-maxime.chevallier@bootlin.com |
---|---|
Headers | show |
Series | Introduce an ethernet port representation | expand |
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!
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,