[PATCH v2 00/11] net: phy: Add support for rate adaptation

[PATCH v2 00/11] net: phy: Add support for rate adaptation

[Sean Anderson]

This adds support for phy rate adaptation: when a phy adapts between
differing phy interface and link speeds. It was originally submitted as
part of [1], which is considered "v1" of this series.

We need support for rate adaptation for two reasons. First, the phy
consumer needs to know if the phy will perform rate adaptation in order to
program the correct advertising. An unaware consumer will only program
support for link modes at the phy interface mode's native speed. This will
cause autonegotiation to fail if the link partner only advertises support
for lower speed link modes.

Second, to reduce packet loss it may be desirable to throttle packet
throughput. In past discussions [2-4], this behavior has been
controversial. It is the opinion of several developers that it is the
responsibility of the system integrator or end user to set the link
settings appropriately for rate adaptation. In particular, it was argued
that it is difficult to determine whether a particular phy has rate
adaptation enabled, and it is simpler to keep such determinations out of
the kernel. Another criticism is that packet loss may happen anyway, such
as if a faster link is used with a switch or repeater that does not support
pause frames.

I believe that our current approach is limiting, especially when
considering that rate adaptation (in two forms) has made it into IEEE
standards. In general, When we have appropriate information we should set
sensible defaults. To consider use a contrasting example, we enable pause
frames by default for switches which autonegotiate for them. When it's the
phy itself generating these frames, we don't even have to autonegotiate to
know that we should enable pause frames.

Our current approach also encourages workarounds, such as commit
73a21fa817f0 ("dpaa_eth: support all modes with rate adapting PHYs").
These workarounds are fine for phylib drivers, but phylink drivers cannot
use this approach (since there is no direct access to the phy). Note that
even when we determine (e.g.) the pause settings based on whether rate
adaptation is enabled, they can still be overridden by userspace (using
ethtool). It might be prudent to allow disabling of rate adaptation
generally in ethtool as well.

[1] https://lore.kernel.org/netdev/20220715215954.1449214-1-sean.anderson@seco.com/T/#t
[2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@oss.nxp.com/
[3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@oss.nxp.com/
[4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@gmail.com/

Changes in v2:
- Use int/defines instead of enum to allow for use in ioctls/netlink
- Add locking to phy_get_rate_adaptation
- Add (read-only) ethtool support for rate adaptation
- Move part of commit message to cover letter, as it gives a good
  overview of the whole series, and allows this patch to focus more on
  the specifics.
- Support keeping track of link duplex
- Rewrite commit message for clarity
- Expand documentation of (link_)?(speed|duplex)
- Fix handling of speed/duplex gotten from MAC drivers
- Use the phy's rate adaptation setting to determine whether to use its
  link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
- Always use the rate adaptation setting to determine the interface
  speed/duplex (instead of sometimes using the interface mode).
- Determine the interface speed and max mac speed directly instead of
  guessing based on the caps.
- Add comments clarifying the register defines
- Reorder variables in aqr107_read_rate

Sean Anderson (11):
  net: dpaa: Fix <1G ethernet on LS1046ARDB
  net: phy: Add 1000BASE-KX interface mode
  net: phylink: Export phylink_caps_to_linkmodes
  net: phylink: Generate caps and convert to linkmodes separately
  net: phy: Add support for rate adaptation
  net: phylink: Support differing link/interface speed/duplex
  net: phylink: Adjust link settings based on rate adaptation
  net: phylink: Adjust advertisement based on rate adaptation
  [RFC] net: phylink: Add support for CRS-based rate adaptation
  net: phy: aquantia: Add some additional phy interfaces
  net: phy: aquantia: Add support for rate adaptation

 Documentation/networking/ethtool-netlink.rst  |   2 +
 .../net/ethernet/freescale/dpaa/dpaa_eth.c    |   6 +-
 drivers/net/phy/aquantia_main.c               |  68 +++-
 drivers/net/phy/phy-core.c                    |  15 +
 drivers/net/phy/phy.c                         |  28 ++
 drivers/net/phy/phylink.c                     | 340 +++++++++++++++---
 include/linux/phy.h                           |  26 +-
 include/linux/phylink.h                       |  23 +-
 include/uapi/linux/ethtool.h                  |  18 +-
 include/uapi/linux/ethtool_netlink.h          |   1 +
 net/ethtool/ioctl.c                           |   1 +
 net/ethtool/linkmodes.c                       |   5 +
 12 files changed, 470 insertions(+), 63 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 01/11] net: dpaa: Fix <1G ethernet on LS1046ARDB

[Sean Anderson]

As discussed in commit 73a21fa817f0 ("dpaa_eth: support all modes with
rate adapting PHYs"), we must add a workaround for Aquantia phys with
in-tree support in order to keep 1G support working. Update this
workaround for the AQR113C phy found on revision C LS1046ARDB boards.

Fixes: 12cf1b89a668 ("net: phy: Add support for AQR113C EPHY")
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
In a previous version of this commit, I referred to an AQR115, however
on further inspection this appears to be an AQR113C. Confusingly, the
higher-numbered phys support lower data rates.

(no changes since v1)

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 45634579adb6..a770bab4d1ed 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2886,6 +2886,7 @@ static void dpaa_adjust_link(struct net_device *net_dev)
 
 /* The Aquantia PHYs are capable of performing rate adaptation */
 #define PHY_VEND_AQUANTIA	0x03a1b400
+#define PHY_VEND_AQUANTIA2	0x31c31c00
 
 static int dpaa_phy_init(struct net_device *net_dev)
 {
@@ -2893,6 +2894,7 @@ static int dpaa_phy_init(struct net_device *net_dev)
 	struct mac_device *mac_dev;
 	struct phy_device *phy_dev;
 	struct dpaa_priv *priv;
+	u32 phy_vendor;
 
 	priv = netdev_priv(net_dev);
 	mac_dev = priv->mac_dev;
@@ -2905,9 +2907,11 @@ static int dpaa_phy_init(struct net_device *net_dev)
 		return -ENODEV;
 	}
 
+	phy_vendor = phy_dev->drv->phy_id & GENMASK(31, 10);
 	/* Unless the PHY is capable of rate adaptation */
 	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
-	    ((phy_dev->drv->phy_id & GENMASK(31, 10)) != PHY_VEND_AQUANTIA)) {
+	    (phy_vendor != PHY_VEND_AQUANTIA &&
+	     phy_vendor != PHY_VEND_AQUANTIA2)) {
 		/* remove any features not supported by the controller */
 		ethtool_convert_legacy_u32_to_link_mode(mask,
 							mac_dev->if_support);
-- 
2.35.1.1320.gc452695387.dirty

RE: [PATCH v2 01/11] net: dpaa: Fix <1G ethernet on LS1046ARDB

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@seco.com>
> Sent: Wednesday, July 20, 2022 2:50
> To: netdev@vger.kernel.org; Andrew Lunn <andrew@lunn.ch>; Heiner
> Kallweit <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>
> Cc: Alexandru Marginean <alexandru.marginean@nxp.com>; Paolo Abeni
> <pabeni@redhat.com>; David S . Miller <davem@davemloft.net>; linux-
> kernel@vger.kernel.org; Vladimir Oltean <olteanv@gmail.com>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Sean Anderson <sean.anderson@seco.com>; Bhadram Varka
> <vbhadram@nvidia.com>; Madalin Bucur <madalin.bucur@nxp.com>
> Subject: [PATCH v2 01/11] net: dpaa: Fix <1G ethernet on LS1046ARDB
> 
> As discussed in commit 73a21fa817f0 ("dpaa_eth: support all modes with
> rate adapting PHYs"), we must add a workaround for Aquantia phys with
> in-tree support in order to keep 1G support working. Update this
> workaround for the AQR113C phy found on revision C LS1046ARDB boards.
> 
> Fixes: 12cf1b89a668 ("net: phy: Add support for AQR113C EPHY")
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> In a previous version of this commit, I referred to an AQR115, however
> on further inspection this appears to be an AQR113C. Confusingly, the
> higher-numbered phys support lower data rates.
> 
> (no changes since v1)

Acked-by: Camelia Groza <camelia.groza@nxp.com>

[PATCH v2 02/11] net: phy: Add 1000BASE-KX interface mode

[Sean Anderson]

Add 1000BASE-KX interface mode. This 1G backplane ethernet as described in
clause 70. Clause 73 autonegotiation is mandatory, and only full duplex
operation is supported.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 1 +
 include/linux/phy.h       | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9bd69328dc4d..b08716fe22c1 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -344,6 +344,7 @@ void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
 	case PHY_INTERFACE_MODE_1000BASEX:
 		caps |= MAC_1000HD;
 		fallthrough;
+	case PHY_INTERFACE_MODE_1000BASEKX:
 	case PHY_INTERFACE_MODE_TRGMII:
 		caps |= MAC_1000FD;
 		break;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 87638c55d844..81ce76c3e799 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -115,6 +115,7 @@ extern const int phy_10gbit_features_array[1];
  * @PHY_INTERFACE_MODE_25GBASER: 25G BaseR
  * @PHY_INTERFACE_MODE_USXGMII:  Universal Serial 10GE MII
  * @PHY_INTERFACE_MODE_10GKR: 10GBASE-KR - with Clause 73 AN
+ * @PHY_INTERFACE_MODE_1000BASEKX: 1000Base-KX - with Clause 73 AN
  * @PHY_INTERFACE_MODE_MAX: Book keeping
  *
  * Describes the interface between the MAC and PHY.
@@ -152,6 +153,7 @@ typedef enum {
 	PHY_INTERFACE_MODE_USXGMII,
 	/* 10GBASE-KR - with Clause 73 AN */
 	PHY_INTERFACE_MODE_10GKR,
+	PHY_INTERFACE_MODE_1000BASEKX,
 	PHY_INTERFACE_MODE_MAX,
 } phy_interface_t;
 
@@ -249,6 +251,8 @@ static inline const char *phy_modes(phy_interface_t interface)
 		return "trgmii";
 	case PHY_INTERFACE_MODE_1000BASEX:
 		return "1000base-x";
+	case PHY_INTERFACE_MODE_1000BASEKX:
+		return "1000base-kx";
 	case PHY_INTERFACE_MODE_2500BASEX:
 		return "2500base-x";
 	case PHY_INTERFACE_MODE_5GBASER:
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 03/11] net: phylink: Export phylink_caps_to_linkmodes

[Sean Anderson]

This function is convenient for MAC drivers. They can use it to add or
remove particular link modes based on capabilities (such as if half
duplex is not supported for a particular interface mode).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 12 ++++++++++--
 include/linux/phylink.h   |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index b08716fe22c1..51e66320526f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -155,8 +155,15 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
-static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
-				      unsigned long caps)
+/**
+ * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
+ * @linkmodes: ethtool linkmode mask (must be already initialised)
+ * @caps: bitmask of MAC capabilities
+ *
+ * Set all possible pause, speed and duplex linkmodes in @linkmodes that are
+ * supported by the @caps. @linkmodes must have been initialised previously.
+ */
+void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 {
 	if (caps & MAC_SYM_PAUSE)
 		__set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);
@@ -295,6 +302,7 @@ static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
 		__set_bit(ETHTOOL_LINK_MODE_400000baseCR4_Full_BIT, linkmodes);
 	}
 }
+EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
 /**
  * phylink_get_linkmodes() - get acceptable link modes
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..6463bd64eaa4 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -518,6 +518,7 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		 phy_interface_t interface, int speed, int duplex);
 #endif
 
+void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
 void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
 			   unsigned long mac_capabilities);
 void phylink_generic_validate(struct phylink_config *config,
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 04/11] net: phylink: Generate caps and convert to linkmodes separately

[Sean Anderson]

If we call phylink_caps_to_linkmodes directly from
phylink_get_linkmodes, it is difficult to re-use this functionality in
MAC drivers. This is because MAC drivers must then work with an ethtool
linkmode bitmap, instead of with mac capabilities. Instead, let the
caller of phylink_get_linkmodes do the conversion. To reflect this
change, rename the function to phylink_get_capabilities.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 21 +++++++++++----------
 include/linux/phylink.h   |  4 ++--
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 51e66320526f..68a58ab6a8ed 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -305,17 +305,15 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
 /**
- * phylink_get_linkmodes() - get acceptable link modes
- * @linkmodes: ethtool linkmode mask (must be already initialised)
+ * phylink_get_capabilities() - get capabilities for a given MAC
  * @interface: phy interface mode defined by &typedef phy_interface_t
  * @mac_capabilities: bitmask of MAC capabilities
  *
- * Set all possible pause, speed and duplex linkmodes in @linkmodes that
- * are supported by the @interface mode and @mac_capabilities. @linkmodes
- * must have been initialised previously.
+ * Get the MAC capabilities that are supported by the @interface mode and
+ * @mac_capabilities.
  */
-void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
-			   unsigned long mac_capabilities)
+unsigned long phylink_get_capabilities(phy_interface_t interface,
+				       unsigned long mac_capabilities)
 {
 	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
 
@@ -390,9 +388,9 @@ void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
 		break;
 	}
 
-	phylink_caps_to_linkmodes(linkmodes, caps & mac_capabilities);
+	return caps & mac_capabilities;
 }
-EXPORT_SYMBOL_GPL(phylink_get_linkmodes);
+EXPORT_SYMBOL_GPL(phylink_get_capabilities);
 
 /**
  * phylink_generic_validate() - generic validate() callback implementation
@@ -408,11 +406,14 @@ void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state)
 {
+	unsigned long caps;
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
-	phylink_get_linkmodes(mask, state->interface, config->mac_capabilities);
+	caps = phylink_get_capabilities(state->interface,
+					config->mac_capabilities);
+	phylink_caps_to_linkmodes(mask, caps);
 
 	linkmode_and(supported, supported, mask);
 	linkmode_and(state->advertising, state->advertising, mask);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6463bd64eaa4..5008ec3dcade 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -519,8 +519,8 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 #endif
 
 void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
-void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
-			   unsigned long mac_capabilities);
+unsigned long phylink_get_capabilities(phy_interface_t interface,
+				       unsigned long mac_capabilities);
 void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 05/11] net: phy: Add support for rate adaptation

[Sean Anderson]

This adds support for rate adaptation to the phy subsystem. The general
idea is that the phy interface runs at one speed, and the MAC throttles
the rate at which it sends packets to the link speed. There's a good
overview of several techniques for achieving this at [1]. This patch
adds support for three: pause-frame based (such as in Aquantia phys),
CRS-based (such as in 10PASS-TS and 2BASE-TL), and open-loop-based (such
as in 10GBASE-W).

This patch makes a few assumptions and a few non assumptions about the
types of rate adaptation available. First, it assumes that different phys
may use different forms of rate adaptation. Second, it assumes that phys
can use rate adaptation for any of their supported link speeds (e.g. if a
phy supports 10BASE-T and XGMII, then it can adapt XGMII to 10BASE-T).
Third, it does not assume that all interface modes will use the same form
of rate adaptation. Fourth, it does not assume that all phy devices will
support rate adaptation (even some do). Relaxing or strengthening these
(non-)assumptions could result in a different API. For example, if all
interface modes were assumed to use the same form of rate adaptation, then
a bitmask of interface modes supportting rate adaptation would suffice.

For some better visibility into the process, the current rate adaptation
mode is exposed as part of the ethtool ksettings. For the moment, only
read access is supported. I'm not sure what userspace might want to
configure yet (disable it altogether, disable just one mode, specify the
mode to use, etc.). For the moment, since only pause-based rate
adaptation support is added in the next few commits, rate adaptation can
be disabled altogether by adjusting the advertisement.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Should the unimplemented adaptation modes be kept in?

Changes in v2:
- Use int/defines instead of enum to allow for use in ioctls/netlink
- Add locking to phy_get_rate_adaptation
- Add (read-only) ethtool support for rate adaptation
- Move part of commit message to cover letter, as it gives a good
  overview of the whole series, and allows this patch to focus more on
  the specifics.

 Documentation/networking/ethtool-netlink.rst |  2 ++
 drivers/net/phy/phy-core.c                   | 15 +++++++++++
 drivers/net/phy/phy.c                        | 28 ++++++++++++++++++++
 include/linux/phy.h                          | 22 ++++++++++++++-
 include/uapi/linux/ethtool.h                 | 18 +++++++++++--
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/ioctl.c                          |  1 +
 net/ethtool/linkmodes.c                      |  5 ++++
 8 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index dbca3e9ec782..65ed29e78499 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -426,6 +426,7 @@ Kernel response contents:
   ``ETHTOOL_A_LINKMODES_DUPLEX``              u8      duplex mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG``    u8      Master/slave port mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE``  u8      Master/slave port state
+  ``ETHTOOL_A_LINKMODES_RATE_ADAPTATION``     u8      PHY rate adaptation
   ==========================================  ======  ==========================
 
 For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
@@ -449,6 +450,7 @@ Request contents:
   ``ETHTOOL_A_LINKMODES_SPEED``               u32     link speed (Mb/s)
   ``ETHTOOL_A_LINKMODES_DUPLEX``              u8      duplex mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG``    u8      Master/slave port mode
+  ``ETHTOOL_A_LINKMODES_RATE_ADAPTATION``     u8      PHY rate adaptation
   ``ETHTOOL_A_LINKMODES_LANES``               u32     lanes
   ==========================================  ======  ==========================
 
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 1f2531a1a876..dc70a9088544 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -74,6 +74,21 @@ const char *phy_duplex_to_str(unsigned int duplex)
 }
 EXPORT_SYMBOL_GPL(phy_duplex_to_str);
 
+const char *phy_rate_adaptation_to_str(int rate_adaptation)
+{
+	switch (rate_adaptation) {
+	case RATE_ADAPT_NONE:
+		return "none";
+	case RATE_ADAPT_PAUSE:
+		return "pause";
+	case RATE_ADAPT_CRS:
+		return "crs";
+	case RATE_ADAPT_OPEN_LOOP:
+		return "open-loop";
+	}
+	return "Unsupported (update phy-core.c)";
+}
+
 /* A mapping of all SUPPORTED settings to speed/duplex.  This table
  * must be grouped by speed and sorted in descending match priority
  * - iow, descending speed.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8d3ee3a6495b..77cbf07852e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -114,6 +114,33 @@ void phy_print_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_print_status);
 
+/**
+ * phy_get_rate_adaptation - determine if rate adaptation is supported
+ * @phydev: The phy device to return rate adaptation for
+ * @iface: The interface mode to use
+ *
+ * This determines the type of rate adaptation (if any) that @phy supports
+ * using @iface. @iface may be %PHY_INTERFACE_MODE_NA to determine if any
+ * interface supports rate adaptation.
+ *
+ * Return: The type of rate adaptation @phy supports for @iface, or
+ *         %RATE_ADAPT_NONE.
+ */
+int phy_get_rate_adaptation(struct phy_device *phydev,
+			    phy_interface_t iface)
+{
+	int ret = RATE_ADAPT_NONE;
+
+	if (phydev->drv->get_rate_adaptation) {
+		mutex_lock(&phydev->lock);
+		ret = phydev->drv->get_rate_adaptation(phydev, iface);
+		mutex_unlock(&phydev->lock);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_get_rate_adaptation);
+
 /**
  * phy_config_interrupt - configure the PHY device for the requested interrupts
  * @phydev: the phy_device struct
@@ -256,6 +283,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 	cmd->base.duplex = phydev->duplex;
 	cmd->base.master_slave_cfg = phydev->master_slave_get;
 	cmd->base.master_slave_state = phydev->master_slave_state;
+	cmd->base.rate_adaptation = phydev->rate_adaptation;
 	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
 		cmd->base.port = PORT_BNC;
 	else
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 81ce76c3e799..4ba8126b64f3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -276,7 +276,6 @@ static inline const char *phy_modes(phy_interface_t interface)
 	}
 }
 
-
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_FORCE_TIMEOUT	10
 
@@ -570,6 +569,7 @@ struct macsec_ops;
  * @lp_advertising: Current link partner advertised linkmodes
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
  * @autoneg: Flag autoneg being used
+ * @rate_adaptation: Current rate adaptation mode
  * @link: Current link state
  * @autoneg_complete: Flag auto negotiation of the link has completed
  * @mdix: Current crossover
@@ -637,6 +637,8 @@ struct phy_device {
 	unsigned irq_suspended:1;
 	unsigned irq_rerun:1;
 
+	int rate_adaptation;
+
 	enum phy_state state;
 
 	u32 dev_flags;
@@ -801,6 +803,21 @@ struct phy_driver {
 	 */
 	int (*get_features)(struct phy_device *phydev);
 
+	/**
+	 * @get_rate_adaptation: Get the supported type of rate adaptation for a
+	 * particular phy interface. This is used by phy consumers to determine
+	 * whether to advertise lower-speed modes for that interface. It is
+	 * assumed that if a rate adaptation mode is supported on an interface,
+	 * then that interface's rate can be adapted to all slower link speeds
+	 * supported by the phy. If iface is %PHY_INTERFACE_MODE_NA, and the phy
+	 * supports any kind of rate adaptation for any interface, then it must
+	 * return that rate adaptation mode (preferring %RATE_ADAPT_PAUSE, to
+	 * %RATE_ADAPT_CRS). If the interface is not supported, this should
+	 * return %RATE_ADAPT_NONE.
+	 */
+	int (*get_rate_adaptation)(struct phy_device *phydev,
+				   phy_interface_t iface);
+
 	/* PHY Power Management */
 	/** @suspend: Suspend the hardware, saving state if needed */
 	int (*suspend)(struct phy_device *phydev);
@@ -967,6 +984,7 @@ struct phy_fixup {
 
 const char *phy_speed_to_str(int speed);
 const char *phy_duplex_to_str(unsigned int duplex);
+const char *phy_rate_adaptation_to_str(int rate_adaptation);
 
 /* A structure for mapping a particular speed and duplex
  * combination to a particular SUPPORTED and ADVERTISED value
@@ -1681,6 +1699,8 @@ int phy_disable_interrupts(struct phy_device *phydev);
 void phy_request_interrupt(struct phy_device *phydev);
 void phy_free_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
+int phy_get_rate_adaptation(struct phy_device *phydev,
+			    phy_interface_t iface);
 void phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
 void phy_advertise_supported(struct phy_device *phydev);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e0f0ee9bc89e..3978f9c3fb83 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1840,6 +1840,20 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define MASTER_SLAVE_STATE_SLAVE		3
 #define MASTER_SLAVE_STATE_ERR			4
 
+/* These are used to throttle the rate of data on the phy interface when the
+ * native speed of the interface is higher than the link speed. These should
+ * not be used for phy interfaces which natively support multiple speeds (e.g.
+ * MII or SGMII).
+ */
+/* No rate adaptation performed. */
+#define RATE_ADAPT_NONE		0
+/* The phy sends pause frames to throttle the MAC. */
+#define RATE_ADAPT_PAUSE	1
+/* The phy asserts CRS to prevent the MAC from transmitting. */
+#define RATE_ADAPT_CRS		2
+/* The MAC is programmed with a sufficiently-large IPG. */
+#define RATE_ADAPT_OPEN_LOOP	3
+
 /* Which connector port. */
 #define PORT_TP			0x00
 #define PORT_AUI		0x01
@@ -2033,8 +2047,8 @@ enum ethtool_reset_flags {
  *	reported consistently by PHYLIB.  Read-only.
  * @master_slave_cfg: Master/slave port mode.
  * @master_slave_state: Master/slave port state.
+ * @rate_adaptation: Rate adaptation performed by the PHY
  * @reserved: Reserved for future use; see the note on reserved space.
- * @reserved1: Reserved for future use; see the note on reserved space.
  * @link_mode_masks: Variable length bitmaps.
  *
  * If autonegotiation is disabled, the speed and @duplex represent the
@@ -2085,7 +2099,7 @@ struct ethtool_link_settings {
 	__u8	transceiver;
 	__u8	master_slave_cfg;
 	__u8	master_slave_state;
-	__u8	reserved1[1];
+	__u8	rate_adaptation;
 	__u32	reserved[7];
 	__u32	link_mode_masks[0];
 	/* layout of link_mode_masks fields:
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index d2fb4f7be61b..3a5d81769ff4 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -242,6 +242,7 @@ enum {
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,	/* u8 */
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,	/* u8 */
 	ETHTOOL_A_LINKMODES_LANES,		/* u32 */
+	ETHTOOL_A_LINKMODES_RATE_ADAPTATION,	/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKMODES_CNT,
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 6a7308de192d..ef0ad300393a 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -571,6 +571,7 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
 		= __ETHTOOL_LINK_MODE_MASK_NU32;
 	link_ksettings.base.master_slave_cfg = MASTER_SLAVE_CFG_UNSUPPORTED;
 	link_ksettings.base.master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
+	link_ksettings.base.rate_adaptation = RATE_ADAPT_NONE;
 
 	return store_link_ksettings_for_user(useraddr, &link_ksettings);
 }
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 99b29b4fe947..7905bd985c7f 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -70,6 +70,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_LANES */
 		+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
+		+ nla_total_size(sizeof(u8)) /* LINKMODES_RATE_ADAPTATION */
 		+ 0;
 	ret = ethnl_bitset_size(ksettings->link_modes.advertising,
 				ksettings->link_modes.supported,
@@ -143,6 +144,10 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 		       lsettings->master_slave_state))
 		return -EMSGSIZE;
 
+	if (nla_put_u8(skb, ETHTOOL_A_LINKMODES_RATE_ADAPTATION,
+		       lsettings->rate_adaptation))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex

[Sean Anderson]

This adds support for cases when the link speed or duplex differs from
the speed or duplex of the phy interface mode. Such cases can occur when
some kind of rate adaptation is occurring.

The following terms are used within this and the following patches. I
do not believe the meaning of these terms are uncommon or surprising,
but for maximum clarity I would like to be explicit:

- Phy interface mode: the protocol used to communicate between the MAC
  or PCS (if used) and the phy. If no phy is in use, this is the same as
  the link mode. Each phy interface mode supported by Linux is a member
  of phy_interface_t.
- Link mode: the protocol used to communicate between the local phy (or
  PCS) and the remote phy (or PCS) over the physical medium. Each link
  mode supported by Linux is a member of ethtool_link_mode_bit_indices.
- Phy interface mode speed: the speed of unidirectional data transfer
  over a phy interface mode, including encoding overhead, but excluding
  protocol and flow-control overhead. The speed of a phy interface mode
  may vary. For example, SGMII may have a speed of 10, 100, or 1000
  Mbit/s.
- Link mode speed: similarly, the speed of unidirectional data transfer
  over a physical medium, including overhead, but excluding protocol and
  flow-control overhead. The speed of a link mode is usually fixed, but
  some exceptional link modes (such as 2BASE-TL) may vary their speed
  depending on the medium characteristics.

Before this patch, phylink assumed that the link mode speed was the same
as the phy interface mode speed. This is typically the case; however,
some phys have the ability to adapt between differing link mode and phy
interface mode speeds. To support these phys, this patch removes this
assumption, and adds a separate variable for link speed. Additionally,
to support rate adaptation, a MAC may need to have a certain duplex
(such as half or full). This may be different from the link's duplex. To
keep track of this distunction, this patch adds another variable to
track link duplex.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Support keeping track of link duplex
- Rewrite commit message for clarity
- Expand documentation of (link_)?(speed|duplex)
- Fix handling of speed/duplex gotten from MAC drivers

 drivers/net/phy/phylink.c | 112 ++++++++++++++++++++++++++------------
 include/linux/phylink.h   |  14 ++++-
 2 files changed, 88 insertions(+), 38 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 68a58ab6a8ed..da0623d94a64 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -155,6 +155,23 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
+/**
+ * phylink_state_fill_speed_duplex() - Update a state's interface speed and duplex
+ * @state: A link state
+ *
+ * Update the .speed and .duplex members of @state. We can determine them based
+ * on the .link_speed and .link_duplex. This function should be called whenever
+ * .link_speed and .link_duplex are updated.  For example, userspace deals with
+ * link speed and duplex, and not the interface speed and duplex. Similarly,
+ * phys deal with link speed and duplex and only implicitly the interface speed
+ * and duplex.
+ */
+static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
+{
+	state->speed = state->link_speed;
+	state->duplex = state->link_duplex;
+}
+
 /**
  * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
  * @linkmodes: ethtool linkmode mask (must be already initialised)
@@ -524,11 +541,11 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 	if (fixed_node) {
 		ret = fwnode_property_read_u32(fixed_node, "speed", &speed);
 
-		pl->link_config.speed = speed;
-		pl->link_config.duplex = DUPLEX_HALF;
+		pl->link_config.link_speed = speed;
+		pl->link_config.link_duplex = DUPLEX_HALF;
 
 		if (fwnode_property_read_bool(fixed_node, "full-duplex"))
-			pl->link_config.duplex = DUPLEX_FULL;
+			pl->link_config.link_duplex = DUPLEX_FULL;
 
 		/* We treat the "pause" and "asym-pause" terminology as
 		 * defining the link partner's ability.
@@ -566,9 +583,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 		ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
 						     prop, ARRAY_SIZE(prop));
 		if (!ret) {
-			pl->link_config.duplex = prop[1] ?
+			pl->link_config.link_duplex = prop[1] ?
 						DUPLEX_FULL : DUPLEX_HALF;
-			pl->link_config.speed = prop[2];
+			pl->link_config.link_speed = prop[2];
 			if (prop[3])
 				__set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 					  pl->link_config.lp_advertising);
@@ -578,16 +595,18 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 		}
 	}
 
-	if (pl->link_config.speed > SPEED_1000 &&
-	    pl->link_config.duplex != DUPLEX_FULL)
+	if (pl->link_config.link_speed > SPEED_1000 &&
+	    pl->link_config.link_duplex != DUPLEX_FULL)
 		phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
-			     pl->link_config.speed);
+			     pl->link_config.link_speed);
 
+	phylink_state_fill_speed_duplex(&pl->link_config);
 	bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	linkmode_copy(pl->link_config.advertising, pl->supported);
 	phylink_validate(pl, pl->supported, &pl->link_config);
 
-	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
+	s = phy_lookup_setting(pl->link_config.link_speed,
+			       pl->link_config.link_duplex,
 			       pl->supported, true);
 	linkmode_zero(pl->supported);
 	phylink_set(pl->supported, MII);
@@ -599,8 +618,8 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 		__set_bit(s->bit, pl->link_config.lp_advertising);
 	} else {
 		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
-			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
-			     pl->link_config.speed);
+			     pl->link_config.link_duplex == DUPLEX_FULL ? "full" : "half",
+			     pl->link_config.link_speed);
 	}
 
 	linkmode_and(pl->link_config.advertising, pl->link_config.advertising,
@@ -757,7 +776,7 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
 	bool tx_pause, rx_pause;
 
 	state->pause = MLO_PAUSE_NONE;
-	if (state->duplex == DUPLEX_FULL) {
+	if (state->link_duplex == DUPLEX_FULL) {
 		linkmode_resolve_pause(state->advertising,
 				       state->lp_advertising,
 				       &tx_pause, &rx_pause);
@@ -925,12 +944,16 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	linkmode_zero(state->lp_advertising);
 	state->interface = pl->link_config.interface;
 	state->an_enabled = pl->link_config.an_enabled;
-	if  (state->an_enabled) {
+	if (state->an_enabled) {
+		state->link_speed = SPEED_UNKNOWN;
+		state->link_duplex = DUPLEX_UNKNOWN;
 		state->speed = SPEED_UNKNOWN;
 		state->duplex = DUPLEX_UNKNOWN;
 		state->pause = MLO_PAUSE_NONE;
 	} else {
-		state->speed =  pl->link_config.speed;
+		state->link_speed = pl->link_config.link_speed;
+		state->link_duplex = pl->link_config.link_duplex;
+		state->speed = pl->link_config.speed;
 		state->duplex = pl->link_config.duplex;
 		state->pause = pl->link_config.pause;
 	}
@@ -944,6 +967,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 		pl->mac_ops->mac_pcs_get_state(pl->config, state);
 	else
 		state->link = 0;
+
+	state->link_speed = state->speed;
+	state->link_duplex = state->duplex;
 }
 
 /* The fixed state is... fixed except for the link state,
@@ -953,10 +979,17 @@ static void phylink_get_fixed_state(struct phylink *pl,
 				    struct phylink_link_state *state)
 {
 	*state = pl->link_config;
-	if (pl->config->get_fixed_state)
+	if (pl->config->get_fixed_state) {
 		pl->config->get_fixed_state(pl->config, state);
-	else if (pl->link_gpio)
+		/* FIXME: these should not be updated, but
+		 * bcm_sf2_sw_fixed_state does it anyway
+		 */
+		state->link_speed = state->speed;
+		state->link_duplex = state->duplex;
+		phylink_state_fill_speed_duplex(state);
+	} else if (pl->link_gpio) {
 		state->link = !!gpiod_get_value_cansleep(pl->link_gpio);
+	}
 
 	phylink_resolve_flow(state);
 }
@@ -1027,8 +1060,8 @@ static void phylink_link_up(struct phylink *pl,
 
 	phylink_info(pl,
 		     "Link is Up - %s/%s - flow control %s\n",
-		     phy_speed_to_str(link_state.speed),
-		     phy_duplex_to_str(link_state.duplex),
+		     phy_speed_to_str(link_state.link_speed),
+		     phy_duplex_to_str(link_state.link_duplex),
 		     phylink_pause_to_str(link_state.pause));
 }
 
@@ -1279,8 +1312,9 @@ struct phylink *phylink_create(struct phylink_config *config,
 		pl->link_port = PORT_MII;
 	pl->link_config.interface = iface;
 	pl->link_config.pause = MLO_PAUSE_AN;
-	pl->link_config.speed = SPEED_UNKNOWN;
-	pl->link_config.duplex = DUPLEX_UNKNOWN;
+	pl->link_config.link_speed = SPEED_UNKNOWN;
+	pl->link_config.link_duplex = DUPLEX_UNKNOWN;
+	phylink_state_fill_speed_duplex(&pl->link_config);
 	pl->link_config.an_enabled = true;
 	pl->mac_ops = mac_ops;
 	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
@@ -1344,8 +1378,8 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	phy_get_pause(phydev, &tx_pause, &rx_pause);
 
 	mutex_lock(&pl->state_mutex);
-	pl->phy_state.speed = phydev->speed;
-	pl->phy_state.duplex = phydev->duplex;
+	pl->phy_state.link_speed = phydev->speed;
+	pl->phy_state.link_duplex = phydev->duplex;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	if (tx_pause)
 		pl->phy_state.pause |= MLO_PAUSE_TX;
@@ -1353,6 +1387,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 		pl->phy_state.pause |= MLO_PAUSE_RX;
 	pl->phy_state.interface = phydev->interface;
 	pl->phy_state.link = up;
+	phylink_state_fill_speed_duplex(&pl->phy_state);
 	mutex_unlock(&pl->state_mutex);
 
 	phylink_run_resolve(pl);
@@ -1422,8 +1457,9 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->phydev = phy;
 	pl->phy_state.interface = interface;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
-	pl->phy_state.speed = SPEED_UNKNOWN;
-	pl->phy_state.duplex = DUPLEX_UNKNOWN;
+	pl->phy_state.link_speed = SPEED_UNKNOWN;
+	pl->phy_state.link_duplex = DUPLEX_UNKNOWN;
+	phylink_state_fill_speed_duplex(&pl->phy_state);
 	linkmode_copy(pl->supported, supported);
 	linkmode_copy(pl->link_config.advertising, config.advertising);
 
@@ -1866,8 +1902,8 @@ static void phylink_get_ksettings(const struct phylink_link_state *state,
 {
 	phylink_merge_link_mode(kset->link_modes.advertising, state->advertising);
 	linkmode_copy(kset->link_modes.lp_advertising, state->lp_advertising);
-	kset->base.speed = state->speed;
-	kset->base.duplex = state->duplex;
+	kset->base.speed = state->link_speed;
+	kset->base.duplex = state->link_duplex;
 	kset->base.autoneg = state->an_enabled ? AUTONEG_ENABLE :
 				AUTONEG_DISABLE;
 }
@@ -1983,14 +2019,14 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		 * If the link parameters match, accept them but do nothing.
 		 */
 		if (pl->cur_link_an_mode == MLO_AN_FIXED) {
-			if (s->speed != pl->link_config.speed ||
-			    s->duplex != pl->link_config.duplex)
+			if (s->speed != pl->link_config.link_speed ||
+			    s->duplex != pl->link_config.link_duplex)
 				return -EINVAL;
 			return 0;
 		}
 
-		config.speed = s->speed;
-		config.duplex = s->duplex;
+		config.link_speed = s->speed;
+		config.link_duplex = s->duplex;
 		break;
 
 	case AUTONEG_ENABLE:
@@ -2005,8 +2041,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 			return 0;
 		}
 
-		config.speed = SPEED_UNKNOWN;
-		config.duplex = DUPLEX_UNKNOWN;
+		config.link_speed = SPEED_UNKNOWN;
+		config.link_duplex = DUPLEX_UNKNOWN;
 		break;
 
 	default:
@@ -2036,6 +2072,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		}
 
 		/* Revalidate with the selected interface */
+		phylink_state_fill_speed_duplex(&config);
 		linkmode_copy(support, pl->supported);
 		if (phylink_validate(pl, support, &config)) {
 			phylink_err(pl, "validation of %s/%s with support %*pb failed\n",
@@ -2046,6 +2083,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		}
 	} else {
 		/* Validate without changing the current supported mask. */
+		phylink_state_fill_speed_duplex(&config);
 		linkmode_copy(support, pl->supported);
 		if (phylink_validate(pl, support, &config))
 			return -EINVAL;
@@ -2056,9 +2094,10 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		return -EINVAL;
 
 	mutex_lock(&pl->state_mutex);
-	pl->link_config.speed = config.speed;
-	pl->link_config.duplex = config.duplex;
+	pl->link_config.link_speed = config.link_speed;
+	pl->link_config.link_duplex = config.link_duplex;
 	pl->link_config.an_enabled = config.an_enabled;
+	phylink_state_fill_speed_duplex(&pl->link_config);
 
 	if (pl->link_config.interface != config.interface) {
 		/* The interface changed, e.g. 1000base-X <-> 2500base-X */
@@ -2597,10 +2636,11 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
 	memset(&config, 0, sizeof(config));
 	linkmode_copy(config.advertising, advertising);
 	config.interface = PHY_INTERFACE_MODE_NA;
-	config.speed = SPEED_UNKNOWN;
-	config.duplex = DUPLEX_UNKNOWN;
+	config.link_speed = SPEED_UNKNOWN;
+	config.link_duplex = DUPLEX_UNKNOWN;
 	config.pause = MLO_PAUSE_AN;
 	config.an_enabled = pl->link_config.an_enabled;
+	phylink_state_fill_speed_duplex(&config);
 
 	/* Ignore errors if we're expecting a PHY to attach later */
 	ret = phylink_validate(pl, support, &config);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 5008ec3dcade..ab5edc1e5330 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -56,8 +56,16 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
  * @lp_advertising: ethtool bitmask containing link partner advertised link
  *   modes
  * @interface: link &typedef phy_interface_t mode
- * @speed: link speed, one of the SPEED_* constants.
- * @duplex: link duplex mode, one of DUPLEX_* constants.
+ * @speed: interface speed, one of the SPEED_* constants. This is the speed of
+ *   the host-side interface to the phy. If @rate_adaptation is being
+ *   performed, this will be different from @link_speed.
+ * @link_speed: link speed, one of the SPEED_* constants. This is the speed of
+ *   the line-side interface to the link partner.
+ * @duplex: interface duplex mode, one of DUPLEX_* constants. This is the
+ *   duplex of then host-side interface to the phy. If @rate_adaptation is
+ *   being performed, this may be different from @link_duplex.
+ * @link_duplex: link duplex, one of the DUPLEX_* constants. This is the duplex
+ *   of the line-side interface to the link partner.
  * @pause: link pause state, described by MLO_PAUSE_* constants.
  * @link: true if the link is up.
  * @an_enabled: true if autonegotiation is enabled/desired.
@@ -68,7 +76,9 @@ struct phylink_link_state {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
 	phy_interface_t interface;
 	int speed;
+	int link_speed;
 	int duplex;
+	int link_duplex;
 	int pause;
 	unsigned int link:1;
 	unsigned int an_enabled:1;
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex

[Russell King (Oracle)]

On Tue, Jul 19, 2022 at 07:49:56PM -0400, Sean Anderson wrote:
> This adds support for cases when the link speed or duplex differs from
> the speed or duplex of the phy interface mode. Such cases can occur when
> some kind of rate adaptation is occurring.
> 
> The following terms are used within this and the following patches. I
> do not believe the meaning of these terms are uncommon or surprising,
> but for maximum clarity I would like to be explicit:
> 
> - Phy interface mode: the protocol used to communicate between the MAC
>   or PCS (if used) and the phy. If no phy is in use, this is the same as
>   the link mode. Each phy interface mode supported by Linux is a member
>   of phy_interface_t.
> - Link mode: the protocol used to communicate between the local phy (or
>   PCS) and the remote phy (or PCS) over the physical medium. Each link
>   mode supported by Linux is a member of ethtool_link_mode_bit_indices.
> - Phy interface mode speed: the speed of unidirectional data transfer
>   over a phy interface mode, including encoding overhead, but excluding
>   protocol and flow-control overhead. The speed of a phy interface mode
>   may vary. For example, SGMII may have a speed of 10, 100, or 1000
>   Mbit/s.
> - Link mode speed: similarly, the speed of unidirectional data transfer
>   over a physical medium, including overhead, but excluding protocol and
>   flow-control overhead. The speed of a link mode is usually fixed, but
>   some exceptional link modes (such as 2BASE-TL) may vary their speed
>   depending on the medium characteristics.
> 
> Before this patch, phylink assumed that the link mode speed was the same
> as the phy interface mode speed. This is typically the case; however,
> some phys have the ability to adapt between differing link mode and phy
> interface mode speeds. To support these phys, this patch removes this
> assumption, and adds a separate variable for link speed. Additionally,
> to support rate adaptation, a MAC may need to have a certain duplex
> (such as half or full). This may be different from the link's duplex. To
> keep track of this distunction, this patch adds another variable to
> track link duplex.

I thought we had decided that using the term "link" in these new members
was a bad idea.

> @@ -925,12 +944,16 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>  	linkmode_zero(state->lp_advertising);
>  	state->interface = pl->link_config.interface;
>  	state->an_enabled = pl->link_config.an_enabled;
> -	if  (state->an_enabled) {
> +	if (state->an_enabled) {
> +		state->link_speed = SPEED_UNKNOWN;
> +		state->link_duplex = DUPLEX_UNKNOWN;
>  		state->speed = SPEED_UNKNOWN;
>  		state->duplex = DUPLEX_UNKNOWN;
>  		state->pause = MLO_PAUSE_NONE;
>  	} else {
> -		state->speed =  pl->link_config.speed;
> +		state->link_speed = pl->link_config.link_speed;
> +		state->link_duplex = pl->link_config.link_duplex;
> +		state->speed = pl->link_config.speed;
>  		state->duplex = pl->link_config.duplex;
>  		state->pause = pl->link_config.pause;
>  	}
> @@ -944,6 +967,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>  		pl->mac_ops->mac_pcs_get_state(pl->config, state);
>  	else
>  		state->link = 0;
> +
> +	state->link_speed = state->speed;
> +	state->link_duplex = state->duplex;

Why do you need to set link_speed and link_duple above if they're always
copied over here?

>  /* The fixed state is... fixed except for the link state,
> @@ -953,10 +979,17 @@ static void phylink_get_fixed_state(struct phylink *pl,
>  				    struct phylink_link_state *state)
>  {
>  	*state = pl->link_config;
> -	if (pl->config->get_fixed_state)
> +	if (pl->config->get_fixed_state) {
>  		pl->config->get_fixed_state(pl->config, state);
> -	else if (pl->link_gpio)
> +		/* FIXME: these should not be updated, but
> +		 * bcm_sf2_sw_fixed_state does it anyway
> +		 */
> +		state->link_speed = state->speed;
> +		state->link_duplex = state->duplex;
> +		phylink_state_fill_speed_duplex(state);

This looks weird. Why copy state->xxx to state->link_xxx and then copy
them back to state->xxx in a helper function?

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

Re: [PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex

[Sean Anderson]

On 7/20/22 2:43 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:56PM -0400, Sean Anderson wrote:
>> This adds support for cases when the link speed or duplex differs from
>> the speed or duplex of the phy interface mode. Such cases can occur when
>> some kind of rate adaptation is occurring.
>> 
>> The following terms are used within this and the following patches. I
>> do not believe the meaning of these terms are uncommon or surprising,
>> but for maximum clarity I would like to be explicit:
>> 
>> - Phy interface mode: the protocol used to communicate between the MAC
>>   or PCS (if used) and the phy. If no phy is in use, this is the same as
>>   the link mode. Each phy interface mode supported by Linux is a member
>>   of phy_interface_t.
>> - Link mode: the protocol used to communicate between the local phy (or
>>   PCS) and the remote phy (or PCS) over the physical medium. Each link
>>   mode supported by Linux is a member of ethtool_link_mode_bit_indices.
>> - Phy interface mode speed: the speed of unidirectional data transfer
>>   over a phy interface mode, including encoding overhead, but excluding
>>   protocol and flow-control overhead. The speed of a phy interface mode
>>   may vary. For example, SGMII may have a speed of 10, 100, or 1000
>>   Mbit/s.
>> - Link mode speed: similarly, the speed of unidirectional data transfer
>>   over a physical medium, including overhead, but excluding protocol and
>>   flow-control overhead. The speed of a link mode is usually fixed, but
>>   some exceptional link modes (such as 2BASE-TL) may vary their speed
>>   depending on the medium characteristics.
>> 
>> Before this patch, phylink assumed that the link mode speed was the same
>> as the phy interface mode speed. This is typically the case; however,
>> some phys have the ability to adapt between differing link mode and phy
>> interface mode speeds. To support these phys, this patch removes this
>> assumption, and adds a separate variable for link speed. Additionally,
>> to support rate adaptation, a MAC may need to have a certain duplex
>> (such as half or full). This may be different from the link's duplex. To
>> keep track of this distunction, this patch adds another variable to
>> track link duplex.
> 
> I thought we had decided that using the term "link" in these new members
> was a bad idea.

I saw that you and Andrew were not in favor, but I did not get a response to
my defense of this terminology. That said, this is not a terribly large
change to make.

>> @@ -925,12 +944,16 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>>  	linkmode_zero(state->lp_advertising);
>>  	state->interface = pl->link_config.interface;
>>  	state->an_enabled = pl->link_config.an_enabled;
>> -	if  (state->an_enabled) {
>> +	if (state->an_enabled) {
>> +		state->link_speed = SPEED_UNKNOWN;
>> +		state->link_duplex = DUPLEX_UNKNOWN;
>>  		state->speed = SPEED_UNKNOWN;
>>  		state->duplex = DUPLEX_UNKNOWN;
>>  		state->pause = MLO_PAUSE_NONE;
>>  	} else {
>> -		state->speed =  pl->link_config.speed;
>> +		state->link_speed = pl->link_config.link_speed;
>> +		state->link_duplex = pl->link_config.link_duplex;
>> +		state->speed = pl->link_config.speed;
>>  		state->duplex = pl->link_config.duplex;
>>  		state->pause = pl->link_config.pause;
>>  	}
>> @@ -944,6 +967,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>>  		pl->mac_ops->mac_pcs_get_state(pl->config, state);
>>  	else
>>  		state->link = 0;
>> +
>> +	state->link_speed = state->speed;
>> +	state->link_duplex = state->duplex;
> 
> Why do you need to set link_speed and link_duple above if they're always
> copied over here?

This will be conditional on the rate adaptation in the next patch. I should
have been more clear in the commit message, but this patch is not really useful
on its own, and primarily serves to break up the changes to make things easier
to review.

>>  /* The fixed state is... fixed except for the link state,
>> @@ -953,10 +979,17 @@ static void phylink_get_fixed_state(struct phylink *pl,
>>  				    struct phylink_link_state *state)
>>  {
>>  	*state = pl->link_config;
>> -	if (pl->config->get_fixed_state)
>> +	if (pl->config->get_fixed_state) {
>>  		pl->config->get_fixed_state(pl->config, state);
>> -	else if (pl->link_gpio)
>> +		/* FIXME: these should not be updated, but
>> +		 * bcm_sf2_sw_fixed_state does it anyway
>> +		 */
>> +		state->link_speed = state->speed;
>> +		state->link_duplex = state->duplex;
>> +		phylink_state_fill_speed_duplex(state);
> 
> This looks weird. Why copy state->xxx to state->link_xxx and then copy
> them back to state->xxx in a helper function?
> 

Because in the next patch the speed/rate could be different if rate adaptation
is enabled. This is not really necessary for now, since fixed state links cannot
specify rate adaptation, but I have tried to be complete.

--Sean

Re: [PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex

[Andrew Lunn]

> > I thought we had decided that using the term "link" in these new members
> > was a bad idea.
> 
> I saw that you and Andrew were not in favor, but I did not get a response to
> my defense of this terminology. That said, this is not a terribly large
> change to make.

I know Russell tends to use media side, and i use line side. I would
be happy with either. I think we both use host side.

"link" is way to ambiguous.

I do understand you not wanting to change phydev->speed, it is used in
a lot of places. But maybe changing it is good, you then get to look
at the code and decide does it want the media speed, or the host
speed.

	Andrew

[PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Sean Anderson]

If the phy is configured to use pause-based rate adaptation, ensure that
the link is full duplex with pause frame reception enabled. As
suggested, if pause-based rate adaptation is enabled by the phy, then
pause reception is unconditionally enabled.

The interface duplex is determined based on the rate adaptation type.
When rate adaptation is enabled, so is the speed. We assume the maximum
interface speed is used. This is only relevant for MLO_AN_PHY. For
MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Use the phy's rate adaptation setting to determine whether to use its
  link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
- Always use the rate adaptation setting to determine the interface
  speed/duplex (instead of sometimes using the interface mode).

 drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++----
 include/linux/phylink.h   |   1 +
 2 files changed, 114 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index da0623d94a64..619ef553476f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode)
  * @state: A link state
  *
  * Update the .speed and .duplex members of @state. We can determine them based
- * on the .link_speed and .link_duplex. This function should be called whenever
- * .link_speed and .link_duplex are updated.  For example, userspace deals with
- * link speed and duplex, and not the interface speed and duplex. Similarly,
- * phys deal with link speed and duplex and only implicitly the interface speed
- * and duplex.
+ * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This
+ * function should be called whenever .link_speed and .link_duplex are updated.
+ * For example, userspace deals with link speed and duplex, and not the
+ * interface speed and duplex. Similarly, phys deal with link speed and duplex
+ * and only implicitly the interface speed and duplex.
  */
 static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
 {
-	state->speed = state->link_speed;
-	state->duplex = state->link_duplex;
+	switch (state->rate_adaptation) {
+	case RATE_ADAPT_NONE:
+		state->speed = state->link_speed;
+		state->duplex = state->link_duplex;
+		return;
+	case RATE_ADAPT_PAUSE:
+		state->duplex = DUPLEX_FULL;
+		break;
+	case RATE_ADAPT_CRS:
+		state->duplex = DUPLEX_HALF;
+		break;
+	case RATE_ADAPT_OPEN_LOOP:
+		state->duplex = state->link_duplex;
+		break;
+	}
+
+	/* Use the max speed of the interface */
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_SMII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_MII:
+		state->speed = SPEED_100;
+		return;
+
+	case PHY_INTERFACE_MODE_TBI:
+	case PHY_INTERFACE_MODE_MOCA:
+	case PHY_INTERFACE_MODE_RTBI:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_1000BASEKX:
+	case PHY_INTERFACE_MODE_TRGMII:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_GMII:
+		state->speed = SPEED_1000;
+		return;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		state->speed = SPEED_2500;
+		return;
+
+	case PHY_INTERFACE_MODE_5GBASER:
+		state->speed = SPEED_5000;
+		return;
+
+	case PHY_INTERFACE_MODE_XGMII:
+	case PHY_INTERFACE_MODE_RXAUI:
+	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_10GKR:
+	case PHY_INTERFACE_MODE_USXGMII:
+		state->speed = SPEED_10000;
+		return;
+
+	case PHY_INTERFACE_MODE_25GBASER:
+		state->speed = SPEED_25000;
+		return;
+
+	case PHY_INTERFACE_MODE_XLGMII:
+		state->speed = SPEED_40000;
+		return;
+
+	case PHY_INTERFACE_MODE_INTERNAL:
+		state->speed = state->link_speed;
+		return;
+
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_MAX:
+		state->speed = SPEED_UNKNOWN;
+		return;
+	}
+
+	WARN_ON(1);
 }
 
 /**
@@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl,
 			       const struct phylink_link_state *state)
 {
 	phylink_dbg(pl,
-		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
+		    "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
 		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
 		    phy_modes(state->interface),
 		    phy_speed_to_str(state->speed),
 		    phy_duplex_to_str(state->duplex),
+		    phy_rate_adaptation_to_str(state->rate_adaptation),
 		    __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
 		    state->pause, state->link, state->an_enabled);
 
@@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	linkmode_zero(state->lp_advertising);
 	state->interface = pl->link_config.interface;
 	state->an_enabled = pl->link_config.an_enabled;
+	state->rate_adaptation = pl->link_config.rate_adaptation;
 	if (state->an_enabled) {
 		state->link_speed = SPEED_UNKNOWN;
 		state->link_duplex = DUPLEX_UNKNOWN;
@@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	else
 		state->link = 0;
 
-	state->link_speed = state->speed;
-	state->link_duplex = state->duplex;
+	if (state->rate_adaptation == RATE_ADAPT_NONE) {
+		state->link_speed = state->speed;
+		state->link_duplex = state->duplex;
+	}
 }
 
 /* The fixed state is... fixed except for the link state,
@@ -1043,6 +1124,8 @@ static void phylink_link_up(struct phylink *pl,
 	struct net_device *ndev = pl->netdev;
 
 	pl->cur_interface = link_state.interface;
+	if (link_state.rate_adaptation == RATE_ADAPT_PAUSE)
+		link_state.pause |= MLO_PAUSE_RX;
 
 	if (pl->pcs && pl->pcs->ops->pcs_link_up)
 		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
@@ -1145,6 +1228,18 @@ static void phylink_resolve(struct work_struct *w)
 				}
 				link_state.interface = pl->phy_state.interface;
 
+				/* If we are doing rate adaptation, then the
+				 * link speed/duplex comes from the PHY
+				 */
+				if (pl->phy_state.rate_adaptation) {
+					link_state.rate_adaptation =
+						pl->phy_state.rate_adaptation;
+					link_state.link_speed =
+						pl->phy_state.link_speed;
+					link_state.link_duplex =
+						pl->phy_state.link_duplex;
+				}
+
 				/* If we have a PHY, we need to update with
 				 * the PHY flow control bits.
 				 */
@@ -1380,6 +1475,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	mutex_lock(&pl->state_mutex);
 	pl->phy_state.link_speed = phydev->speed;
 	pl->phy_state.link_duplex = phydev->duplex;
+	pl->phy_state.rate_adaptation = phydev->rate_adaptation;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	if (tx_pause)
 		pl->phy_state.pause |= MLO_PAUSE_TX;
@@ -1392,10 +1488,11 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 
 	phylink_run_resolve(pl);
 
-	phylink_dbg(pl, "phy link %s %s/%s/%s/%s\n", up ? "up" : "down",
+	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s\n", up ? "up" : "down",
 		    phy_modes(phydev->interface),
 		    phy_speed_to_str(phydev->speed),
 		    phy_duplex_to_str(phydev->duplex),
+		    phy_rate_adaptation_to_str(phydev->rate_adaptation),
 		    phylink_pause_to_str(pl->phy_state.pause));
 }
 
@@ -1459,6 +1556,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	pl->phy_state.link_speed = SPEED_UNKNOWN;
 	pl->phy_state.link_duplex = DUPLEX_UNKNOWN;
+	pl->phy_state.rate_adaptation = RATE_ADAPT_NONE;
 	phylink_state_fill_speed_duplex(&pl->phy_state);
 	linkmode_copy(pl->supported, supported);
 	linkmode_copy(pl->link_config.advertising, config.advertising);
@@ -1902,8 +2000,10 @@ static void phylink_get_ksettings(const struct phylink_link_state *state,
 {
 	phylink_merge_link_mode(kset->link_modes.advertising, state->advertising);
 	linkmode_copy(kset->link_modes.lp_advertising, state->lp_advertising);
-	kset->base.speed = state->link_speed;
-	kset->base.duplex = state->link_duplex;
+	if (kset->base.rate_adaptation == RATE_ADAPT_NONE) {
+		kset->base.speed = state->link_speed;
+		kset->base.duplex = state->link_duplex;
+	}
 	kset->base.autoneg = state->an_enabled ? AUTONEG_ENABLE :
 				AUTONEG_DISABLE;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index ab5edc1e5330..f32b97f27ddc 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -80,6 +80,7 @@ struct phylink_link_state {
 	int duplex;
 	int link_duplex;
 	int pause;
+	int rate_adaptation;
 	unsigned int link:1;
 	unsigned int an_enabled:1;
 	unsigned int an_complete:1;
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Russell King (Oracle)]

On Tue, Jul 19, 2022 at 07:49:57PM -0400, Sean Anderson wrote:
> If the phy is configured to use pause-based rate adaptation, ensure that
> the link is full duplex with pause frame reception enabled. As
> suggested, if pause-based rate adaptation is enabled by the phy, then
> pause reception is unconditionally enabled.
> 
> The interface duplex is determined based on the rate adaptation type.
> When rate adaptation is enabled, so is the speed. We assume the maximum
> interface speed is used. This is only relevant for MLO_AN_PHY. For
> MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Use the phy's rate adaptation setting to determine whether to use its
>   link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
> - Always use the rate adaptation setting to determine the interface
>   speed/duplex (instead of sometimes using the interface mode).
> 
>  drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++----
>  include/linux/phylink.h   |   1 +
>  2 files changed, 114 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index da0623d94a64..619ef553476f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode)
>   * @state: A link state
>   *
>   * Update the .speed and .duplex members of @state. We can determine them based
> - * on the .link_speed and .link_duplex. This function should be called whenever
> - * .link_speed and .link_duplex are updated.  For example, userspace deals with
> - * link speed and duplex, and not the interface speed and duplex. Similarly,
> - * phys deal with link speed and duplex and only implicitly the interface speed
> - * and duplex.
> + * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This
> + * function should be called whenever .link_speed and .link_duplex are updated.
> + * For example, userspace deals with link speed and duplex, and not the
> + * interface speed and duplex. Similarly, phys deal with link speed and duplex
> + * and only implicitly the interface speed and duplex.
>   */
>  static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
>  {
> -	state->speed = state->link_speed;
> -	state->duplex = state->link_duplex;
> +	switch (state->rate_adaptation) {
> +	case RATE_ADAPT_NONE:
> +		state->speed = state->link_speed;
> +		state->duplex = state->link_duplex;
> +		return;
> +	case RATE_ADAPT_PAUSE:
> +		state->duplex = DUPLEX_FULL;
> +		break;
> +	case RATE_ADAPT_CRS:
> +		state->duplex = DUPLEX_HALF;
> +		break;
> +	case RATE_ADAPT_OPEN_LOOP:
> +		state->duplex = state->link_duplex;
> +		break;
> +	}
> +
> +	/* Use the max speed of the interface */
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_REVRMII:
> +	case PHY_INTERFACE_MODE_RMII:
> +	case PHY_INTERFACE_MODE_SMII:
> +	case PHY_INTERFACE_MODE_REVMII:
> +	case PHY_INTERFACE_MODE_MII:
> +		state->speed = SPEED_100;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_TBI:
> +	case PHY_INTERFACE_MODE_MOCA:
> +	case PHY_INTERFACE_MODE_RTBI:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEKX:
> +	case PHY_INTERFACE_MODE_TRGMII:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_GMII:
> +		state->speed = SPEED_1000;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		state->speed = SPEED_2500;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_5GBASER:
> +		state->speed = SPEED_5000;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_XGMII:
> +	case PHY_INTERFACE_MODE_RXAUI:
> +	case PHY_INTERFACE_MODE_XAUI:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_10GKR:
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		state->speed = SPEED_10000;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_25GBASER:
> +		state->speed = SPEED_25000;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_XLGMII:
> +		state->speed = SPEED_40000;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_INTERNAL:
> +		state->speed = state->link_speed;
> +		return;
> +
> +	case PHY_INTERFACE_MODE_NA:
> +	case PHY_INTERFACE_MODE_MAX:
> +		state->speed = SPEED_UNKNOWN;
> +		return;
> +	}
> +
> +	WARN_ON(1);
>  }
>  
>  /**
> @@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl,
>  			       const struct phylink_link_state *state)
>  {
>  	phylink_dbg(pl,
> -		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> +		    "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
>  		    phy_modes(state->interface),
>  		    phy_speed_to_str(state->speed),
>  		    phy_duplex_to_str(state->duplex),
> +		    phy_rate_adaptation_to_str(state->rate_adaptation),
>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>  		    state->pause, state->link, state->an_enabled);
>  
> @@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>  	linkmode_zero(state->lp_advertising);
>  	state->interface = pl->link_config.interface;
>  	state->an_enabled = pl->link_config.an_enabled;
> +	state->rate_adaptation = pl->link_config.rate_adaptation;
>  	if (state->an_enabled) {
>  		state->link_speed = SPEED_UNKNOWN;
>  		state->link_duplex = DUPLEX_UNKNOWN;
> @@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>  	else
>  		state->link = 0;
>  
> -	state->link_speed = state->speed;
> -	state->link_duplex = state->duplex;
> +	if (state->rate_adaptation == RATE_ADAPT_NONE) {
> +		state->link_speed = state->speed;
> +		state->link_duplex = state->duplex;
> +	}

So we need to have every PCS driver be udpated to fill in link_speed
and link_duplex if rate_adaption != none.

There's got to be a better way - maybe what I suggested in the last
round of only doing the rate adaption thing in the link_up() functions,
since that seems to be the only real difference.

I'm not even sure we need to do that - in the "open loop" case, we
need to be passing the media speed to the MAC driver with the knowledge
that it should be increasing the IPG.

So, I'm thinking we don't want any of these changes, what we instead
should be doing is passing the media speed/duplex and the interface
speed/duplex to the PCS and MAC.

We can do that by storing the PHY rate adaption state, and processing
that in phylink_link_up().

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

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Russell King (Oracle)]

On Wed, Jul 20, 2022 at 07:50:52AM +0100, Russell King (Oracle) wrote:
> We can do that by storing the PHY rate adaption state, and processing
> that in phylink_link_up().

Something like this? I haven't included the IPG (open loop) stuff in
this - I think when we come to implement that, we need a new mac
method to call to set the IPG just before calling mac_link_up().
Something like:

 void mac_set_ipg(struct phylink_config *config, int packet_speed,
		  int interface_speed);

Note that we also have PCS that do rate adaption too, and I think
fitting those in with the code below may be easier than storing the
media and phy interface speed/duplex separately.

A few further question though - does rate adaption make sense with
interface modes that can already natively handle the different speeds,
such as SGMII, RGMII, USXGMII, etc?

 drivers/net/phy/phylink.c | 103 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |   1 +
 2 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9bd69328dc4d..c89eb74458cd 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -994,23 +994,105 @@ static const char *phylink_pause_to_str(int pause)
 	}
 }
 
+static int phylink_interface_max_speed(phy_interface_t interface)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_SMII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_MII:
+		return SPEED_100;
+
+	case PHY_INTERFACE_MODE_TBI:
+	case PHY_INTERFACE_MODE_MOCA:
+	case PHY_INTERFACE_MODE_RTBI:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_1000BASEKX:
+	case PHY_INTERFACE_MODE_TRGMII:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_GMII:
+		return SPEED_1000;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		return SPEED_2500;
+
+	case PHY_INTERFACE_MODE_5GBASER:
+		return SPEED_5000;
+
+	case PHY_INTERFACE_MODE_XGMII:
+	case PHY_INTERFACE_MODE_RXAUI:
+	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_10GKR:
+	case PHY_INTERFACE_MODE_USXGMII:
+		return SPEED_10000;
+
+	case PHY_INTERFACE_MODE_25GBASER:
+		return SPEED_25000;
+
+	case PHY_INTERFACE_MODE_XLGMII:
+		return SPEED_40000;
+
+	case PHY_INTERFACE_MODE_INTERNAL:
+		/* Rate adaption is probably not supported */
+		return 0;
+
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_MAX:
+		return SPEED_UNKNOWN;
+	}
+}
+
 static void phylink_link_up(struct phylink *pl,
 			    struct phylink_link_state link_state)
 {
 	struct net_device *ndev = pl->netdev;
+	int speed, duplex;
+	bool rx_pause;
+
+	speed = link_state.speed;
+	duplex = link_state.duplex;
+	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
+
+	switch (state->rate_adaption) {
+	case RATE_ADAPT_PAUSE:
+		/* The PHY is doing rate adaption from the media rate (in
+		 * the link_state) to the interface speed, and will send
+		 * pause frames to the MAC to limit its transmission speed.
+		 */
+		speed = phylink_interface_max_speed(link_state.interface);
+		duplex = DUPLEX_FULL;
+		rx_pause = true;
+		break;
+
+	case RATE_ADAPT_CRS:
+		/* The PHY is doing rate adaption from the media rate (in
+		 * the link_state) to the interface speed, and will cause
+		 * collisions to the MAC to limit its transmission speed.
+		 */
+		speed = phylink_interface_max_speed(link_state.interface);
+		duplex = DUPLEX_HALF;
+		break;
+	}
 
 	pl->cur_interface = link_state.interface;
 
 	if (pl->pcs && pl->pcs->ops->pcs_link_up)
 		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
-					 pl->cur_interface,
-					 link_state.speed, link_state.duplex);
+					 pl->cur_interface, speed, duplex);
 
 	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
 				 pl->cur_link_an_mode, pl->cur_interface,
-				 link_state.speed, link_state.duplex,
+				 speed, duplex,
 				 !!(link_state.pause & MLO_PAUSE_TX),
-				 !!(link_state.pause & MLO_PAUSE_RX));
+				 rx_pause);
 
 	if (ndev)
 		netif_carrier_on(ndev);
@@ -1102,6 +1184,17 @@ static void phylink_resolve(struct work_struct *w)
 				}
 				link_state.interface = pl->phy_state.interface;
 
+				/* If we are doing rate adaption, then the
+				 * media speed/duplex has to come from the PHY.
+				 */
+				if (pl->phy_state.rate_adaption) {
+					link_state.rate_adaption =
+						pl->phy_state.rate_adaption;
+					link_state.speed = pl->phy_state.speed;
+					link_state.duplex =
+						pl->phy_state.duplex;
+				}
+
 				/* If we have a PHY, we need to update with
 				 * the PHY flow control bits.
 				 */
@@ -1337,6 +1430,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	pl->phy_state.speed = phydev->speed;
 	pl->phy_state.duplex = phydev->duplex;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
+	pl->phy_state.rate_adaption = phydev->rate_adaption;
 	if (tx_pause)
 		pl->phy_state.pause |= MLO_PAUSE_TX;
 	if (rx_pause)
@@ -1414,6 +1508,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	pl->phy_state.speed = SPEED_UNKNOWN;
 	pl->phy_state.duplex = DUPLEX_UNKNOWN;
+	pl->phy_state.rate_adaption = RATE_ADAPT_NONE;
 	linkmode_copy(pl->supported, supported);
 	linkmode_copy(pl->link_config.advertising, config.advertising);
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..65301e7961b0 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -70,6 +70,7 @@ struct phylink_link_state {
 	int speed;
 	int duplex;
 	int pause;
+	int rate_adaption;
 	unsigned int link:1;
 	unsigned int an_enabled:1;
 	unsigned int an_complete:1;

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

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Sean Anderson]

On 7/20/22 2:32 PM, Russell King (Oracle) wrote:
> On Wed, Jul 20, 2022 at 07:50:52AM +0100, Russell King (Oracle) wrote:
>> We can do that by storing the PHY rate adaption state, and processing
>> that in phylink_link_up().
> 
> Something like this? I haven't included the IPG (open loop) stuff in
> this - I think when we come to implement that, we need a new mac
> method to call to set the IPG just before calling mac_link_up().
> Something like:
> 
>  void mac_set_ipg(struct phylink_config *config, int packet_speed,
> 		  int interface_speed);
> 
> Note that we also have PCS that do rate adaption too, and I think
> fitting those in with the code below may be easier than storing the
> media and phy interface speed/duplex separately.

This is another area where the MAC has to know a lot about the PCS.
We don't keep track of the PCS interface mode, so the MAC has to know
how to connect to the PCS. That could already include some rate
adaptation, but I suspect it is all done like GMII (where the clock
speed changes).

The only drawback I see with this approach is that we don't use the
MAC/PCS's speed/duplex when in in-band mode. But I think that only
matters for things like SGMII, which (as noted below) probably
shouldn't use rate adaptation.

> A few further question though - does rate adaption make sense with
> interface modes that can already natively handle the different speeds,
> such as SGMII, RGMII, USXGMII, etc?

Generally, no. I think it's reasonable to let the phy decide what's
going on, and just do whatever it says.

>  drivers/net/phy/phylink.c | 103 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/phylink.h   |   1 +
>  2 files changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 9bd69328dc4d..c89eb74458cd 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -994,23 +994,105 @@ static const char *phylink_pause_to_str(int pause)
>  	}
>  }
>  
> +static int phylink_interface_max_speed(phy_interface_t interface)
> +{
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_REVRMII:
> +	case PHY_INTERFACE_MODE_RMII:
> +	case PHY_INTERFACE_MODE_SMII:
> +	case PHY_INTERFACE_MODE_REVMII:
> +	case PHY_INTERFACE_MODE_MII:
> +		return SPEED_100;
> +
> +	case PHY_INTERFACE_MODE_TBI:
> +	case PHY_INTERFACE_MODE_MOCA:
> +	case PHY_INTERFACE_MODE_RTBI:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEKX:
> +	case PHY_INTERFACE_MODE_TRGMII:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_GMII:
> +		return SPEED_1000;
> +
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		return SPEED_2500;
> +
> +	case PHY_INTERFACE_MODE_5GBASER:
> +		return SPEED_5000;
> +
> +	case PHY_INTERFACE_MODE_XGMII:
> +	case PHY_INTERFACE_MODE_RXAUI:
> +	case PHY_INTERFACE_MODE_XAUI:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_10GKR:
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		return SPEED_10000;
> +
> +	case PHY_INTERFACE_MODE_25GBASER:
> +		return SPEED_25000;
> +
> +	case PHY_INTERFACE_MODE_XLGMII:
> +		return SPEED_40000;
> +
> +	case PHY_INTERFACE_MODE_INTERNAL:
> +		/* Rate adaption is probably not supported */
> +		return 0;
> +
> +	case PHY_INTERFACE_MODE_NA:
> +	case PHY_INTERFACE_MODE_MAX:
> +		return SPEED_UNKNOWN;
> +	}
> +}
> +
>  static void phylink_link_up(struct phylink *pl,
>  			    struct phylink_link_state link_state)
>  {
>  	struct net_device *ndev = pl->netdev;
> +	int speed, duplex;
> +	bool rx_pause;
> +
> +	speed = link_state.speed;
> +	duplex = link_state.duplex;
> +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
> +
> +	switch (state->rate_adaption) {
> +	case RATE_ADAPT_PAUSE:
> +		/* The PHY is doing rate adaption from the media rate (in
> +		 * the link_state) to the interface speed, and will send
> +		 * pause frames to the MAC to limit its transmission speed.
> +		 */
> +		speed = phylink_interface_max_speed(link_state.interface);
> +		duplex = DUPLEX_FULL;
> +		rx_pause = true;
> +		break;
> +
> +	case RATE_ADAPT_CRS:
> +		/* The PHY is doing rate adaption from the media rate (in
> +		 * the link_state) to the interface speed, and will cause
> +		 * collisions to the MAC to limit its transmission speed.
> +		 */
> +		speed = phylink_interface_max_speed(link_state.interface);
> +		duplex = DUPLEX_HALF;
> +		break;
> +	}
>  
>  	pl->cur_interface = link_state.interface;
>  
>  	if (pl->pcs && pl->pcs->ops->pcs_link_up)
>  		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
> -					 pl->cur_interface,
> -					 link_state.speed, link_state.duplex);
> +					 pl->cur_interface, speed, duplex);
>  
>  	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
>  				 pl->cur_link_an_mode, pl->cur_interface,
> -				 link_state.speed, link_state.duplex,
> +				 speed, duplex,
>  				 !!(link_state.pause & MLO_PAUSE_TX),
> -				 !!(link_state.pause & MLO_PAUSE_RX));
> +				 rx_pause);
>  
>  	if (ndev)
>  		netif_carrier_on(ndev);
> @@ -1102,6 +1184,17 @@ static void phylink_resolve(struct work_struct *w)
>  				}
>  				link_state.interface = pl->phy_state.interface;
>  
> +				/* If we are doing rate adaption, then the
> +				 * media speed/duplex has to come from the PHY.
> +				 */
> +				if (pl->phy_state.rate_adaption) {
> +					link_state.rate_adaption =
> +						pl->phy_state.rate_adaption;
> +					link_state.speed = pl->phy_state.speed;
> +					link_state.duplex =
> +						pl->phy_state.duplex;
> +				}
> +
>  				/* If we have a PHY, we need to update with
>  				 * the PHY flow control bits.
>  				 */
> @@ -1337,6 +1430,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
>  	pl->phy_state.speed = phydev->speed;
>  	pl->phy_state.duplex = phydev->duplex;
>  	pl->phy_state.pause = MLO_PAUSE_NONE;
> +	pl->phy_state.rate_adaption = phydev->rate_adaption;
>  	if (tx_pause)
>  		pl->phy_state.pause |= MLO_PAUSE_TX;
>  	if (rx_pause)
> @@ -1414,6 +1508,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
>  	pl->phy_state.pause = MLO_PAUSE_NONE;
>  	pl->phy_state.speed = SPEED_UNKNOWN;
>  	pl->phy_state.duplex = DUPLEX_UNKNOWN;
> +	pl->phy_state.rate_adaption = RATE_ADAPT_NONE;
>  	linkmode_copy(pl->supported, supported);
>  	linkmode_copy(pl->link_config.advertising, config.advertising);
>  
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 6d06896fc20d..65301e7961b0 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -70,6 +70,7 @@ struct phylink_link_state {
>  	int speed;
>  	int duplex;
>  	int pause;
> +	int rate_adaption;
>  	unsigned int link:1;
>  	unsigned int an_enabled:1;
>  	unsigned int an_complete:1;
> 

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Russell King (Oracle)]

On Thu, Jul 21, 2022 at 05:48:05PM -0400, Sean Anderson wrote:
> On 7/20/22 2:32 PM, Russell King (Oracle) wrote:
> > On Wed, Jul 20, 2022 at 07:50:52AM +0100, Russell King (Oracle) wrote:
> >> We can do that by storing the PHY rate adaption state, and processing
> >> that in phylink_link_up().
> > 
> > Something like this? I haven't included the IPG (open loop) stuff in
> > this - I think when we come to implement that, we need a new mac
> > method to call to set the IPG just before calling mac_link_up().
> > Something like:
> > 
> >  void mac_set_ipg(struct phylink_config *config, int packet_speed,
> > 		  int interface_speed);
> > 
> > Note that we also have PCS that do rate adaption too, and I think
> > fitting those in with the code below may be easier than storing the
> > media and phy interface speed/duplex separately.
> 
> This is another area where the MAC has to know a lot about the PCS.
> We don't keep track of the PCS interface mode, so the MAC has to know
> how to connect to the PCS. That could already include some rate
> adaptation, but I suspect it is all done like GMII (where the clock
> speed changes).

In many cases, we don't even know what the interface used to connect the
PCS to the MAC actually is (we'd have to use something like _INTERNAL).
Particularly when the PCS and MAC are integrated on the same die,
manufacturers tend not to tell people how the two blocks are connected.

Even if we assume did use GMII internally for everything (and I mean
everything), then decoding the GMII interface mode to mean SPEED_1000
won't work for anything over 1G speeds - so we can't do that. The
more I think about it, the less meaning the interface mode between
the PCS and MAC is for our purposes - unless we positively know for
every device what that mode is, and can reliably translate that into
the speed of that connection to derive the correct "speed" for the
MAC.

The point of bringing this up was just to bear it in mind, and I
think when we add support for this, then...

> >  static void phylink_link_up(struct phylink *pl,
> >  			    struct phylink_link_state link_state)
> >  {
> >  	struct net_device *ndev = pl->netdev;
> > +	int speed, duplex;
> > +	bool rx_pause;
> > +
> > +	speed = link_state.speed;
> > +	duplex = link_state.duplex;
> > +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
> > +
> > +	switch (state->rate_adaption) {
> > +	case RATE_ADAPT_PAUSE:
> > +		/* The PHY is doing rate adaption from the media rate (in
> > +		 * the link_state) to the interface speed, and will send
> > +		 * pause frames to the MAC to limit its transmission speed.
> > +		 */
> > +		speed = phylink_interface_max_speed(link_state.interface);
> > +		duplex = DUPLEX_FULL;
> > +		rx_pause = true;
> > +		break;
> > +
> > +	case RATE_ADAPT_CRS:
> > +		/* The PHY is doing rate adaption from the media rate (in
> > +		 * the link_state) to the interface speed, and will cause
> > +		 * collisions to the MAC to limit its transmission speed.
> > +		 */
> > +		speed = phylink_interface_max_speed(link_state.interface);
> > +		duplex = DUPLEX_HALF;
> > +		break;
> > +	}
> >  
> >  	pl->cur_interface = link_state.interface;
> >  
> >  	if (pl->pcs && pl->pcs->ops->pcs_link_up)
> >  		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
> > -					 pl->cur_interface,
> > -					 link_state.speed, link_state.duplex);
> > +					 pl->cur_interface, speed, duplex);
> >  

... we would want to update the speed, duplex and rx_pause parameters
here for the MAC.

> >  	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
> >  				 pl->cur_link_an_mode, pl->cur_interface,
> > -				 link_state.speed, link_state.duplex,
> > +				 speed, duplex,
> >  				 !!(link_state.pause & MLO_PAUSE_TX),
> > -				 !!(link_state.pause & MLO_PAUSE_RX));
> > +				 rx_pause);

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

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[Sean Anderson]

On 7/20/22 2:50 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:57PM -0400, Sean Anderson wrote:
>> If the phy is configured to use pause-based rate adaptation, ensure that
>> the link is full duplex with pause frame reception enabled. As
>> suggested, if pause-based rate adaptation is enabled by the phy, then
>> pause reception is unconditionally enabled.
>> 
>> The interface duplex is determined based on the rate adaptation type.
>> When rate adaptation is enabled, so is the speed. We assume the maximum
>> interface speed is used. This is only relevant for MLO_AN_PHY. For
>> MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v2:
>> - Use the phy's rate adaptation setting to determine whether to use its
>>   link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
>> - Always use the rate adaptation setting to determine the interface
>>   speed/duplex (instead of sometimes using the interface mode).
>> 
>>  drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++----
>>  include/linux/phylink.h   |   1 +
>>  2 files changed, 114 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index da0623d94a64..619ef553476f 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode)
>>   * @state: A link state
>>   *
>>   * Update the .speed and .duplex members of @state. We can determine them based
>> - * on the .link_speed and .link_duplex. This function should be called whenever
>> - * .link_speed and .link_duplex are updated.  For example, userspace deals with
>> - * link speed and duplex, and not the interface speed and duplex. Similarly,
>> - * phys deal with link speed and duplex and only implicitly the interface speed
>> - * and duplex.
>> + * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This
>> + * function should be called whenever .link_speed and .link_duplex are updated.
>> + * For example, userspace deals with link speed and duplex, and not the
>> + * interface speed and duplex. Similarly, phys deal with link speed and duplex
>> + * and only implicitly the interface speed and duplex.
>>   */
>>  static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
>>  {
>> -	state->speed = state->link_speed;
>> -	state->duplex = state->link_duplex;
>> +	switch (state->rate_adaptation) {
>> +	case RATE_ADAPT_NONE:
>> +		state->speed = state->link_speed;
>> +		state->duplex = state->link_duplex;
>> +		return;
>> +	case RATE_ADAPT_PAUSE:
>> +		state->duplex = DUPLEX_FULL;
>> +		break;
>> +	case RATE_ADAPT_CRS:
>> +		state->duplex = DUPLEX_HALF;
>> +		break;
>> +	case RATE_ADAPT_OPEN_LOOP:
>> +		state->duplex = state->link_duplex;
>> +		break;
>> +	}
>> +
>> +	/* Use the max speed of the interface */
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_100BASEX:
>> +	case PHY_INTERFACE_MODE_REVRMII:
>> +	case PHY_INTERFACE_MODE_RMII:
>> +	case PHY_INTERFACE_MODE_SMII:
>> +	case PHY_INTERFACE_MODE_REVMII:
>> +	case PHY_INTERFACE_MODE_MII:
>> +		state->speed = SPEED_100;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_TBI:
>> +	case PHY_INTERFACE_MODE_MOCA:
>> +	case PHY_INTERFACE_MODE_RTBI:
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +	case PHY_INTERFACE_MODE_1000BASEKX:
>> +	case PHY_INTERFACE_MODE_TRGMII:
>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_GMII:
>> +		state->speed = SPEED_1000;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		state->speed = SPEED_2500;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_5GBASER:
>> +		state->speed = SPEED_5000;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_XGMII:
>> +	case PHY_INTERFACE_MODE_RXAUI:
>> +	case PHY_INTERFACE_MODE_XAUI:
>> +	case PHY_INTERFACE_MODE_10GBASER:
>> +	case PHY_INTERFACE_MODE_10GKR:
>> +	case PHY_INTERFACE_MODE_USXGMII:
>> +		state->speed = SPEED_10000;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_25GBASER:
>> +		state->speed = SPEED_25000;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_XLGMII:
>> +		state->speed = SPEED_40000;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_INTERNAL:
>> +		state->speed = state->link_speed;
>> +		return;
>> +
>> +	case PHY_INTERFACE_MODE_NA:
>> +	case PHY_INTERFACE_MODE_MAX:
>> +		state->speed = SPEED_UNKNOWN;
>> +		return;
>> +	}
>> +
>> +	WARN_ON(1);
>>  }
>>  
>>  /**
>> @@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl,
>>  			       const struct phylink_link_state *state)
>>  {
>>  	phylink_dbg(pl,
>> -		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>> +		    "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>>  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
>>  		    phy_modes(state->interface),
>>  		    phy_speed_to_str(state->speed),
>>  		    phy_duplex_to_str(state->duplex),
>> +		    phy_rate_adaptation_to_str(state->rate_adaptation),
>>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>>  		    state->pause, state->link, state->an_enabled);
>>  
>> @@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>>  	linkmode_zero(state->lp_advertising);
>>  	state->interface = pl->link_config.interface;
>>  	state->an_enabled = pl->link_config.an_enabled;
>> +	state->rate_adaptation = pl->link_config.rate_adaptation;
>>  	if (state->an_enabled) {
>>  		state->link_speed = SPEED_UNKNOWN;
>>  		state->link_duplex = DUPLEX_UNKNOWN;
>> @@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>>  	else
>>  		state->link = 0;
>>  
>> -	state->link_speed = state->speed;
>> -	state->link_duplex = state->duplex;
>> +	if (state->rate_adaptation == RATE_ADAPT_NONE) {
>> +		state->link_speed = state->speed;
>> +		state->link_duplex = state->duplex;
>> +	}
> 
> So we need to have every PCS driver be udpated to fill in link_speed
> and link_duplex if rate_adaption != none.

The PCS doesn't know what the link speed/duplex is. If rate adaptation is
enabled, then the PCS only knows what the interface speed/duplex is.

> There's got to be a better way - maybe what I suggested in the last
> round of only doing the rate adaption thing in the link_up() functions,
> since that seems to be the only real difference.
> 
> I'm not even sure we need to do that - in the "open loop" case, we
> need to be passing the media speed to the MAC driver with the knowledge
> that it should be increasing the IPG.
> 
> So, I'm thinking we don't want any of these changes, what we instead
> should be doing is passing the media speed/duplex and the interface
> speed/duplex to the PCS and MAC.

> We can do that by storing the PHY rate adaption state, and processing
> that in phylink_link_up().

This approach sounds better. You patch below looks good. I'll test it
and use it for v3.

--Sean

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[kernel test robot]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master horms-ipvs/master linus/master v5.19-rc7 next-20220719]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/net-phy-Add-support-for-rate-adaptation/20220720-075438
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1f17708b47a99ca5bcad594a6f8d14cb016edfd2
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220720/202207201640.6pB23GJN-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project dd5635541cd7bbd62cd59b6694dfb759b6e9a0d8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a17fd2b01914c1c5779a76167def6910a6dd1185
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sean-Anderson/net-phy-Add-support-for-rate-adaptation/20220720-075438
        git checkout a17fd2b01914c1c5779a76167def6910a6dd1185
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "phy_rate_adaptation_to_str" [drivers/net/phy/phylink.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation

[kernel test robot]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master horms-ipvs/master linus/master v5.19-rc7 next-20220719]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/net-phy-Add-support-for-rate-adaptation/20220720-075438
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1f17708b47a99ca5bcad594a6f8d14cb016edfd2
config: i386-randconfig-a005 (https://download.01.org/0day-ci/archive/20220720/202207201727.9nqTCybA-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/a17fd2b01914c1c5779a76167def6910a6dd1185
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sean-Anderson/net-phy-Add-support-for-rate-adaptation/20220720-075438
        git checkout a17fd2b01914c1c5779a76167def6910a6dd1185
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "phy_rate_adaptation_to_str" [drivers/net/phy/phylink.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Sean Anderson]

This adds support for adjusting the advertisement for pause-based rate
adaptation. This may result in a lossy link, since the final link settings
are not adjusted. Asymmetric pause support is necessary. It would be
possible for a MAC supporting only symmetric pause to use pause-based rate
adaptation, but only if pause reception was enabled as well.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Determine the interface speed and max mac speed directly instead of
  guessing based on the caps.

 drivers/net/phy/phylink.c | 87 +++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |  5 ++-
 2 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 619ef553476f..f61040c93f3c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -398,18 +398,65 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 }
 EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
+static int phylink_caps_to_speed(unsigned long caps)
+{
+	unsigned int max_cap = __fls(caps);
+
+	if (max_cap == __fls(MAC_10HD) || max_cap == __fls(MAC_10FD))
+		return SPEED_10;
+	if (max_cap == __fls(MAC_100HD) || max_cap == __fls(MAC_100FD))
+		return SPEED_100;
+	if (max_cap == __fls(MAC_1000HD) || max_cap == __fls(MAC_1000FD))
+		return SPEED_1000;
+	if (max_cap == __fls(MAC_2500FD))
+		return SPEED_2500;
+	if (max_cap == __fls(MAC_5000FD))
+		return SPEED_5000;
+	if (max_cap == __fls(MAC_10000FD))
+		return SPEED_10000;
+	if (max_cap == __fls(MAC_20000FD))
+		return SPEED_20000;
+	if (max_cap == __fls(MAC_25000FD))
+		return SPEED_25000;
+	if (max_cap == __fls(MAC_40000FD))
+		return SPEED_40000;
+	if (max_cap == __fls(MAC_50000FD))
+		return SPEED_50000;
+	if (max_cap == __fls(MAC_56000FD))
+		return SPEED_56000;
+	if (max_cap == __fls(MAC_100000FD))
+		return SPEED_100000;
+	if (max_cap == __fls(MAC_200000FD))
+		return SPEED_200000;
+	if (max_cap == __fls(MAC_400000FD))
+		return SPEED_400000;
+	return SPEED_UNKNOWN;
+}
+
 /**
  * phylink_get_capabilities() - get capabilities for a given MAC
  * @interface: phy interface mode defined by &typedef phy_interface_t
  * @mac_capabilities: bitmask of MAC capabilities
+ * @rate_adaptation: type of rate adaptation being performed
  *
  * Get the MAC capabilities that are supported by the @interface mode and
  * @mac_capabilities.
  */
 unsigned long phylink_get_capabilities(phy_interface_t interface,
-				       unsigned long mac_capabilities)
+				       unsigned long mac_capabilities,
+				       int rate_adaptation)
 {
 	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+	unsigned long adapted_caps = 0;
+	struct phylink_link_state state = {
+		.interface = interface,
+		.rate_adaptation = rate_adaptation,
+		.link_speed = SPEED_UNKNOWN,
+		.link_duplex = DUPLEX_UNKNOWN,
+	};
+
+	/* Look up the interface speed */
+	phylink_state_fill_speed_duplex(&state);
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_USXGMII:
@@ -482,7 +529,39 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
 		break;
 	}
 
-	return caps & mac_capabilities;
+	switch (rate_adaptation) {
+	case RATE_ADAPT_NONE:
+		break;
+	case RATE_ADAPT_PAUSE: {
+		/* The MAC must support asymmetric pause towards the local
+		 * device for this. We could allow just symmetric pause, but
+		 * then we might have to renegotiate if the link partner
+		 * doesn't support pause.
+		 */
+		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
+		    !(mac_capabilities & MAC_ASYM_PAUSE))
+			break;
+
+		/* Can't adapt if the MAC doesn't support the interface's max
+		 * speed
+		 */
+		if (state.speed != phylink_caps_to_speed(mac_capabilities))
+			break;
+
+		adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
+		/* We can't use pause frames in half-duplex mode */
+		adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);
+		break;
+	}
+	case RATE_ADAPT_CRS:
+		/* TODO */
+		break;
+	case RATE_ADAPT_OPEN_LOOP:
+		/* TODO */
+		break;
+	}
+
+	return (caps & mac_capabilities) | adapted_caps;
 }
 EXPORT_SYMBOL_GPL(phylink_get_capabilities);
 
@@ -506,7 +585,8 @@ void phylink_generic_validate(struct phylink_config *config,
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	caps = phylink_get_capabilities(state->interface,
-					config->mac_capabilities);
+					config->mac_capabilities,
+					state->rate_adaptation);
 	phylink_caps_to_linkmodes(mask, caps);
 
 	linkmode_and(supported, supported, mask);
@@ -1529,6 +1609,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		config.interface = PHY_INTERFACE_MODE_NA;
 	else
 		config.interface = interface;
+	config.rate_adaptation = phy_get_rate_adaptation(phy, config.interface);
 
 	ret = phylink_validate(pl, supported, &config);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index f32b97f27ddc..73e1aa73e30f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -70,6 +70,8 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
  * @link: true if the link is up.
  * @an_enabled: true if autonegotiation is enabled/desired.
  * @an_complete: true if autonegotiation has completed.
+ * @rate_adaptation: method of throttling @interface_speed to @speed, one of
+ *   RATE_ADAPT_* constants.
  */
 struct phylink_link_state {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
@@ -531,7 +533,8 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 
 void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
 unsigned long phylink_get_capabilities(phy_interface_t interface,
-				       unsigned long mac_capabilities);
+				       unsigned long mac_capabilities,
+				       int rate_adaptation);
 void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Russell King (Oracle)]

On Tue, Jul 19, 2022 at 07:49:58PM -0400, Sean Anderson wrote:
> +static int phylink_caps_to_speed(unsigned long caps)
> +{
> +	unsigned int max_cap = __fls(caps);
> +
> +	if (max_cap == __fls(MAC_10HD) || max_cap == __fls(MAC_10FD))
> +		return SPEED_10;
> +	if (max_cap == __fls(MAC_100HD) || max_cap == __fls(MAC_100FD))
> +		return SPEED_100;
> +	if (max_cap == __fls(MAC_1000HD) || max_cap == __fls(MAC_1000FD))
> +		return SPEED_1000;
> +	if (max_cap == __fls(MAC_2500FD))
> +		return SPEED_2500;
> +	if (max_cap == __fls(MAC_5000FD))
> +		return SPEED_5000;
> +	if (max_cap == __fls(MAC_10000FD))
> +		return SPEED_10000;
> +	if (max_cap == __fls(MAC_20000FD))
> +		return SPEED_20000;
> +	if (max_cap == __fls(MAC_25000FD))
> +		return SPEED_25000;
> +	if (max_cap == __fls(MAC_40000FD))
> +		return SPEED_40000;
> +	if (max_cap == __fls(MAC_50000FD))
> +		return SPEED_50000;
> +	if (max_cap == __fls(MAC_56000FD))
> +		return SPEED_56000;
> +	if (max_cap == __fls(MAC_100000FD))
> +		return SPEED_100000;
> +	if (max_cap == __fls(MAC_200000FD))
> +		return SPEED_200000;
> +	if (max_cap == __fls(MAC_400000FD))
> +		return SPEED_400000;
> +	return SPEED_UNKNOWN;
> +}

One of my recent patches introduced "phylink_caps_params" table into
the DSA code (which isn't merged) but it's about converting the caps
into the SPEED_* and DUPLEX_*. This is doing more or less the same
7thing but with a priority for speed rather than duplex. The question
about whether it should be this way for the DSA case or whether speed
should take priority was totally ignored by all reviewers of the code
despite being explicitly asked.

Maybe this could be reused here rather than having similar code.

> @@ -482,7 +529,39 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
>  		break;
>  	}
>  
> -	return caps & mac_capabilities;
> +	switch (rate_adaptation) {
> +	case RATE_ADAPT_NONE:
> +		break;
> +	case RATE_ADAPT_PAUSE: {
> +		/* The MAC must support asymmetric pause towards the local
> +		 * device for this. We could allow just symmetric pause, but
> +		 * then we might have to renegotiate if the link partner
> +		 * doesn't support pause.

Why do we need to renegotiate, and what would this achieve? The link
partner isn't going to say "oh yes I do support pause after all",
and in any case this function is working out what the capabilities
of the system is prior to bringing anything up.

All that we need to know here is whether the MAC supports receiving
pause frames from the PHY - if it doesn't, then the MAC is
incompatible with the PHY using rate adaption.

> +		 */
> +		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
> +		    !(mac_capabilities & MAC_ASYM_PAUSE))
> +			break;
> +
> +		/* Can't adapt if the MAC doesn't support the interface's max
> +		 * speed
> +		 */
> +		if (state.speed != phylink_caps_to_speed(mac_capabilities))
> +			break;

I'm not sure this is the right way to check. If the MAC supports e.g.
10G, 1G, 100M and 10M, but we have a PHY operating in 1000base-X mode
to the PCS/MAC and is using rate adaption, then phylink_caps_to_speed()
will return 10G, but state.speed will be 1G.

Don't we instead want to check whether the MAC capabilities has the FD
bit corresponding to state.speed set?

> +
> +		adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
> +		/* We can't use pause frames in half-duplex mode */
> +		adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);

Have you checked the PHY documentation to see what the behaviour is
in rate adaption mode with pause frames and it negotiates HD on the
media side? Does it handle the HD issue internally?

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

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Sean Anderson]

On 7/20/22 3:08 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:58PM -0400, Sean Anderson wrote:
>> +static int phylink_caps_to_speed(unsigned long caps)
>> +{
>> +	unsigned int max_cap = __fls(caps);
>> +
>> +	if (max_cap == __fls(MAC_10HD) || max_cap == __fls(MAC_10FD))
>> +		return SPEED_10;
>> +	if (max_cap == __fls(MAC_100HD) || max_cap == __fls(MAC_100FD))
>> +		return SPEED_100;
>> +	if (max_cap == __fls(MAC_1000HD) || max_cap == __fls(MAC_1000FD))
>> +		return SPEED_1000;
>> +	if (max_cap == __fls(MAC_2500FD))
>> +		return SPEED_2500;
>> +	if (max_cap == __fls(MAC_5000FD))
>> +		return SPEED_5000;
>> +	if (max_cap == __fls(MAC_10000FD))
>> +		return SPEED_10000;
>> +	if (max_cap == __fls(MAC_20000FD))
>> +		return SPEED_20000;
>> +	if (max_cap == __fls(MAC_25000FD))
>> +		return SPEED_25000;
>> +	if (max_cap == __fls(MAC_40000FD))
>> +		return SPEED_40000;
>> +	if (max_cap == __fls(MAC_50000FD))
>> +		return SPEED_50000;
>> +	if (max_cap == __fls(MAC_56000FD))
>> +		return SPEED_56000;
>> +	if (max_cap == __fls(MAC_100000FD))
>> +		return SPEED_100000;
>> +	if (max_cap == __fls(MAC_200000FD))
>> +		return SPEED_200000;
>> +	if (max_cap == __fls(MAC_400000FD))
>> +		return SPEED_400000;
>> +	return SPEED_UNKNOWN;
>> +}
> 
> One of my recent patches introduced "phylink_caps_params" table into
> the DSA code (which isn't merged) but it's about converting the caps
> into the SPEED_* and DUPLEX_*. This is doing more or less the same
> 7thing but with a priority for speed rather than duplex. The question
> about whether it should be this way for the DSA case or whether speed
> should take priority was totally ignored by all reviewers of the code
> despite being explicitly asked.
> 
> Maybe this could be reused here rather than having similar code.

I'm in favor of that.

>> @@ -482,7 +529,39 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
>>  		break;
>>  	}
>>  
>> -	return caps & mac_capabilities;
>> +	switch (rate_adaptation) {
>> +	case RATE_ADAPT_NONE:
>> +		break;
>> +	case RATE_ADAPT_PAUSE: {
>> +		/* The MAC must support asymmetric pause towards the local
>> +		 * device for this. We could allow just symmetric pause, but
>> +		 * then we might have to renegotiate if the link partner
>> +		 * doesn't support pause.
> 
> Why do we need to renegotiate, and what would this achieve? The link
> partner isn't going to say "oh yes I do support pause after all",
> and in any case this function is working out what the capabilities
> of the system is prior to bringing anything up.
> 
> All that we need to know here is whether the MAC supports receiving
> pause frames from the PHY - if it doesn't, then the MAC is
> incompatible with the PHY using rate adaption.

AIUI, MAC_SYM_PAUSE and MAC_ASYM_PAUSE correspond to the PAUSE and
ASM_DIR bits used in autonegotiation. For reference, Table 28B-2 from
802.3 is:

PAUSE (A5) ASM_DIR (A6) Capability
========== ============ ================================================
         0            0 No PAUSE
         0            1 Asymmetric PAUSE toward link partner
         1            0 Symmetric PAUSE
	 1            1 Both Symmetric PAUSE and Asymmetric PAUSE toward
                        local device

These correspond to the following valid values for MLO_PAUSE:

MAC_SYM_PAUSE MAC_ASYM_PAUSE Valid pause modes
============= ============== ==============================
            0              0 MLO_PAUSE_NONE
            0              1 MLO_PAUSE_NONE, MLO_PAUSE_TX
            1              0 MLO_PAUSE_NONE, MLO_PAUSE_TXRX
	    1              1 MLO_PAUSE_NONE, MLO_PAUSE_RX,
                             MLO_PAUSE_TXRX

In order to support pause-based rate adaptation, we need MLO_PAUSE_RX to
be valid. This rules out the top two rows. In the bottom mode, we can
enable MLO_PAUSE_RX without MLO_PAUSE_TX. Whatever our link partner
supports, we can still enable it. For the third row, however, we can
only enable MLO_PAUSE_RX if we also enable MLO_PAUSE_TX. This can be a
problem if the link partner does not support pause frames (or the user
has disabled MLO_PAUSE_AN and MLO_PAUSE_TX). So if we were to enable
advertisement of pause-based, rate-adapted modes when only MAC_SYM_PAUSE
was present, then we might end up in a situation where we'd have to
renegotiate without those modes in order to get a valid link state. I
don't want to have to implement that, so for now we only advertise
pause-based, rate-adapted modes if we support MLO_PAUSE_RX without
MLO_PAUSE_TX.

>> +		 */
>> +		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
>> +		    !(mac_capabilities & MAC_ASYM_PAUSE))
>> +			break;
>> +
>> +		/* Can't adapt if the MAC doesn't support the interface's max
>> +		 * speed
>> +		 */
>> +		if (state.speed != phylink_caps_to_speed(mac_capabilities))
>> +			break;
> 
> I'm not sure this is the right way to check. If the MAC supports e.g.
> 10G, 1G, 100M and 10M, but we have a PHY operating in 1000base-X mode
> to the PCS/MAC and is using rate adaption, then phylink_caps_to_speed()
> will return 10G, but state.speed will be 1G.
> 
> Don't we instead want to check whether the MAC capabilities has the FD
> bit corresponding to state.speed set?

Yes, that seems correct.

>> +
>> +		adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
>> +		/* We can't use pause frames in half-duplex mode */
>> +		adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);
> 
> Have you checked the PHY documentation to see what the behaviour is
> in rate adaption mode with pause frames and it negotiates HD on the
> media side? Does it handle the HD issue internally?

It's not documented. This is just conservative. Presumably, there exists
(or could exist) a duplex-adapting phy, but I don't know if I have one.

--Sean

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Russell King (Oracle)]

On Thu, Jul 21, 2022 at 12:55:16PM -0400, Sean Anderson wrote:
> On 7/20/22 3:08 AM, Russell King (Oracle) wrote:
> > On Tue, Jul 19, 2022 at 07:49:58PM -0400, Sean Anderson wrote:
> >> @@ -482,7 +529,39 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
> >>  		break;
> >>  	}
> >>  
> >> -	return caps & mac_capabilities;
> >> +	switch (rate_adaptation) {
> >> +	case RATE_ADAPT_NONE:
> >> +		break;
> >> +	case RATE_ADAPT_PAUSE: {
> >> +		/* The MAC must support asymmetric pause towards the local
> >> +		 * device for this. We could allow just symmetric pause, but
> >> +		 * then we might have to renegotiate if the link partner
> >> +		 * doesn't support pause.
> > 
> > Why do we need to renegotiate, and what would this achieve? The link
> > partner isn't going to say "oh yes I do support pause after all",
> > and in any case this function is working out what the capabilities
> > of the system is prior to bringing anything up.
> > 
> > All that we need to know here is whether the MAC supports receiving
> > pause frames from the PHY - if it doesn't, then the MAC is
> > incompatible with the PHY using rate adaption.
> 
> AIUI, MAC_SYM_PAUSE and MAC_ASYM_PAUSE correspond to the PAUSE and
> ASM_DIR bits used in autonegotiation. For reference, Table 28B-2 from
> 802.3 is:
> 
> PAUSE (A5) ASM_DIR (A6) Capability
> ========== ============ ================================================
>          0            0 No PAUSE
>          0            1 Asymmetric PAUSE toward link partner
>          1            0 Symmetric PAUSE
> 	 1            1 Both Symmetric PAUSE and Asymmetric PAUSE toward
>                         local device
> 
> These correspond to the following valid values for MLO_PAUSE:
> 
> MAC_SYM_PAUSE MAC_ASYM_PAUSE Valid pause modes
> ============= ============== ==============================
>             0              0 MLO_PAUSE_NONE
>             0              1 MLO_PAUSE_NONE, MLO_PAUSE_TX
>             1              0 MLO_PAUSE_NONE, MLO_PAUSE_TXRX
> 	    1              1 MLO_PAUSE_NONE, MLO_PAUSE_RX,
>                              MLO_PAUSE_TXRX
> 
> In order to support pause-based rate adaptation, we need MLO_PAUSE_RX to
> be valid. This rules out the top two rows. In the bottom mode, we can
> enable MLO_PAUSE_RX without MLO_PAUSE_TX. Whatever our link partner
> supports, we can still enable it. For the third row, however, we can
> only enable MLO_PAUSE_RX if we also enable MLO_PAUSE_TX. This can be a
> problem if the link partner does not support pause frames (or the user
> has disabled MLO_PAUSE_AN and MLO_PAUSE_TX). So if we were to enable
> advertisement of pause-based, rate-adapted modes when only MAC_SYM_PAUSE
> was present, then we might end up in a situation where we'd have to
> renegotiate without those modes in order to get a valid link state. I
> don't want to have to implement that, so for now we only advertise
> pause-based, rate-adapted modes if we support MLO_PAUSE_RX without
> MLO_PAUSE_TX.

Ah, I see. Yes, I agree that we shouldn't do that, and only allow rate
adaption in pause mode to be used if we can enable RX pause without TX
pause on our local MAC.

> > Have you checked the PHY documentation to see what the behaviour is
> > in rate adaption mode with pause frames and it negotiates HD on the
> > media side? Does it handle the HD issue internally?
> 
> It's not documented. This is just conservative. Presumably, there exists
> (or could exist) a duplex-adapting phy, but I don't know if I have one.

I guess it would depend on the structure of the PHY - whether the PHY
is structured similar to a two port switch internally, having a MAC
facing the host and another MAC facing the media side. (I believe this
is exactly how the MACSEC versions of the 88x3310 are structured.)

If you don't have that kind of structure, then I would guess that doing
duplex adaption could be problematical.

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

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Andrew Lunn]

> I guess it would depend on the structure of the PHY - whether the PHY
> is structured similar to a two port switch internally, having a MAC
> facing the host and another MAC facing the media side. (I believe this
> is exactly how the MACSEC versions of the 88x3310 are structured.)
> 
> If you don't have that kind of structure, then I would guess that doing
> duplex adaption could be problematical.

If you don't have that sort of structure, i think rate adaptation
would have problems in general. Pause is not very fine grained. You
need to somehow buffer packets because what comes from the MAC is
likely to be bursty. And when that buffer overflows, you want to be
selective about what you throw away. You want ARP, OSPF and other
signalling packets to have priority, and user data gets
tossed. Otherwise your network collapses.

	Andrew

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Dave Taht]

On Thu, Jul 21, 2022 at 11:42 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I guess it would depend on the structure of the PHY - whether the PHY
> > is structured similar to a two port switch internally, having a MAC
> > facing the host and another MAC facing the media side. (I believe this
> > is exactly how the MACSEC versions of the 88x3310 are structured.)
> >
> > If you don't have that kind of structure, then I would guess that doing
> > duplex adaption could be problematical.
>
> If you don't have that sort of structure, i think rate adaptation
> would have problems in general. Pause is not very fine grained. You
> need to somehow buffer packets because what comes from the MAC is
> likely to be bursty. And when that buffer overflows, you want to be
> selective about what you throw away. You want ARP, OSPF and other
> signalling packets to have priority, and user data gets
> tossed. Otherwise your network collapses.

Most of our protocols (like arp) are tolerant of some loss and taking
extraordinary measures to preserve "signalling" packets shouldn't be
necessary.

That said, how much buffering can exist here? How much latency between
emission, receipt, and response to a pause will there be?

>
>         Andrew



-- 
FQ World Domination pending: https://blog.cerowrt.org/post/state_of_fq_codel/
Dave Täht CEO, TekLibre, LLC

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Sean Anderson]

On 7/21/22 2:36 PM, Andrew Lunn wrote:
>> I guess it would depend on the structure of the PHY - whether the PHY
>> is structured similar to a two port switch internally, having a MAC
>> facing the host and another MAC facing the media side. (I believe this
>> is exactly how the MACSEC versions of the 88x3310 are structured.)
>> 
>> If you don't have that kind of structure, then I would guess that doing
>> duplex adaption could be problematical.
> 
> If you don't have that sort of structure, i think rate adaptation
> would have problems in general. Pause is not very fine grained. You
> need to somehow buffer packets because what comes from the MAC is
> likely to be bursty. And when that buffer overflows, you want to be
> selective about what you throw away. You want ARP, OSPF and other
> signalling packets to have priority, and user data gets
> tossed. Otherwise your network collapses.

I performed some experiments using iperf to attempt to determine how
things worked. On one end, I had a LS1046ARDB with an AQR113C connected
via 10GBASE-R at address 10.0.0.1. On the other end, I had a custom
board supporting 100BASE-TX at address 10.0.0.2. I ran tests in both
directions at once. In a regular link (both sides using MII), I would
expect around 90Mbit/sec in both directions for full duplex, and around
40Mbit/s in both directions for half duplex (or less?). I would also
expect no retries (since collisions should be handled by the MAC and not
TCP).

Direction Duplex   Interval            Transfer      Bandwidth       Write/Err  Retries  
========= ======== =================== ============= =============== ========== =======
1->2      Full     0.0000-10.0185 sec   111 MBytes    92.8 Mbits/sec      888/0       0
2->1      Full     0.0000-10.0865 sec    99.1 MBytes  82.4 Mbits/sec      794/0       0
1->2      Half     0.0000-10.0098 sec    36.6 MBytes  30.7 Mbits/sec      294/0    1241
2->1      Half     0.0000-10.1764 sec    1.13 MBytes 927 Kbits/sec         10/0     155

From the second line, it appears like the 100BASE-TX device wasn't able
to saturate the link. I'm not sure why that is (it doesn't match
earlier test results I had), but it is reproducable with other iperf
servers (so I don't think it's due to the rate adaptation).

For the second two lines, we can see that the rate adapting sender is
much faster, and has many more retries. This suggests to me that the
rate adapting phy is not performing collision avoidance, causing high
packet loss and starving the 100BASE-TX device.

Clearly half duplex data transfer works, but I don't know if it is
functioning properly.

--Sean

Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Russell King (Oracle)]

On Thu, Jul 21, 2022 at 08:36:03PM +0200, Andrew Lunn wrote:
> > I guess it would depend on the structure of the PHY - whether the PHY
> > is structured similar to a two port switch internally, having a MAC
> > facing the host and another MAC facing the media side. (I believe this
> > is exactly how the MACSEC versions of the 88x3310 are structured.)
> > 
> > If you don't have that kind of structure, then I would guess that doing
> > duplex adaption could be problematical.
> 
> If you don't have that sort of structure, i think rate adaptation
> would have problems in general. Pause is not very fine grained. You
> need to somehow buffer packets because what comes from the MAC is
> likely to be bursty. And when that buffer overflows, you want to be
> selective about what you throw away. You want ARP, OSPF and other
> signalling packets to have priority, and user data gets
> tossed. Otherwise your network collapses.

I don't think rate adaption is that inteligent - it's all about slowing
the MAC down to the speed of the media. From what I remember looking at
pause frames, they can specify how long to delay further transmission
by the receiver, and I would expect this to be set according to the
media speed for setups that use pause packets.

For those which don't, then that's a whole different ball game, because
they tend not to have MACs, and then you're probably down to the
capabilities of nothing more than a FIFO in the PHY.

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

[PATCH v2 09/11] [RFC] net: phylink: Add support for CRS-based rate adaptation

[Sean Anderson]

This adds support for CRS-based rate adaptation, such as the type used
for 10PASS-TS and 2BASE-TL. As these link modes are not supported by any
in-tree phy, this patch is marked as RFC. It serves chiefly to
illustrate the approach to adding support for another rate adaptation
type.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f61040c93f3c..75b4994d68c8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -553,9 +553,18 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
 		adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);
 		break;
 	}
-	case RATE_ADAPT_CRS:
-		/* TODO */
+	case RATE_ADAPT_CRS: {
+		/* The MAC must support half duplex at the interface speed */
+		if (state.speed == SPEED_1000) {
+			if (mac_capabilities & MAC_1000HD)
+				adapted_caps = MAC_100 | MAC_10;
+		} else if (state.speed == SPEED_1000) {
+			if (mac_capabilities & MAC_100HD)
+				adapted_caps = MAC_10;
+		}
+		adapted_caps &= mac_capabilities;
 		break;
+	}
 	case RATE_ADAPT_OPEN_LOOP:
 		/* TODO */
 		break;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 10/11] net: phy: aquantia: Add some additional phy interfaces

[Sean Anderson]

These are documented in the AQR115 register reference. I haven't tested
them, but perhaps they'll be useful to someone.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---

(no changes since v1)

 drivers/net/phy/aquantia_main.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 8b7a46db30e0..1e7036945a4e 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -27,9 +27,12 @@
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KX	1
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XFI	2
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_USXGMII	3
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI	4
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
 
 #define MDIO_AN_VEND_PROV			0xc400
@@ -91,6 +94,19 @@
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
 #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
 
+/* The following registers all have similar layouts; first the registers... */
+#define VEND1_GLOBAL_CFG_10M			0x0310
+#define VEND1_GLOBAL_CFG_100M			0x031b
+#define VEND1_GLOBAL_CFG_1G			0x031c
+#define VEND1_GLOBAL_CFG_2_5G			0x031d
+#define VEND1_GLOBAL_CFG_5G			0x031e
+#define VEND1_GLOBAL_CFG_10G			0x031f
+/* ...and now the fields */
+#define VEND1_GLOBAL_CFG_RATE_ADAPT		GENMASK(8, 7)
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
+
 #define VEND1_GLOBAL_RSVD_STAT1			0xc885
 #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
 #define VEND1_GLOBAL_RSVD_STAT1_PROV_ID		GENMASK(3, 0)
@@ -335,6 +351,7 @@ static int aqr_read_status(struct phy_device *phydev)
 
 static int aqr107_read_rate(struct phy_device *phydev)
 {
+	u32 config_reg;
 	int val;
 
 	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
@@ -392,15 +409,24 @@ static int aqr107_read_status(struct phy_device *phydev)
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR:
 		phydev->interface = PHY_INTERFACE_MODE_10GKR;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_KX:
+		phydev->interface = PHY_INTERFACE_MODE_1000BASEKX;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_XFI:
 		phydev->interface = PHY_INTERFACE_MODE_10GBASER;
 		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_USXGMII:
 		phydev->interface = PHY_INTERFACE_MODE_USXGMII;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI:
+		phydev->interface = PHY_INTERFACE_MODE_XAUI;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII:
 		phydev->interface = PHY_INTERFACE_MODE_SGMII;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI:
+		phydev->interface = PHY_INTERFACE_MODE_RXAUI;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII:
 		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
 		break;
@@ -513,11 +539,14 @@ static int aqr107_config_init(struct phy_device *phydev)
 
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phydev->interface != PHY_INTERFACE_MODE_1000BASEKX &&
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII &&
 	    phydev->interface != PHY_INTERFACE_MODE_10GKR &&
-	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
+	    phydev->interface != PHY_INTERFACE_MODE_10GBASER &&
+	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
+	    phydev->interface != PHY_INTERFACE_MODE_RXAUI)
 		return -ENODEV;
 
 	WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII,
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 10/11] net: phy: aquantia: Add some additional phy interfaces

[Russell King (Oracle)]

On Tue, Jul 19, 2022 at 07:50:00PM -0400, Sean Anderson wrote:
> +/* The following registers all have similar layouts; first the registers... */
> +#define VEND1_GLOBAL_CFG_10M			0x0310
> +#define VEND1_GLOBAL_CFG_100M			0x031b
> +#define VEND1_GLOBAL_CFG_1G			0x031c
> +#define VEND1_GLOBAL_CFG_2_5G			0x031d
> +#define VEND1_GLOBAL_CFG_5G			0x031e
> +#define VEND1_GLOBAL_CFG_10G			0x031f
> +/* ...and now the fields */
> +#define VEND1_GLOBAL_CFG_RATE_ADAPT		GENMASK(8, 7)
> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
> +

Shouldn't these definitions be in patch 11? They don't appear to be used
in this patch.

Thanks.

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

Re: [PATCH v2 10/11] net: phy: aquantia: Add some additional phy interfaces

[Sean Anderson]

On 7/20/22 7:35 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:50:00PM -0400, Sean Anderson wrote:
>> +/* The following registers all have similar layouts; first the registers... */
>> +#define VEND1_GLOBAL_CFG_10M			0x0310
>> +#define VEND1_GLOBAL_CFG_100M			0x031b
>> +#define VEND1_GLOBAL_CFG_1G			0x031c
>> +#define VEND1_GLOBAL_CFG_2_5G			0x031d
>> +#define VEND1_GLOBAL_CFG_5G			0x031e
>> +#define VEND1_GLOBAL_CFG_10G			0x031f
>> +/* ...and now the fields */
>> +#define VEND1_GLOBAL_CFG_RATE_ADAPT		GENMASK(8, 7)
>> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
>> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
>> +#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
>> +
> 
> Shouldn't these definitions be in patch 11? They don't appear to be used
> in this patch.

You're right. It looks like I added these too early.

--Sean

[PATCH v2 11/11] net: phy: aquantia: Add support for rate adaptation

[Sean Anderson]

This adds support for rate adaptation for phys similar to the AQR107. We
assume that all phys using aqr107_read_status support rate adaptation.
However, it could be possible to determine support based on the firmware
revision if there are phys discovered which do not support rate adaptation.
However, as rate adaptation is advertised in the datasheets for these phys,
I suspect it is supported most boards.

Despite the name, the "config" registers are updated with the current rate
adaptation method (if any). Because they appear to be updated
automatically, I don't know if these registers can be used to disable rate
adaptation.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Add comments clarifying the register defines
- Reorder variables in aqr107_read_rate

 drivers/net/phy/aquantia_main.c | 37 +++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 1e7036945a4e..dd73891cf68a 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -358,34 +358,50 @@ static int aqr107_read_rate(struct phy_device *phydev)
 	if (val < 0)
 		return val;
 
+	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
 	switch (FIELD_GET(MDIO_AN_TX_VEND_STATUS1_RATE_MASK, val)) {
 	case MDIO_AN_TX_VEND_STATUS1_10BASET:
 		phydev->speed = SPEED_10;
+		config_reg = VEND1_GLOBAL_CFG_10M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_100BASETX:
 		phydev->speed = SPEED_100;
+		config_reg = VEND1_GLOBAL_CFG_100M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_1000BASET:
 		phydev->speed = SPEED_1000;
+		config_reg = VEND1_GLOBAL_CFG_1G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_2500BASET:
 		phydev->speed = SPEED_2500;
+		config_reg = VEND1_GLOBAL_CFG_2_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_5000BASET:
 		phydev->speed = SPEED_5000;
+		config_reg = VEND1_GLOBAL_CFG_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_10GBASET:
 		phydev->speed = SPEED_10000;
+		config_reg = VEND1_GLOBAL_CFG_10G;
 		break;
 	default:
 		phydev->speed = SPEED_UNKNOWN;
-		break;
+		return 0;
 	}
 
-	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
-		phydev->duplex = DUPLEX_FULL;
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, config_reg);
+	if (val < 0)
+		return val;
+
+	if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
+	    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+		phydev->rate_adaptation = RATE_ADAPT_PAUSE;
 	else
-		phydev->duplex = DUPLEX_HALF;
+		phydev->rate_adaptation = RATE_ADAPT_NONE;
 
 	return 0;
 }
@@ -626,6 +642,16 @@ static void aqr107_link_change_notify(struct phy_device *phydev)
 		phydev_info(phydev, "Aquantia 1000Base-T2 mode active\n");
 }
 
+static int aqr107_get_rate_adaptation(struct phy_device *phydev,
+				      phy_interface_t iface)
+{
+	if (iface == PHY_INTERFACE_MODE_10GBASER ||
+	    iface == PHY_INTERFACE_MODE_2500BASEX ||
+	    iface == PHY_INTERFACE_MODE_NA)
+		return RATE_ADAPT_PAUSE;
+	return RATE_ADAPT_NONE;
+}
+
 static int aqr107_suspend(struct phy_device *phydev)
 {
 	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
@@ -687,6 +713,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR107),
 	.name		= "Aquantia AQR107",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -705,6 +732,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQCS109),
 	.name		= "Aquantia AQCS109",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -731,6 +759,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
 	.name           = "Aquantia AQR113C",
 	.probe          = aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init    = aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr    = aqr_config_intr,
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 00/11] net: phy: Add support for rate adaptation

[Russell King (Oracle)]

On Tue, Jul 19, 2022 at 07:49:50PM -0400, Sean Anderson wrote:
> Second, to reduce packet loss it may be desirable to throttle packet
> throughput. In past discussions [2-4], this behavior has been
> controversial.

It isn't controversial at all. It's something we need to support, but
the point I've been making is that if we're adding rate adaption, then
we need to do a better job when designing the infrastructure to cater
for all currently known forms of rate adaption amongst the knowledge
pool that we have, not just one. That's why I brought up the IPG-based
method used by 88x3310.

Phylink development is extremely difficult, and takes months or years
for changes to get into mainline when updates to drivers are required -
this is why I have a massive queue of changes all the time.

> It is the opinion of several developers that it is the
> responsibility of the system integrator or end user to set the link
> settings appropriately for rate adaptation. In particular, it was argued
> that it is difficult to determine whether a particular phy has rate
> adaptation enabled, and it is simpler to keep such determinations out of
> the kernel.

I don't think I've ever said that...

> Another criticism is that packet loss may happen anyway, such
> as if a faster link is used with a switch or repeater that does not support
> pause frames.

That isn't what I've said. Packet loss may happen if (a) pause frames
can not be sent by a PHY in rate adaption mode and (b) if the MAC can't
pace its transmission for the media/line speed. This is a fundamental
fact where a PHY will only have so much buffering capability, that if
the MAC sends packets at a faster rate than the PHY can get them out, it
runs out of buffer space. That isn't a criticism, it's a statement of
fact.

> I believe that our current approach is limiting, especially when
> considering that rate adaptation (in two forms) has made it into IEEE
> standards. In general, When we have appropriate information we should set
> sensible defaults. To consider use a contrasting example, we enable pause
> frames by default for switches which autonegotiate for them. When it's the
> phy itself generating these frames, we don't even have to autonegotiate to
> know that we should enable pause frames.

I'm not sure I understand what you're saying, because it doesn't match
what I've seen.

"we enable pause frames by default for swithes which autonegotiate for
them" - what are you talking about there? The "user" ports on the
switch, or the DSA/CPU ports? It has been argued that pause frames
should not be enabled for the CPU port, particularly when the CPU port
runs at a slower speed than the switch - which happens e.g. on the VF610
platforms.

Most CPU ports to switches I'm aware of are specified either using a
fixed link in firmware or default to a fixed link both without pause
frames. Maybe this is just a quirk of the mv88e6xxx setup.

"when it's the phy itself generating these frames, we don't even have to
autonegotiate to know that we should enable pause frames." I'm not sure
that's got any relevance. When a PHY is in rate adapting mode, there are
two separate things that are going on. There's the media side link
negotiation and parameters, and then there's the requirements of the
host-side link. The parameters of the host-side link do not need to be
negotiated with the link partner, but they do potentially affect what
link modes we can negotiate with our link partner (for example, if the
PHY can't handle HD on the media side with the MAC operating FD). In any
case, if the PHY requires the MAC to receive pause frames for its rate
adaption to work, then this doesn't affect the media side
autonegotiation at all. Hence, I don't understand this comment.

> Note that
> even when we determine (e.g.) the pause settings based on whether rate
> adaptation is enabled, they can still be overridden by userspace (using
> ethtool). It might be prudent to allow disabling of rate adaptation
> generally in ethtool as well.

This is no longer true as this patch set overrides whatever receive
pause state has been negotiated or requested by userspace so that rate
adaption can still work.

The future work here is to work out whether we should disable rate
adaption if userspace requests receive pause frames to be disabled, or
whether switching to another form of controlling rate adaption would be
appropriate and/or possible.

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

Re: [PATCH v2 00/11] net: phy: Add support for rate adaptation

[Sean Anderson]

On 7/20/22 8:03 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:50PM -0400, Sean Anderson wrote:
>> Second, to reduce packet loss it may be desirable to throttle packet
>> throughput. In past discussions [2-4], this behavior has been
>> controversial.
> 
> It isn't controversial at all. It's something we need to support, but
> the point I've been making is that if we're adding rate adaption, then
> we need to do a better job when designing the infrastructure to cater
> for all currently known forms of rate adaption amongst the knowledge
> pool that we have, not just one. That's why I brought up the IPG-based
> method used by 88x3310.
> 
> Phylink development is extremely difficult, and takes months or years
> for changes to get into mainline when updates to drivers are required -
> this is why I have a massive queue of changes all the time.
> 
>> It is the opinion of several developers that it is the
>> responsibility of the system integrator or end user to set the link
>> settings appropriately for rate adaptation. In particular, it was argued
>> that it is difficult to determine whether a particular phy has rate
>> adaptation enabled, and it is simpler to keep such determinations out of
>> the kernel.
> 
> I don't think I've ever said that...

You haven't. This mostly stems from

https://lore.kernel.org/netdev/DB8PR04MB6985139D4ABED85B701445A9EC050@DB8PR04MB6985.eurprd04.prod.outlook.com/

where there was some discussion about whose responsibility it was to determine
whether rate adaptation was supported. The implication being that we should
delay support for rate adaptation until we could reliably determine whether
it was supported. This of course is not quite implemented yet. While we can
determine whether rate adaptation is actually in-use, I don't know if we can
determine whether it is available before trying to bring the link up.

>> Another criticism is that packet loss may happen anyway, such
>> as if a faster link is used with a switch or repeater that does not support
>> pause frames.
> 
> That isn't what I've said. Packet loss may happen if (a) pause frames
> can not be sent by a PHY in rate adaption mode and (b) if the MAC can't
> pace its transmission for the media/line speed. This is a fundamental
> fact where a PHY will only have so much buffering capability, that if
> the MAC sends packets at a faster rate than the PHY can get them out, it
> runs out of buffer space. That isn't a criticism, it's a statement of
> fact.

You're right. I mainly wanted to bring up what you just noted: that we may
have packet loss anyway, and that higher-layer protocols already deal with
packet loss. So a MAC unaware of the rate adaptation is not necessarily the
worst thing.

>> I believe that our current approach is limiting, especially when
>> considering that rate adaptation (in two forms) has made it into IEEE
>> standards. In general, When we have appropriate information we should set
>> sensible defaults. To consider use a contrasting example, we enable pause
>> frames by default for switches which autonegotiate for them. When it's the
>> phy itself generating these frames, we don't even have to autonegotiate to
>> know that we should enable pause frames.
> 
> I'm not sure I understand what you're saying, because it doesn't match
> what I've seen.
> 
Sorry, I was unclear here. I meant link partners, not local (DSA) switches.

> "we enable pause frames by default for swithes which autonegotiate for
> them" - what are you talking about there? The "user" ports on the
> switch, or the DSA/CPU ports? It has been argued that pause frames
> should not be enabled for the CPU port, particularly when the CPU port
> runs at a slower speed than the switch - which happens e.g. on the VF610
> platforms.
> 
> Most CPU ports to switches I'm aware of are specified either using a
> fixed link in firmware or default to a fixed link both without pause
> frames. Maybe this is just a quirk of the mv88e6xxx setup.
> 
> "when it's the phy itself generating these frames, we don't even have to
> autonegotiate to know that we should enable pause frames." I'm not sure
> that's got any relevance. When a PHY is in rate adapting mode, there are
> two separate things that are going on. There's the media side link
> negotiation and parameters, and then there's the requirements of the
> host-side link. The parameters of the host-side link do not need to be
> negotiated with the link partner, but they do potentially affect what
> link modes we can negotiate with our link partner (for example, if the
> PHY can't handle HD on the media side with the MAC operating FD). In any
> case, if the PHY requires the MAC to receive pause frames for its rate
> adaption to work, then this doesn't affect the media side
> autonegotiation at all. Hence, I don't understand this comment.
> 
>> Note that
>> even when we determine (e.g.) the pause settings based on whether rate
>> adaptation is enabled, they can still be overridden by userspace (using
>> ethtool). It might be prudent to allow disabling of rate adaptation
>> generally in ethtool as well.
> 
> This is no longer true as this patch set overrides whatever receive
> pause state has been negotiated or requested by userspace so that rate
> adaption can still work.

Right, I forgot to edit this.

> The future work here is to work out whether we should disable rate
> adaption if userspace requests receive pause frames to be disabled, or
> whether switching to another form of controlling rate adaption would be
> appropriate and/or possible.
> 

I'm not sure what the best course here is either.

--Sean