* [PATCH net v2 1/4] net: phy: fix write to mii-ctrl1000 register
2019-10-04 16:05 [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade Russell King - ARM Linux admin
@ 2019-10-04 16:05 ` Russell King
2019-10-04 16:06 ` [PATCH net v2 2/4] net: phy: extract link partner advertisement reading Russell King
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Russell King @ 2019-10-04 16:05 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
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 <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy.c | 5 +++++
include/linux/mii.h | 9 +++++++++
2 files changed, 14 insertions(+)
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/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
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net v2 2/4] net: phy: extract link partner advertisement reading
2019-10-04 16:05 [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade Russell King - ARM Linux admin
2019-10-04 16:05 ` [PATCH net v2 1/4] net: phy: fix write to mii-ctrl1000 register Russell King
@ 2019-10-04 16:06 ` Russell King
2019-10-04 17:52 ` Andrew Lunn
2019-10-04 16:06 ` [PATCH net v2 3/4] net: phy: extract pause mode Russell King
` (2 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Russell King @ 2019-10-04 16:06 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
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 <tinywrkb@gmail.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++++-----------------
include/linux/phy.h | 1 +
2 files changed, 41 insertions(+), 25 deletions(-)
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 @@ 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);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a7ecbe0e55aa..7abee820d05c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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);
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v2 2/4] net: phy: extract link partner advertisement reading
2019-10-04 16:06 ` [PATCH net v2 2/4] net: phy: extract link partner advertisement reading Russell King
@ 2019-10-04 17:52 ` Andrew Lunn
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Lunn @ 2019-10-04 17:52 UTC (permalink / raw)
To: Russell King
Cc: Florian Fainelli, Heiner Kallweit, tinywrkb, David S. Miller, netdev
On Fri, Oct 04, 2019 at 05:06:04PM +0100, Russell King wrote:
> 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 <tinywrkb@gmail.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v2 3/4] net: phy: extract pause mode
2019-10-04 16:05 [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade Russell King - ARM Linux admin
2019-10-04 16:05 ` [PATCH net v2 1/4] net: phy: fix write to mii-ctrl1000 register Russell King
2019-10-04 16:06 ` [PATCH net v2 2/4] net: phy: extract link partner advertisement reading Russell King
@ 2019-10-04 16:06 ` Russell King
2019-10-04 16:06 ` [PATCH net v2 4/4] net: phy: at803x: use operating parameters from PHY-specific status Russell King
2019-10-05 1:11 ` [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Russell King @ 2019-10-04 16:06 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
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 <tinywrkb@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phy-core.c | 20 +++++++++++++-------
include/linux/phy.h | 1 +
2 files changed, 14 insertions(+), 7 deletions(-)
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/include/linux/phy.h b/include/linux/phy.h
index 7abee820d05c..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);
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net v2 4/4] net: phy: at803x: use operating parameters from PHY-specific status
2019-10-04 16:05 [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade Russell King - ARM Linux admin
` (2 preceding siblings ...)
2019-10-04 16:06 ` [PATCH net v2 3/4] net: phy: extract pause mode Russell King
@ 2019-10-04 16:06 ` Russell King
2019-10-05 1:11 ` [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Russell King @ 2019-10-04 16:06 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, tinywrkb
Cc: David S. Miller, netdev
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 <tinywrkb@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: tinywrkb <tinywrkb@gmail.com>
Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
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,
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade
2019-10-04 16:05 [PATCH net v2 0/4] Fix regression with AR8035 speed downgrade Russell King - ARM Linux admin
` (3 preceding siblings ...)
2019-10-04 16:06 ` [PATCH net v2 4/4] net: phy: at803x: use operating parameters from PHY-specific status Russell King
@ 2019-10-05 1:11 ` David Miller
4 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2019-10-05 1:11 UTC (permalink / raw)
To: linux; +Cc: andrew, f.fainelli, hkallweit1, tinywrkb, netdev
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Fri, 4 Oct 2019 17:05:25 +0100
> 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.
...
Series applied.
^ permalink raw reply [flat|nested] 7+ messages in thread