| Message ID | 20250213101606.1154014-6-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce an ethernet port representation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next-1 |
On Thu, Feb 13, 2025 at 11:15:53AM +0100, Maxime Chevallier wrote: > Some PHY devices may be used as media-converters to drive SFP ports (for > example, to allow using SFP when the SoC can only output RGMII). This is > already supported to some extend by allowing PHY drivers to registers > themselves as being SFP upstream. > > However, the logic to drive the SFP can actually be split to a per-port > control logic, allowing support for multi-port PHYs, or PHYs that can > either drive SFPs or Copper. > > To that extent, create a phy_port when registering an SFP bus onto a > PHY. This port is considered a "serdes" port, in that it can feed data > to anther entity on the link. The PHY driver needs to specify the > various PHY_INTERFACE_MODE_XXX that this port supports. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> With this change, using phy_port requires phylink to also be built in an appropriate manner. Currently, phylink depends on phylib. phy_port becomes part of phylib. This patch makes phylib depend on phylink, thereby creating a circular dependency when modular. I think a different approach is needed here.
Hello Russell, On Sat, 15 Feb 2025 18:57:01 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Feb 13, 2025 at 11:15:53AM +0100, Maxime Chevallier wrote: > > Some PHY devices may be used as media-converters to drive SFP ports (for > > example, to allow using SFP when the SoC can only output RGMII). This is > > already supported to some extend by allowing PHY drivers to registers > > themselves as being SFP upstream. > > > > However, the logic to drive the SFP can actually be split to a per-port > > control logic, allowing support for multi-port PHYs, or PHYs that can > > either drive SFPs or Copper. > > > > To that extent, create a phy_port when registering an SFP bus onto a > > PHY. This port is considered a "serdes" port, in that it can feed data > > to anther entity on the link. The PHY driver needs to specify the > > various PHY_INTERFACE_MODE_XXX that this port supports. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > With this change, using phy_port requires phylink to also be built in > an appropriate manner. Currently, phylink depends on phylib. phy_port > becomes part of phylib. This patch makes phylib depend on phylink, > thereby creating a circular dependency when modular. > > I think a different approach is needed here. That's true. One way to avoid that would be to extract out of phylink/phylib all the functions for linkmode handling that aren't tied to phylink/phylib directly, but are about managing the capabilities of each interface, linkmode, speed, duplex, etc. For phylink, that would be : phylink_merge_link_mode phylink_get_capabilities phylink_cap_from_speed_duplex phylink_limit_mac_speed phylink_caps_to_linkmodes phylink_interface_max_speed phylink_interface_signal_rate phylink_is_empty_linkmode phylink_an_mode_str phylink_set_port_modes For now all these are phylink internal and that makes sense, but if we want phy-driven SFP support, stackable PHYs and so on, we'll need some ways for the PHY to expose its media-side capabilities, and we'd reuse these. These would go into linkmode.c/h for example, and we'd have a shared set of helpers that we can use in phylink, phylib and phy_port. Before I go around and rearrange that, are you OK with this approach ? Maxime
> One way to avoid that would be to extract out of phylink/phylib all the > functions for linkmode handling that aren't tied to phylink/phylib > directly, but are about managing the capabilities of each interface, > linkmode, speed, duplex, etc. For phylink, that would be : > > phylink_merge_link_mode > phylink_get_capabilities > phylink_cap_from_speed_duplex > phylink_limit_mac_speed > phylink_caps_to_linkmodes > phylink_interface_max_speed > phylink_interface_signal_rate > phylink_is_empty_linkmode > phylink_an_mode_str > phylink_set_port_modes ... > These would go into linkmode.c/h for example, and we'd have a shared set > of helpers that we can use in phylink, phylib and phy_port. Please be careful with the scope of these. Heiner is going through phylib and trying to reduce the scope of some of the functions we exporting in include/linux/phy.h to just being available in drivers/net/phy. That will help stop MAC drivers abuse them. We should do the same here, limit what can actually use these helpers to stop abuse. Andrew
On Mon, Feb 17, 2025 at 09:29:11AM +0100, Maxime Chevallier wrote: > Hello Russell, > > On Sat, 15 Feb 2025 18:57:01 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Thu, Feb 13, 2025 at 11:15:53AM +0100, Maxime Chevallier wrote: > > > Some PHY devices may be used as media-converters to drive SFP ports (for > > > example, to allow using SFP when the SoC can only output RGMII). This is > > > already supported to some extend by allowing PHY drivers to registers > > > themselves as being SFP upstream. > > > > > > However, the logic to drive the SFP can actually be split to a per-port > > > control logic, allowing support for multi-port PHYs, or PHYs that can > > > either drive SFPs or Copper. > > > > > > To that extent, create a phy_port when registering an SFP bus onto a > > > PHY. This port is considered a "serdes" port, in that it can feed data > > > to anther entity on the link. The PHY driver needs to specify the > > > various PHY_INTERFACE_MODE_XXX that this port supports. > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > With this change, using phy_port requires phylink to also be built in > > an appropriate manner. Currently, phylink depends on phylib. phy_port > > becomes part of phylib. This patch makes phylib depend on phylink, > > thereby creating a circular dependency when modular. > > > > I think a different approach is needed here. > > That's true. > > One way to avoid that would be to extract out of phylink/phylib all the > functions for linkmode handling that aren't tied to phylink/phylib > directly, but are about managing the capabilities of each interface, > linkmode, speed, duplex, etc. For phylink, that would be : > > phylink_merge_link_mode > phylink_get_capabilities > phylink_cap_from_speed_duplex > phylink_limit_mac_speed > phylink_caps_to_linkmodes > phylink_interface_max_speed > phylink_interface_signal_rate > phylink_is_empty_linkmode > phylink_an_mode_str > phylink_set_port_modes > > For now all these are phylink internal and that makes sense, but if we want > phy-driven SFP support, stackable PHYs and so on, we'll need some ways for > the PHY to expose its media-side capabilities, and we'd reuse these. > > These would go into linkmode.c/h for example, and we'd have a shared set > of helpers that we can use in phylink, phylib and phy_port. > > Before I go around and rearrange that, are you OK with this approach ? I'm not convinced. If you're thinking of that level of re-use, you're probably going to miss out on a lot of logic that's in phylink. Maybe there should be a way to re-use phylink in its entirety between the PHY and SFP. Some of the above (that deal only with linkmodes) would make sense to move out though.
Hi Andrew, On Mon, 17 Feb 2025 14:43:00 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > One way to avoid that would be to extract out of phylink/phylib all the > > functions for linkmode handling that aren't tied to phylink/phylib > > directly, but are about managing the capabilities of each interface, > > linkmode, speed, duplex, etc. For phylink, that would be : > > > > phylink_merge_link_mode > > phylink_get_capabilities > > phylink_cap_from_speed_duplex > > phylink_limit_mac_speed > > phylink_caps_to_linkmodes > > phylink_interface_max_speed > > phylink_interface_signal_rate > > phylink_is_empty_linkmode > > phylink_an_mode_str > > phylink_set_port_modes > > ... > > > These would go into linkmode.c/h for example, and we'd have a shared set > > of helpers that we can use in phylink, phylib and phy_port. > > Please be careful with the scope of these. Heiner is going through > phylib and trying to reduce the scope of some of the functions we > exporting in include/linux/phy.h to just being available in > drivers/net/phy. That will help stop MAC drivers abuse them. We should > do the same here, limit what can actually use these helpers to stop > abuse. Can we consider having an header file sitting in drivers/net/phy directly for this kind of things ? Maxime
On Mon, 17 Feb 2025 14:21:29 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Mon, Feb 17, 2025 at 09:29:11AM +0100, Maxime Chevallier wrote: > > Hello Russell, > > > > On Sat, 15 Feb 2025 18:57:01 +0000 > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > > > On Thu, Feb 13, 2025 at 11:15:53AM +0100, Maxime Chevallier wrote: > > > > Some PHY devices may be used as media-converters to drive SFP ports (for > > > > example, to allow using SFP when the SoC can only output RGMII). This is > > > > already supported to some extend by allowing PHY drivers to registers > > > > themselves as being SFP upstream. > > > > > > > > However, the logic to drive the SFP can actually be split to a per-port > > > > control logic, allowing support for multi-port PHYs, or PHYs that can > > > > either drive SFPs or Copper. > > > > > > > > To that extent, create a phy_port when registering an SFP bus onto a > > > > PHY. This port is considered a "serdes" port, in that it can feed data > > > > to anther entity on the link. The PHY driver needs to specify the > > > > various PHY_INTERFACE_MODE_XXX that this port supports. > > > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > > > With this change, using phy_port requires phylink to also be built in > > > an appropriate manner. Currently, phylink depends on phylib. phy_port > > > becomes part of phylib. This patch makes phylib depend on phylink, > > > thereby creating a circular dependency when modular. > > > > > > I think a different approach is needed here. > > > > That's true. > > > > One way to avoid that would be to extract out of phylink/phylib all the > > functions for linkmode handling that aren't tied to phylink/phylib > > directly, but are about managing the capabilities of each interface, > > linkmode, speed, duplex, etc. For phylink, that would be : > > > > phylink_merge_link_mode > > phylink_get_capabilities > > phylink_cap_from_speed_duplex > > phylink_limit_mac_speed > > phylink_caps_to_linkmodes > > phylink_interface_max_speed > > phylink_interface_signal_rate > > phylink_is_empty_linkmode > > phylink_an_mode_str > > phylink_set_port_modes > > > > For now all these are phylink internal and that makes sense, but if we want > > phy-driven SFP support, stackable PHYs and so on, we'll need some ways for > > the PHY to expose its media-side capabilities, and we'd reuse these. > > > > These would go into linkmode.c/h for example, and we'd have a shared set > > of helpers that we can use in phylink, phylib and phy_port. > > > > Before I go around and rearrange that, are you OK with this approach ? > > I'm not convinced. If you're thinking of that level of re-use, you're > probably going to miss out on a lot of logic that's in phylink. Maybe > there should be a way to re-use phylink in its entirety between the > PHY and SFP. > > Some of the above (that deal only with linkmodes) would make sense > to move out though. Yeah I'm thinking about moving only stuff that is phylink-independent and only deals with linkmodes indeed. I'll spin a quick series to see what it looks like then :) Thanks, Maxime
> > Please be careful with the scope of these. Heiner is going through > > phylib and trying to reduce the scope of some of the functions we > > exporting in include/linux/phy.h to just being available in > > drivers/net/phy. That will help stop MAC drivers abuse them. We should > > do the same here, limit what can actually use these helpers to stop > > abuse. > > Can we consider having an header file sitting in drivers/net/phy > directly for this kind of things ? Yes, we then only need to worry about phy drivers, not MAC drivers. PHY driver writers tend to abuse things less. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index fc24fb001b92..fc0b3a344cee 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1460,6 +1460,23 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port) phydev->n_ports--; } +static int phy_setup_sfp_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; + + port->is_serdes = true; + + phy_add_port(phydev, port); + + return 0; +} + /** * phy_sfp_probe - probe for a SFP cage attached to this PHY device * @phydev: Pointer to phy_device @@ -1481,6 +1498,10 @@ int phy_sfp_probe(struct phy_device *phydev, ret = sfp_bus_add_upstream(bus, phydev, ops); sfp_bus_put(bus); } + + if (phydev->sfp_bus) + ret = phy_setup_sfp_port(phydev); + return ret; } EXPORT_SYMBOL(phy_sfp_probe); diff --git a/drivers/net/phy/phy_port.c b/drivers/net/phy/phy_port.c index 94e21523e2b7..6680fcaf4d6e 100644 --- a/drivers/net/phy/phy_port.c +++ b/drivers/net/phy/phy_port.c @@ -7,6 +7,7 @@ #include <linux/linkmode.h> #include <linux/of.h> #include <linux/phy_port.h> +#include <linux/phylink.h> /** * phy_port_alloc: Allocate a new phy_port @@ -146,6 +147,15 @@ void phy_port_update_supported(struct phy_port *port) ethtool_medium_get_supported(supported, i, port->lanes); linkmode_or(port->supported, port->supported, supported); } + + /* Serdes ports supported may through SFP may not have any medium set, + * as they will output PHY_INTERFACE_MODE_XXX modes. In that case, derive + * the supported list based on these interfaces + */ + if (port->is_serdes && linkmode_empty(supported)) + phylink_interfaces_to_linkmodes(port->supported, + port->interfaces); + } EXPORT_SYMBOL_GPL(phy_port_update_supported); diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 214b62fba991..a49edc012636 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -673,6 +673,38 @@ static void phylink_validate_mask_caps(unsigned long *supported, linkmode_and(state->advertising, state->advertising, mask); } +/** + * phylink_interfaces_to_linkmodes() - List all possible linkmodes based on a + * set of supported interfaces, assuming no + * rate matching. + * @linkmodes: the supported linkmodes + * @interfaces: Set of interfaces (PHY_INTERFACE_MODE_XXX) + * + * Compute the exhaustive list of modes that can conceivably be achieved from a + * set of MII interfaces. This is derived from the possible speeds and duplex + * achievable from these interfaces. This list is likely too exhaustive (there + * may not exist any device out there that can convert from an interface to a + * linkmode) and it needs further filtering based on real HW capabilities. + */ +void phylink_interfaces_to_linkmodes(unsigned long *linkmodes, + const unsigned long *interfaces) +{ + phy_interface_t interface; + unsigned long caps = 0; + + linkmode_zero(linkmodes); + + for_each_set_bit(interface, interfaces, PHY_INTERFACE_MODE_MAX) + caps = phylink_get_capabilities(interface, + GENMASK(__fls(MAC_400000FD), + __fls(MAC_10HD)), + RATE_MATCH_NONE); + + phylink_set(linkmodes, Autoneg); + phylink_caps_to_linkmodes(linkmodes, caps); +} +EXPORT_SYMBOL_GPL(phylink_interfaces_to_linkmodes); + static int phylink_validate_mac_and_pcs(struct phylink *pl, unsigned long *supported, struct phylink_link_state *state) diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 898b00451bbf..ffd8dd32ff3d 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -176,6 +176,8 @@ struct phylink_config { }; void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed); +void phylink_interfaces_to_linkmodes(unsigned long *linkmodes, + const unsigned long *interfaces); /** * struct phylink_mac_ops - MAC operations structure.
Some PHY devices may be used as media-converters to drive SFP ports (for example, to allow using SFP when the SoC can only output RGMII). This is already supported to some extend by allowing PHY drivers to registers themselves as being SFP upstream. However, the logic to drive the SFP can actually be split to a per-port control logic, allowing support for multi-port PHYs, or PHYs that can either drive SFPs or Copper. To that extent, create a phy_port when registering an SFP bus onto a PHY. This port is considered a "serdes" port, in that it can feed data to anther entity on the link. The PHY driver needs to specify the various PHY_INTERFACE_MODE_XXX that this port supports. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- V4: Fixed phylink_interfaces_to_linkmodes() to use |= instead of = drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++ drivers/net/phy/phy_port.c | 10 ++++++++++ drivers/net/phy/phylink.c | 32 ++++++++++++++++++++++++++++++++ include/linux/phylink.h | 2 ++ 4 files changed, 65 insertions(+)