[PATCH REPOST3 net-next 0/3] net: add phylink support for PCS

[PATCH REPOST3 net-next 0/3] net: add phylink support for PCS

[Russell King - ARM Linux admin]

Depends on "net: mii clause 37 helpers".

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

1. add accessors for modifying a MDIO device register, and use them in
   phylib, rather than duplicating the code from phylib.
2. add support for decoding the advertisement from clause 22 compatible
   register sets for clause 37 advertisements and SGMII advertisements.
3. add support for clause 45 register sets for 10GBASE-R PCS.

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

This is a re-post of the series previously sent, but with the first two
patches separated out; the conclusion of the discussion with Vladimir
seemed to be that there was no issue with the patches themselves.

 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/phy.h        |  19 ++++
 include/linux/phylink.h    |   8 ++
 include/uapi/linux/mii.h   |   5 +
 7 files changed, 327 insertions(+), 31 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/3] 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 7a08023bdbc5..c56166487036 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -750,6 +750,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 2/3] 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 a8eeaabb2d18..7ca427c46d9f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2041,4 +2041,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

[PATCH net-next 3/3] 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 7ca427c46d9f..bff570f59d5c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2247,4 +2247,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 3/3] net: phylink: pcs: add 802.3 clause 45 helpers

[Andrew Lunn]

On Sat, Mar 14, 2020 at 10:31:34AM +0000, Russell King wrote:
> 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 7ca427c46d9f..bff570f59d5c 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2247,4 +2247,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))

Hi Russell

That seems like a macro that should be made global.

dsa/mv88e6xxx/serdes.c: int reg_c45 = MII_ADDR_C45 | device << 16 | reg;
dsa/mv88e6xxx/serdes.c: int reg_c45 = MII_ADDR_C45 | device << 16 | reg;
ethernet/intel/ixgbe/ixgbe_main.c:                      regnum |= (devad << 16) | MII_ADDR_C45;
ethernet/intel/ixgbe/ixgbe_main.c:                      regnum |= (devad << 16) | MII_ADDR_C45;
phy/phylink.c:          devad = MII_ADDR_C45 | devad << 16 | reg;
phy/phylink.c:          devad = MII_ADDR_C45 | devad << 16 | reg;
phy/phylink.c:          devad = MII_ADDR_C45 | devad << 16 | reg;
phy/phylink.c:          devad = MII_ADDR_C45 | devad << 16 | reg;
phy/phy-core.c:         u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
phy/phy-core.c:         u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
phy/phy_device.c:       reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS2;
phy/phy_device.c:       reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS1;
phy/phy_device.c:               reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1;
phy/phy_device.c:               reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;
phy/phy.c:                      devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
phy/phy.c:                      devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
phy/bcm87xx.c:          u32 regnum = MII_ADDR_C45 | (devid << 16) | reg;

I'm not suggesting you convert all these cases, just make the macro
available and we can make more use of it later.

Thanks
	Andrew

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

[Andrew Lunn]

On Sat, Mar 14, 2020 at 10:31:24AM +0000, Russell King wrote:
> 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());

Hi Russell

There seems to be growing push back on using BUG_ON and its
variants. If should only be used if the system is so badly messed up,
going further would only cause more damage. What really happens here
if it is called in interrupt context? The mutex lock probably won't
work, and we might corrupt the state of the PCS. That is not the end
of the world. So i would suggest a WARN_ON here.

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

  Andrew

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

[Andrew Lunn]

On Sat, Mar 14, 2020 at 10:31:02AM +0000, Russell King - ARM Linux admin wrote:
> Depends on "net: mii clause 37 helpers".
> 
> This series adds support for IEEE 802.3 register set compliant PCS
> for phylink.  In order to do this, we:
> 
> 1. add accessors for modifying a MDIO device register, and use them in
>    phylib, rather than duplicating the code from phylib.
> 2. add support for decoding the advertisement from clause 22 compatible
>    register sets for clause 37 advertisements and SGMII advertisements.
> 3. add support for clause 45 register sets for 10GBASE-R PCS.

Hi Russell

How big is the patchset which actually makes use of this code? It is
normal to add helpers and at least one user in the same patchset. But
if that would make the patchset too big, there could be some leeway.

   Thanks
	Andrew

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

[Russell King - ARM Linux admin]

On Sat, Mar 14, 2020 at 11:00:18PM +0100, Andrew Lunn wrote:
> On Sat, Mar 14, 2020 at 10:31:02AM +0000, Russell King - ARM Linux admin wrote:
> > Depends on "net: mii clause 37 helpers".
> > 
> > This series adds support for IEEE 802.3 register set compliant PCS
> > for phylink.  In order to do this, we:
> > 
> > 1. add accessors for modifying a MDIO device register, and use them in
> >    phylib, rather than duplicating the code from phylib.
> > 2. add support for decoding the advertisement from clause 22 compatible
> >    register sets for clause 37 advertisements and SGMII advertisements.
> > 3. add support for clause 45 register sets for 10GBASE-R PCS.
> 
> Hi Russell
> 
> How big is the patchset which actually makes use of this code? It is
> normal to add helpers and at least one user in the same patchset. But
> if that would make the patchset too big, there could be some leeway.

The minimum is three patches:

arm64: dts: lx2160a: add PCS MDIO nodes
dpaa2-mac: add 1000BASE-X/SGMII PCS support
dpaa2-mac: add 10GBASE-R PCS support

but for it to actually be usable on hardware, it needs more than that:

arm64: dts: lx2160a-clearfog-itx: add SFP support

and, at the moment, depending on whether you want 1G or 10G speeds,
changes to the board firmware to select the serdes group mode.

The DTS patches can't go through netdev obviously, and it may be
too late to get them queued through the proper channels.

-- 
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 1/3] net: mdiobus: add APIs for modifying a MDIO device register

[Russell King - ARM Linux admin]

On Sat, Mar 14, 2020 at 10:57:28PM +0100, Andrew Lunn wrote:
> On Sat, Mar 14, 2020 at 10:31:24AM +0000, Russell King wrote:
> > 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());
> 
> Hi Russell
> 
> There seems to be growing push back on using BUG_ON and its
> variants. If should only be used if the system is so badly messed up,
> going further would only cause more damage. What really happens here
> if it is called in interrupt context? The mutex lock probably won't
> work, and we might corrupt the state of the PCS. That is not the end
> of the world. So i would suggest a WARN_ON here.

Do we even need these checks? (phylib has them scattered throughout
on the bus accessors.)  Aren't the might_sleep() checks that are
already in the locking functions already sufficient?

-- 
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 1/3] net: mdiobus: add APIs for modifying a MDIO device register

[Andrew Lunn]

> > Hi Russell
> > 
> > There seems to be growing push back on using BUG_ON and its
> > variants. If should only be used if the system is so badly messed up,
> > going further would only cause more damage. What really happens here
> > if it is called in interrupt context? The mutex lock probably won't
> > work, and we might corrupt the state of the PCS. That is not the end
> > of the world. So i would suggest a WARN_ON here.
> 
> Do we even need these checks? (phylib has them scattered throughout
> on the bus accessors.)  Aren't the might_sleep() checks that are
> already in the locking functions already sufficient?

Hi Russell

I agree, the might_sleep() should be sufficient.

  Andrew

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

[Andrew Lunn]

On Sat, Mar 14, 2020 at 10:44:59PM +0000, Russell King - ARM Linux admin wrote:
> On Sat, Mar 14, 2020 at 11:00:18PM +0100, Andrew Lunn wrote:
> > On Sat, Mar 14, 2020 at 10:31:02AM +0000, Russell King - ARM Linux admin wrote:
> > > Depends on "net: mii clause 37 helpers".
> > > 
> > > This series adds support for IEEE 802.3 register set compliant PCS
> > > for phylink.  In order to do this, we:
> > > 
> > > 1. add accessors for modifying a MDIO device register, and use them in
> > >    phylib, rather than duplicating the code from phylib.
> > > 2. add support for decoding the advertisement from clause 22 compatible
> > >    register sets for clause 37 advertisements and SGMII advertisements.
> > > 3. add support for clause 45 register sets for 10GBASE-R PCS.
> > 
> > Hi Russell
> > 
> > How big is the patchset which actually makes use of this code? It is
> > normal to add helpers and at least one user in the same patchset. But
> > if that would make the patchset too big, there could be some leeway.
> 
> The minimum is three patches:
> 
> arm64: dts: lx2160a: add PCS MDIO nodes
> dpaa2-mac: add 1000BASE-X/SGMII PCS support
> dpaa2-mac: add 10GBASE-R PCS support

Hi Russell

Are the two dpaa2-mac changes safe without the DT changes? I guess
so. So it seems sensible to post a set of 5 patches.

> and, at the moment, depending on whether you want 1G or 10G speeds,
> changes to the board firmware to select the serdes group mode.

And this is where we start speculating. I guess a new firmware API
will be needed to allow for runtime selection of the serdes group
mode. But i guess such an API change would not invalidate the PCS
work?  There is still likely to be two PCS. So it seems O.K. to merge
this, and then fix it up later to work with whatever is added to the
firmware. There is on KAPI involved here?

	Andrew

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

[Russell King - ARM Linux admin]

On Tue, Mar 17, 2020 at 03:18:39PM +0100, Andrew Lunn wrote:
> On Sat, Mar 14, 2020 at 10:44:59PM +0000, Russell King - ARM Linux admin wrote:
> > On Sat, Mar 14, 2020 at 11:00:18PM +0100, Andrew Lunn wrote:
> > > On Sat, Mar 14, 2020 at 10:31:02AM +0000, Russell King - ARM Linux admin wrote:
> > > > Depends on "net: mii clause 37 helpers".
> > > > 
> > > > This series adds support for IEEE 802.3 register set compliant PCS
> > > > for phylink.  In order to do this, we:
> > > > 
> > > > 1. add accessors for modifying a MDIO device register, and use them in
> > > >    phylib, rather than duplicating the code from phylib.
> > > > 2. add support for decoding the advertisement from clause 22 compatible
> > > >    register sets for clause 37 advertisements and SGMII advertisements.
> > > > 3. add support for clause 45 register sets for 10GBASE-R PCS.
> > > 
> > > Hi Russell
> > > 
> > > How big is the patchset which actually makes use of this code? It is
> > > normal to add helpers and at least one user in the same patchset. But
> > > if that would make the patchset too big, there could be some leeway.
> > 
> > The minimum is three patches:
> > 
> > arm64: dts: lx2160a: add PCS MDIO nodes
> > dpaa2-mac: add 1000BASE-X/SGMII PCS support
> > dpaa2-mac: add 10GBASE-R PCS support
> 
> Hi Russell
> 
> Are the two dpaa2-mac changes safe without the DT changes? I guess
> so. So it seems sensible to post a set of 5 patches.

That would need to be tested; it hasn't yet been tested to prove
that nothing breaks as a result.

> > and, at the moment, depending on whether you want 1G or 10G speeds,
> > changes to the board firmware to select the serdes group mode.
> 
> And this is where we start speculating.

It is not speculation, what I've said is factual.  "At the moment"
describes the present situation.  If it was any different, then the
discussions that are going on between SolidRun and NXP would have
been over very quickly.  This was raised back in December, and
conference calls are still on-going on this issue, so that's about
four months so far.

There is also some evidence that if we attempt to reprogram the
Serdes PLLs at runtime, that will raise an exception and completely
reset the chip. There is also some evidence that experimenting with
changing the setup somehow bricked one of the Honeycomb boards.

So no, what I've said is not speculation.

The fact is that today, I need two different firmware images, one for
1G speeds and another for 10G speeds on _all_ the SFP+/QSFP+ cages -
it's either all at 1G or all at 10G.  This is due to the "reset
configuration word" block that is loaded from boot media at reset time
to configure the hardware.

Even different RAM speeds need different RCW contents and therefore
different firmware.

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