[PATCH net-next 0/5] add phylink support for PCS

[PATCH net-next 0/5] add phylink support for PCS

[Russell King - ARM Linux admin]

Hi,

This series adds support for IEEE 802.3 register set compliant PCS
for phylink.  In order to do this, we:

1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
   to a linkmode variant.
2. add a helper for clause 37 advertisements, supporting both the
   1000baseX and defacto 2500baseX variants. Note that ethtool does
   not support half duplex for either of these, and we make no effort
   to do so.
3. add accessors for modifying a MDIO device register, and use them in
   phylib, rather than duplicating the code from phylib.
4. add support for decoding the advertisement from clause 22 compatible
   register sets for clause 37 advertisements and SGMII advertisements.
5. add support for clause 45 register sets for 10GBASE-R PCS.

These have been tested on the LX2160A Clearfog-CX platform.

 drivers/net/phy/mdio_bus.c |  55 +++++++++++
 drivers/net/phy/phy-core.c |  31 ------
 drivers/net/phy/phylink.c  | 236 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |   4 +
 include/linux/mii.h        |  57 +++++++----
 include/linux/phy.h        |  19 ++++
 include/linux/phylink.h    |   8 ++
 include/uapi/linux/mii.h   |   5 +
 8 files changed, 366 insertions(+), 49 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[PATCH net-next 1/5] net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant

[Russell King]

Add a LPA to linkmode decoder for 1000BASE-X protocols; this decoder
only provides the modify semantics similar to other such decoders.
This replaces the unused mii_lpa_to_ethtool_lpa_x() helper.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 include/linux/mii.h | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/include/linux/mii.h b/include/linux/mii.h
index 18c6208f56fc..309de4a3e6e7 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -354,24 +354,6 @@ static inline u32 mii_adv_to_ethtool_adv_x(u32 adv)
 	return result;
 }
 
-/**
- * mii_lpa_to_ethtool_lpa_x
- * @adv: value of the MII_LPA register
- *
- * A small helper function that translates MII_LPA
- * bits, when in 1000Base-X mode, to ethtool
- * LP advertisement settings.
- */
-static inline u32 mii_lpa_to_ethtool_lpa_x(u32 lpa)
-{
-	u32 result = 0;
-
-	if (lpa & LPA_LPACK)
-		result |= ADVERTISED_Autoneg;
-
-	return result | mii_adv_to_ethtool_adv_x(lpa);
-}
-
 /**
  * mii_lpa_mod_linkmode_adv_sgmii
  * @lp_advertising: pointer to destination link mode.
@@ -535,6 +517,25 @@ static inline u32 linkmode_adv_to_lcl_adv_t(unsigned long *advertising)
 	return lcl_adv;
 }
 
+/**
+ * mii_lpa_mod_linkmode_x - decode the link partner's config_reg to linkmodes
+ * @linkmodes: link modes array
+ * @lpa: config_reg word from link partner
+ * @fd_bit: link mode for 1000XFULL bit
+ */
+static inline void mii_lpa_mod_linkmode_x(unsigned long *linkmodes, u16 lpa,
+					 int fd_bit)
+{
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, linkmodes,
+			 lpa & LPA_LPACK);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes,
+			 lpa & LPA_1000XPAUSE);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes,
+			 lpa & LPA_1000XPAUSE_ASYM);
+	linkmode_mod_bit(fd_bit, linkmodes,
+			 lpa & LPA_1000XFULL);
+}
+
 /**
  * mii_advertise_flowctrl - get flow control advertisement flags
  * @cap: Flow control capabilities (FLOW_CTRL_RX, FLOW_CTRL_TX or both)
-- 
2.20.1

[PATCH net-next 2/5] net: mii: add linkmode_adv_to_mii_adv_x()

[Russell King]

Add a helper to convert a linkmode advertisement to a clause 37
advertisement value for 1000base-x and 2500base-x.

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

diff --git a/include/linux/mii.h b/include/linux/mii.h
index 309de4a3e6e7..219b93cad1dd 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -536,6 +536,26 @@ static inline void mii_lpa_mod_linkmode_x(unsigned long *linkmodes, u16 lpa,
 			 lpa & LPA_1000XFULL);
 }
 
+/**
+ * linkmode_adv_to_mii_adv_x - encode a linkmode to config_reg
+ * @linkmodes: linkmodes
+ * @fd_bit: full duplex bit
+ */
+static inline u16 linkmode_adv_to_mii_adv_x(const unsigned long *linkmodes,
+					    int fd_bit)
+{
+	u16 adv = 0;
+
+	if (linkmode_test_bit(fd_bit, linkmodes))
+		adv |= ADVERTISE_1000XFULL;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes))
+		adv |= ADVERTISE_1000XPAUSE;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes))
+		adv |= ADVERTISE_1000XPSE_ASYM;
+
+	return adv;
+}
+
 /**
  * mii_advertise_flowctrl - get flow control advertisement flags
  * @cap: Flow control capabilities (FLOW_CTRL_RX, FLOW_CTRL_TX or both)
-- 
2.20.1

[PATCH net-next 3/5] net: mdiobus: add APIs for modifying a MDIO device register

[Russell King]

Add APIs for modifying a MDIO device register, similar to the existing
phy_modify() group of functions, but at mdiobus level instead.  Adapt
__phy_modify_changed() to use the new mdiobus level helper.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/mdio_bus.c | 55 ++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy-core.c | 31 ---------------------
 include/linux/mdio.h       |  4 +++
 include/linux/phy.h        | 19 +++++++++++++
 4 files changed, 78 insertions(+), 31 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3ab9ca7614d1..b33d1e793686 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -824,6 +824,38 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(__mdiobus_write);
 
+/**
+ * __mdiobus_modify_changed - Unlocked version of the mdiobus_modify function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to modify
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ *
+ * Read, modify, and if any change, write the register value back to the
+ * device. Any error returns a negative number.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
+			     u16 mask, u16 set)
+{
+	int new, ret;
+
+	ret = __mdiobus_read(bus, addr, regnum);
+	if (ret < 0)
+		return ret;
+
+	new = (ret & ~mask) | set;
+	if (new == ret)
+		return 0;
+
+	ret = __mdiobus_write(bus, addr, regnum, new);
+
+	return ret < 0 ? ret : 1;
+}
+EXPORT_SYMBOL_GPL(__mdiobus_modify_changed);
+
 /**
  * mdiobus_read_nested - Nested version of the mdiobus_read function
  * @bus: the mii_bus struct
@@ -928,6 +960,29 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(mdiobus_write);
 
+/**
+ * mdiobus_modify - Convenience function for modifying a given mdio device
+ *	register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ */
+int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask, u16 set)
+{
+	int err;
+
+	BUG_ON(in_interrupt());
+
+	mutex_lock(&bus->mdio_lock);
+	err = __mdiobus_modify_changed(bus, addr, regnum, mask, set);
+	mutex_unlock(&bus->mdio_lock);
+
+	return err < 0 ? err : 0;
+}
+EXPORT_SYMBOL_GPL(mdiobus_modify);
+
 /**
  * mdio_bus_match - determine if given MDIO driver supports the given
  *		    MDIO device
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index e083e7a76ada..94cd85b1e49b 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -488,37 +488,6 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(phy_write_mmd);
 
-/**
- * __phy_modify_changed() - Convenience function for modifying a PHY register
- * @phydev: a pointer to a &struct phy_device
- * @regnum: register number
- * @mask: bit mask of bits to clear
- * @set: bit mask of bits to set
- *
- * Unlocked helper function which allows a PHY register to be modified as
- * new register value = (old register value & ~mask) | set
- *
- * Returns negative errno, 0 if there was no change, and 1 in case of change
- */
-int __phy_modify_changed(struct phy_device *phydev, u32 regnum, u16 mask,
-			 u16 set)
-{
-	int new, ret;
-
-	ret = __phy_read(phydev, regnum);
-	if (ret < 0)
-		return ret;
-
-	new = (ret & ~mask) | set;
-	if (new == ret)
-		return 0;
-
-	ret = __phy_write(phydev, regnum, new);
-
-	return ret < 0 ? ret : 1;
-}
-EXPORT_SYMBOL_GPL(__phy_modify_changed);
-
 /**
  * phy_modify_changed - Function for modifying a PHY register
  * @phydev: the phy_device struct
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index a7604248777b..917e4bb2ed71 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -316,11 +316,15 @@ static inline void mii_10gbt_stat_mod_linkmode_lpa_t(unsigned long *advertising,
 
 int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
 int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
+int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
+			     u16 mask, u16 set);
 
 int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
 int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum);
 int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
 int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
+int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
+		   u16 set);
 
 int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e72dbd0d2d6a..d3cd10ed945c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -747,6 +747,25 @@ static inline int __phy_write(struct phy_device *phydev, u32 regnum, u16 val)
 			       val);
 }
 
+/**
+ * __phy_modify_changed() - Convenience function for modifying a PHY register
+ * @phydev: a pointer to a &struct phy_device
+ * @regnum: register number
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ *
+ * Unlocked helper function which allows a PHY register to be modified as
+ * new register value = (old register value & ~mask) | set
+ *
+ * Returns negative errno, 0 if there was no change, and 1 in case of change
+ */
+static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
+				       u16 mask, u16 set)
+{
+	return __mdiobus_modify_changed(phydev->mdio.bus, phydev->mdio.addr,
+					regnum, mask, set);
+}
+
 /**
  * phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
-- 
2.20.1

[PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Russell King]

Implement helpers for PCS accessed via the MII bus using 802.3 clause
22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
for the advertisement word.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |   6 ++
 include/uapi/linux/mii.h  |   5 +
 3 files changed, 217 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 19db68d74cb4..3ef20b34f5d6 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
 }
 EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
 
+static void phylink_decode_c37_word(struct phylink_link_state *state,
+				    uint16_t config_reg, int speed)
+{
+	bool tx_pause, rx_pause;
+	int fd_bit;
+
+	if (speed == SPEED_2500)
+		fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
+	else
+		fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
+
+	mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
+
+	if (linkmode_test_bit(fd_bit, state->advertising) &&
+	    linkmode_test_bit(fd_bit, state->lp_advertising)) {
+		state->speed = speed;
+		state->duplex = DUPLEX_FULL;
+	} else {
+		/* negotiation failure */
+		state->link = false;
+	}
+
+	linkmode_resolve_pause(state->advertising, state->lp_advertising,
+			       &tx_pause, &rx_pause);
+
+	if (tx_pause)
+		state->pause |= MLO_PAUSE_TX;
+	if (rx_pause)
+		state->pause |= MLO_PAUSE_RX;
+}
+
+static void phylink_decode_sgmii_word(struct phylink_link_state *state,
+				      uint16_t config_reg)
+{
+	if (!(config_reg & LPA_SGMII_LINK)) {
+		state->link = false;
+		return;
+	}
+
+	switch (config_reg & LPA_SGMII_SPD_MASK) {
+	case LPA_SGMII_10:
+		state->speed = SPEED_10;
+		break;
+	case LPA_SGMII_100:
+		state->speed = SPEED_100;
+		break;
+	case LPA_SGMII_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->link = false;
+		return;
+	}
+	if (config_reg & LPA_SGMII_FULL_DUPLEX)
+		state->duplex = DUPLEX_FULL;
+	else
+		state->duplex = DUPLEX_HALF;
+}
+
+/**
+ * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
+ * @pcs: a pointer to a &struct mdio_device.
+ * @state: a pointer to a &struct phylink_link_state.
+ *
+ * Helper for MAC PCS supporting the 802.3 clause 22 register set for
+ * clause 37 negotiation and/or SGMII control.
+ *
+ * Read the MAC PCS state from the MII device configured in @config and
+ * parse the Clause 37 or Cisco SGMII link partner negotiation word into
+ * the phylink @state structure. This is suitable to be directly plugged
+ * into the mac_pcs_get_state() member of the struct phylink_mac_ops
+ * structure.
+ */
+void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
+				   struct phylink_link_state *state)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+	if (!state->link)
+		return;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		phylink_decode_c37_word(state, lpa, SPEED_1000);
+		break;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		phylink_decode_c37_word(state, lpa, SPEED_2500);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		phylink_decode_sgmii_word(state, lpa);
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
+
+/**
+ * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
+ *	advertisement
+ * @pcs: a pointer to a &struct mdio_device.
+ * @state: a pointer to the state being configured.
+ *
+ * Helper for MAC PCS supporting the 802.3 clause 22 register set for
+ * clause 37 negotiation and/or SGMII control.
+ *
+ * Configure the clause 37 PCS advertisement as specified by @state. This
+ * does not trigger a renegotiation; phylink will do that via the
+ * mac_an_restart() method of the struct phylink_mac_ops structure.
+ *
+ * Returns negative error code on failure to configure the advertisement,
+ * zero if no change has been made, or one if the advertisement has changed.
+ */
+int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
+					const struct phylink_link_state *state)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	int val, ret;
+	u16 adv;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		adv = ADVERTISE_1000XFULL;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPAUSE;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPSE_ASYM;
+
+		val = mdiobus_read(bus, addr, MII_ADVERTISE);
+		if (val < 0)
+			return val;
+
+		if (val == adv)
+			return 0;
+
+		ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
+		if (ret < 0)
+			return ret;
+
+		return 1;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		val = mdiobus_read(bus, addr, MII_ADVERTISE);
+		if (val < 0)
+			return val;
+
+		if (val == 0x0001)
+			return 0;
+
+		ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
+		if (ret < 0)
+			return ret;
+
+		return 1;
+
+	default:
+		/* Nothing to do for other modes */
+		return 0;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
+
+/**
+ * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
+ * @pcs: a pointer to a &struct mdio_device.
+ *
+ * Helper for MAC PCS supporting the 802.3 clause 22 register set for
+ * clause 37 negotiation.
+ *
+ * Restart the clause 37 negotiation with the link partner. This is
+ * suitable to be directly plugged into the mac_pcs_get_state() member
+ * of the struct phylink_mac_ops structure.
+ */
+void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
+{
+	struct mii_bus *bus = pcs->bus;
+	int val, addr = pcs->addr;
+
+	val = mdiobus_read(bus, addr, MII_BMCR);
+	if (val >= 0) {
+		val |= BMCR_ANRESTART;
+
+		mdiobus_write(bus, addr, MII_BMCR, val);
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 2180eb1aa254..de591c2fb37e 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
 void phylink_set_port_modes(unsigned long *bits);
 void phylink_helper_basex_speed(struct phylink_link_state *state);
 
+void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
+				   struct phylink_link_state *state);
+int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
+					const struct phylink_link_state *state);
+void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
+
 #endif
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 0b9c3beda345..90f9b4e1ba27 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -134,11 +134,16 @@
 /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
 #define ADVERTISE_SGMII		0x0001	/* MAC can do SGMII            */
 #define LPA_SGMII		0x0001	/* PHY can do SGMII            */
+#define LPA_SGMII_SPD_MASK	0x0c00	/* SGMII speed mask            */
+#define LPA_SGMII_FULL_DUPLEX	0x1000	/* SGMII full duplex           */
 #define LPA_SGMII_DPX_SPD_MASK	0x1C00	/* SGMII duplex and speed bits */
+#define LPA_SGMII_10		0x0000	/* 10Mbps                      */
 #define LPA_SGMII_10HALF	0x0000	/* Can do 10mbps half-duplex   */
 #define LPA_SGMII_10FULL	0x1000	/* Can do 10mbps full-duplex   */
+#define LPA_SGMII_100		0x0400	/* 100Mbps                     */
 #define LPA_SGMII_100HALF	0x0400	/* Can do 100mbps half-duplex  */
 #define LPA_SGMII_100FULL	0x1400	/* Can do 100mbps full-duplex  */
+#define LPA_SGMII_1000		0x0800	/* 1000Mbps                    */
 #define LPA_SGMII_1000HALF	0x0800	/* Can do 1000mbps half-duplex */
 #define LPA_SGMII_1000FULL	0x1800	/* Can do 1000mbps full-duplex */
 #define LPA_SGMII_LINK		0x8000	/* PHY link with copper-side partner */
-- 
2.20.1

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 14:08, Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Implement helpers for PCS accessed via the MII bus using 802.3 clause
> 22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
> for the advertisement word.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |   6 ++
>  include/uapi/linux/mii.h  |   5 +
>  3 files changed, 217 insertions(+)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 19db68d74cb4..3ef20b34f5d6 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
>  }
>  EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
>
> +static void phylink_decode_c37_word(struct phylink_link_state *state,
> +                                   uint16_t config_reg, int speed)
> +{
> +       bool tx_pause, rx_pause;
> +       int fd_bit;
> +
> +       if (speed == SPEED_2500)
> +               fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
> +       else
> +               fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> +
> +       mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
> +
> +       if (linkmode_test_bit(fd_bit, state->advertising) &&
> +           linkmode_test_bit(fd_bit, state->lp_advertising)) {
> +               state->speed = speed;
> +               state->duplex = DUPLEX_FULL;
> +       } else {
> +               /* negotiation failure */
> +               state->link = false;
> +       }
> +
> +       linkmode_resolve_pause(state->advertising, state->lp_advertising,
> +                              &tx_pause, &rx_pause);
> +
> +       if (tx_pause)
> +               state->pause |= MLO_PAUSE_TX;
> +       if (rx_pause)
> +               state->pause |= MLO_PAUSE_RX;
> +}
> +
> +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> +                                     uint16_t config_reg)
> +{
> +       if (!(config_reg & LPA_SGMII_LINK)) {
> +               state->link = false;
> +               return;
> +       }
> +
> +       switch (config_reg & LPA_SGMII_SPD_MASK) {

Did you consider using or adapting the mii_lpa_mod_linkmode_adv_sgmii helper?

> +       case LPA_SGMII_10:
> +               state->speed = SPEED_10;
> +               break;
> +       case LPA_SGMII_100:
> +               state->speed = SPEED_100;
> +               break;
> +       case LPA_SGMII_1000:
> +               state->speed = SPEED_1000;
> +               break;
> +       default:
> +               state->link = false;
> +               return;
> +       }
> +       if (config_reg & LPA_SGMII_FULL_DUPLEX)
> +               state->duplex = DUPLEX_FULL;
> +       else
> +               state->duplex = DUPLEX_HALF;
> +}
> +
> +/**
> + * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
> + * @pcs: a pointer to a &struct mdio_device.
> + * @state: a pointer to a &struct phylink_link_state.
> + *
> + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> + * clause 37 negotiation and/or SGMII control.
> + *
> + * Read the MAC PCS state from the MII device configured in @config and
> + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> + * the phylink @state structure. This is suitable to be directly plugged
> + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> + * structure.
> + */
> +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> +                                  struct phylink_link_state *state)
> +{
> +       struct mii_bus *bus = pcs->bus;
> +       int addr = pcs->addr;
> +       int bmsr, lpa;
> +
> +       bmsr = mdiobus_read(bus, addr, MII_BMSR);
> +       lpa = mdiobus_read(bus, addr, MII_LPA);
> +       if (bmsr < 0 || lpa < 0) {
> +               state->link = false;
> +               return;
> +       }
> +
> +       state->link = !!(bmsr & BMSR_LSTATUS);
> +       state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> +       if (!state->link)
> +               return;
> +
> +       switch (state->interface) {
> +       case PHY_INTERFACE_MODE_1000BASEX:
> +               phylink_decode_c37_word(state, lpa, SPEED_1000);
> +               break;
> +
> +       case PHY_INTERFACE_MODE_2500BASEX:
> +               phylink_decode_c37_word(state, lpa, SPEED_2500);
> +               break;
> +
> +       case PHY_INTERFACE_MODE_SGMII:
> +               phylink_decode_sgmii_word(state, lpa);
> +               break;
> +
> +       default:
> +               state->link = false;
> +               break;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
> +
> +/**
> + * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
> + *     advertisement
> + * @pcs: a pointer to a &struct mdio_device.
> + * @state: a pointer to the state being configured.
> + *
> + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> + * clause 37 negotiation and/or SGMII control.
> + *
> + * Configure the clause 37 PCS advertisement as specified by @state. This
> + * does not trigger a renegotiation; phylink will do that via the
> + * mac_an_restart() method of the struct phylink_mac_ops structure.
> + *
> + * Returns negative error code on failure to configure the advertisement,
> + * zero if no change has been made, or one if the advertisement has changed.
> + */
> +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> +                                       const struct phylink_link_state *state)
> +{
> +       struct mii_bus *bus = pcs->bus;
> +       int addr = pcs->addr;
> +       int val, ret;
> +       u16 adv;
> +
> +       switch (state->interface) {
> +       case PHY_INTERFACE_MODE_1000BASEX:
> +       case PHY_INTERFACE_MODE_2500BASEX:
> +               adv = ADVERTISE_1000XFULL;
> +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +                                     state->advertising))
> +                       adv |= ADVERTISE_1000XPAUSE;
> +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +                                     state->advertising))
> +                       adv |= ADVERTISE_1000XPSE_ASYM;
> +
> +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> +               if (val < 0)
> +                       return val;
> +
> +               if (val == adv)
> +                       return 0;
> +
> +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> +               if (ret < 0)
> +                       return ret;
> +
> +               return 1;
> +
> +       case PHY_INTERFACE_MODE_SGMII:
> +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> +               if (val < 0)
> +                       return val;
> +
> +               if (val == 0x0001)
> +                       return 0;
> +
> +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
> +               if (ret < 0)
> +                       return ret;
> +
> +               return 1;
> +
> +       default:
> +               /* Nothing to do for other modes */
> +               return 0;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
> +
> +/**
> + * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
> + * @pcs: a pointer to a &struct mdio_device.
> + *
> + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> + * clause 37 negotiation.
> + *
> + * Restart the clause 37 negotiation with the link partner. This is
> + * suitable to be directly plugged into the mac_pcs_get_state() member
> + * of the struct phylink_mac_ops structure.
> + */
> +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
> +{
> +       struct mii_bus *bus = pcs->bus;
> +       int val, addr = pcs->addr;
> +
> +       val = mdiobus_read(bus, addr, MII_BMCR);
> +       if (val >= 0) {
> +               val |= BMCR_ANRESTART;
> +
> +               mdiobus_write(bus, addr, MII_BMCR, val);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 2180eb1aa254..de591c2fb37e 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
>  void phylink_set_port_modes(unsigned long *bits);
>  void phylink_helper_basex_speed(struct phylink_link_state *state);
>
> +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> +                                  struct phylink_link_state *state);
> +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> +                                       const struct phylink_link_state *state);
> +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
> +
>  #endif
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> index 0b9c3beda345..90f9b4e1ba27 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -134,11 +134,16 @@
>  /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
>  #define ADVERTISE_SGMII                0x0001  /* MAC can do SGMII            */
>  #define LPA_SGMII              0x0001  /* PHY can do SGMII            */
> +#define LPA_SGMII_SPD_MASK     0x0c00  /* SGMII speed mask            */
> +#define LPA_SGMII_FULL_DUPLEX  0x1000  /* SGMII full duplex           */
>  #define LPA_SGMII_DPX_SPD_MASK 0x1C00  /* SGMII duplex and speed bits */
> +#define LPA_SGMII_10           0x0000  /* 10Mbps                      */
>  #define LPA_SGMII_10HALF       0x0000  /* Can do 10mbps half-duplex   */
>  #define LPA_SGMII_10FULL       0x1000  /* Can do 10mbps full-duplex   */
> +#define LPA_SGMII_100          0x0400  /* 100Mbps                     */
>  #define LPA_SGMII_100HALF      0x0400  /* Can do 100mbps half-duplex  */
>  #define LPA_SGMII_100FULL      0x1400  /* Can do 100mbps full-duplex  */
> +#define LPA_SGMII_1000         0x0800  /* 1000Mbps                    */

These seem a bit mixed up to say the least.
If you're not going to use the (minimal) existing infrastructure could
you also refactor the one existing user? I think it would be better
than just keeping adding conflicting definitions.

>  #define LPA_SGMII_1000HALF     0x0800  /* Can do 1000mbps half-duplex */
>  #define LPA_SGMII_1000FULL     0x1800  /* Can do 1000mbps full-duplex */
>  #define LPA_SGMII_LINK         0x8000  /* PHY link with copper-side partner */
> --
> 2.20.1
>

Regards,
-Vladimir

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 04:06:52PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 14:08, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Implement helpers for PCS accessed via the MII bus using 802.3 clause
> > 22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
> > for the advertisement word.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/phylink.h   |   6 ++
> >  include/uapi/linux/mii.h  |   5 +
> >  3 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 19db68d74cb4..3ef20b34f5d6 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
> >  }
> >  EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
> >
> > +static void phylink_decode_c37_word(struct phylink_link_state *state,
> > +                                   uint16_t config_reg, int speed)
> > +{
> > +       bool tx_pause, rx_pause;
> > +       int fd_bit;
> > +
> > +       if (speed == SPEED_2500)
> > +               fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
> > +       else
> > +               fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> > +
> > +       mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
> > +
> > +       if (linkmode_test_bit(fd_bit, state->advertising) &&
> > +           linkmode_test_bit(fd_bit, state->lp_advertising)) {
> > +               state->speed = speed;
> > +               state->duplex = DUPLEX_FULL;
> > +       } else {
> > +               /* negotiation failure */
> > +               state->link = false;
> > +       }
> > +
> > +       linkmode_resolve_pause(state->advertising, state->lp_advertising,
> > +                              &tx_pause, &rx_pause);
> > +
> > +       if (tx_pause)
> > +               state->pause |= MLO_PAUSE_TX;
> > +       if (rx_pause)
> > +               state->pause |= MLO_PAUSE_RX;
> > +}
> > +
> > +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> > +                                     uint16_t config_reg)
> > +{
> > +       if (!(config_reg & LPA_SGMII_LINK)) {
> > +               state->link = false;
> > +               return;
> > +       }
> > +
> > +       switch (config_reg & LPA_SGMII_SPD_MASK) {
> 
> Did you consider using or adapting the mii_lpa_mod_linkmode_adv_sgmii helper?

Yes, and we _do not_ want to be touching state->lp_advertising here.
The link partner advertisement comes from the attached PHY, not from
this information.

The config_reg information is not an advertisement - it is a
_configuration word_ coming from the PHY to tell the MAC what speed
and duplex to operate at.  It conveys nothing about whether it's
1000baseT or 1000baseX, so link modes mean nothing as far as a SGMII
configuration word is concerned.

Hence, _this_ is a more correct implementation than
mii_lpa_mod_linkmode_adv_sgmii().

> > +       case LPA_SGMII_10:
> > +               state->speed = SPEED_10;
> > +               break;
> > +       case LPA_SGMII_100:
> > +               state->speed = SPEED_100;
> > +               break;
> > +       case LPA_SGMII_1000:
> > +               state->speed = SPEED_1000;
> > +               break;
> > +       default:
> > +               state->link = false;
> > +               return;
> > +       }
> > +       if (config_reg & LPA_SGMII_FULL_DUPLEX)
> > +               state->duplex = DUPLEX_FULL;
> > +       else
> > +               state->duplex = DUPLEX_HALF;
> > +}
> > +
> > +/**
> > + * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
> > + * @pcs: a pointer to a &struct mdio_device.
> > + * @state: a pointer to a &struct phylink_link_state.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation and/or SGMII control.
> > + *
> > + * Read the MAC PCS state from the MII device configured in @config and
> > + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> > + * the phylink @state structure. This is suitable to be directly plugged
> > + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> > + * structure.
> > + */
> > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > +                                  struct phylink_link_state *state)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int addr = pcs->addr;
> > +       int bmsr, lpa;
> > +
> > +       bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > +       lpa = mdiobus_read(bus, addr, MII_LPA);
> > +       if (bmsr < 0 || lpa < 0) {
> > +               state->link = false;
> > +               return;
> > +       }
> > +
> > +       state->link = !!(bmsr & BMSR_LSTATUS);
> > +       state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > +       if (!state->link)
> > +               return;
> > +
> > +       switch (state->interface) {
> > +       case PHY_INTERFACE_MODE_1000BASEX:
> > +               phylink_decode_c37_word(state, lpa, SPEED_1000);
> > +               break;
> > +
> > +       case PHY_INTERFACE_MODE_2500BASEX:
> > +               phylink_decode_c37_word(state, lpa, SPEED_2500);
> > +               break;
> > +
> > +       case PHY_INTERFACE_MODE_SGMII:
> > +               phylink_decode_sgmii_word(state, lpa);
> > +               break;
> > +
> > +       default:
> > +               state->link = false;
> > +               break;
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
> > +
> > +/**
> > + * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
> > + *     advertisement
> > + * @pcs: a pointer to a &struct mdio_device.
> > + * @state: a pointer to the state being configured.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation and/or SGMII control.
> > + *
> > + * Configure the clause 37 PCS advertisement as specified by @state. This
> > + * does not trigger a renegotiation; phylink will do that via the
> > + * mac_an_restart() method of the struct phylink_mac_ops structure.
> > + *
> > + * Returns negative error code on failure to configure the advertisement,
> > + * zero if no change has been made, or one if the advertisement has changed.
> > + */
> > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > +                                       const struct phylink_link_state *state)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int addr = pcs->addr;
> > +       int val, ret;
> > +       u16 adv;
> > +
> > +       switch (state->interface) {
> > +       case PHY_INTERFACE_MODE_1000BASEX:
> > +       case PHY_INTERFACE_MODE_2500BASEX:
> > +               adv = ADVERTISE_1000XFULL;
> > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +                                     state->advertising))
> > +                       adv |= ADVERTISE_1000XPAUSE;
> > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +                                     state->advertising))
> > +                       adv |= ADVERTISE_1000XPSE_ASYM;
> > +
> > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > +               if (val < 0)
> > +                       return val;
> > +
> > +               if (val == adv)
> > +                       return 0;
> > +
> > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               return 1;
> > +
> > +       case PHY_INTERFACE_MODE_SGMII:
> > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > +               if (val < 0)
> > +                       return val;
> > +
> > +               if (val == 0x0001)
> > +                       return 0;
> > +
> > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               return 1;
> > +
> > +       default:
> > +               /* Nothing to do for other modes */
> > +               return 0;
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
> > +
> > +/**
> > + * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
> > + * @pcs: a pointer to a &struct mdio_device.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation.
> > + *
> > + * Restart the clause 37 negotiation with the link partner. This is
> > + * suitable to be directly plugged into the mac_pcs_get_state() member
> > + * of the struct phylink_mac_ops structure.
> > + */
> > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int val, addr = pcs->addr;
> > +
> > +       val = mdiobus_read(bus, addr, MII_BMCR);
> > +       if (val >= 0) {
> > +               val |= BMCR_ANRESTART;
> > +
> > +               mdiobus_write(bus, addr, MII_BMCR, val);
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 2180eb1aa254..de591c2fb37e 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
> >  void phylink_set_port_modes(unsigned long *bits);
> >  void phylink_helper_basex_speed(struct phylink_link_state *state);
> >
> > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > +                                  struct phylink_link_state *state);
> > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > +                                       const struct phylink_link_state *state);
> > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
> > +
> >  #endif
> > diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> > index 0b9c3beda345..90f9b4e1ba27 100644
> > --- a/include/uapi/linux/mii.h
> > +++ b/include/uapi/linux/mii.h
> > @@ -134,11 +134,16 @@
> >  /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
> >  #define ADVERTISE_SGMII                0x0001  /* MAC can do SGMII            */
> >  #define LPA_SGMII              0x0001  /* PHY can do SGMII            */
> > +#define LPA_SGMII_SPD_MASK     0x0c00  /* SGMII speed mask            */
> > +#define LPA_SGMII_FULL_DUPLEX  0x1000  /* SGMII full duplex           */
> >  #define LPA_SGMII_DPX_SPD_MASK 0x1C00  /* SGMII duplex and speed bits */
> > +#define LPA_SGMII_10           0x0000  /* 10Mbps                      */
> >  #define LPA_SGMII_10HALF       0x0000  /* Can do 10mbps half-duplex   */
> >  #define LPA_SGMII_10FULL       0x1000  /* Can do 10mbps full-duplex   */
> > +#define LPA_SGMII_100          0x0400  /* 100Mbps                     */
> >  #define LPA_SGMII_100HALF      0x0400  /* Can do 100mbps half-duplex  */
> >  #define LPA_SGMII_100FULL      0x1400  /* Can do 100mbps full-duplex  */
> > +#define LPA_SGMII_1000         0x0800  /* 1000Mbps                    */
> 
> These seem a bit mixed up to say the least.
> If you're not going to use the (minimal) existing infrastructure could
> you also refactor the one existing user? I think it would be better
> than just keeping adding conflicting definitions.

Sorry, but you need to explain better what you would like to see here.
The additions I'm adding are to the SGMII specification; I find your
existing definitions to be obscure because they conflate two different
bit fields together to produce something for the ethtool linkmodes
(which I think is a big mistake.)

> 
> >  #define LPA_SGMII_1000HALF     0x0800  /* Can do 1000mbps half-duplex */
> >  #define LPA_SGMII_1000FULL     0x1800  /* Can do 1000mbps full-duplex */
> >  #define LPA_SGMII_LINK         0x8000  /* PHY link with copper-side partner */
> > --
> > 2.20.1
> >
> 
> Regards,
> -Vladimir
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 19:09, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 11, 2020 at 04:06:52PM +0200, Vladimir Oltean wrote:
> > On Wed, 11 Mar 2020 at 14:08, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > >
> > > Implement helpers for PCS accessed via the MII bus using 802.3 clause
> > > 22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
> > > for the advertisement word.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/phylink.h   |   6 ++
> > >  include/uapi/linux/mii.h  |   5 +
> > >  3 files changed, 217 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 19db68d74cb4..3ef20b34f5d6 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
> > >  }
> > >  EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
> > >
> > > +static void phylink_decode_c37_word(struct phylink_link_state *state,
> > > +                                   uint16_t config_reg, int speed)
> > > +{
> > > +       bool tx_pause, rx_pause;
> > > +       int fd_bit;
> > > +
> > > +       if (speed == SPEED_2500)
> > > +               fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
> > > +       else
> > > +               fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> > > +
> > > +       mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
> > > +
> > > +       if (linkmode_test_bit(fd_bit, state->advertising) &&
> > > +           linkmode_test_bit(fd_bit, state->lp_advertising)) {
> > > +               state->speed = speed;
> > > +               state->duplex = DUPLEX_FULL;
> > > +       } else {
> > > +               /* negotiation failure */
> > > +               state->link = false;
> > > +       }
> > > +
> > > +       linkmode_resolve_pause(state->advertising, state->lp_advertising,
> > > +                              &tx_pause, &rx_pause);
> > > +
> > > +       if (tx_pause)
> > > +               state->pause |= MLO_PAUSE_TX;
> > > +       if (rx_pause)
> > > +               state->pause |= MLO_PAUSE_RX;
> > > +}
> > > +
> > > +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> > > +                                     uint16_t config_reg)
> > > +{
> > > +       if (!(config_reg & LPA_SGMII_LINK)) {
> > > +               state->link = false;
> > > +               return;
> > > +       }
> > > +
> > > +       switch (config_reg & LPA_SGMII_SPD_MASK) {
> >
> > Did you consider using or adapting the mii_lpa_mod_linkmode_adv_sgmii helper?
>
> Yes, and we _do not_ want to be touching state->lp_advertising here.
> The link partner advertisement comes from the attached PHY, not from
> this information.
>

Understood, and I never suggested to touch state->lp_advertising, but
rather pcs->lp_advertising.

> The config_reg information is not an advertisement - it is a
> _configuration word_ coming from the PHY to tell the MAC what speed
> and duplex to operate at.

Arguably the abuse here is Cisco's, since config_reg _was_ supposed to
be an auto-neg advertisement as per clause 37. It is just the SGMII
spec that makes it asymmetrical.

> It conveys nothing about whether it's
> 1000baseT or 1000baseX, so link modes mean nothing as far as a SGMII
> configuration word is concerned.

My initial point was that _for this pcs device_, the link partner _is_
the PHY if it's SGMII. Otherwise you wouldn't find the config_reg in
register 5.

>
> Hence, _this_ is a more correct implementation than
> mii_lpa_mod_linkmode_adv_sgmii().
>
> > > +       case LPA_SGMII_10:
> > > +               state->speed = SPEED_10;
> > > +               break;
> > > +       case LPA_SGMII_100:
> > > +               state->speed = SPEED_100;
> > > +               break;
> > > +       case LPA_SGMII_1000:
> > > +               state->speed = SPEED_1000;
> > > +               break;
> > > +       default:
> > > +               state->link = false;
> > > +               return;
> > > +       }
> > > +       if (config_reg & LPA_SGMII_FULL_DUPLEX)
> > > +               state->duplex = DUPLEX_FULL;
> > > +       else
> > > +               state->duplex = DUPLEX_HALF;
> > > +}
> > > +
> > > +/**
> > > + * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
> > > + * @pcs: a pointer to a &struct mdio_device.
> > > + * @state: a pointer to a &struct phylink_link_state.
> > > + *
> > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > + * clause 37 negotiation and/or SGMII control.
> > > + *
> > > + * Read the MAC PCS state from the MII device configured in @config and
> > > + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> > > + * the phylink @state structure. This is suitable to be directly plugged
> > > + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> > > + * structure.
> > > + */
> > > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > > +                                  struct phylink_link_state *state)
> > > +{
> > > +       struct mii_bus *bus = pcs->bus;
> > > +       int addr = pcs->addr;
> > > +       int bmsr, lpa;
> > > +
> > > +       bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > > +       lpa = mdiobus_read(bus, addr, MII_LPA);
> > > +       if (bmsr < 0 || lpa < 0) {
> > > +               state->link = false;
> > > +               return;
> > > +       }
> > > +
> > > +       state->link = !!(bmsr & BMSR_LSTATUS);
> > > +       state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > > +       if (!state->link)
> > > +               return;
> > > +
> > > +       switch (state->interface) {
> > > +       case PHY_INTERFACE_MODE_1000BASEX:
> > > +               phylink_decode_c37_word(state, lpa, SPEED_1000);
> > > +               break;
> > > +
> > > +       case PHY_INTERFACE_MODE_2500BASEX:
> > > +               phylink_decode_c37_word(state, lpa, SPEED_2500);
> > > +               break;
> > > +
> > > +       case PHY_INTERFACE_MODE_SGMII:
> > > +               phylink_decode_sgmii_word(state, lpa);
> > > +               break;
> > > +
> > > +       default:
> > > +               state->link = false;
> > > +               break;
> > > +       }
> > > +}
> > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
> > > +
> > > +/**
> > > + * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
> > > + *     advertisement
> > > + * @pcs: a pointer to a &struct mdio_device.
> > > + * @state: a pointer to the state being configured.
> > > + *
> > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > + * clause 37 negotiation and/or SGMII control.
> > > + *
> > > + * Configure the clause 37 PCS advertisement as specified by @state. This
> > > + * does not trigger a renegotiation; phylink will do that via the
> > > + * mac_an_restart() method of the struct phylink_mac_ops structure.
> > > + *
> > > + * Returns negative error code on failure to configure the advertisement,
> > > + * zero if no change has been made, or one if the advertisement has changed.
> > > + */
> > > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > > +                                       const struct phylink_link_state *state)
> > > +{
> > > +       struct mii_bus *bus = pcs->bus;
> > > +       int addr = pcs->addr;
> > > +       int val, ret;
> > > +       u16 adv;
> > > +
> > > +       switch (state->interface) {
> > > +       case PHY_INTERFACE_MODE_1000BASEX:
> > > +       case PHY_INTERFACE_MODE_2500BASEX:
> > > +               adv = ADVERTISE_1000XFULL;
> > > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > > +                                     state->advertising))
> > > +                       adv |= ADVERTISE_1000XPAUSE;
> > > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > > +                                     state->advertising))
> > > +                       adv |= ADVERTISE_1000XPSE_ASYM;
> > > +
> > > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > > +               if (val < 0)
> > > +                       return val;
> > > +
> > > +               if (val == adv)
> > > +                       return 0;
> > > +
> > > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +
> > > +               return 1;
> > > +
> > > +       case PHY_INTERFACE_MODE_SGMII:
> > > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > > +               if (val < 0)
> > > +                       return val;
> > > +
> > > +               if (val == 0x0001)
> > > +                       return 0;
> > > +
> > > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +
> > > +               return 1;
> > > +
> > > +       default:
> > > +               /* Nothing to do for other modes */
> > > +               return 0;
> > > +       }
> > > +}
> > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
> > > +
> > > +/**
> > > + * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
> > > + * @pcs: a pointer to a &struct mdio_device.
> > > + *
> > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > + * clause 37 negotiation.
> > > + *
> > > + * Restart the clause 37 negotiation with the link partner. This is
> > > + * suitable to be directly plugged into the mac_pcs_get_state() member
> > > + * of the struct phylink_mac_ops structure.
> > > + */
> > > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
> > > +{
> > > +       struct mii_bus *bus = pcs->bus;
> > > +       int val, addr = pcs->addr;
> > > +
> > > +       val = mdiobus_read(bus, addr, MII_BMCR);
> > > +       if (val >= 0) {
> > > +               val |= BMCR_ANRESTART;
> > > +
> > > +               mdiobus_write(bus, addr, MII_BMCR, val);
> > > +       }
> > > +}
> > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
> > > +
> > >  MODULE_LICENSE("GPL v2");
> > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > > index 2180eb1aa254..de591c2fb37e 100644
> > > --- a/include/linux/phylink.h
> > > +++ b/include/linux/phylink.h
> > > @@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
> > >  void phylink_set_port_modes(unsigned long *bits);
> > >  void phylink_helper_basex_speed(struct phylink_link_state *state);
> > >
> > > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > > +                                  struct phylink_link_state *state);
> > > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > > +                                       const struct phylink_link_state *state);
> > > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
> > > +
> > >  #endif
> > > diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> > > index 0b9c3beda345..90f9b4e1ba27 100644
> > > --- a/include/uapi/linux/mii.h
> > > +++ b/include/uapi/linux/mii.h
> > > @@ -134,11 +134,16 @@
> > >  /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
> > >  #define ADVERTISE_SGMII                0x0001  /* MAC can do SGMII            */
> > >  #define LPA_SGMII              0x0001  /* PHY can do SGMII            */
> > > +#define LPA_SGMII_SPD_MASK     0x0c00  /* SGMII speed mask            */
> > > +#define LPA_SGMII_FULL_DUPLEX  0x1000  /* SGMII full duplex           */
> > >  #define LPA_SGMII_DPX_SPD_MASK 0x1C00  /* SGMII duplex and speed bits */
> > > +#define LPA_SGMII_10           0x0000  /* 10Mbps                      */
> > >  #define LPA_SGMII_10HALF       0x0000  /* Can do 10mbps half-duplex   */
> > >  #define LPA_SGMII_10FULL       0x1000  /* Can do 10mbps full-duplex   */
> > > +#define LPA_SGMII_100          0x0400  /* 100Mbps                     */
> > >  #define LPA_SGMII_100HALF      0x0400  /* Can do 100mbps half-duplex  */
> > >  #define LPA_SGMII_100FULL      0x1400  /* Can do 100mbps full-duplex  */
> > > +#define LPA_SGMII_1000         0x0800  /* 1000Mbps                    */
> >
> > These seem a bit mixed up to say the least.
> > If you're not going to use the (minimal) existing infrastructure could
> > you also refactor the one existing user? I think it would be better
> > than just keeping adding conflicting definitions.
>
> Sorry, but you need to explain better what you would like to see here.
> The additions I'm adding are to the SGMII specification; I find your
> existing definitions to be obscure because they conflate two different
> bit fields together to produce something for the ethtool linkmodes
> (which I think is a big mistake.)
>

I'm saying that there were already LPA_SGMII definitions in there.
There are 2 "generic" solutions proposed now and yet they cannot agree
on config_reg definitions. Omitting the fact that you did have a
chance to point out that big mistake before it got merged, I'm
wondering why you didn't remove them and add your new ones instead.
The code rework is minimal. Is it because the definitions are in UAPI?
If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
would user space care about the SGMII config_reg? There's no user even
of the previous SGMII definitions as far as I can tell.

> >
> > >  #define LPA_SGMII_1000HALF     0x0800  /* Can do 1000mbps half-duplex */
> > >  #define LPA_SGMII_1000FULL     0x1800  /* Can do 1000mbps full-duplex */
> > >  #define LPA_SGMII_LINK         0x8000  /* PHY link with copper-side partner */
> > > --
> > > 2.20.1
> > >
> >
> > Regards,
> > -Vladimir
> >
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 08:44:54PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 19:09, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Mar 11, 2020 at 04:06:52PM +0200, Vladimir Oltean wrote:
> > > On Wed, 11 Mar 2020 at 14:08, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > > >
> > > > Implement helpers for PCS accessed via the MII bus using 802.3 clause
> > > > 22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
> > > > for the advertisement word.
> > > >
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/phylink.h   |   6 ++
> > > >  include/uapi/linux/mii.h  |   5 +
> > > >  3 files changed, 217 insertions(+)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 19db68d74cb4..3ef20b34f5d6 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
> > > >
> > > > +static void phylink_decode_c37_word(struct phylink_link_state *state,
> > > > +                                   uint16_t config_reg, int speed)
> > > > +{
> > > > +       bool tx_pause, rx_pause;
> > > > +       int fd_bit;
> > > > +
> > > > +       if (speed == SPEED_2500)
> > > > +               fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
> > > > +       else
> > > > +               fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> > > > +
> > > > +       mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
> > > > +
> > > > +       if (linkmode_test_bit(fd_bit, state->advertising) &&
> > > > +           linkmode_test_bit(fd_bit, state->lp_advertising)) {
> > > > +               state->speed = speed;
> > > > +               state->duplex = DUPLEX_FULL;
> > > > +       } else {
> > > > +               /* negotiation failure */
> > > > +               state->link = false;
> > > > +       }
> > > > +
> > > > +       linkmode_resolve_pause(state->advertising, state->lp_advertising,
> > > > +                              &tx_pause, &rx_pause);
> > > > +
> > > > +       if (tx_pause)
> > > > +               state->pause |= MLO_PAUSE_TX;
> > > > +       if (rx_pause)
> > > > +               state->pause |= MLO_PAUSE_RX;
> > > > +}
> > > > +
> > > > +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> > > > +                                     uint16_t config_reg)
> > > > +{
> > > > +       if (!(config_reg & LPA_SGMII_LINK)) {
> > > > +               state->link = false;
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       switch (config_reg & LPA_SGMII_SPD_MASK) {
> > >
> > > Did you consider using or adapting the mii_lpa_mod_linkmode_adv_sgmii helper?
> >
> > Yes, and we _do not_ want to be touching state->lp_advertising here.
> > The link partner advertisement comes from the attached PHY, not from
> > this information.
> >
> 
> Understood, and I never suggested to touch state->lp_advertising, but
> rather pcs->lp_advertising.
> 
> > The config_reg information is not an advertisement - it is a
> > _configuration word_ coming from the PHY to tell the MAC what speed
> > and duplex to operate at.
> 
> Arguably the abuse here is Cisco's, since config_reg _was_ supposed to
> be an auto-neg advertisement as per clause 37. It is just the SGMII
> spec that makes it asymmetrical.
> 
> > It conveys nothing about whether it's
> > 1000baseT or 1000baseX, so link modes mean nothing as far as a SGMII
> > configuration word is concerned.
> 
> My initial point was that _for this pcs device_, the link partner _is_
> the PHY if it's SGMII. Otherwise you wouldn't find the config_reg in
> register 5.

So, why abuse some other subsystem's datastructure for something that
is entirely separate, potentially making the maintanence of that
subsystem more difficult for the maintainers?  I don't get why one
would think this is an acceptable approach.

What you've said is that you want to use struct phy_device, but you
don't want to publish it into the device model, you don't want to
use mdio accesses, you don't want to use phylib helpers.  So, what's
the point of using struct phy_device?  I don't see _any_ reason to
do that and make things unnecessarily more difficult for the phylib
maintainers.

> > Hence, _this_ is a more correct implementation than
> > mii_lpa_mod_linkmode_adv_sgmii().
> >
> > > > +       case LPA_SGMII_10:
> > > > +               state->speed = SPEED_10;
> > > > +               break;
> > > > +       case LPA_SGMII_100:
> > > > +               state->speed = SPEED_100;
> > > > +               break;
> > > > +       case LPA_SGMII_1000:
> > > > +               state->speed = SPEED_1000;
> > > > +               break;
> > > > +       default:
> > > > +               state->link = false;
> > > > +               return;
> > > > +       }
> > > > +       if (config_reg & LPA_SGMII_FULL_DUPLEX)
> > > > +               state->duplex = DUPLEX_FULL;
> > > > +       else
> > > > +               state->duplex = DUPLEX_HALF;
> > > > +}
> > > > +
> > > > +/**
> > > > + * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
> > > > + * @pcs: a pointer to a &struct mdio_device.
> > > > + * @state: a pointer to a &struct phylink_link_state.
> > > > + *
> > > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > > + * clause 37 negotiation and/or SGMII control.
> > > > + *
> > > > + * Read the MAC PCS state from the MII device configured in @config and
> > > > + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> > > > + * the phylink @state structure. This is suitable to be directly plugged
> > > > + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> > > > + * structure.
> > > > + */
> > > > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > > > +                                  struct phylink_link_state *state)
> > > > +{
> > > > +       struct mii_bus *bus = pcs->bus;
> > > > +       int addr = pcs->addr;
> > > > +       int bmsr, lpa;
> > > > +
> > > > +       bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > > > +       lpa = mdiobus_read(bus, addr, MII_LPA);
> > > > +       if (bmsr < 0 || lpa < 0) {
> > > > +               state->link = false;
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       state->link = !!(bmsr & BMSR_LSTATUS);
> > > > +       state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > > > +       if (!state->link)
> > > > +               return;
> > > > +
> > > > +       switch (state->interface) {
> > > > +       case PHY_INTERFACE_MODE_1000BASEX:
> > > > +               phylink_decode_c37_word(state, lpa, SPEED_1000);
> > > > +               break;
> > > > +
> > > > +       case PHY_INTERFACE_MODE_2500BASEX:
> > > > +               phylink_decode_c37_word(state, lpa, SPEED_2500);
> > > > +               break;
> > > > +
> > > > +       case PHY_INTERFACE_MODE_SGMII:
> > > > +               phylink_decode_sgmii_word(state, lpa);
> > > > +               break;
> > > > +
> > > > +       default:
> > > > +               state->link = false;
> > > > +               break;
> > > > +       }
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
> > > > +
> > > > +/**
> > > > + * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
> > > > + *     advertisement
> > > > + * @pcs: a pointer to a &struct mdio_device.
> > > > + * @state: a pointer to the state being configured.
> > > > + *
> > > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > > + * clause 37 negotiation and/or SGMII control.
> > > > + *
> > > > + * Configure the clause 37 PCS advertisement as specified by @state. This
> > > > + * does not trigger a renegotiation; phylink will do that via the
> > > > + * mac_an_restart() method of the struct phylink_mac_ops structure.
> > > > + *
> > > > + * Returns negative error code on failure to configure the advertisement,
> > > > + * zero if no change has been made, or one if the advertisement has changed.
> > > > + */
> > > > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > > > +                                       const struct phylink_link_state *state)
> > > > +{
> > > > +       struct mii_bus *bus = pcs->bus;
> > > > +       int addr = pcs->addr;
> > > > +       int val, ret;
> > > > +       u16 adv;
> > > > +
> > > > +       switch (state->interface) {
> > > > +       case PHY_INTERFACE_MODE_1000BASEX:
> > > > +       case PHY_INTERFACE_MODE_2500BASEX:
> > > > +               adv = ADVERTISE_1000XFULL;
> > > > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > > > +                                     state->advertising))
> > > > +                       adv |= ADVERTISE_1000XPAUSE;
> > > > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > > > +                                     state->advertising))
> > > > +                       adv |= ADVERTISE_1000XPSE_ASYM;
> > > > +
> > > > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > > > +               if (val < 0)
> > > > +                       return val;
> > > > +
> > > > +               if (val == adv)
> > > > +                       return 0;
> > > > +
> > > > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +
> > > > +               return 1;
> > > > +
> > > > +       case PHY_INTERFACE_MODE_SGMII:
> > > > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > > > +               if (val < 0)
> > > > +                       return val;
> > > > +
> > > > +               if (val == 0x0001)
> > > > +                       return 0;
> > > > +
> > > > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +
> > > > +               return 1;
> > > > +
> > > > +       default:
> > > > +               /* Nothing to do for other modes */
> > > > +               return 0;
> > > > +       }
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
> > > > +
> > > > +/**
> > > > + * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
> > > > + * @pcs: a pointer to a &struct mdio_device.
> > > > + *
> > > > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > > > + * clause 37 negotiation.
> > > > + *
> > > > + * Restart the clause 37 negotiation with the link partner. This is
> > > > + * suitable to be directly plugged into the mac_pcs_get_state() member
> > > > + * of the struct phylink_mac_ops structure.
> > > > + */
> > > > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
> > > > +{
> > > > +       struct mii_bus *bus = pcs->bus;
> > > > +       int val, addr = pcs->addr;
> > > > +
> > > > +       val = mdiobus_read(bus, addr, MII_BMCR);
> > > > +       if (val >= 0) {
> > > > +               val |= BMCR_ANRESTART;
> > > > +
> > > > +               mdiobus_write(bus, addr, MII_BMCR, val);
> > > > +       }
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
> > > > +
> > > >  MODULE_LICENSE("GPL v2");
> > > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > > > index 2180eb1aa254..de591c2fb37e 100644
> > > > --- a/include/linux/phylink.h
> > > > +++ b/include/linux/phylink.h
> > > > @@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
> > > >  void phylink_set_port_modes(unsigned long *bits);
> > > >  void phylink_helper_basex_speed(struct phylink_link_state *state);
> > > >
> > > > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > > > +                                  struct phylink_link_state *state);
> > > > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > > > +                                       const struct phylink_link_state *state);
> > > > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
> > > > +
> > > >  #endif
> > > > diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> > > > index 0b9c3beda345..90f9b4e1ba27 100644
> > > > --- a/include/uapi/linux/mii.h
> > > > +++ b/include/uapi/linux/mii.h
> > > > @@ -134,11 +134,16 @@
> > > >  /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
> > > >  #define ADVERTISE_SGMII                0x0001  /* MAC can do SGMII            */
> > > >  #define LPA_SGMII              0x0001  /* PHY can do SGMII            */
> > > > +#define LPA_SGMII_SPD_MASK     0x0c00  /* SGMII speed mask            */
> > > > +#define LPA_SGMII_FULL_DUPLEX  0x1000  /* SGMII full duplex           */
> > > >  #define LPA_SGMII_DPX_SPD_MASK 0x1C00  /* SGMII duplex and speed bits */
> > > > +#define LPA_SGMII_10           0x0000  /* 10Mbps                      */
> > > >  #define LPA_SGMII_10HALF       0x0000  /* Can do 10mbps half-duplex   */
> > > >  #define LPA_SGMII_10FULL       0x1000  /* Can do 10mbps full-duplex   */
> > > > +#define LPA_SGMII_100          0x0400  /* 100Mbps                     */
> > > >  #define LPA_SGMII_100HALF      0x0400  /* Can do 100mbps half-duplex  */
> > > >  #define LPA_SGMII_100FULL      0x1400  /* Can do 100mbps full-duplex  */
> > > > +#define LPA_SGMII_1000         0x0800  /* 1000Mbps                    */
> > >
> > > These seem a bit mixed up to say the least.
> > > If you're not going to use the (minimal) existing infrastructure could
> > > you also refactor the one existing user? I think it would be better
> > > than just keeping adding conflicting definitions.
> >
> > Sorry, but you need to explain better what you would like to see here.
> > The additions I'm adding are to the SGMII specification; I find your
> > existing definitions to be obscure because they conflate two different
> > bit fields together to produce something for the ethtool linkmodes
> > (which I think is a big mistake.)
> 
> I'm saying that there were already LPA_SGMII definitions in there.
> There are 2 "generic" solutions proposed now and yet they cannot agree
> on config_reg definitions. Omitting the fact that you did have a
> chance to point out that big mistake before it got merged, I'm
> wondering why you didn't remove them and add your new ones instead.
> The code rework is minimal. Is it because the definitions are in UAPI?
> If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> would user space care about the SGMII config_reg? There's no user even
> of the previous SGMII definitions as far as I can tell.

I don't see it as a big deal - certainly not the kind of fuss you're
making over it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>

>
> So, why abuse some other subsystem's datastructure for something that
> is entirely separate, potentially making the maintanence of that
> subsystem more difficult for the maintainers?  I don't get why one
> would think this is an acceptable approach.
>
> What you've said is that you want to use struct phy_device, but you
> don't want to publish it into the device model, you don't want to
> use mdio accesses, you don't want to use phylib helpers.  So, what's
> the point of using struct phy_device?  I don't see _any_ reason to
> do that and make things unnecessarily more difficult for the phylib
> maintainers.
>

So if it's such a big mistake...

> > > Sorry, but you need to explain better what you would like to see here.
> > > The additions I'm adding are to the SGMII specification; I find your
> > > existing definitions to be obscure because they conflate two different
> > > bit fields together to produce something for the ethtool linkmodes
> > > (which I think is a big mistake.)
> >
> > I'm saying that there were already LPA_SGMII definitions in there.
> > There are 2 "generic" solutions proposed now and yet they cannot agree
> > on config_reg definitions. Omitting the fact that you did have a
> > chance to point out that big mistake before it got merged, I'm
> > wondering why you didn't remove them and add your new ones instead.
> > The code rework is minimal. Is it because the definitions are in UAPI?
> > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > would user space care about the SGMII config_reg? There's no user even
> > of the previous SGMII definitions as far as I can tell.
>
> I don't see it as a big deal - certainly not the kind of fuss you're
> making over it.
>

...why keep it?
I'm all for creating a common interface for configuring this. It just
makes me wonder how common it is going to be, if there's already a
driver in-tree, from the same PCS hardware vendor, which after the
patchset you're proposing is still going to use a different
infrastructure.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Thanks,
-Vladimir

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 09:59:18PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > So, why abuse some other subsystem's datastructure for something that
> > is entirely separate, potentially making the maintanence of that
> > subsystem more difficult for the maintainers?  I don't get why one
> > would think this is an acceptable approach.
> >
> > What you've said is that you want to use struct phy_device, but you
> > don't want to publish it into the device model, you don't want to
> > use mdio accesses, you don't want to use phylib helpers.  So, what's
> > the point of using struct phy_device?  I don't see _any_ reason to
> > do that and make things unnecessarily more difficult for the phylib
> > maintainers.
> >
> 
> So if it's such a big mistake...
> 
> > > > Sorry, but you need to explain better what you would like to see here.
> > > > The additions I'm adding are to the SGMII specification; I find your
> > > > existing definitions to be obscure because they conflate two different
> > > > bit fields together to produce something for the ethtool linkmodes
> > > > (which I think is a big mistake.)
> > >
> > > I'm saying that there were already LPA_SGMII definitions in there.
> > > There are 2 "generic" solutions proposed now and yet they cannot agree
> > > on config_reg definitions. Omitting the fact that you did have a
> > > chance to point out that big mistake before it got merged, I'm
> > > wondering why you didn't remove them and add your new ones instead.
> > > The code rework is minimal. Is it because the definitions are in UAPI?
> > > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > > would user space care about the SGMII config_reg? There's no user even
> > > of the previous SGMII definitions as far as I can tell.
> >
> > I don't see it as a big deal - certainly not the kind of fuss you're
> > making over it.
> >
> 
> ...why keep it?
> I'm all for creating a common interface for configuring this. It just
> makes me wonder how common it is going to be, if there's already a
> driver in-tree, from the same PCS hardware vendor, which after the
> patchset you're proposing is still going to use a different
> infrastructure.

Do you see any reason why felix_vsc9959 couldn't make use of the code
I'm proposing?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 22:32, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 11, 2020 at 09:59:18PM +0200, Vladimir Oltean wrote:
> > On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > So, why abuse some other subsystem's datastructure for something that
> > > is entirely separate, potentially making the maintanence of that
> > > subsystem more difficult for the maintainers?  I don't get why one
> > > would think this is an acceptable approach.
> > >
> > > What you've said is that you want to use struct phy_device, but you
> > > don't want to publish it into the device model, you don't want to
> > > use mdio accesses, you don't want to use phylib helpers.  So, what's
> > > the point of using struct phy_device?  I don't see _any_ reason to
> > > do that and make things unnecessarily more difficult for the phylib
> > > maintainers.
> > >
> >
> > So if it's such a big mistake...
> >
> > > > > Sorry, but you need to explain better what you would like to see here.
> > > > > The additions I'm adding are to the SGMII specification; I find your
> > > > > existing definitions to be obscure because they conflate two different
> > > > > bit fields together to produce something for the ethtool linkmodes
> > > > > (which I think is a big mistake.)
> > > >
> > > > I'm saying that there were already LPA_SGMII definitions in there.
> > > > There are 2 "generic" solutions proposed now and yet they cannot agree
> > > > on config_reg definitions. Omitting the fact that you did have a
> > > > chance to point out that big mistake before it got merged, I'm
> > > > wondering why you didn't remove them and add your new ones instead.
> > > > The code rework is minimal. Is it because the definitions are in UAPI?
> > > > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > > > would user space care about the SGMII config_reg? There's no user even
> > > > of the previous SGMII definitions as far as I can tell.
> > >
> > > I don't see it as a big deal - certainly not the kind of fuss you're
> > > making over it.
> > >
> >
> > ...why keep it?
> > I'm all for creating a common interface for configuring this. It just
> > makes me wonder how common it is going to be, if there's already a
> > driver in-tree, from the same PCS hardware vendor, which after the
> > patchset you're proposing is still going to use a different
> > infrastructure.
>
> Do you see any reason why felix_vsc9959 couldn't make use of the code
> I'm proposing?
>

No. But the intentions just from reading the cover letter and the
patches seemed a bit unclear. The fact that there are no proposed
users in this series, and that in your private cex7 branch only dpaa2
uses it, it seemed to me that at least some clarification was due.
I have no further comments. The patches themselves are fairly trivial.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Thanks
-Vladimir

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Russell King - ARM Linux admin]

On Thu, Mar 12, 2020 at 02:54:55PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 22:32, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Mar 11, 2020 at 09:59:18PM +0200, Vladimir Oltean wrote:
> > > On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > > So, why abuse some other subsystem's datastructure for something that
> > > > is entirely separate, potentially making the maintanence of that
> > > > subsystem more difficult for the maintainers?  I don't get why one
> > > > would think this is an acceptable approach.
> > > >
> > > > What you've said is that you want to use struct phy_device, but you
> > > > don't want to publish it into the device model, you don't want to
> > > > use mdio accesses, you don't want to use phylib helpers.  So, what's
> > > > the point of using struct phy_device?  I don't see _any_ reason to
> > > > do that and make things unnecessarily more difficult for the phylib
> > > > maintainers.
> > > >
> > >
> > > So if it's such a big mistake...
> > >
> > > > > > Sorry, but you need to explain better what you would like to see here.
> > > > > > The additions I'm adding are to the SGMII specification; I find your
> > > > > > existing definitions to be obscure because they conflate two different
> > > > > > bit fields together to produce something for the ethtool linkmodes
> > > > > > (which I think is a big mistake.)
> > > > >
> > > > > I'm saying that there were already LPA_SGMII definitions in there.
> > > > > There are 2 "generic" solutions proposed now and yet they cannot agree
> > > > > on config_reg definitions. Omitting the fact that you did have a
> > > > > chance to point out that big mistake before it got merged, I'm
> > > > > wondering why you didn't remove them and add your new ones instead.
> > > > > The code rework is minimal. Is it because the definitions are in UAPI?
> > > > > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > > > > would user space care about the SGMII config_reg? There's no user even
> > > > > of the previous SGMII definitions as far as I can tell.
> > > >
> > > > I don't see it as a big deal - certainly not the kind of fuss you're
> > > > making over it.
> > > >
> > >
> > > ...why keep it?
> > > I'm all for creating a common interface for configuring this. It just
> > > makes me wonder how common it is going to be, if there's already a
> > > driver in-tree, from the same PCS hardware vendor, which after the
> > > patchset you're proposing is still going to use a different
> > > infrastructure.
> >
> > Do you see any reason why felix_vsc9959 couldn't make use of the code
> > I'm proposing?
> >
> 
> No. But the intentions just from reading the cover letter and the
> patches seemed a bit unclear. The fact that there are no proposed
> users in this series, and that in your private cex7 branch only dpaa2
> uses it, it seemed to me that at least some clarification was due.
> I have no further comments. The patches themselves are fairly trivial.

I have been told by Andrew to send small series, so that's what I do.

I have not included the DPAA2 changes in this series because it was
not ready for submission - I had to initially hard-code the physical
addresses of the MDIO blocks, but I've later moved to describing them
in the DTS, which now brings with it additional complexities since
(a) existing DTS need to continue working and (b) working out how to
submit those changes since the DTS changes and the net changes should
go via different paths, and ensuring that no breakage will occur
should they become separated.

If you look at the branches that I publish, you will notice that they
are based on v5.5, and so do not contain your changes to felix_vsc9959
that you have been talking about, so felix_vsc9959 is not yet on my
radar.

However, it seems we take different approaches to contributing code to
the kernel; I look to see whether there is value to providing common
infrastructure and then provide it, whereas you seem to take the
approach of writing specific drivers and hope that someone else spots
the code in your driver and converts it to something generic.  I
disagree with your approach because it's been well proven over the
years that the kernel has been around that relying on others to spot
code that could be refactored into common helpers just doesn't happen.
Yes, it happens but only occasionally, and not always when common
helpers get introduced.

You have already proven the worth of having a set of common helpers -
it seems that felix_vsc9959 and DPAA2 can both make use of these,
which does not surprise me one bit, since these helpers are only
implementing what is published in industry standards or defacto
industry standards - and as such are likely to be implemented by a lot
of vendors.  Sure, there will be exceptions and augmentations, which
is something I considered when creating these common helpers.  That's
why they are helpers rather than being mandatory implementations.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

[Vladimir Oltean]

On Thu, 12 Mar 2020 at 15:13, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Mar 12, 2020 at 02:54:55PM +0200, Vladimir Oltean wrote:
> > On Wed, 11 Mar 2020 at 22:32, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Mar 11, 2020 at 09:59:18PM +0200, Vladimir Oltean wrote:
> > > > On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > > > So, why abuse some other subsystem's datastructure for something that
> > > > > is entirely separate, potentially making the maintanence of that
> > > > > subsystem more difficult for the maintainers?  I don't get why one
> > > > > would think this is an acceptable approach.
> > > > >
> > > > > What you've said is that you want to use struct phy_device, but you
> > > > > don't want to publish it into the device model, you don't want to
> > > > > use mdio accesses, you don't want to use phylib helpers.  So, what's
> > > > > the point of using struct phy_device?  I don't see _any_ reason to
> > > > > do that and make things unnecessarily more difficult for the phylib
> > > > > maintainers.
> > > > >
> > > >
> > > > So if it's such a big mistake...
> > > >
> > > > > > > Sorry, but you need to explain better what you would like to see here.
> > > > > > > The additions I'm adding are to the SGMII specification; I find your
> > > > > > > existing definitions to be obscure because they conflate two different
> > > > > > > bit fields together to produce something for the ethtool linkmodes
> > > > > > > (which I think is a big mistake.)
> > > > > >
> > > > > > I'm saying that there were already LPA_SGMII definitions in there.
> > > > > > There are 2 "generic" solutions proposed now and yet they cannot agree
> > > > > > on config_reg definitions. Omitting the fact that you did have a
> > > > > > chance to point out that big mistake before it got merged, I'm
> > > > > > wondering why you didn't remove them and add your new ones instead.
> > > > > > The code rework is minimal. Is it because the definitions are in UAPI?
> > > > > > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > > > > > would user space care about the SGMII config_reg? There's no user even
> > > > > > of the previous SGMII definitions as far as I can tell.
> > > > >
> > > > > I don't see it as a big deal - certainly not the kind of fuss you're
> > > > > making over it.
> > > > >
> > > >
> > > > ...why keep it?
> > > > I'm all for creating a common interface for configuring this. It just
> > > > makes me wonder how common it is going to be, if there's already a
> > > > driver in-tree, from the same PCS hardware vendor, which after the
> > > > patchset you're proposing is still going to use a different
> > > > infrastructure.
> > >
> > > Do you see any reason why felix_vsc9959 couldn't make use of the code
> > > I'm proposing?
> > >
> >
> > No. But the intentions just from reading the cover letter and the
> > patches seemed a bit unclear. The fact that there are no proposed
> > users in this series, and that in your private cex7 branch only dpaa2
> > uses it, it seemed to me that at least some clarification was due.
> > I have no further comments. The patches themselves are fairly trivial.
>
> I have been told by Andrew to send small series, so that's what I do.
>
> I have not included the DPAA2 changes in this series because it was
> not ready for submission - I had to initially hard-code the physical
> addresses of the MDIO blocks, but I've later moved to describing them
> in the DTS, which now brings with it additional complexities since
> (a) existing DTS need to continue working and (b) working out how to
> submit those changes since the DTS changes and the net changes should
> go via different paths, and ensuring that no breakage will occur
> should they become separated.
>

I think even asking for firmware ABI changes in MC is worth a shot? It
might be preferable to get away with the firmware giving you the PCS
base address for a DPMAC rather than getting it from DT, for the
reasons that you've mentioned. Or even asking for ABI for the firmware
to perform the MDIO read/write itself.

> If you look at the branches that I publish, you will notice that they
> are based on v5.5, and so do not contain your changes to felix_vsc9959
> that you have been talking about, so felix_vsc9959 is not yet on my
> radar.
>

Yes, I noticed.

> However, it seems we take different approaches to contributing code to
> the kernel; I look to see whether there is value to providing common
> infrastructure and then provide it, whereas you seem to take the
> approach of writing specific drivers and hope that someone else spots
> the code in your driver and converts it to something generic.  I
> disagree with your approach because it's been well proven over the
> years that the kernel has been around that relying on others to spot
> code that could be refactored into common helpers just doesn't happen.
> Yes, it happens but only occasionally, and not always when common
> helpers get introduced.
>
> You have already proven the worth of having a set of common helpers -
> it seems that felix_vsc9959 and DPAA2 can both make use of these,
> which does not surprise me one bit, since these helpers are only
> implementing what is published in industry standards or defacto
> industry standards - and as such are likely to be implemented by a lot
> of vendors.  Sure, there will be exceptions and augmentations, which
> is something I considered when creating these common helpers.  That's
> why they are helpers rather than being mandatory implementations.
>

Nothing wrong with that. I'm willing to try to rework felix using
these helpers, but I just don't have the time right now. That and
DPAA2 make it look like it may take a while for the first users to
come... Apologies in advance if there are other immediate potential
users which are not on my radar.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

-Vladimir

[PATCH net-next 5/5] net: phylink: pcs: add 802.3 clause 45 helpers

[Russell King]

Implement helpers for PCS accessed via the MII bus using 802.3 clause
45 cycles for 10GBASE-R. Only link up/down is supported, 10G full
duplex is assumed.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 30 ++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3ef20b34f5d6..b83236b4d10a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2241,4 +2241,34 @@ void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
 
+#define C45_ADDR(d,a)	(MII_ADDR_C45 | (d) << 16 | (a))
+void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
+				   struct phylink_link_state *state)
+{
+	struct mii_bus *bus = pcs->bus;
+	int addr = pcs->addr;
+	int stat;
+
+	stat = mdiobus_read(bus, addr, C45_ADDR(MDIO_MMD_PCS, MDIO_STAT1));
+	if (stat < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(stat & MDIO_STAT1_LSTATUS);
+	if (!state->link)
+		return;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_10GBASER:
+		state->speed = SPEED_10000;
+		state->duplex = DUPLEX_FULL;
+		break;
+
+	default:
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c45_pcs_get_state);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index de591c2fb37e..8fa6df3b881b 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -323,4 +323,6 @@ int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
 					const struct phylink_link_state *state);
 void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
 
+void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
+				   struct phylink_link_state *state);
 #endif
-- 
2.20.1

Re: [PATCH net-next 0/5] add phylink support for PCS

[Vladimir Oltean]

Hi Russell,

On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> Hi,
>
> This series adds support for IEEE 802.3 register set compliant PCS
> for phylink.  In order to do this, we:
>
> 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
>    to a linkmode variant.
> 2. add a helper for clause 37 advertisements, supporting both the
>    1000baseX and defacto 2500baseX variants. Note that ethtool does
>    not support half duplex for either of these, and we make no effort
>    to do so.
> 3. add accessors for modifying a MDIO device register, and use them in
>    phylib, rather than duplicating the code from phylib.

Have you considered accessing the PCS as a phy_device structure, a la
drivers/net/dsa/ocelot/felix_vsc9959.c?

> 4. add support for decoding the advertisement from clause 22 compatible
>    register sets for clause 37 advertisements and SGMII advertisements.
> 5. add support for clause 45 register sets for 10GBASE-R PCS.
>
> These have been tested on the LX2160A Clearfog-CX platform.
>
>  drivers/net/phy/mdio_bus.c |  55 +++++++++++
>  drivers/net/phy/phy-core.c |  31 ------
>  drivers/net/phy/phylink.c  | 236 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mdio.h       |   4 +
>  include/linux/mii.h        |  57 +++++++----
>  include/linux/phy.h        |  19 ++++
>  include/linux/phylink.h    |   8 ++
>  include/uapi/linux/mii.h   |   5 +
>  8 files changed, 366 insertions(+), 49 deletions(-)
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Regards,
-Vladimir

Re: [PATCH net-next 0/5] add phylink support for PCS

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 02:46:33PM +0200, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > Hi,
> >
> > This series adds support for IEEE 802.3 register set compliant PCS
> > for phylink.  In order to do this, we:
> >
> > 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
> >    to a linkmode variant.
> > 2. add a helper for clause 37 advertisements, supporting both the
> >    1000baseX and defacto 2500baseX variants. Note that ethtool does
> >    not support half duplex for either of these, and we make no effort
> >    to do so.
> > 3. add accessors for modifying a MDIO device register, and use them in
> >    phylib, rather than duplicating the code from phylib.
> 
> Have you considered accessing the PCS as a phy_device structure, a la
> drivers/net/dsa/ocelot/felix_vsc9959.c?

I don't want to tie this into phylib, because I don't think phylib
should be dealing with PCS.  It brings with it many problems, such as:

1. how do we know whether the Clause 22 registers are supposed to be
   Clause 37 format.
2. how do we program the PCS appropriately for the negotiation results
   (which phylib doesn't support).
3. how do we deal with selecting the appropriate device for the mode
   selected (LX2160A has multiple different PCS which depend on the
   mode selected.)

Note that a phy_device structure embeds a mdio_device structure, and
so these helpers can be used inside phylib if one desires - so this
approach is more flexible than "bolt it into phylib" approach would
be.

> > 4. add support for decoding the advertisement from clause 22 compatible
> >    register sets for clause 37 advertisements and SGMII advertisements.
> > 5. add support for clause 45 register sets for 10GBASE-R PCS.
> >
> > These have been tested on the LX2160A Clearfog-CX platform.
> >
> >  drivers/net/phy/mdio_bus.c |  55 +++++++++++
> >  drivers/net/phy/phy-core.c |  31 ------
> >  drivers/net/phy/phylink.c  | 236 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mdio.h       |   4 +
> >  include/linux/mii.h        |  57 +++++++----
> >  include/linux/phy.h        |  19 ++++
> >  include/linux/phylink.h    |   8 ++
> >  include/uapi/linux/mii.h   |   5 +
> >  8 files changed, 366 insertions(+), 49 deletions(-)
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 0/5] add phylink support for PCS

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 14:54, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 11, 2020 at 02:46:33PM +0200, Vladimir Oltean wrote:
> > Hi Russell,
> >
> > On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > Hi,
> > >
> > > This series adds support for IEEE 802.3 register set compliant PCS
> > > for phylink.  In order to do this, we:
> > >
> > > 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
> > >    to a linkmode variant.
> > > 2. add a helper for clause 37 advertisements, supporting both the
> > >    1000baseX and defacto 2500baseX variants. Note that ethtool does
> > >    not support half duplex for either of these, and we make no effort
> > >    to do so.
> > > 3. add accessors for modifying a MDIO device register, and use them in
> > >    phylib, rather than duplicating the code from phylib.
> >
> > Have you considered accessing the PCS as a phy_device structure, a la
> > drivers/net/dsa/ocelot/felix_vsc9959.c?
>
> I don't want to tie this into phylib, because I don't think phylib
> should be dealing with PCS.  It brings with it many problems, such as:
>

Agree that the struct mdio_device -> struct phy_device diff is pretty
much useless to a PCS.

> 1. how do we know whether the Clause 22 registers are supposed to be
>    Clause 37 format.

Well, they are, aren't they?

> 2. how do we program the PCS appropriately for the negotiation results
>    (which phylib doesn't support).

You mean how to read the LPA and logically-AND it with ADV?
The PCS doesn't need to be "programmed" according to the resolved link
state. Maybe the MAC does.

> 3. how do we deal with selecting the appropriate device for the mode
>    selected (LX2160A has multiple different PCS which depend on the
>    mode selected.)

What I've been doing is to call get_phy_device with an is_c45 argument
depending on the PHY interface type.
Actually the real problem in your case is that the LX2160A doesn't
expose a valid PHY ID in registers 2&3 (unlike other Layerscape PCS
implementations), so get_phy_device is likely going to fail unless
some sort of PHY ID fixup is not done.

>
> Note that a phy_device structure embeds a mdio_device structure, and
> so these helpers can be used inside phylib if one desires - so this
> approach is more flexible than "bolt it into phylib" approach would
> be.
>

It's hard to really say without seeing more than one caller of these
new helpers.
For example the sja1105 DSA switch has a PCS for SGMII (not supported
yet in mainline) that kind-of-emulates a C22 register map, except that
it's accessed over SPI, and that the "pcs_get_state" needs to look at
some vendor-specific registers too. From that perspective, I was
thinking that PHYLINK could be given a phy_device structure with the
advertising, supported and lp_advertising linkmode bit fields
populated who-knows-how, and PHYLINK just resolves that into its
phylink_link_state structure.
But then I guess that sort of hardware is not among your target
candidates for the generic helpers. Whoever can't expose an MDIO bus
or needs to access any vendor-specific register just shouldn't use
these functions. And maybe you're right, I don't really know what the
balance in practice will be.


> > > 4. add support for decoding the advertisement from clause 22 compatible
> > >    register sets for clause 37 advertisements and SGMII advertisements.
> > > 5. add support for clause 45 register sets for 10GBASE-R PCS.
> > >
> > > These have been tested on the LX2160A Clearfog-CX platform.
> > >
> > >  drivers/net/phy/mdio_bus.c |  55 +++++++++++
> > >  drivers/net/phy/phy-core.c |  31 ------
> > >  drivers/net/phy/phylink.c  | 236 +++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mdio.h       |   4 +
> > >  include/linux/mii.h        |  57 +++++++----
> > >  include/linux/phy.h        |  19 ++++
> > >  include/linux/phylink.h    |   8 ++
> > >  include/uapi/linux/mii.h   |   5 +
> > >  8 files changed, 366 insertions(+), 49 deletions(-)
> > >
> > > --
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Regards,
-Vladimir

Re: [PATCH net-next 0/5] add phylink support for PCS

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 03:57:18PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 14:54, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Mar 11, 2020 at 02:46:33PM +0200, Vladimir Oltean wrote:
> > > Hi Russell,
> > >
> > > On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > Hi,
> > > >
> > > > This series adds support for IEEE 802.3 register set compliant PCS
> > > > for phylink.  In order to do this, we:
> > > >
> > > > 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
> > > >    to a linkmode variant.
> > > > 2. add a helper for clause 37 advertisements, supporting both the
> > > >    1000baseX and defacto 2500baseX variants. Note that ethtool does
> > > >    not support half duplex for either of these, and we make no effort
> > > >    to do so.
> > > > 3. add accessors for modifying a MDIO device register, and use them in
> > > >    phylib, rather than duplicating the code from phylib.
> > >
> > > Have you considered accessing the PCS as a phy_device structure, a la
> > > drivers/net/dsa/ocelot/felix_vsc9959.c?
> >
> > I don't want to tie this into phylib, because I don't think phylib
> > should be dealing with PCS.  It brings with it many problems, such as:
> >
> 
> Agree that the struct mdio_device -> struct phy_device diff is pretty
> much useless to a PCS.
> 
> > 1. how do we know whether the Clause 22 registers are supposed to be
> >    Clause 37 format.
> 
> Well, they are, aren't they?

For a PCS, yes, but phylib generally deals with clause 22 format
registers (which define the copper capabilities rather than 1000baseX
which clause 37 covers.)

> 
> > 2. how do we program the PCS appropriately for the negotiation results
> >    (which phylib doesn't support).
> 
> You mean how to read the LPA and logically-AND it with ADV?
> The PCS doesn't need to be "programmed" according to the resolved link
> state. Maybe the MAC does.

No, I'm talking about configuring the PCS for SGMII mode when in-band
AN is not being used.

> > 3. how do we deal with selecting the appropriate device for the mode
> >    selected (LX2160A has multiple different PCS which depend on the
> >    mode selected.)
> 
> What I've been doing is to call get_phy_device with an is_c45 argument
> depending on the PHY interface type.
> Actually the real problem in your case is that the LX2160A doesn't
> expose a valid PHY ID in registers 2&3 (unlike other Layerscape PCS
> implementations), so get_phy_device is likely going to fail unless
> some sort of PHY ID fixup is not done.

What SolidRun are pressing NXP for on the LX2160A is to move the
networking support to a more dynamic arrangement than it is at
present - I know there was a conference call on Monday about this,
but I only found out about it too late to be part of it, and so far
no one has filled me in on what was discussed.

However, SolidRun wish the networking to be more dynamically
configurable on the LX2160A - right now, we have a problem that we
can either configure _all_ the QSFP+ and SFP ports (e.g.) to 10G
mode, or _all_ QSFP+ and SFP ports to 1G mode - which basically
makes the QSFP+ useless.  It's too inflexible.

What I would like to see are individual ports or groups of ports
being able to be reconfigured on the fly, which means sticking to
one particular PCS will not be possible.

Other platforms do support dynamically switching between different
components depending on the speed, and I see no reason to prevent
such flexibility in phylink by designing into it a "you can only
have one PCS device" assumption.

> > Note that a phy_device structure embeds a mdio_device structure, and
> > so these helpers can be used inside phylib if one desires - so this
> > approach is more flexible than "bolt it into phylib" approach would
> > be.
> >
> 
> It's hard to really say without seeing more than one caller of these
> new helpers.
> For example the sja1105 DSA switch has a PCS for SGMII (not supported
> yet in mainline) that kind-of-emulates a C22 register map, except that
> it's accessed over SPI, and that the "pcs_get_state" needs to look at
> some vendor-specific registers too.

I don't think "it's accessed over SPI" is much of an issue at all.
The solution to that is trivial - as has been already shown with
PHYs that are accessed over I2C.

> From that perspective, I was
> thinking that PHYLINK could be given a phy_device structure with the
> advertising, supported and lp_advertising linkmode bit fields
> populated who-knows-how, and PHYLINK just resolves that into its
> phylink_link_state structure.
> But then I guess that sort of hardware is not among your target
> candidates for the generic helpers. Whoever can't expose an MDIO bus
> or needs to access any vendor-specific register just shouldn't use
> these functions. And maybe you're right, I don't really know what the
> balance in practice will be.

I don't see why you'd think that having a phy_device structure would
make it easier to access a SPI based PHY.  If you can't access the
PHY over MDIO, none of the phylib helpers can be used, so you're
just using struct phy_device as a container and wrapping it with a
load of custom code.

As long as the phy_device structure is registered with the device
model, phy drivers can potentially bind with the "special" phy device
and attempt to access it via the MDIO bus - it sounds like that's
something you don't want to happen.

So, I'd question whether it makes any sense to (ab)use struct phy_device
for something that is not going to use phylib at all.

It seems way more sensible to have a "struct pcs_device" that operates
entirely separately to phylib - and maybe we can lift some of the phylib
functionality to mdio_device level (such as what I've done with
accessors, but maybe more stuff) so it can be spared between PCS and
conventional phylib users.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 0/5] add phylink support for PCS

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 19:05, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 11, 2020 at 03:57:18PM +0200, Vladimir Oltean wrote:
> > On Wed, 11 Mar 2020 at 14:54, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Mar 11, 2020 at 02:46:33PM +0200, Vladimir Oltean wrote:
> > > > Hi Russell,
> > > >
> > > > On Wed, 11 Mar 2020 at 14:09, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > This series adds support for IEEE 802.3 register set compliant PCS
> > > > > for phylink.  In order to do this, we:
> > > > >
> > > > > 1. convert the existing (unused) mii_lpa_to_ethtool_lpa_x() function
> > > > >    to a linkmode variant.
> > > > > 2. add a helper for clause 37 advertisements, supporting both the
> > > > >    1000baseX and defacto 2500baseX variants. Note that ethtool does
> > > > >    not support half duplex for either of these, and we make no effort
> > > > >    to do so.
> > > > > 3. add accessors for modifying a MDIO device register, and use them in
> > > > >    phylib, rather than duplicating the code from phylib.
> > > >
> > > > Have you considered accessing the PCS as a phy_device structure, a la
> > > > drivers/net/dsa/ocelot/felix_vsc9959.c?
> > >
> > > I don't want to tie this into phylib, because I don't think phylib
> > > should be dealing with PCS.  It brings with it many problems, such as:
> > >
> >
> > Agree that the struct mdio_device -> struct phy_device diff is pretty
> > much useless to a PCS.
> >
> > > 1. how do we know whether the Clause 22 registers are supposed to be
> > >    Clause 37 format.
> >
> > Well, they are, aren't they?
>
> For a PCS, yes, but phylib generally deals with clause 22 format
> registers (which define the copper capabilities rather than 1000baseX
> which clause 37 covers.)
>

I still don't get the point of why would [phylib] be confused about
whether to interpret capabilities as copper or fiber. Because in my
proposal, the MAC driver would populate the capabilities and not
phylib (genphy_read_abilities).

> >
> > > 2. how do we program the PCS appropriately for the negotiation results
> > >    (which phylib doesn't support).
> >
> > You mean how to read the LPA and logically-AND it with ADV?
> > The PCS doesn't need to be "programmed" according to the resolved link
> > state. Maybe the MAC does.
>
> No, I'm talking about configuring the PCS for SGMII mode when in-band
> AN is not being used.
>

Again, not with phylib.

> > > 3. how do we deal with selecting the appropriate device for the mode
> > >    selected (LX2160A has multiple different PCS which depend on the
> > >    mode selected.)
> >
> > What I've been doing is to call get_phy_device with an is_c45 argument
> > depending on the PHY interface type.
> > Actually the real problem in your case is that the LX2160A doesn't
> > expose a valid PHY ID in registers 2&3 (unlike other Layerscape PCS
> > implementations), so get_phy_device is likely going to fail unless
> > some sort of PHY ID fixup is not done.
>
> What SolidRun are pressing NXP for on the LX2160A is to move the
> networking support to a more dynamic arrangement than it is at
> present - I know there was a conference call on Monday about this,
> but I only found out about it too late to be part of it, and so far
> no one has filled me in on what was discussed.
>
> However, SolidRun wish the networking to be more dynamically
> configurable on the LX2160A - right now, we have a problem that we
> can either configure _all_ the QSFP+ and SFP ports (e.g.) to 10G
> mode, or _all_ QSFP+ and SFP ports to 1G mode - which basically
> makes the QSFP+ useless.  It's too inflexible.
>
> What I would like to see are individual ports or groups of ports
> being able to be reconfigured on the fly, which means sticking to
> one particular PCS will not be possible.
>
> Other platforms do support dynamically switching between different
> components depending on the speed, and I see no reason to prevent
> such flexibility in phylink by designing into it a "you can only
> have one PCS device" assumption.
>

DPAA2 is not exactly my turf anyway, but let's keep the discussion on point.
I think that neither your or my proposal would be inherently limited
to having a static PCS object, be it an mdio_device or a phy_device.
I'm not really sure how this came to be an argument. Hardware design
limitations are a separate topic.

> > > Note that a phy_device structure embeds a mdio_device structure, and
> > > so these helpers can be used inside phylib if one desires - so this
> > > approach is more flexible than "bolt it into phylib" approach would
> > > be.
> > >
> >
> > It's hard to really say without seeing more than one caller of these
> > new helpers.
> > For example the sja1105 DSA switch has a PCS for SGMII (not supported
> > yet in mainline) that kind-of-emulates a C22 register map, except that
> > it's accessed over SPI, and that the "pcs_get_state" needs to look at
> > some vendor-specific registers too.
>
> I don't think "it's accessed over SPI" is much of an issue at all.
> The solution to that is trivial - as has been already shown with
> PHYs that are accessed over I2C.
>

So your recommendation is to write an mdio-to-spi bridge, and keep
using the mdio_device object for a SPI-controlled PCS. Basically
another container.

> > From that perspective, I was
> > thinking that PHYLINK could be given a phy_device structure with the
> > advertising, supported and lp_advertising linkmode bit fields
> > populated who-knows-how, and PHYLINK just resolves that into its
> > phylink_link_state structure.
> > But then I guess that sort of hardware is not among your target
> > candidates for the generic helpers. Whoever can't expose an MDIO bus
> > or needs to access any vendor-specific register just shouldn't use
> > these functions. And maybe you're right, I don't really know what the
> > balance in practice will be.
>
> I don't see why you'd think that having a phy_device structure would
> make it easier to access a SPI based PHY.  If you can't access the
> PHY over MDIO, none of the phylib helpers can be used,

Same can be said when using the mdio_device structure for the PCS, really.

> so you're
> just using struct phy_device as a container and wrapping it with a
> load of custom code.
>

Yes.

> As long as the phy_device structure is registered with the device
> model, phy drivers can potentially bind with the "special" phy device
> and attempt to access it via the MDIO bus - it sounds like that's
> something you don't want to happen.
>

Yes, correct. But it's not registered with the device model. It's just
a data structure privately used by the MAC driver.

> So, I'd question whether it makes any sense to (ab)use struct phy_device
> for something that is not going to use phylib at all.
>

My reasoning was that I didn't want to do the bit twiddling with the
config word in the driver, and PHYLIB already having helpers for that
sounded good.
At no point had it crossed my mind that PHYLINK would even be a
candidate for hosting these functions. I guess PHYLINK is the new
PHYLIB now?

> It seems way more sensible to have a "struct pcs_device" that operates
> entirely separately to phylib - and maybe we can lift some of the phylib
> functionality to mdio_device level (such as what I've done with
> accessors, but maybe more stuff) so it can be spared between PCS and
> conventional phylib users.
>

Yes, with a little more feedback on the "Convert Felix DSA switch to
PHYLINK" series, I would have probably ended up doing exactly that.
Abusing a phy_device structure is not exactly amazing.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up