Make BCM84891-based RollBall SFP-10G-T modules work, and stop them costing 1.7 load average

drivers/net/mdio/mdio-i2c.c

@@ -11,6 +11,7 @@
  * of their settings.
  */
 #include <linux/i2c.h>
+#include <linux/jiffies.h>
 #include <linux/mdio/mdio-i2c.h>
 #include <linux/phy.h>
 #include <linux/sfp.h>
@@ -195,6 +196,26 @@ static int smbus_byte_mii_write_default_c22(struct mii_bus *bus, int phy_id,
 #define ROLLBALL_CMD_READ		0x02
 #define ROLLBALL_CMD_DONE		0x04
 
+/* Wall-clock budget for i2c_rollball_mii_poll(): generous enough to
+ * ride out the module's post-insertion wake window, during which the
+ * mailbox can stall for hundreds of ms.
+ */
+#define ROLLBALL_POLL_TIMEOUT_MS	1000
+
+/* Completion-poll backoff: first sleep is short so modules that answer
+ * in a couple of ms return promptly; the interval then doubles up to
+ * the cap so modules that take tens of ms per command are not hammered
+ * with status reads (each poll iteration costs four I2C transactions
+ * for the page save/set/read/restore dance).
+ */
+#define ROLLBALL_POLL_SLEEP_MIN_US	2000
+#define ROLLBALL_POLL_SLEEP_MAX_US	32000
+
+/* Number of times a command is re-issued if the module does not
+ * signal ROLLBALL_CMD_DONE within the polling budget.
+ */
+#define ROLLBALL_CMD_RETRIES		3
+
 #define SFP_PAGE_ROLLBALL_MDIO		3
 
 static int __i2c_transfer_err(struct i2c_adapter *i2c, struct i2c_msg *msgs,
@@ -298,7 +319,9 @@ static int i2c_rollball_mii_poll(struct mii_bus *bus, int bus_addr, u8 *buf,
 	struct i2c_adapter *i2c = bus->priv;
 	struct i2c_msg msgs[2];
 	u8 cmd_addr, tmp, *res;
-	int i, ret;
+	unsigned long deadline;
+	unsigned long sleep_us;
+	int ret;
 
 	cmd_addr = ROLLBALL_CMD_ADDR;
 
@@ -315,20 +338,33 @@ static int i2c_rollball_mii_poll(struct mii_bus *bus, int bus_addr, u8 *buf,
 	msgs[1].len = len;
 	msgs[1].buf = res;
 
-	/* By experiment it takes up to 70 ms to access a register for these
-	 * SFPs. Sleep 20ms between iterations and try 10 times.
+	/* Some of these SFPs answer within a couple of ms once running,
+	 * others take tens of ms per command, and any of them can stall
+	 * for hundreds of ms while the module is still waking after
+	 * insertion. Poll with exponential backoff: the first iteration
+	 * sleeps only a couple of ms so a fast module returns promptly
+	 * instead of paying a fixed 20 ms per register access, while the
+	 * backoff keeps the number of status-poll I2C transactions for a
+	 * slow module close to the fixed-interval scheme's. The whole
+	 * loop is bounded by a generous wall-clock budget for the wake
+	 * window. Sleep as TASK_IDLE (usleep_range_idle): the CPU is free
+	 * while we wait, so the poll must not inflate the load average.
 	 */
-	i = 10;
+	deadline = jiffies + msecs_to_jiffies(ROLLBALL_POLL_TIMEOUT_MS);
+	sleep_us = ROLLBALL_POLL_SLEEP_MIN_US;
 	do {
-		msleep(20);
+		usleep_range_idle(sleep_us, sleep_us * 2);
 
 		ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
 		if (ret)
 			return ret;
 
 		if (*res == ROLLBALL_CMD_DONE)
 			return 0;
-	} while (i-- > 0);
+
+		sleep_us = min_t(unsigned long, sleep_us * 2,
+				 ROLLBALL_POLL_SLEEP_MAX_US);
+	} while (time_before(jiffies, deadline));
 
 	dev_dbg(&bus->dev, "poll timed out\n");
 
@@ -362,7 +398,7 @@ static int i2c_mii_read_rollball(struct mii_bus *bus, int phy_id, int devad,
 				 int reg)
 {
 	u8 buf[4], res[6];
-	int bus_addr, ret;
+	int bus_addr, ret, i;
 	u16 val;
 
 	bus_addr = i2c_mii_phy_addr(phy_id);
@@ -374,12 +410,23 @@ static int i2c_mii_read_rollball(struct mii_bus *bus, int phy_id, int devad,
 	buf[2] = (reg >> 8) & 0xff;
 	buf[3] = reg & 0xff;
 
-	ret = i2c_rollball_mii_cmd(bus, bus_addr, ROLLBALL_CMD_READ, buf,
-				   sizeof(buf));
-	if (ret < 0)
-		return ret;
+	/* Some modules (e.g. OEM SFP-10G-T with a BCM84891) occasionally
+	 * take longer than the polling budget to execute a command. Since
+	 * returning 0xffff for a timed-out read is indistinguishable from
+	 * register data, retry the command a few times first; re-issuing
+	 * the same read is idempotent.
+	 */
+	for (i = 0; i < ROLLBALL_CMD_RETRIES; i++) {
+		ret = i2c_rollball_mii_cmd(bus, bus_addr, ROLLBALL_CMD_READ,
+					   buf, sizeof(buf));
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_rollball_mii_poll(bus, bus_addr, res, sizeof(res));
+		if (ret != -ETIMEDOUT)
+			break;
+	}
 
-	ret = i2c_rollball_mii_poll(bus, bus_addr, res, sizeof(res));
 	if (ret == -ETIMEDOUT)
 		return 0xffff;
 	else if (ret < 0)
@@ -393,7 +440,7 @@ static int i2c_mii_read_rollball(struct mii_bus *bus, int phy_id, int devad,
 static int i2c_mii_write_rollball(struct mii_bus *bus, int phy_id, int devad,
 				  int reg, u16 val)
 {
-	int bus_addr, ret;
+	int bus_addr, ret, i;
 	u8 buf[6];
 
 	bus_addr = i2c_mii_phy_addr(phy_id);
@@ -407,12 +454,17 @@ static int i2c_mii_write_rollball(struct mii_bus *bus, int phy_id, int devad,
 	buf[4] = val >> 8;
 	buf[5] = val & 0xff;
 
-	ret = i2c_rollball_mii_cmd(bus, bus_addr, ROLLBALL_CMD_WRITE, buf,
-				   sizeof(buf));
-	if (ret < 0)
-		return ret;
+	for (i = 0; i < ROLLBALL_CMD_RETRIES; i++) {
+		ret = i2c_rollball_mii_cmd(bus, bus_addr, ROLLBALL_CMD_WRITE,
+					   buf, sizeof(buf));
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_rollball_mii_poll(bus, bus_addr, NULL, 0);
+		if (ret != -ETIMEDOUT)
+			break;
+	}
 
-	ret = i2c_rollball_mii_poll(bus, bus_addr, NULL, 0);
 	if (ret < 0)
 		return ret;
 

drivers/net/phy/bcm84881.c

@@ -83,10 +83,24 @@ static int bcm84881_config_init(struct phy_device *phydev)
 
 static int bcm8489x_config_init(struct phy_device *phydev)
 {
+	bcm84881_fill_possible_interfaces(phydev);
 	__set_bit(PHY_INTERFACE_MODE_USXGMII, phydev->possible_interfaces);
 
-	if (phydev->interface != PHY_INTERFACE_MODE_USXGMII)
+	/* The BCM84891 is found both soldered down and attached over
+	 * USXGMII, and inside SFP+ copper modules (e.g. various
+	 * "OEM SFP-10G-T" RollBall modules), where the host-side
+	 * interface is 10GBASE-R with SGMII/2500BASE-X rate switching,
+	 * as with the BCM84881. Accept both attachments.
+	 */
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_USXGMII:
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	/* MDIO_CTRL1_LPOWER is set at boot on the tested platform. Does not
 	 * recur on ifdown/ifup, cable events, or link-partner advertisement
@@ -246,6 +260,32 @@ static int bcm84881_get_features(struct phy_device *phydev)
 	return 0;
 }
 
+static const int bcm8489x_features[] = {
+	ETHTOOL_LINK_MODE_Autoneg_BIT,
+	ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+	ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+};
+
+static int bcm8489x_get_features(struct phy_device *phydev)
+{
+	/* The PMA/PMD abilities of this family are fixed and known, and
+	 * on some modules every MDIO access is expensive (RollBall
+	 * MDIO-over-I2C mailbox, tens to hundreds of ms per register),
+	 * so do not spend half a dozen reads at attach time discovering
+	 * constants. EEE abilities may vary with firmware; keep reading
+	 * those.
+	 */
+	linkmode_set_bit_array(bcm8489x_features,
+			       ARRAY_SIZE(bcm8489x_features),
+			       phydev->supported);
+
+	return genphy_c45_read_eee_abilities(phydev);
+}
+
 static int bcm84881_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;
@@ -303,8 +343,45 @@ static int bcm84881_aneg_done(struct phy_device *phydev)
 
 static int bcm84881_read_status(struct phy_device *phydev)
 {
+	bool was_resolved;
 	unsigned int mode;
-	int bmsr, val;
+	int bmsr, val, stat1;
+
+	/* Whether the previous poll left a fully resolved link whose
+	 * negotiated parameters (lp_advertising, speed, duplex, pause,
+	 * mdix, interface) are still valid. They can only change through
+	 * a renegotiation, which is observable below as a link drop, an
+	 * autoneg restart or autoneg-complete deasserting.
+	 */
+	was_resolved = phydev->link && phydev->autoneg_complete &&
+		       phydev->speed != SPEED_UNKNOWN;
+
+	stat1 = -1;
+
+	/* In polling mode with the link previously up and resolved, a
+	 * single read of the AN status register is normally sufficient
+	 * to confirm nothing changed: the link status bit is latched
+	 * low, so a momentary drop or a renegotiation since the last
+	 * poll reads as 0 even if the link has already come back, and
+	 * autoneg-complete deasserts across a renegotiation. This
+	 * mirrors the polling-mode single-read logic in
+	 * genphy_c45_read_link().
+	 */
+	if (phy_polling_mode(phydev) && was_resolved) {
+		stat1 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+		if (stat1 < 0)
+			return stat1;
+
+		if ((stat1 & MDIO_STAT1_LSTATUS) &&
+		    (stat1 & MDIO_AN_STAT1_COMPLETE))
+			return 0;
+
+		/* Something changed. The latched status has now been
+		 * consumed, so the full evaluation below must reuse
+		 * this value rather than re-read the register, which
+		 * would miss a momentary link drop.
+		 */
+	}
 
 	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
 	if (val < 0)
@@ -315,21 +392,32 @@ static int bcm84881_read_status(struct phy_device *phydev)
 		return 0;
 	}
 
-	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
-	if (val < 0)
-		return val;
+	if (stat1 < 0) {
+		stat1 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+		if (stat1 < 0)
+			return stat1;
+	}
 
 	bmsr = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_C22 + MII_BMSR);
 	if (bmsr < 0)
 		return bmsr;
 
-	phydev->autoneg_complete = !!(val & MDIO_AN_STAT1_COMPLETE) &&
+	phydev->autoneg_complete = !!(stat1 & MDIO_AN_STAT1_COMPLETE) &&
 				   !!(bmsr & BMSR_ANEGCOMPLETE);
-	phydev->link = !!(val & MDIO_STAT1_LSTATUS) &&
+	phydev->link = !!(stat1 & MDIO_STAT1_LSTATUS) &&
 		       !!(bmsr & BMSR_LSTATUS);
 	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
 		phydev->link = false;
 
+	/* On some modules every MDIO access is expensive (RollBall
+	 * MDIO-over-I2C mailbox, tens to hundreds of ms per register).
+	 * If the link was already up and resolved on the previous poll
+	 * and still is, the negotiated parameters cannot have changed:
+	 * skip re-reading them and keep the cached values.
+	 */
+	if (was_resolved && phydev->link && phydev->autoneg_complete)
+		return 0;
+
 	linkmode_zero(phydev->lp_advertising);
 	phydev->speed = SPEED_UNKNOWN;
 	phydev->duplex = DUPLEX_UNKNOWN;
@@ -416,12 +504,30 @@ static unsigned int bcm84881_inband_caps(struct phy_device *phydev,
 	return LINK_INBAND_DISABLE;
 }
 
+static int bcm84881_config_inband(struct phy_device *phydev,
+				  unsigned int modes)
+{
+	/* This PHY does not generate inband signalling in any mode (see
+	 * bcm84881_inband_caps()); inband is permanently disabled in
+	 * hardware. A request to disable inband therefore requires no
+	 * action, but must succeed: phylink treats any error from
+	 * phy_config_inband() (including -EOPNOTSUPP from a missing
+	 * config_inband method) as a major configuration failure which
+	 * forces and holds the link down.
+	 */
+	if (modes == LINK_INBAND_DISABLE)
+		return 0;
+
+	return -EINVAL;
+}
+
 static struct phy_driver bcm84881_drivers[] = {
 	{
 		.phy_id		= 0xae025150,
 		.phy_id_mask	= 0xfffffff0,
 		.name		= "Broadcom BCM84881",
 		.inband_caps	= bcm84881_inband_caps,
+		.config_inband	= bcm84881_config_inband,
 		.config_init	= bcm84881_config_init,
 		.probe		= bcm84881_probe,
 		.get_features	= bcm84881_get_features,
@@ -432,9 +538,10 @@ static struct phy_driver bcm84881_drivers[] = {
 		PHY_ID_MATCH_MODEL(0x35905080),
 		.name		= "Broadcom BCM84891",
 		.inband_caps	= bcm84881_inband_caps,
+		.config_inband	= bcm84881_config_inband,
 		.config_init	= bcm8489x_config_init,
 		.probe		= bcm84881_probe,
-		.get_features	= bcm84881_get_features,
+		.get_features	= bcm8489x_get_features,
 		.config_aneg	= bcm84881_config_aneg,
 		.aneg_done	= bcm84881_aneg_done,
 		.read_status	= bcm84881_read_status,
@@ -446,9 +553,10 @@ static struct phy_driver bcm84881_drivers[] = {
 		PHY_ID_MATCH_MODEL(0x359050a0),
 		.name		= "Broadcom BCM84892",
 		.inband_caps	= bcm84881_inband_caps,
+		.config_inband	= bcm84881_config_inband,
 		.config_init	= bcm8489x_config_init,
 		.probe		= bcm84881_probe,
-		.get_features	= bcm84881_get_features,
+		.get_features	= bcm8489x_get_features,
 		.config_aneg	= bcm84881_config_aneg,
 		.aneg_done	= bcm84881_aneg_done,
 		.read_status	= bcm84881_read_status,