[U-Boot] [PATCH v3 0/6] net: phy: Allow disabling SmartEEE for Atheros PHYs
The patchset adds support for disabling a PHY feature that may cause packet loss, and also cleans up the atheros.c driver a bit.
As part of the cleanup, there are also some functional changes for the AR8035 in the RGMII internal delay area. Due to a mistake, RGMII Tx internal delay used to be implicitly always enabled. This is now corrected. Users of this driver may need to update their bindings to explicitly request the Tx internal delays back.
Changes in v3: * Replaced all magic numbers with macros * Squashed "net: phy: ar803x: Use phy_read_mmd and phy_write_mmd functions" into new "net: phy: ar803x: Clarify the configuration of the CLK_25M output pin" patch * Fixed unconditional enabling of RGMII TxID for AR8035
Vladimir Oltean (6): net: phy: ar803x: Address packet drops at low traffic rate due to SmartEEE feature net: phy: ar803x: Make RGMII Tx delays actually configurable for AR8035 net: phy: ar803x: Use common functions for RGMII internal delays net: phy: ar803x: Explicitly disable RGMII delays net: phy: ar803x: Clarify the configuration of the CLK_25M output pin net: phy: ar803x: Clarify the intention of ar8021_config
drivers/net/phy/Kconfig | 21 ++++++++ drivers/net/phy/atheros.c | 128 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 111 insertions(+), 38 deletions(-)
Delete the extraneous write to debug reg 5 that enables Tx delay
When the driver was originally introduced in commit "6027384a phylib: Add Atheros AR8035 GETH PHY support", the Tx delay was being unconditionally enabled.
Then during "2ec4d10b phy: atheros: add support for RGMII_ID, RGMII_TXID and RGMII_RXID", the author did not notice that code for enabling Tx delay code was already. Therefore, the if condition for Tx delay has always been useless for this PHY since this commit introduced it.
Prior to this patch, every AR8035 PHY in U-boot had Tx delay enabled. After this patch, only those who define the interface as RGMII_TXID or RGMII_ID will. This is to be expected, but will nonetheless break the setups of those who didn't know they rely on Tx delay implicitly.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- Changes in v3: * Patch added in this version.
drivers/net/phy/atheros.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index 7303267..6a98316 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -89,10 +89,6 @@ static int ar8035_config(struct phy_device *phydev) regval = phy_read(phydev, MDIO_DEVAD_NONE, 0xe); phy_write(phydev, MDIO_DEVAD_NONE, 0xe, (regval|0x0018));
- phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x05); - regval = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e); - phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, (regval|0x0100)); - if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) { /* select debug reg 5 */
On Fri, Jan 25, 2019 at 6:41 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
Delete the extraneous write to debug reg 5 that enables Tx delay
When the driver was originally introduced in commit "6027384a phylib: Add Atheros AR8035 GETH PHY support", the Tx delay was being unconditionally enabled.
Then during "2ec4d10b phy: atheros: add support for RGMII_ID, RGMII_TXID and RGMII_RXID", the author did not notice that code for enabling Tx delay code was already. Therefore, the if condition for Tx delay has always been useless for this PHY since this commit introduced it.
Prior to this patch, every AR8035 PHY in U-boot had Tx delay enabled. After this patch, only those who define the interface as RGMII_TXID or RGMII_ID will. This is to be expected, but will nonetheless break the setups of those who didn't know they rely on Tx delay implicitly.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com
Acked-by: Joe Hershberger joe.hershberger@ni.com
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031362/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
According to the AR8031 and AR8035 datasheets, smartEEE mode (active by default) makes the PHY enter sleep after a configurable idle time. It does this autonomously, without LPI (Low Power Idle) signals coming from MAC. AR8021 does not appear to support this.
This patch allows disabling the SmartEEE feature of above PHYs.
Tested with ping (default of 1 second interval) over back-to-back RGMII between 2 boards having AR8035 at both ends: - Without SmartEEE: 225 packets transmitted, 145 received, 35% packet loss, time 229334ms - With SmartEEE: 144 packets transmitted, 144 received, 0% packet loss, time 146378ms
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com Acked-by: Joe Hershberger joe.hershberger@ni.com --- Changes in v3: * Got rid of magic numbers.
drivers/net/phy/Kconfig | 21 +++++++++++++++++++++ drivers/net/phy/atheros.c | 26 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 3dc0822..6abe8c5 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -94,6 +94,27 @@ config PHY_AQUANTIA_FW_NAME config PHY_ATHEROS bool "Atheros Ethernet PHYs support"
+config PHY_ATHEROS_SMART_EEE + depends on PHY_ATHEROS + default y + tristate "SmartEEE feature for Atheros PHYs" + help + Enables the Atheros SmartEEE feature (not IEEE 802.3az). When 2 PHYs + which support this feature are connected back-to-back, they may + negotiate a low-power sleep mode autonomously, without the Ethernet + controller's knowledge. This setting may cause issues under 2 known + circumstances (both noticed at low traffic rates): + - If the voltage rails on the PHY are unstable, then the PHY can + actually reset as it enters the low power state. This means that + the frames it is supposed to buffer until it wakes up are going + to be dropped instead. + - If 1588/PTP synchronization is the only traffic source over this + PHY, the delays caused by the sleep/wakeup time are going to add + to the synchronization error between the master and the slave. + Default y, which means that the PHY's out-of-reset state is not + changed (SmartEEE active). To work around the issues described + above, change to n. + config PHY_BROADCOM bool "Broadcom Ethernet PHYs support"
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index 3783d15..7303267 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -5,6 +5,7 @@ * Copyright 2011, 2013 Freescale Semiconductor, Inc. * author Andy Fleming */ +#include <linux/bitops.h> #include <common.h> #include <phy.h>
@@ -17,6 +18,21 @@ #define AR803x_DEBUG_REG_0 0x0 #define AR803x_RGMII_RX_CLK_DLY 0x8000
+#define AR803x_SMART_EEE_CTRL3_REG 0x805D +#define AR803x_LPI_EN BIT(8) + +static void ar803x_enable_smart_eee(struct phy_device *phydev, bool on) +{ + int regval; + + regval = phy_read_mmd(phydev, MDIO_MMD_PCS, AR803x_SMART_EEE_CTRL3_REG); + if (on) + regval |= AR803x_LPI_EN; + else + regval &= ~AR803x_LPI_EN; + phy_write_mmd(phydev, MDIO_MMD_PCS, AR803x_SMART_EEE_CTRL3_REG, regval); +} + static int ar8021_config(struct phy_device *phydev) { phy_write(phydev, MDIO_DEVAD_NONE, 0x00, 0x1200); @@ -29,6 +45,11 @@ static int ar8021_config(struct phy_device *phydev)
static int ar8031_config(struct phy_device *phydev) { +#ifdef CONFIG_PHY_ATHEROS_SMART_EEE + ar803x_enable_smart_eee(phydev, true); +#else + ar803x_enable_smart_eee(phydev, false); +#endif if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, @@ -57,6 +78,11 @@ static int ar8035_config(struct phy_device *phydev) { int regval;
+#ifdef CONFIG_PHY_ATHEROS_SMART_EEE + ar803x_enable_smart_eee(phydev, true); +#else + ar803x_enable_smart_eee(phydev, false); +#endif phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x0007); phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016); phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031361/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- Changes in v3: * Removed access to magic register 0x5 which turned out to be duplicated code with unwanted consequences. Broke that out into separate patch (2/6).
drivers/net/phy/atheros.c | 69 ++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index 6a98316..72e7fac 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -13,10 +13,10 @@ #define AR803x_PHY_DEBUG_DATA_REG 0x1e
#define AR803x_DEBUG_REG_5 0x5 -#define AR803x_RGMII_TX_CLK_DLY 0x100 +#define AR803x_RGMII_TX_CLK_DLY BIT(8)
#define AR803x_DEBUG_REG_0 0x0 -#define AR803x_RGMII_RX_CLK_DLY 0x8000 +#define AR803x_RGMII_RX_CLK_DLY BIT(15)
#define AR803x_SMART_EEE_CTRL3_REG 0x805D #define AR803x_LPI_EN BIT(8) @@ -33,11 +33,40 @@ static void ar803x_enable_smart_eee(struct phy_device *phydev, bool on) phy_write_mmd(phydev, MDIO_MMD_PCS, AR803x_SMART_EEE_CTRL3_REG, regval); }
+static void ar803x_enable_rx_delay(struct phy_device *phydev, bool on) +{ + int regval; + + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, + AR803x_DEBUG_REG_0); + regval = phy_read(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG); + if (on) + regval |= AR803x_RGMII_RX_CLK_DLY; + else + regval &= ~AR803x_RGMII_RX_CLK_DLY; + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, regval); +} + +static void ar803x_enable_tx_delay(struct phy_device *phydev, bool on) +{ + int regval; + + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, + AR803x_DEBUG_REG_5); + regval = phy_read(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG); + if (on) + regval |= AR803x_RGMII_TX_CLK_DLY; + else + regval &= ~AR803x_RGMII_TX_CLK_DLY; + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, regval); +} + static int ar8021_config(struct phy_device *phydev) { phy_write(phydev, MDIO_DEVAD_NONE, 0x00, 0x1200); - phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x05); - phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x3D47); + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, + AR803x_DEBUG_REG_5); + phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, 0x3D47);
phydev->supported = phydev->drv->features; return 0; @@ -51,20 +80,12 @@ static int ar8031_config(struct phy_device *phydev) ar803x_enable_smart_eee(phydev, false); #endif if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, - AR803x_DEBUG_REG_5); - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, - AR803x_RGMII_TX_CLK_DLY); - } + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + ar803x_enable_tx_delay(phydev, true);
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) { - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, - AR803x_DEBUG_REG_0); - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, - AR803x_RGMII_RX_CLK_DLY); - } + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + ar803x_enable_rx_delay(phydev, true);
phydev->supported = phydev->drv->features;
@@ -90,20 +111,12 @@ static int ar8035_config(struct phy_device *phydev) phy_write(phydev, MDIO_DEVAD_NONE, 0xe, (regval|0x0018));
if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || - (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) { - /* select debug reg 5 */ - phy_write(phydev, MDIO_DEVAD_NONE, 0x1D, 0x5); - /* enable tx delay */ - phy_write(phydev, MDIO_DEVAD_NONE, 0x1E, 0x0100); - } + (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) + ar803x_enable_tx_delay(phydev, true);
if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || - (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)) { - /* select debug reg 0 */ - phy_write(phydev, MDIO_DEVAD_NONE, 0x1D, 0x0); - /* enable rx delay */ - phy_write(phydev, MDIO_DEVAD_NONE, 0x1E, 0x8000); - } + (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)) + ar803x_enable_rx_delay(phydev, true);
phydev->supported = phydev->drv->features;
On Fri, Jan 25, 2019 at 6:42 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com
Acked-by: Joe Hershberger joe.hreshberger@ni.com
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031363/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
Also take the opportunity to use the phy_read_mmd and phy_write_mmd convenience functions.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- Changes in v3: * Patch added in this version. Partially squashed with patch 1/3 from v2, since addressing the comments on that patch gave its commit message a new meaning.
drivers/net/phy/atheros.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index 02aa61f..a2d0e0d 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -18,6 +18,15 @@ #define AR803x_DEBUG_REG_0 0x0 #define AR803x_RGMII_RX_CLK_DLY BIT(15)
+/* CLK_25M register is at MMD 7, address 0x8016 */ +#define AR803x_CLK_25M_SEL_REG 0x8016 +/* AR8035: Select frequency on CLK_25M pin through bits 4:3 */ +#define AR8035_CLK_25M_FREQ_25M (0 | 0) +#define AR8035_CLK_25M_FREQ_50M (0 | BIT(3)) +#define AR8035_CLK_25M_FREQ_62M (BIT(4) | 0) +#define AR8035_CLK_25M_FREQ_125M (BIT(4) | BIT(3)) +#define AR8035_CLK_25M_MASK GENMASK(4, 3) + #define AR803x_SMART_EEE_CTRL3_REG 0x805D #define AR803x_LPI_EN BIT(8)
@@ -108,11 +117,11 @@ static int ar8035_config(struct phy_device *phydev) #else ar803x_enable_smart_eee(phydev, false); #endif - phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x0007); - phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016); - phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007); - regval = phy_read(phydev, MDIO_DEVAD_NONE, 0xe); - phy_write(phydev, MDIO_DEVAD_NONE, 0xe, (regval|0x0018)); + /* Configure CLK_25M output clock at 125 MHz */ + regval = phy_read_mmd(phydev, MDIO_MMD_AN, AR803x_CLK_25M_SEL_REG); + regval &= ~AR8035_CLK_25M_MASK; /* No surprises */ + regval |= AR8035_CLK_25M_FREQ_125M; + phy_write_mmd(phydev, MDIO_MMD_AN, AR803x_CLK_25M_SEL_REG, regval);
if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID))
On Fri, Jan 25, 2019 at 6:43 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
Also take the opportunity to use the phy_read_mmd and phy_write_mmd convenience functions.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com
Acked-by: Joe Hershberger joe.hreshberger@ni.com
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031364/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
To eliminate any doubts about the out-of-reset value of the PHY, that the driver previously relied on.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- Changes in v3: * Patch added in this version.
drivers/net/phy/atheros.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index 72e7fac..02aa61f 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -82,10 +82,14 @@ static int ar8031_config(struct phy_device *phydev) if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) ar803x_enable_tx_delay(phydev, true); + else + ar803x_enable_tx_delay(phydev, false);
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) ar803x_enable_rx_delay(phydev, true); + else + ar803x_enable_rx_delay(phydev, false);
phydev->supported = phydev->drv->features;
@@ -113,10 +117,14 @@ static int ar8035_config(struct phy_device *phydev) if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) ar803x_enable_tx_delay(phydev, true); + else + ar803x_enable_tx_delay(phydev, false);
if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)) ar803x_enable_rx_delay(phydev, true); + else + ar803x_enable_rx_delay(phydev, false);
phydev->supported = phydev->drv->features;
On Fri, Jan 25, 2019 at 6:43 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
To eliminate any doubts about the out-of-reset value of the PHY, that the driver previously relied on.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com
Acked-by: Joe Hershberger joe.hreshberger@ni.com
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031365/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
Debug register 5 contains TX_CLK DELAY at bit 8 and reserved values at the other bit positions, just like the other PHYs in the family do. Therefore, it is not necessary to hardcode the reserved values, but instead simply follow the read-modify-write procedure from the common function.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com --- Changes in v3: * Patch added in this version. Addresses the previous comments on patch 3/3 from v2.
drivers/net/phy/atheros.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c index a2d0e0d..90568d5 100644 --- a/drivers/net/phy/atheros.c +++ b/drivers/net/phy/atheros.c @@ -72,10 +72,10 @@ static void ar803x_enable_tx_delay(struct phy_device *phydev, bool on)
static int ar8021_config(struct phy_device *phydev) { - phy_write(phydev, MDIO_DEVAD_NONE, 0x00, 0x1200); - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG, - AR803x_DEBUG_REG_5); - phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG, 0x3D47); + phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, + BMCR_ANENABLE | BMCR_ANRESTART); + + ar803x_enable_tx_delay(phydev, true);
phydev->supported = phydev->drv->features; return 0;
On Fri, Jan 25, 2019 at 6:42 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
Debug register 5 contains TX_CLK DELAY at bit 8 and reserved values at the other bit positions, just like the other PHYs in the family do. Therefore, it is not necessary to hardcode the reserved values, but instead simply follow the read-modify-write procedure from the common function.
Signed-off-by: Vladimir Oltean vladimir.oltean@nxp.com
Acked-by: Joe Hershberger joe.hreshberger@ni.com
Hi Vladimir,
https://patchwork.ozlabs.org/patch/1031366/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
participants (2)
-
Joe Hershberger -
Vladimir Oltean