Message-ID: <ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk>
Date: Mon, 15 May 2023 13:22:50 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Daniel Machon <daniel.machon@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Felix Fietkau <nbd@....name>,
	Florian Fainelli <f.fainelli@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Jakub Kicinski <kuba@...nel.org>, John Crispin <john@...ozen.org>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	Lars Povlsen <lars.povlsen@...rochip.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Marcin Wojtas <mw@...ihalf.com>,
	Mark Lee <Mark-MC.Lee@...iatek.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Paolo Abeni <pabeni@...hat.com>, Sean Wang <sean.wang@...iatek.com>,
	Steen Hegelund <Steen.Hegelund@...rochip.com>,
	Taras Chornyi <taras.chornyi@...ision.eu>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	UNGLinuxDriver@...rochip.com, Vladimir Oltean <olteanv@...il.com>
Cc: linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org
Subject: [PATCH RFC] Providing a helper for PCS inband negotiation

All,

One issue that seems to come up with PCS drivers is when should inband
negotiation be enabled. Some SFP modules have specific requirements when
it comes to whether inband AN should be enabled.

Having each network driver implement their own decision making is rather
sub-optimal as this doesn't lead to maximum inter-operability and a
consistent experience with SFP modules when used with different network
drivers.

This has been bugging me for a while, and I've had in my net-queue some
patches that introduced a simple helper that took the code in
phylink_mii_c22_pcs_config() and pulled that into an inline function:

        if (mode == MLO_AN_INBAND &&
            (interface == PHY_INTERFACE_MODE_SGMII ||
             interface == PHY_INTERFACE_MODE_QSGMII ||
             linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)))

but I haven't been happy with this approach. I did this when working on
the mv88e6xxx conversion to phylink-PCS, because I didn't want to
duplicate that decision making in that driver.

Having been through all PCS drivers thus far, the majority of them are
concerned with "should some form of in-band signalling be enabled or
not?" and a simple boolean would suffice for that.

However, there are two (sparx5 and lan966x) which appear to want
slightly more information than that. These have a port configuration
structure that wants to know if we are using inband mode (in other
words, phylink is in MLO_AN_INBAND mode) and whether autoneg is
enabled (the Autoneg bit in the advertising bitmap.) See
lan966x_pcs_config().

This is further decoded in lan966x_port_pcs_set() such that if we're in
MLO_AN_INBAND mode, and:
 if we're using SGMII, or
 if the number of ports is four (QSGMII, QUSGMII), or
 if we're in 1000base-X mode and Autoneg is set,
then inband mode is enabled. However, if we aren't in MLO_AN_INBAND
mode, then an "outband" mode is used (driver's terminology, but the
comments about it seem to talk about "inband" here.)

It looks like Sparx5 is pretty similar to lan966x.

So, it seems that a boolean won't do, and we at least need a tristate
outband/inband-an-disabled/inband-an-enabled indication.

However, we also have to remember that there are protocols such as
10GBASE-R which do not have any inband capability. This suggests we
actually need a quad-state - none/outband/inband-an-disabled/
inband-an-enabled. That said, we have hardly any PCS that this would
end up returning "NONE" - but there are some (e.g. mvneta and mvpp2
which always provide a PCS, even for e.g. RGMII.)

So, to cover this, I'm proposing a series of steps:

1. introduce a new helper function - phylink_pcs_neg_mode() - as per
   the patch below
2. switch phylink_mii_c22_pcs_config() to use it
3. pull that helper into phylink_mii_c22_pcs_config() callers
4. convert all pcs_config() implementations that fiddle with inband AN
   configuration to use this helper
5. finally pull the helper up into phylink and change the pcs_config()
   method prototype:

-       int (*pcs_config)(struct phylink_pcs *pcs, unsigned int mode,
-                         phy_interface_t interface,
+       int (*pcs_config)(struct phylink_pcs *pcs, phy_interface_t interface,
                          const unsigned long *advertising,
+                         unsigned int neg_mode,
                          bool permit_pause_to_mac);

There are two questions that came up while creating the patches for the
above five steps:

1. Should 10GBASE-KR be included in the SGMII et.al. case in the code?
   Any other interface modes that should be there? Obviously,
   PHYLINK_PCS_NEG_NONE is not correct for 10GBASE-KR since it does use
   inband AN. Does it make sense for the user to disable inband AN for
   10GBASE-KR? If so, maybe it should be under the 1000base-X case.

2. XLGMII.. Looking at the XPCS driver, it's unclear whether Clause 73
   AN gets used for this. A quick scan of IEEE 802.3 suggests that
   XLGMII doesn't have any support for any inband signalling, and it's
   just an intermediary protocol between the MAC (more specifically the
   RS, but for the purposes of this I'll just refer to MAC) and the
   attached PCS, and any autonegotiation happens after the XLGMII link.

   In terms of PCS, until PHY_INTERFACE_MODE_XLGMII was introduced, the
   PHY interface mode for serdes based protocols described the protocol
   used on the media side of the PCS and the next part of the system
   towards the media, but XLGMII describes the MAC-to-PCS protocol
   (which could equally be GMII, XGMII etc). For example, in 802.3, the
   stack for 1000BASE-X describes the MAC-to-PCS link as GMII, but we
   have omitted that detail, and we just call it
   PHY_INTERFACE_MODE_1000BASEX.

   PHY_INTERFACE_MODE_XLGMII has as much meaning whether the PCS uses
   media-facing inband AN as PHY_INTERFACE_MODE_GMII would do if it
   were to be used with a PCS operating in SGMII or 1000BASE-X mode.

   If XLGMII is used with a PCS that supports clause 73 AN, then there
   is obviously AN involved. However, XLGMII can also be used with a
   PCS/PMA/PMD that ends up producing 40GBASE-R which has no inband AN.

   So, I'm not sure what the behaviour of phylink_pcs_neg_mode() should
   be when faced with PHY_INTERFACE_MODE_XLGMII - does this need yet
   another state?

3. Should the pcs_link_up() method also be passed this "neg_mode" state
   and should the existing "mode" argument be dropped?

Finally, there's the obvious question whether this approach is even a
good idea.

Please let me have some thoughts.

8<===
From: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Subject: [PATCH] net: phylink: add phylink_pcs_neg_mode()

Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
---
 include/linux/phylink.h | 72 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 8968094503d6..465248818276 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -21,6 +21,18 @@ enum {
 	MLO_AN_FIXED,	/* Fixed-link mode */
 	MLO_AN_INBAND,	/* In-band protocol */
 
+	/* PCS "negotiation" mode.
+	 *  PHYLINK_PCS_NEG_NONE - protocol has no inband capability
+	 *  PHYLINK_PCS_NEG_OUTBAND - some out of band or fixed link setting
+	 *  PHYLINK_PCS_NEG_INBAND_DISABLED - inband mode disabled, e.g.
+	 *				      1000base-X with autoneg off
+	 *  PHYLINK_PCS_NEG_INBAND_ENABLED - inband mode enabled
+	 */
+	PHYLINK_PCS_NEG_NONE = 0,
+	PHYLINK_PCS_NEG_OUTBAND,
+	PHYLINK_PCS_NEG_INBAND_DISABLED,
+	PHYLINK_PCS_NEG_INBAND_ENABLED,
+
 	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our
 	 * autonegotiation advertisement. They correspond to the PAUSE and
 	 * ASM_DIR bits defined by 802.3, respectively.
@@ -79,6 +91,67 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
 	return mode == MLO_AN_INBAND;
 }
 
+/**
+ * phylink_pcs_neg_mode() - helper to determine PCS inband mode
+ * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
+ * @interface: interface mode to be used
+ * @advertising: adertisement ethtool link mode mask
+ *
+ * Determines the negotiation mode to be used by the PCS, and returns
+ * one of:
+ * %PHYLINK_PCS_NEG_NONE: interface mode does not support inband
+ * %PHYLINK_PCS_NEG_OUTBAND: an out of band mode (e.g. reading the PHY,
+ *   or fixed link) will be used.
+ * %PHYLINK_PCS_NEG_INBAND_DISABLED: inband mode selected but autoneg disabled
+ * %PHYLINK_PCS_NEG_INBAND_ENABLED: inband mode selected and autoneg enabled
+ */
+static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
+						phy_interface_t interface,
+						const unsigned long *advertising)
+{
+	unsigned int neg_mode;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_QUSGMII:
+	case PHY_INTERFACE_MODE_USXGMII:
+		/* These protocols are designed for use with a PHY which
+		 * communicates its negotiation result back to the MAC via
+		 * inband communication. Note: there exist PHYs that run
+		 * with SGMII but do not send the inband data.
+		 */
+		if (!phylink_autoneg_inband(mode))
+			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
+		else
+			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
+		break;
+
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		/* 1000base-X is designed for use media-side for Fibre
+		 * connections, and thus the Autoneg bit needs to be
+		 * taken into account. We also do this for 2500base-X
+		 * as well, but drivers may not support this, so may
+		 * need to override this.
+		 */
+		if (!phylink_autoneg_inband(mode))
+			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
+		else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					   advertising))
+			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
+		else
+			neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED;
+		break;
+
+	default:
+		neg_mode = PHYLINK_PCS_NEG_NONE;
+		break;
+	}
+
+	return neg_mode;
+}
+
 /**
  * struct phylink_link_state - link state structure
  * @advertising: ethtool bitmask containing advertised link modes
-- 
2.30.2


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists