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

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   | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 36c6994..240e33f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -748,6 +748,9 @@ void phy_state_machine(struct work_struct *work)
 	if (phydev->adjust_state)
 		phydev->adjust_state(phydev->attached_dev);
 
+	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 64ab823..85826eb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -467,6 +467,8 @@ 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);
 
+	void (*adjust_state)(struct phy_device *dev);
+
 	struct device_driver driver;
 };
 #define to_phy_driver(d) container_of(d, struct phy_driver, driver)

-- 
1.8.4.2

When the link goes down and up again quickly and multiple times in a
row, the AT803x PHY gets stuck and won't recover unless it is soft-reset
via the BMCR register.

Unfortunately, there is no way to detect this state, as all registers
contain perfectly sane values in such cases. Hence, the only option is
to always soft-reset the PHY when the link goes down.

Reset logic is based on a patch from Marek Belisko.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: Marek Belisko <redacted>
---
 drivers/net/phy/at803x.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index bc71947..303c5ae 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c

@@ -32,10 +32,56 @@
 #define AT803X_DEBUG_SYSTEM_MODE_CTRL		0x05
 #define AT803X_DEBUG_RGMII_TX_CLK_DLY		BIT(8)
 
+#define AT803X_BMCR_SOFTRESET			BIT(15)
+
 MODULE_DESCRIPTION("Atheros 803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
+struct at803x_priv {
+	bool phy_reset;
+};
+
+static void at803x_adjust_state(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	if (phydev->state == PHY_NOLINK) {
+		unsigned long timeout;
+		unsigned int val;
+
+		if (priv->phy_reset)
+			return;
+
+		val = phy_read(phydev, MII_BMCR);
+		val |= AT803X_BMCR_SOFTRESET;
+		phy_write(phydev, MII_BMCR, val);
+
+		timeout = jiffies + HZ/10;
+		do {
+			cpu_relax();
+		} while ((phy_read(phydev, MII_BMCR) & AT803X_BMCR_SOFTRESET) &&
+			 time_after(timeout, jiffies));
+
+		WARN(phy_read(phydev, MII_BMCR) & AT803X_BMCR_SOFTRESET,
+		     "failed to soft-reset phy\n");
+
+		priv->phy_reset = true;
+	} else {
+		priv->phy_reset = false;
+	}
+}
+
+static int at803x_probe(struct phy_device *phydev)
+{
+	phydev->priv = devm_kzalloc(&phydev->dev, sizeof(struct at803x_priv),
+				    GFP_KERNEL);
+	if (!phydev->priv)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int at803x_set_wol(struct phy_device *phydev,
 			  struct ethtool_wolinfo *wol)
 {

@@ -197,6 +243,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id		= 0x004dd072,
 	.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,

@@ -206,6 +253,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,
 	},

@@ -214,6 +262,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id		= 0x004dd076,
 	.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,

@@ -223,6 +272,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,
 	},

@@ -231,6 +281,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id		= 0x004dd074,
 	.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,

@@ -240,6 +291,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,
 	},

-- 
1.8.4.2

From: Daniel Mack <zonque@gmail.com>
Date: Wed, 13 Nov 2013 22:07:49 +0100

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>

So you're using this to reset the entire PHY via the reset bit in the
BMCR register when the link goes down.

But this is going to break things.

If the phy library previously programmed a non-autonegotiated static
link configuration into the BMCR register, your reset is going to
undo that.

Now the configuration phylib thinks the chip has and the one it
acutally does is out of sync.

I'm not applying these patches, sorry.

Hello.

On 11/14/2013 12:07 AM, Daniel Mack wrote:

When the link goes down and up again quickly and multiple times in a
row, the AT803x PHY gets stuck and won't recover unless it is soft-reset
via the BMCR register.

Unfortunately, there is no way to detect this state, as all registers
contain perfectly sane values in such cases. Hence, the only option is
to always soft-reset the PHY when the link goes down.

Reset logic is based on a patch from Marek Belisko.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: Marek Belisko <redacted>
---
  drivers/net/phy/at803x.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 52 insertions(+)

    This is the standard bit (BMCR_RESET) #define'd in the same file as MII_BMCR.

WBR, Sergei

On 11/14/2013 10:32 PM, David Miller wrote:

From: Daniel Mack <zonque@gmail.com>
Date: Wed, 13 Nov 2013 22:07:49 +0100

quoted

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>

So you're using this to reset the entire PHY via the reset bit in the
BMCR register when the link goes down.

But this is going to break things.

If the phy library previously programmed a non-autonegotiated static
link configuration into the BMCR register, your reset is going to
undo that.

Now the configuration phylib thinks the chip has and the one it
acutally does is out of sync.

You're right, thanks for the review. Let me just state that I'm really
unhappy about that approach as well.

However, I currently see no better way than resetting the PHY every time
the link goes down, as we have no way of telling whether the chip has
entered its locked-up state which it seemingly does arbitrarily when the
link changes. This is really not nice, but the only thing that avoided
the effect in our tests.

That means that we either have to tell the PHY core about what we did,
or manually preserve the registers contents across the reset. I'll have
another look next week. Any pointers appreciated :)


Daniel