diff mbox series

[RFC,v2,net-next,07/15] net: phylink: centralize phy_interface_mode_is_8023z() && phylink_autoneg_inband() checks

Message ID 20230923134904.3627402-8-vladimir.oltean@nxp.com
State New
Headers show
Series Add C72/C73 copper backplane support for LX2160 | expand

Commit Message

Vladimir Oltean Sept. 23, 2023, 1:48 p.m. UTC 
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(-)

Comments

Vladimir Oltean Oct. 3, 2023, 9:03 p.m. UTC | #1 
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 mbox series

Patch

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);