[PATCH 0/3] Handle stuck TX queue bug in AT8030 PHY

[PATCH 0/3] Handle stuck TX queue bug in AT8030 PHY

[Daniel Mack]

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.

I tried to fix this with a software reset routine a few months back,
but it turns out that a software reset does in fact not help here.

This new series hence makes use of the newly added way to augment
auto-probed PHY devices with DT information. This way, a 'reset-gpios'
node can be specified, and if present, the gpio will be used to reset
the PHY. In contrast to the first series I sent, this one now saves
the register contents before the reset cycle and restores them
afterwards.

I'm well aware that such a hardware reset is not a nice thing to do,
but we have to cope with hardware that leaves us no other choice.


Thanks,
Daniel


Daniel Mack (3):
  net: phylib: add adjust_state 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 | 119 ++++++++++++++++++++++++++++++++++++++++++++---
 drivers/net/phy/phy.c    |   3 ++
 include/linux/phy.h      |   3 ++
 3 files changed, 119 insertions(+), 6 deletions(-)

-- 
1.9.3

[PATCH 1/3] net: phylib: add adjust_state callback to phy device

[Daniel Mack]

Allow phy drivers to take action when the core does its link adjustment.
No change for drivers that do not implement this callback.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/phy/phy.c | 3 +++
 include/linux/phy.h   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3bc079a..f9c5577 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -720,6 +720,9 @@ void phy_state_machine(struct work_struct *work)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->drv->adjust_state)
+		phydev->drv->adjust_state(phydev);
+
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_STARTING:
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 51d15f6..27cbdf4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -529,6 +529,9 @@ 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 when the PHY core adjusts the link of a PHY device */
+	void (*adjust_state)(struct phy_device *dev);
+
 	struct device_driver driver;
 };
 #define to_phy_driver(d) container_of(d, struct phy_driver, driver)
-- 
1.9.3

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

[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 <zonque@gmail.com>
---
 drivers/net/phy/at803x.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b256083..045e446 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -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 },
 	{ }
 };
 
-- 
1.9.3

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

[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 <zonque@gmail.com>
---
 drivers/net/phy/at803x.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 045e446..edadb7f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -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,12 +255,48 @@ static int at803x_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static void at803x_adjust_state(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 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,
+	.probe		= at803x_probe,
 	.config_init	= at803x_config_init,
 	.set_wol	= at803x_set_wol,
 	.get_wol	= at803x_get_wol,
@@ -208,6 +306,7 @@ static struct phy_driver at803x_driver[] = {
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
+	.adjust_state	= at803x_adjust_state,
 	.driver		= {
 		.owner = THIS_MODULE,
 	},
@@ -216,6 +315,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id		= ATH8030_PHY_ID,
 	.name		= "Atheros 8030 ethernet",
 	.phy_id_mask	= 0xffffffef,
+	.probe		= at803x_probe,
 	.config_init	= at803x_config_init,
 	.set_wol	= at803x_set_wol,
 	.get_wol	= at803x_get_wol,
@@ -225,6 +325,7 @@ static struct phy_driver at803x_driver[] = {
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
+	.adjust_state	= at803x_adjust_state,
 	.driver		= {
 		.owner = THIS_MODULE,
 	},
@@ -233,6 +334,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id		= ATH8031_PHY_ID,
 	.name		= "Atheros 8031 ethernet",
 	.phy_id_mask	= 0xffffffef,
+	.probe		= at803x_probe,
 	.config_init	= at803x_config_init,
 	.set_wol	= at803x_set_wol,
 	.get_wol	= at803x_get_wol,
@@ -242,6 +344,7 @@ static struct phy_driver at803x_driver[] = {
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
+	.adjust_state	= at803x_adjust_state,
 	.ack_interrupt  = &at803x_ack_interrupt,
 	.config_intr    = &at803x_config_intr,
 	.driver		= {
-- 
1.9.3

Re: [PATCH 1/3] net: phylib: add adjust_state callback to phy device

[Florian Fainelli]

2014-06-04 2:00 GMT-07:00 Daniel Mack <zonque@gmail.com>:
> Allow phy drivers to take action when the core does its link adjustment.
> No change for drivers that do not implement this callback.

This sounds potentially dangerous if misused, PHY drivers would
basically be allowed to do arbitrary link state changes based on their
custom needs. I really need to review your workaround here to better
understand whether we can come up with a solution that allows for less
liberty in PHY drivers.

Thanks

>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/net/phy/phy.c | 3 +++
>  include/linux/phy.h   | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3bc079a..f9c5577 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -720,6 +720,9 @@ void phy_state_machine(struct work_struct *work)
>
>         mutex_lock(&phydev->lock);
>
> +       if (phydev->drv->adjust_state)
> +               phydev->drv->adjust_state(phydev);
> +
>         switch (phydev->state) {
>         case PHY_DOWN:
>         case PHY_STARTING:
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 51d15f6..27cbdf4 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -529,6 +529,9 @@ 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 when the PHY core adjusts the link of a PHY device */
> +       void (*adjust_state)(struct phy_device *dev);
> +
>         struct device_driver driver;
>  };
>  #define to_phy_driver(d) container_of(d, struct phy_driver, driver)
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH 1/3] net: phylib: add adjust_state callback to phy device

[Daniel Mack]

Hi Florian,

On 06/05/2014 07:11 AM, Florian Fainelli wrote:
> 2014-06-04 2:00 GMT-07:00 Daniel Mack <zonque@gmail.com>:
>> Allow phy drivers to take action when the core does its link adjustment.
>> No change for drivers that do not implement this callback.
> 
> This sounds potentially dangerous if misused, PHY drivers would
> basically be allowed to do arbitrary link state changes based on their
> custom needs.

Yes, and this is basically what my quirk handler does. It takes action
when the link goes down (PHY_NOLINK), as we unfortunately need to reset
the chip every time this happens.

> I really need to review your workaround here to better
> understand whether we can come up with a solution that allows for less
> liberty in PHY drivers.

Sure, any other way of calling back to the PHY when the core enters the
PHY_NOLINK state would do. I just thought that an adjust_state callback
is most versatile, but I understand that it could be misused.


Thanks for having a look!

Daniel

Re: [PATCH 1/3] net: phylib: add adjust_state callback to phy device

[Florian Fainelli]

Hi Daniel,

2014-06-05 0:14 GMT-07:00 Daniel Mack <daniel@zonque.org>:
> Hi Florian,
>
> On 06/05/2014 07:11 AM, Florian Fainelli wrote:
>> 2014-06-04 2:00 GMT-07:00 Daniel Mack <zonque@gmail.com>:
>>> Allow phy drivers to take action when the core does its link adjustment.
>>> No change for drivers that do not implement this callback.
>>
>> This sounds potentially dangerous if misused, PHY drivers would
>> basically be allowed to do arbitrary link state changes based on their
>> custom needs.
>
> Yes, and this is basically what my quirk handler does. It takes action
> when the link goes down (PHY_NOLINK), as we unfortunately need to reset
> the chip every time this happens.

In fact, the callback name is sort of a misnomer here, as you are not
really adjusting the PHY device state here, you are reading from it to
do something about the PHY device based on this state.

Could you rename it to "state_notify" for instance to make it clear
that this must absolutely be a read-only operation and the PHY driver
is by no means allow to mess with the PHY device structure at all?
Constifying the phy_device argument here would not really help
unfortunately, and providing you with just the 'state' as a RO
argument would not help either since you need to access the PHY
device...

>
>> I really need to review your workaround here to better
>> understand whether we can come up with a solution that allows for less
>> liberty in PHY drivers.
>
> Sure, any other way of calling back to the PHY when the core enters the
> PHY_NOLINK state would do. I just thought that an adjust_state callback
> is most versatile, but I understand that it could be misused.

I was thinking about having a notifier callchain here that would
execute synchronously and in the same context as your proposed
adjust_link() callback, although that might be be too heavy weighted
for basically just one notified callback.

>
>
> Thanks for having a look!
>
> Daniel
>



-- 
Florian

Re: [PATCH 1/3] net: phylib: add adjust_state callback to phy device

[Daniel Mack]

Hi Florian,

On 06/05/2014 08:12 PM, Florian Fainelli wrote:
> 2014-06-05 0:14 GMT-07:00 Daniel Mack <daniel@zonque.org>:

>>> This sounds potentially dangerous if misused, PHY drivers would
>>> basically be allowed to do arbitrary link state changes based on their
>>> custom needs.
>>
>> Yes, and this is basically what my quirk handler does. It takes action
>> when the link goes down (PHY_NOLINK), as we unfortunately need to reset
>> the chip every time this happens.
> 
> In fact, the callback name is sort of a misnomer here, as you are not
> really adjusting the PHY device state here, you are reading from it to
> do something about the PHY device based on this state.
> 
> Could you rename it to "state_notify" for instance to make it clear
> that this must absolutely be a read-only operation and the PHY driver
> is by no means allow to mess with the PHY device structure at all?
> Constifying the phy_device argument here would not really help
> unfortunately, and providing you with just the 'state' as a RO
> argument would not help either since you need to access the PHY
> device...

Sure, I can rename it, no problem.

> I was thinking about having a notifier callchain here that would
> execute synchronously and in the same context as your proposed
> adjust_link() callback, although that might be be too heavy weighted
> for basically just one notified callback.

Plus, it wouldn't really solve the issue you described above, right?


I'll resend after -rc1 is out, with the changed name for the callback.


Again, thanks!
Daniel