[PATCH v4] lxt PHY: Support for the buggy LXT973 rev A2

This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding
precautions linked to ERRATA Item 4:

Revision A2 of LXT973 chip randomly returns the contents of the previous even
register when you read a odd register regularly

Signed-off-by: Christophe Leroy <redacted>

diff -u a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c

--- a/drivers/net/phy/lxt.c	2012-09-23 03:08:48.000000000 +0200
+++ b/drivers/net/phy/lxt.c	2012-09-23 03:18:00.000000000 +0200

@@ -122,6 +122,123 @@
 	return err;
 }
 
+/*
+ * A2 version of LXT973 chip has an ERRATA: it randomly return the contents
+ * of the previous even register when you read a odd register regularly
+ */
+
+static int lxt973a2_update_link(struct phy_device *phydev)
+{
+	int status;
+	int control;
+	int retry = 8; /* we try 8 times */
+
+	/* Do a fake read */
+	status = phy_read(phydev, MII_BMSR);
+
+	if (status < 0)
+		return status;
+
+	control = phy_read(phydev, MII_BMCR);
+	if (control < 0)
+		return control;
+
+	do {
+		/* Read link and autonegotiation status */
+		status = phy_read(phydev, MII_BMSR);
+	} while (status >= 0 && retry-- && status == control);
+
+	if (status < 0)
+		return status;
+
+	if ((status & BMSR_LSTATUS) == 0)
+		phydev->link = 0;
+	else
+		phydev->link = 1;
+
+	return 0;
+}
+
+int lxt973a2_read_status(struct phy_device *phydev)
+{
+	int adv;
+	int err;
+	int lpa;
+	int lpagb = 0;
+
+	/* Update the link, but return if there was an error */
+	err = lxt973a2_update_link(phydev);
+	if (err)
+		return err;
+
+	if (AUTONEG_ENABLE == phydev->autoneg) {
+		int retry = 1;
+
+		adv = phy_read(phydev, MII_ADVERTISE);
+
+		if (adv < 0)
+			return adv;
+
+		do {
+			lpa = phy_read(phydev, MII_LPA);
+
+			if (lpa < 0)
+				return lpa;
+
+			/* If both registers are equal, it is suspect but not
+			* impossible, hence a new try
+			*/
+		} while (lpa == adv && retry--);
+
+		lpa &= adv;
+
+		phydev->speed = SPEED_10;
+		phydev->duplex = DUPLEX_HALF;
+		phydev->pause = phydev->asym_pause = 0;
+
+		if (lpagb & (LPA_1000FULL | LPA_1000HALF)) {
+			phydev->speed = SPEED_1000;
+
+			if (lpagb & LPA_1000FULL)
+				phydev->duplex = DUPLEX_FULL;
+		} else if (lpa & (LPA_100FULL | LPA_100HALF)) {
+			phydev->speed = SPEED_100;
+
+			if (lpa & LPA_100FULL)
+				phydev->duplex = DUPLEX_FULL;
+		} else {
+			if (lpa & LPA_10FULL)
+				phydev->duplex = DUPLEX_FULL;
+		}
+
+		if (phydev->duplex == DUPLEX_FULL) {
+			phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
+			phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
+		}
+	} else {
+		int bmcr = phy_read(phydev, MII_BMCR);
+
+		if (bmcr < 0)
+			return bmcr;
+
+		if (bmcr & BMCR_FULLDPLX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		if (bmcr & BMCR_SPEED1000)
+			phydev->speed = SPEED_1000;
+		else if (bmcr & BMCR_SPEED100)
+			phydev->speed = SPEED_100;
+		else
+			phydev->speed = SPEED_10;
+
+		phydev->pause = phydev->asym_pause = 0;
+	}
+
+	return 0;
+}
+
 static int lxt973_probe(struct phy_device *phydev)
 {
 	int val = phy_read(phydev, MII_LXT973_PCR);

@@ -175,6 +292,16 @@
 	.driver		= { .owner = THIS_MODULE,},
 }, {
 	.phy_id		= 0x00137a10,
+	.name		= "LXT973-A2",
+	.phy_id_mask	= 0xffffffff,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= lxt973a2_read_status,
+	.driver		= { .owner = THIS_MODULE,},
+}, {
+	.phy_id		= 0x00137a10,
 	.name		= "LXT973",
 	.phy_id_mask	= 0xfffffff0,
 	.features	= PHY_BASIC_FEATURES,

This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding
precautions linked to ERRATA Item 4:

Revision A2 of LXT973 chip randomly returns the contents of the previous even
register when you read a odd register regularly

Does reading the PHY registers involve bit-banging an MII interface?
If so this code is likely to stall the system for significant
periods (ready phy registers at all can be a problem).

I know some ethernet mac have hardware blocks for reading MII
and even polling one MII register for changes.

Maybe some of this code ought to be using async software
bit-bang - especially when just polling for link status change.
I'm sure it ought to be possible to do one bit-bang action
per clock tick instead of spinning for the required delays. 

	David

Le 24/09/2012 16:13, David Laight a écrit :

quoted

This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding
precautions linked to ERRATA Item 4:

Revision A2 of LXT973 chip randomly returns the contents of the previous even
register when you read a odd register regularly

Does reading the PHY registers involve bit-banging an MII interface?
If so this code is likely to stall the system for significant
periods (ready phy registers at all can be a problem).

I know some ethernet mac have hardware blocks for reading MII
and even polling one MII register for changes.

Maybe some of this code ought to be using async software
bit-bang - especially when just polling for link status change.
I'm sure it ought to be possible to do one bit-bang action
per clock tick instead of spinning for the required delays.

	David

Not sure I understand what you mean. We have been using this code 
without any problem for about 2 years on our Hardware.
It does almost same as genphy_read_status() except that it also reads 
the BMCR register (which is the register preceeding the BMSR) in order 
to detect the unlikely happening of the bug reported by the ERRATA. In 
case it happens (which is really seldom), it does a re-read.
We are not spinning on any delays here.

Christophe

On Mon, Sep 24, 2012 at 04:00:58PM +0200, Christophe Leroy wrote:

...

I like this way of matching the A2 chips much better than what you had
before. But are you sure this will work correctly?

What do A3 chips have in the last nibble of phy_id?

+	.driver		= { .owner = THIS_MODULE,},
+}, {
+	.phy_id		= 0x00137a10,
 	.name		= "LXT973",
 	.phy_id_mask	= 0xfffffff0,
 	.features	= PHY_BASIC_FEATURES,

Thanks,
Richard

Le 24/09/2012 20:30, Richard Cochran a écrit :

On Mon, Sep 24, 2012 at 04:00:58PM +0200, Christophe Leroy wrote:

quoted

diff -u a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c

--- a/drivers/net/phy/lxt.c	2012-09-23 03:08:48.000000000 +0200
+++ b/drivers/net/phy/lxt.c	2012-09-23 03:18:00.000000000 +0200

...

quoted

@@ -175,6 +292,16 @@
  	.driver		= { .owner = THIS_MODULE,},
  }, {
  	.phy_id		= 0x00137a10,
+	.name		= "LXT973-A2",
+	.phy_id_mask	= 0xffffffff,
+	.features	= PHY_BASIC_FEATURES,
+	.flags		= 0,
+	.probe		= lxt973_probe,
+	.config_aneg	= lxt973_config_aneg,
+	.read_status	= lxt973a2_read_status,

I like this way of matching the A2 chips much better than what you had
before. But are you sure this will work correctly?

Apparently it does.

What do A3 chips have in the last nibble of phy_id?

A2 chip has phy_id 0x00137a10
A3 chip has phy_id 0x00137a11

Christophe

quoted

+	.driver		= { .owner = THIS_MODULE,},
+}, {
+	.phy_id		= 0x00137a10,
  	.name		= "LXT973",
  	.phy_id_mask	= 0xfffffff0,
  	.features	= PHY_BASIC_FEATURES,

Thanks,
Richard

On Tue, Sep 25, 2012 at 08:23:42AM +0200, leroy christophe wrote:

A2 chip has phy_id 0x00137a10
A3 chip has phy_id 0x00137a11

Okay then, thanks.

Acked-by: Richard Cochran <richardcochran@gmail.com>

From: Richard Cochran <richardcochran@gmail.com>
Date: Tue, 25 Sep 2012 09:47:36 +0200

On Tue, Sep 25, 2012 at 08:23:42AM +0200, leroy christophe wrote:

quoted

A2 chip has phy_id 0x00137a10
A3 chip has phy_id 0x00137a11

Okay then, thanks.

Acked-by: Richard Cochran <richardcochran@gmail.com>

Applied to net-next, thanks.