| Message ID | 20230923134904.3627402-8-vladimir.oltean@nxp.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Add C72/C73 copper backplane support for LX2160 | expand |
On Sat, Sep 23, 2023 at 04:48:56PM +0300, Vladimir Oltean wrote: > In a future change, we will extend the PHY interface modes for which > phylink allows the PCS to handle autoneg. Group the existing occurences nit: occurrences ...
On Sat, Sep 23, 2023 at 04:48:56PM +0300, Vladimir Oltean wrote: > In a future change, we will extend the PHY interface modes for which > phylink allows the PCS to handle autoneg. Group the existing occurences > into a common phylink_pcs_handles_an(). I don't see anything wrong with this change, despite my comments on the next patch. However, including INTERNAL in this may cause problems with DSA. I think maybe these two patches need to be tested on DSA setups that make use of INTERNAL.
On Tue, Oct 03, 2023 at 12:27:56PM +0100, Russell King (Oracle) wrote: > On Sat, Sep 23, 2023 at 04:48:56PM +0300, Vladimir Oltean wrote: > > In a future change, we will extend the PHY interface modes for which > > phylink allows the PCS to handle autoneg. Group the existing occurences > > into a common phylink_pcs_handles_an(). > > I don't see anything wrong with this change, despite my comments on the > next patch. However, including INTERNAL in this may cause problems with > DSA. I think maybe these two patches need to be tested on DSA setups > that make use of INTERNAL. Prompted by your observation in a different comment, I've searched for PHY_INTERFACE_MODE_10GKR and found aqr107_read_status(), which sets phydev->interface to this value based on the MDIO_MMD_PHYXS : MDIO_PHYXS_VEND_IF_STATUS field. I am now of the opinion that phy_interface_t is a phylib-only property, more than anything else, and a slightly obsolete/hard to extend concept, at that. The selling point of phylink_pcs is the possibility to have an optional phylink PHY, so I wouldn't want to reinterpret phy-mode = "internal" and managed = "in-band-status" to mean "clause 73 autoneg in the phylink_pcs", because that would quickly clash with the phylib PHY's desire to see the phy-mode set to something else (it is definitely not "internal"). I don't have access to an AQR107 with firmware provisioning for KR on the system interface, but presuming the feature isn't entirely bogus, I shouldn't deliberately close the door for it. I am exploring the possibility of adding support for 'managed = "c73"' as another autoneg mode in phylink, among the existing MLO_AN_FIXED, MLO_AN_PHY and MLO_AN_INBAND. It is quite clear that MLO_AN_INBAND != MLO_AN_C73, because the former is the PCS doing the negotiation and the latter is a dedicated block selecting a technology-specific PCS. So I think there is room for this extra autoneg mode. I am also trying to see if there's anything hardwired into phylink to require a phy-mode when a phylib PHY is absent. I will find that out in the following days, while working on the v3. I guess for optical SFPs, it just made sense to reuse the same data structures, to present MAC drivers the same kind of API as for SFP modules with PHYs. But with C73 autoneg, I don't think it makes as much sense. If phylink can be made to not require a phy-mode, I would prefer to pass PHY_INTERFACE_MODE_NA, and for it to be completely ignored for MLO_AN_C73, both in phylink and in phylink-using drivers.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 0d7354955d62..548130d77302 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1107,12 +1107,17 @@ static void phylink_mac_config(struct phylink *pl, pl->mac_ops->mac_config(pl->config, pl->cur_link_an_mode, &st); } +static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode) +{ + return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode); +} + static void phylink_pcs_an_restart(struct phylink *pl) { if (pl->pcs && linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, pl->link_config.advertising) && - phy_interface_mode_is_8023z(pl->link_config.interface) && - phylink_autoneg_inband(pl->cur_link_an_mode)) + phylink_pcs_handles_an(pl->link_config.interface, + pl->cur_link_an_mode)) pl->pcs->ops->pcs_an_restart(pl->pcs); } @@ -1716,8 +1721,8 @@ EXPORT_SYMBOL_GPL(phylink_destroy); bool phylink_expects_phy(struct phylink *pl) { if (pl->cfg_link_an_mode == MLO_AN_FIXED || - (pl->cfg_link_an_mode == MLO_AN_INBAND && - phy_interface_mode_is_8023z(pl->link_config.interface))) + phylink_pcs_handles_an(pl->link_config.interface, + pl->cfg_link_an_mode)) return false; return true; } @@ -1852,8 +1857,8 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, phy_interface_t interface) { if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED || - (pl->cfg_link_an_mode == MLO_AN_INBAND && - phy_interface_mode_is_8023z(interface) && !pl->sfp_bus))) + (phylink_pcs_handles_an(interface, pl->cfg_link_an_mode) && + !pl->sfp_bus))) return -EINVAL; if (pl->phydev) @@ -1937,10 +1942,11 @@ int phylink_fwnode_phy_connect(struct phylink *pl, struct phy_device *phy_dev; int ret; - /* Fixed links and 802.3z are handled without needing a PHY */ + /* Fixed links and the modes where the PCS can handle autoneg with the + * far end do not need a PHY. + */ if (pl->cfg_link_an_mode == MLO_AN_FIXED || - (pl->cfg_link_an_mode == MLO_AN_INBAND && - phy_interface_mode_is_8023z(pl->link_interface))) + phylink_pcs_handles_an(pl->link_interface, pl->cfg_link_an_mode)) return 0; phy_fwnode = fwnode_get_phy_node(fwnode);
In a future change, we will extend the PHY interface modes for which phylink allows the PCS to handle autoneg. Group the existing occurences into a common phylink_pcs_handles_an(). Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: patch is new drivers/net/phy/phylink.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)