Message-Id: <20221118000124.2754581-5-vladimir.oltean@nxp.com>
Date:   Fri, 18 Nov 2022 02:01:20 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Sean Anderson <sean.anderson@...o.com>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: [PATCH v4 net-next 4/8] net: phylink: add option to sync in-band autoneg setting between PCS and PHY

In the case of an on-board PHY (not on SFP module), phylink parses the
'managed = "in-band-status"' property (which may or may not be present
in addition to the 'phy-handle' property), and based on this, selects
pl->cfg_link_an_mode to be MLO_AN_PHY or MLO_AN_INBAND.

Drivers which make use of phylink can use MLO_AN_PHY as an indication to
disable in-band autoneg when connected to an on-board PHY, and
MLO_AN_INBAND to enable it. That's what most of the drivers seem to do,
except macb_mac_config() which seems to always force in-band autoneg
except for MLO_AN_FIXED.

If one assumes purely Clause 37 compatible state machines in the
PHY-side PCS and in the MAC-side PCS, then in-band autoneg needs to be
enabled in both places, or disabled in both places, to establish a
successful system-side link. The exception seems to be mvneta, which has
a hardware-based fallback on no-inband when inband autoneg was enabled
but failed. Nonetheless, this is not a generally available feature.

While in the case of an SFP module, in-band autoneg is genuinely useful
in passing the link information through the Ethernet channel when we
lack an I2C/MDIO side channel, in the case of on-board PHYs it is
perhaps less so. Nonetheless, settings must be in sync for a functional
link.

There is currently a lack of information within the kernel as to whether
in-band autoneg should be used between a certain MAC and PHY. We rely on
the fwnode specification for this.

Most of the platforms are seemingly okay with the status quo, but there
are 2 real life scenarios where it has limitations:

- A driver recently converted from phylib to phylink. The device trees
  in circulation will not have the 'managed' property (since it's
  phylink specific), but some PHYs will require in-band autoneg enabled
  in the MAC-side PCS to work with them.

- The PHY in-band autoneg setting is not really fixed but configurable,
  and therefore, the property should not really belong to device tree.
  Matters are made worse when PHY drivers in bootloaders (U-Boot) also
  enable/disable this setting, and this can cause what is specified in
  the device tree to be out of sync with reality. Linux PHY drivers do
  not currently configure in-band autoneg according to the 'managed'
  property of the attached MAC.

This change introduces a new opt-in feature called sync_an_inband which
may override the in-band autoneg setting passed to phylink callbacks,
but not to 'true' as ovr_an_inband does, but rather to what the PHY
reports as supported (with a fallback to what was in the device tree, if
the PHY driver reports nothing).

It's quite possible that one more call to phylink_sync_inband_aneg() is
needed when the PHY changes its pl->phy_state.interface and this results
in a change to pl->link_config.interface. This is the
phylink_major_config() code path, and if the PHY has different in-band
autoneg requirements for the new SERDES protocol, we are currently not
informed about those. Unfortunately I don't have access to any board
which supports SERDES protocol change of an on-board PHY, and I don't
know without testing where that extra sync call should be put, so I
haven't put it anywhere.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
v3->v4:
- split the on-board PHY part out of previous patch 2/6 which combines them
- phylink_fixup_inband_aneg() renamed to phylink_sync_an_inband()
- opt-in via phylink_config :: sync_an_inband
- look at pl->link_config.interface rather than pl->link_interface
- clearer comments, add kerneldocs

 drivers/net/phy/phylink.c | 49 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  7 ++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index bf2a5ebfc4f4..598f5feb661e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -156,6 +156,45 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
+/**
+ * phylink_sync_an_inband() - Sync in-band autoneg between PCS and PHY
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @phy: a pointer to a &struct phy_device
+ *
+ * Query the in-band autoneg capability of an on-board PHY in an attempt to
+ * sync the PCS-side link autoneg mode with the PHY autoneg mode. Set the
+ * current link autoneg mode to the mode configured through the fwnode if the
+ * PHY supports it or if its capabilities are unknown, or to an alternative
+ * mode that the PHY can operate in.
+ */
+static void phylink_sync_an_inband(struct phylink *pl, struct phy_device *phy)
+{
+	unsigned int mode = pl->cfg_link_an_mode;
+	int ret;
+
+	if (!pl->config->sync_an_inband)
+		return;
+
+	ret = phy_validate_an_inband(phy, pl->link_config.interface);
+	if (ret == PHY_AN_INBAND_UNKNOWN) {
+		phylink_dbg(pl,
+			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
+			    phylink_autoneg_inband(mode) ? "true" : "false");
+	} else if (phylink_autoneg_inband(mode) && !(ret & PHY_AN_INBAND_ON)) {
+		phylink_err(pl,
+			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
+
+		mode = MLO_AN_PHY;
+	} else if (!phylink_autoneg_inband(mode) && !(ret & PHY_AN_INBAND_OFF)) {
+		phylink_dbg(pl,
+			    "PHY driver requests in-band autoneg, force-enabling it.\n");
+
+		mode = MLO_AN_INBAND;
+	}
+
+	pl->cur_link_an_mode = mode;
+}
+
 /**
  * phylink_interface_max_speed() - get the maximum speed of a phy interface
  * @interface: phy interface mode defined by &typedef phy_interface_t
@@ -1475,6 +1514,12 @@ struct phylink *phylink_create(struct phylink_config *config,
 	struct phylink *pl;
 	int ret;
 
+	if (config->ovr_an_inband && config->sync_an_inband) {
+		dev_err(config->dev,
+			"phylink: error: ovr_an_inband and sync_an_inband cannot be used simultaneously\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (mac_ops->mac_select_pcs &&
 	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
 	      ERR_PTR(-EOPNOTSUPP))
@@ -1725,6 +1770,8 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	phylink_sync_an_inband(pl, phy);
+
 	ret = phylink_attach_phy(pl, phy, pl->link_interface);
 	if (ret < 0)
 		return ret;
@@ -1800,6 +1847,8 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	phylink_sync_an_inband(pl, phy_dev);
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..d4b931bdfdfe 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -124,6 +124,12 @@ enum phylink_op_type {
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
+ * @sync_an_inband: if true, select between %MLO_AN_INBAND and %MLO_AN_PHY
+ *		    according to the capability of the attached on-board PHY
+ *		    (if both modes are supported, the mode deduced from the
+ *		    fwnode specification is used). With PHYs on SFP modules,
+ *		    the automatic selection takes place regardless of this
+ *		    setting. Mutually exclusive with &ovr_an_inband.
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
@@ -137,6 +143,7 @@ struct phylink_config {
 	bool poll_fixed_state;
 	bool mac_managed_pm;
 	bool ovr_an_inband;
+	bool sync_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
-- 
2.34.1

Powered by blists - more mailing lists