aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid S. Miller <davem@davemloft.net>2019-10-04 18:11:08 -0700
committerDavid S. Miller <davem@davemloft.net>2019-10-04 18:11:08 -0700
commite3ba9bf62a1c704d5ce176a44e26d2f445d6d6e8 (patch)
treed5b1e04d66cffe9b3dafdf2b923c30e055e9c437
parent2d819d250a1393a3e725715425ab70a0e0772a71 (diff)
parent06d5f3441b2e80eeb6deb0885aefa00589e463c1 (diff)
download96b-common-e3ba9bf62a1c704d5ce176a44e26d2f445d6d6e8.tar.gz
Merge branch 'Fix-regression-with-AR8035-speed-downgrade'
Russell King says: ==================== Fix regression with AR8035 speed downgrade 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. ==================== Signed-off-by: David S. Miller <davem@davemloft.net>
-rw-r--r--drivers/net/phy/at803x.c69
-rw-r--r--drivers/net/phy/phy-core.c20
-rw-r--r--drivers/net/phy/phy.c5
-rw-r--r--drivers/net/phy/phy_device.c65
-rw-r--r--include/linux/mii.h9
-rw-r--r--include/linux/phy.h2
6 files changed, 138 insertions, 32 deletions
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2aa7b2e60046..1eb5d4fb8925 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -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,
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 369903d9b6ec..9412669b579c 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -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);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7c92afd36bbe..119e6f466056 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d347ddcac45b..9d2bbb13293e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1783,32 +1783,9 @@ done:
}
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);
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 5cd824c1c0ca..4ce8901a1af6 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -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
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a7ecbe0e55aa..9a0e981df502 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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);
/**
@@ -1076,6 +1077,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);