Skip to contentSkip to conversation

Handle stuck TX queue bug in AT8030 PHY

PATCH In review Daniel Mack 2 participants 5 messages Jun 18, 2014 – Jun 21, 2014

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

Daniel Mack

author
#0 lore raw

v2 cover Handle stuck TX queue bug in AT8030 PHY

These three small patches circument a hardware bug in AT8030 PHYs that leads to stuck TX FIFO queues when the link goes away while there are pending patches in der outbound queue. This bug has been confirmed by the vendor, and their only proposed fix is to apply a hardware reset every time the link goes down.

Thanks,
Daniel

v1 -> v2:
* Rename phy device callback from adjust_state to link_change_notify

Daniel Mack (3):
net: phylib: add link_change_notify callback to phy device
net: phy: at803x: use #defines for supported PHY ids
net: phy: at803x: Add support for hardware reset

 drivers/net/phy/at803x.c | 195 ++++++++++++++++++++++++++++++++++++-----------
 drivers/net/phy/phy.c    |   3 +
 include/linux/phy.h      |   9 +++
 3 files changed, 163 insertions(+), 44 deletions(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v2 1/3 net: phylib: add link_change_notify callback to phy device

Add a notify callback to inform phy drivers when the core is about to do its link adjustment. No change for drivers that do not implement this callback.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/net/phy/phy.c | 3 +++
 include/linux/phy.h   | 9 +++++++++
 2 files changed, 12 insertions(+)
drivers/net/phy/phy.c +3 -0
index 3bc079a..f7c6181 100644
@@ -720,6 +720,9 @@ void phy_state_machine(struct work_struct *work)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->drv->link_change_notify)
+		phydev->drv->link_change_notify(phydev);
+
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_STARTING:
include/linux/phy.h +9 -0
index 51d15f6..54b8723 100644
@@ -529,6 +529,15 @@ struct phy_driver {
 	/* See set_wol, but for checking whether Wake on LAN is enabled. */
 	void (*get_wol)(struct phy_device *dev, struct ethtool_wolinfo *wol);
 
+	/*
+	 * Called to inform a PHY device driver when the core is about to
+	 * change the link state. This callback is supposed to be used as
+	 * fixup hook for drivers that need to take action when the link
+	 * state changes. Drivers are by no means allowed to mess with the
+	 * PHY device structure in their implementations.
+	 */
+	void (*link_change_notify)(struct phy_device *dev);
+
 	struct device_driver driver;
 };
 #define to_phy_driver(d) container_of(d, struct phy_driver, driver)
Signature · 1 line
1.9.3
author
#2 lore raw

PATCH v2 2/3 net: phy: at803x: use #defines for supported PHY ids

In reply to Daniel Mack

This removes magic values from two tables and also allows us to match against specific PHY models at runtime.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/net/phy/at803x.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/net/phy/at803x.c +10 -6
index b256083..045e446 100644
@@ -35,6 +35,10 @@
 #define AT803X_DEBUG_SYSTEM_MODE_CTRL		0x05
 #define AT803X_DEBUG_RGMII_TX_CLK_DLY		BIT(8)
 
+#define ATH8030_PHY_ID 0x004dd076
+#define ATH8031_PHY_ID 0x004dd074
+#define ATH8035_PHY_ID 0x004dd072
+
 MODULE_DESCRIPTION("Atheros 803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");

@@ -192,7 +196,7 @@ static int at803x_config_intr(struct phy_device *phydev)
 static struct phy_driver at803x_driver[] = {
 {
 	/* ATHEROS 8035 */
-	.phy_id		= 0x004dd072,
+	.phy_id		= ATH8035_PHY_ID,
 	.name		= "Atheros 8035 ethernet",
 	.phy_id_mask	= 0xffffffef,
 	.config_init	= at803x_config_init,

@@ -209,7 +213,7 @@ static struct phy_driver at803x_driver[] = {
 	},
 }, {
 	/* ATHEROS 8030 */
-	.phy_id		= 0x004dd076,
+	.phy_id		= ATH8030_PHY_ID,
 	.name		= "Atheros 8030 ethernet",
 	.phy_id_mask	= 0xffffffef,
 	.config_init	= at803x_config_init,

@@ -226,7 +230,7 @@ static struct phy_driver at803x_driver[] = {
 	},
 }, {
 	/* ATHEROS 8031 */
-	.phy_id		= 0x004dd074,
+	.phy_id		= ATH8031_PHY_ID,
 	.name		= "Atheros 8031 ethernet",
 	.phy_id_mask	= 0xffffffef,
 	.config_init	= at803x_config_init,

@@ -261,9 +265,9 @@ module_init(atheros_init);
 module_exit(atheros_exit);
 
 static struct mdio_device_id __maybe_unused atheros_tbl[] = {
-	{ 0x004dd076, 0xffffffef },
-	{ 0x004dd074, 0xffffffef },
-	{ 0x004dd072, 0xffffffef },
+	{ ATH8030_PHY_ID, 0xffffffef },
+	{ ATH8031_PHY_ID, 0xffffffef },
+	{ ATH8035_PHY_ID, 0xffffffef },
 	{ }
 };
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH v2 3/3 net: phy: at803x: Add support for hardware reset

In reply to Daniel Mack

The AT8030 will enter a FIFO error mode if a packet is transmitted while the cable is unplugged. This hardware issue is acknowledged by the vendor, and the only proposed solution is to conduct a hardware reset via the external pin each time the link goes down. There is apparantly no way to fix up the state via the register set.

This patch adds support for reading a 'reset-gpios' property from the DT node of the PHY. If present, this gpio is used to apply a hardware reset each time a 'link down' condition is detected. All relevant registers are read out before, and written back after the reset cycle.

Doing this every time the link goes down might seem like overkill, but there is unfortunately no way of figuring out whether the PHY is in such a lock-up state. Hence, this is the only way of reliably fixing up things.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/net/phy/at803x.c | 185 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 144 insertions(+), 41 deletions(-)
drivers/net/phy/at803x.c +144 -41
index 045e446..ce502f0 100644
@@ -16,9 +16,13 @@
 #include <linux/string.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_STATUS			0x13
+#define AT803X_SMART_SPEED			0x14
+#define AT803X_LED_CONTROL			0x18
 #define AT803X_WOL_ENABLE			0x01
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C

@@ -43,6 +47,44 @@ MODULE_DESCRIPTION("Atheros 803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
+struct at803x_priv {
+	bool phy_reset:1;
+	struct gpio_desc *gpiod_reset;
+};
+
+struct at803x_context {
+	u16 bmcr;
+	u16 advertise;
+	u16 control1000;
+	u16 int_enable;
+	u16 smart_speed;
+	u16 led_control;
+};
+
+/* save relevant PHY registers to private copy */
+static void at803x_context_save(struct phy_device *phydev,
+				struct at803x_context *context)
+{
+	context->bmcr = phy_read(phydev, MII_BMCR);
+	context->advertise = phy_read(phydev, MII_ADVERTISE);
+	context->control1000 = phy_read(phydev, MII_CTRL1000);
+	context->int_enable = phy_read(phydev, AT803X_INTR_ENABLE);
+	context->smart_speed = phy_read(phydev, AT803X_SMART_SPEED);
+	context->led_control = phy_read(phydev, AT803X_LED_CONTROL);
+}
+
+/* restore relevant PHY registers from private copy */
+static void at803x_context_restore(struct phy_device *phydev,
+				   const struct at803x_context *context)
+{
+	phy_write(phydev, MII_BMCR, context->bmcr);
+	phy_write(phydev, MII_ADVERTISE, context->advertise);
+	phy_write(phydev, MII_CTRL1000, context->control1000);
+	phy_write(phydev, AT803X_INTR_ENABLE, context->int_enable);
+	phy_write(phydev, AT803X_SMART_SPEED, context->smart_speed);
+	phy_write(phydev, AT803X_LED_CONTROL, context->led_control);
+}
+
 static int at803x_set_wol(struct phy_device *phydev,
 			  struct ethtool_wolinfo *wol)
 {

@@ -146,6 +188,26 @@ static int at803x_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static int at803x_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->dev;
+	struct at803x_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->gpiod_reset = devm_gpiod_get(dev, "reset");
+	if (IS_ERR(priv->gpiod_reset))
+		priv->gpiod_reset = NULL;
+	else
+		gpiod_direction_output(priv->gpiod_reset, 1);
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static int at803x_config_init(struct phy_device *phydev)
 {
 	int ret;

@@ -193,58 +255,99 @@ static int at803x_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static void at803x_link_change_notify(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	/*
+	 * Conduct a hardware reset for AT8030 every time a link loss is
+	 * signalled. This is necessary to circumvent a hardware bug that
+	 * occurs when the cable is unplugged while TX packets are pending
+	 * in the FIFO. In such cases, the FIFO enters an error mode it
+	 * cannot recover from by software.
+	 */
+	if (phydev->drv->phy_id == ATH8030_PHY_ID) {
+		if (phydev->state == PHY_NOLINK) {
+			if (priv->gpiod_reset && !priv->phy_reset) {
+				struct at803x_context context;
+
+				at803x_context_save(phydev, &context);
+
+				gpiod_set_value(priv->gpiod_reset, 0);
+				msleep(1);
+				gpiod_set_value(priv->gpiod_reset, 1);
+				msleep(1);
+
+				at803x_context_restore(phydev, &context);
+
+				dev_dbg(&phydev->dev, "%s(): phy was reset\n",
+					__func__);
+				priv->phy_reset = true;
+			}
+		} else {
+			priv->phy_reset = false;
+		}
+	}
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* ATHEROS 8035 */
-	.phy_id		= ATH8035_PHY_ID,
-	.name		= "Atheros 8035 ethernet",
-	.phy_id_mask	= 0xffffffef,
-	.config_init	= at803x_config_init,
-	.set_wol	= at803x_set_wol,
-	.get_wol	= at803x_get_wol,
-	.suspend	= at803x_suspend,
-	.resume		= at803x_resume,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
-	.driver		= {
+	.phy_id			= ATH8035_PHY_ID,
+	.name			= "Atheros 8035 ethernet",
+	.phy_id_mask		= 0xffffffef,
+	.probe			= at803x_probe,
+	.config_init		= at803x_config_init,
+	.link_change_notify	= at803x_link_change_notify,
+	.set_wol		= at803x_set_wol,
+	.get_wol		= at803x_get_wol,
+	.suspend		= at803x_suspend,
+	.resume			= at803x_resume,
+	.features		= PHY_GBIT_FEATURES,
+	.flags			= PHY_HAS_INTERRUPT,
+	.config_aneg		= genphy_config_aneg,
+	.read_status		= genphy_read_status,
+	.driver			= {
 		.owner = THIS_MODULE,
 	},
 }, {
 	/* ATHEROS 8030 */
-	.phy_id		= ATH8030_PHY_ID,
-	.name		= "Atheros 8030 ethernet",
-	.phy_id_mask	= 0xffffffef,
-	.config_init	= at803x_config_init,
-	.set_wol	= at803x_set_wol,
-	.get_wol	= at803x_get_wol,
-	.suspend	= at803x_suspend,
-	.resume		= at803x_resume,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
-	.driver		= {
+	.phy_id			= ATH8030_PHY_ID,
+	.name			= "Atheros 8030 ethernet",
+	.phy_id_mask		= 0xffffffef,
+	.probe			= at803x_probe,
+	.config_init		= at803x_config_init,
+	.link_change_notify	= at803x_link_change_notify,
+	.set_wol		= at803x_set_wol,
+	.get_wol		= at803x_get_wol,
+	.suspend		= at803x_suspend,
+	.resume			= at803x_resume,
+	.features		= PHY_GBIT_FEATURES,
+	.flags			= PHY_HAS_INTERRUPT,
+	.config_aneg		= genphy_config_aneg,
+	.read_status		= genphy_read_status,
+	.driver			= {
 		.owner = THIS_MODULE,
 	},
 }, {
 	/* ATHEROS 8031 */
-	.phy_id		= ATH8031_PHY_ID,
-	.name		= "Atheros 8031 ethernet",
-	.phy_id_mask	= 0xffffffef,
-	.config_init	= at803x_config_init,
-	.set_wol	= at803x_set_wol,
-	.get_wol	= at803x_get_wol,
-	.suspend	= at803x_suspend,
-	.resume		= at803x_resume,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
-	.ack_interrupt  = &at803x_ack_interrupt,
-	.config_intr    = &at803x_config_intr,
-	.driver		= {
+	.phy_id			= ATH8031_PHY_ID,
+	.name			= "Atheros 8031 ethernet",
+	.phy_id_mask		= 0xffffffef,
+	.probe			= at803x_probe,
+	.config_init		= at803x_config_init,
+	.link_change_notify	= at803x_link_change_notify,
+	.set_wol		= at803x_set_wol,
+	.get_wol		= at803x_get_wol,
+	.suspend		= at803x_suspend,
+	.resume			= at803x_resume,
+	.features		= PHY_GBIT_FEATURES,
+	.flags			= PHY_HAS_INTERRUPT,
+	.config_aneg		= genphy_config_aneg,
+	.read_status		= genphy_read_status,
+	.ack_interrupt		= &at803x_ack_interrupt,
+	.config_intr		= &at803x_config_intr,
+	.driver			= {
 		.owner = THIS_MODULE,
 	},
 } };
Signature · 1 line
1.9.3
maintainer
#4 lore raw
In reply to Daniel Mack

From: Daniel Mack <[email protected]>
Date: Wed, 18 Jun 2014 11:01:40 +0200

Quoted 5 lines

These three small patches circument a hardware bug in AT8030 PHYs that leads to stuck TX FIFO queues when the link goes away while there are pending patches in der outbound queue. This bug has been confirmed by the vendor, and their only proposed fix is to apply a hardware reset every time the link goes down.

...

Quoted 2 lines

v1 -> v2:
* Rename phy device callback from adjust_state to link_change_notify

Series applied, thanks Daniel.