Skip to contentSkip to conversation

Fix regression with AR8035 speed downgrade

PATCH In review Russell King - ARM Linux admin 4 participants 7 messages Oct 4, 2019 – Oct 5, 2019

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Russell King - ARM Linux admin

author reviewer
#0 lore raw

v2 cover Fix regression with AR8035 speed downgrade

Hi,

The following series attempts to address an issue spotted by tinywrkb with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades the negotiated link.

This is version 2, not much has changed other than rebasing on the current net tree. Changes have happend to patch 2 due to conflicts, so I dropped Andrew's reviewed-by. Minor context changes to patch 4 which I don't consider important enough to warrant dropping the reviewed-by.

Before commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status"), we would read not only the link partner's advertisement, but also our own advertisement from the PHY registers, and use both to derive the PHYs current link mode. This works when the AR8035 downgrades the speed, because it appears that the AR8035 clears link mode bits in the advertisement registers as part of the downgrade.

Commentary: what is not yet known is whether the AR8035 restores the
advertisement register when the link goes down to the previous state.

However, since the above referenced commit, we no longer use the PHYs advertisement registers, instead converting the link partner's advertisement to the ethtool link mode array, and combine that with phylib's cached version of our advertisement - which is not updated on speed downgrade.

This results in phylib disagreeing with the actual operating mode of the PHY.

Commentary: I wonder how many more PHY drivers are broken by this
commit, but have yet to be discovered.

The obvious way to address this would be to disable the downgrade feature, and indeed this does fix the problem in tinywrkb's case - his link partner instead downgrades the speed by reducing its advertisement, resulting in phylib correctly evaluating a slower speed.

However, it has a serious drawback - the gigabit control register (MII register 9) appears to become read only. It seems the only way to update the register is to re-enable the downgrade feature, reset the PHY, changing register 9, disable the downgrade feature, and reset the PHY again.

This series attempts to address the problem using a different approach, similar to the approach taken with Marvell PHYs. The AR8031, AR8033 and AR8035 have a PHY-Specific Status register which reports the actual operating mode of the PHY - both speed and duplex. This register correctly reports the operating mode irrespective of whether autoneg is enabled or not. We use this register to fill in phylib's speed and duplex parameters.

In detail:

Patch 1 fixes a bug where writing to register 9 does not update phylib's advertisement mask in the same way that writing register 4 does; this looks like an omission from when gigabit PHY support came into being.

Patch 2 seperates the generic phylib code which reads the link partners advertisement from the PHY, so that we can re-use this in the Atheros PHY driver.

Patch 3 seperates the generic phylib pause mode; phylib provides no help for MAC drivers to ascertain the negotiated pause mode, it merely copies the link partner's pause mode bits into its own variables.

Commentary: Both the aforementioned Atheros PHYs and Marvell PHYs
provide the resolved pause modes in terms of whether we should transmit pause frames, or whether we should allow reception of pause frames. Surely the resolution of this should be in phylib?

Patch 4 provides the Atheros PHY driver with a private "read_status" implementation that fills in phylib's speed and duplex settings depending on the PHY-Specific status register. This ensures that phylib and the MAC driver match the operating mode that the PHY has decided to use. Since the register also gives us MDIX status, we can trivially fill that status in as well.

Note that, although the bits mentioned in this patch for this register match those in th Marvell PHY driver, and it is located at the same address, the meaning of other register bits varies between the PHYs. Therefore, I do not feel that it would be appropriate to make this some kind of generic function.

 drivers/net/phy/at803x.c     | 69 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy-core.c   | 20 ++++++++-----
 drivers/net/phy/phy.c        |  5 ++++
 drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++----------------
 include/linux/mii.h          |  9 ++++++
 include/linux/phy.h          |  2 ++
 6 files changed, 138 insertions(+), 32 deletions(-)
Signature · 3 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
#1 lore raw

PATCH v2 1/4 net: phy: fix write to mii-ctrl1000 register

When userspace writes to the MII_ADVERTISE register, we update phylib's advertising mask and trigger a renegotiation. However, writing to the MII_CTRL1000 register, which contains the gigabit advertisement, does neither. This can lead to phylib's copy of the advertisement becoming de-synced with the values in the PHY register set, which can result in incorrect negotiation resolution.

Fixes 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/phy.c | 5 +++++
 include/linux/mii.h   | 9 +++++++++
 2 files changed, 14 insertions(+)
drivers/net/phy/phy.c +5 -0
index 7c92afd36bbe..119e6f466056 100644
@@ -457,6 +457,11 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 							   val);
 				change_autoneg = true;
 				break;
+			case MII_CTRL1000:
+				mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
+							        val);
+				change_autoneg = true;
+				break;
 			default:
 				/* do nothing */
 				break;
include/linux/mii.h +9 -0
index 5cd824c1c0ca..4ce8901a1af6 100644
@@ -455,6 +455,15 @@ static inline void mii_lpa_mod_linkmode_lpa_t(unsigned long *lp_advertising,
 			 lp_advertising, lpa & LPA_LPACK);
 }
 
+static inline void mii_ctrl1000_mod_linkmode_adv_t(unsigned long *advertising,
+						   u32 ctrl1000)
+{
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, advertising,
+			 ctrl1000 & ADVERTISE_1000HALF);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, advertising,
+			 ctrl1000 & ADVERTISE_1000FULL);
+}
+
 /**
  * linkmode_adv_to_lcl_adv_t
  * @advertising:pointer to linkmode advertising
Signature · 1 line
2.7.4
#2 lore raw

PATCH v2 2/4 net: phy: extract link partner advertisement reading

In reply to Russell King - ARM Linux admin

Move reading the link partner advertisement out of genphy_read_status() into its own separate function. This will allow re-use of this code by PHY drivers that are able to read the resolved status from the PHY.

Tested-by tinywrkb <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++++-----------------
 include/linux/phy.h          |  1 +
 2 files changed, 41 insertions(+), 25 deletions(-)
drivers/net/phy/phy_device.c +40 -25
index d347ddcac45b..9d2bbb13293e 100644
@@ -1783,32 +1783,9 @@ int genphy_update_link(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_update_link);
 
-/**
- * genphy_read_status - check the link status and update current link state
- * @phydev: target phy_device struct
- *
- * Description: Check the link, then figure out the current state
- *   by comparing what we advertise with what the link partner
- *   advertises.  Start by checking the gigabit possibilities,
- *   then move on to 10/100.
- */
-int genphy_read_status(struct phy_device *phydev)
+int genphy_read_lpa(struct phy_device *phydev)
 {
-	int lpa, lpagb, err, old_link = phydev->link;
-
-	/* Update the link, but return if there was an error */
-	err = genphy_update_link(phydev);
-	if (err)
-		return err;
-
-	/* why bother the PHY if nothing can have changed */
-	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
-		return 0;
-
-	phydev->speed = SPEED_UNKNOWN;
-	phydev->duplex = DUPLEX_UNKNOWN;
-	phydev->pause = 0;
-	phydev->asym_pause = 0;
+	int lpa, lpagb;
 
 	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
 		if (phydev->is_gigabit_capable) {

@@ -1838,6 +1815,44 @@ int genphy_read_status(struct phy_device *phydev)
 			return lpa;
 
 		mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_read_lpa);
+
+/**
+ * genphy_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises.  Start by checking the gigabit possibilities,
+ *   then move on to 10/100.
+ */
+int genphy_read_status(struct phy_device *phydev)
+{
+	int err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
 		phy_resolve_aneg_linkmode(phydev);
 	} else if (phydev->autoneg == AUTONEG_DISABLE) {
 		int bmcr = phy_read(phydev, MII_BMCR);
include/linux/phy.h +1 -0
index a7ecbe0e55aa..7abee820d05c 100644
@@ -1076,6 +1076,7 @@ int genphy_config_eee_advert(struct phy_device *phydev);
 int __genphy_config_aneg(struct phy_device *phydev, bool changed);
 int genphy_aneg_done(struct phy_device *phydev);
 int genphy_update_link(struct phy_device *phydev);
+int genphy_read_lpa(struct phy_device *phydev);
 int genphy_read_status(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
Signature · 1 line
2.7.4
maintainer Reviewed-by on v2 2/4
#3 lore raw

Re: [PATCH net v2 2/4] net: phy: extract link partner advertisement reading

Quoted 6 lines · Russell King

Move reading the link partner advertisement out of genphy_read_status() into its own separate function. This will allow re-use of this code by PHY drivers that are able to read the resolved status from the PHY.

Tested-by tinywrkb <[email protected]>
Signed-off-by Russell King <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Andrew

#4 lore raw

PATCH v2 3/4 net: phy: extract pause mode

In reply to Russell King - ARM Linux admin

Extract the update of phylib's software pause mode state from genphy_read_status(), so that we can re-use this functionality with PHYs that have alternative ways to read the negotiation results.

Tested-by tinywrkb <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/phy-core.c | 20 +++++++++++++-------
 include/linux/phy.h        |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)
drivers/net/phy/phy-core.c +13 -7
index 369903d9b6ec..9412669b579c 100644
@@ -283,6 +283,18 @@ void of_set_phy_eee_broken(struct phy_device *phydev)
 	phydev->eee_broken_modes = broken;
 }
 
+void phy_resolve_aneg_pause(struct phy_device *phydev)
+{
+	if (phydev->duplex == DUPLEX_FULL) {
+		phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+						  phydev->lp_advertising);
+		phydev->asym_pause = linkmode_test_bit(
+			ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+			phydev->lp_advertising);
+	}
+}
+EXPORT_SYMBOL_GPL(phy_resolve_aneg_pause);
+
 /**
  * phy_resolve_aneg_linkmode - resolve the advertisements into phy settings
  * @phydev: The phy_device struct

@@ -305,13 +317,7 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
 			break;
 		}
 
-	if (phydev->duplex == DUPLEX_FULL) {
-		phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-						  phydev->lp_advertising);
-		phydev->asym_pause = linkmode_test_bit(
-			ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-			phydev->lp_advertising);
-	}
+	phy_resolve_aneg_pause(phydev);
 }
 EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
include/linux/phy.h +1 -0
index 7abee820d05c..9a0e981df502 100644
@@ -678,6 +678,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
 	return phydev->state >= PHY_UP;
 }
 
+void phy_resolve_aneg_pause(struct phy_device *phydev);
 void phy_resolve_aneg_linkmode(struct phy_device *phydev);
 
 /**
Signature · 1 line
2.7.4
#5 lore raw

PATCH v2 4/4 net: phy: at803x: use operating parameters from PHY-specific status

In reply to Russell King - ARM Linux admin

Read the PHY-specific status register for the current operating mode (speed and duplex) of the PHY. This register reflects the actual mode that the PHY has resolved depending on either the advertisements of autoneg is enabled, or the forced mode if autoneg is disabled.

This ensures that phylib's software state always tracks the hardware state.

It seems both AR8033 (which uses the AR8031 ID) and AR8035 support this status register. AR8030 is not known at the present time.

This patch depends on "net: phy: extract pause mode" and "net: phy:
extract link partner advertisement reading".

Reported-by tinywrkb <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>
Tested-by tinywrkb <[email protected]>
Fixes 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
drivers/net/phy/at803x.c +69 -0
index 2aa7b2e60046..1eb5d4fb8925 100644
@@ -15,6 +15,15 @@
 #include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
 
+#define AT803X_SPECIFIC_STATUS			0x11
+#define AT803X_SS_SPEED_MASK			(3 << 14)
+#define AT803X_SS_SPEED_1000			(2 << 14)
+#define AT803X_SS_SPEED_100			(1 << 14)
+#define AT803X_SS_SPEED_10			(0 << 14)
+#define AT803X_SS_DUPLEX			BIT(13)
+#define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
+#define AT803X_SS_MDIX				BIT(6)
+
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
 #define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)

@@ -357,6 +366,64 @@ static int at803x_aneg_done(struct phy_device *phydev)
 	return aneg_done;
 }
 
+static int at803x_read_status(struct phy_device *phydev)
+{
+	int ss, err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	/* Read the AT8035 PHY-Specific Status register, which indicates the
+	 * speed and duplex that the PHY is actually using, irrespective of
+	 * whether we are in autoneg mode or not.
+	 */
+	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
+	if (ss < 0)
+		return ss;
+
+	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+		switch (ss & AT803X_SS_SPEED_MASK) {
+		case AT803X_SS_SPEED_10:
+			phydev->speed = SPEED_10;
+			break;
+		case AT803X_SS_SPEED_100:
+			phydev->speed = SPEED_100;
+			break;
+		case AT803X_SS_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		}
+		if (ss & AT803X_SS_DUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+		if (ss & AT803X_SS_MDIX)
+			phydev->mdix = ETH_TP_MDI_X;
+		else
+			phydev->mdix = ETH_TP_MDI;
+	}
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+		phy_resolve_aneg_pause(phydev);
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* ATHEROS 8035 */

@@ -370,6 +437,7 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
+	.read_status		= at803x_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 }, {

@@ -399,6 +467,7 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
+	.read_status		= at803x_read_status,
 	.aneg_done		= at803x_aneg_done,
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
Signature · 1 line
2.7.4