Re: [PATCH v2] net: phy: workaround for buggy cable detection by LAN8700 after cable plugging

From: Igor Plyatov
Date: Fri Aug 14 2015 - 04:03:21 EST


Dear Michael,

Hi Igor,

Am Donnerstag, 13. August 2015, 22:18:34 schrieben Sie:

> * Due to HW bug, LAN8700 sometimes does not detect presence of energy in the

> Ethernet cable in Energy Detect Power-Down mode (e.g while EDPWRDOWN bit is

> set, the ENERGYON bit does not asserted sometimes). This is a common bug of

> LAN87xx family of PHY chips.

Is there any offical errata sheet for this PHY family? How do you know, that this is a

common HW bug?


The LAN8700, LAN8710, LAN8720 is a product of the SMSC company. Microchip acquired SMSC in August 2012.

The LAN8700 is a legacy product for Microchip and they will not update anything about it. So, even if Microchip know about HW bug, then there is no chance to have Errata sheet or any new documents about LAN8700.

I think same history is for LAN8710/LAN8720 even if they are not marked as legacy. They are SMSC products.

The workarounds for same issue in LAN8710/LAN8720 was committed by:
* Marek Vasut <marex@xxxxxxx> as b629820d18fa65cc598390e4b9712fd5f83ee693.
* Patrick Trantham <patrick.trantham@xxxxxxxxx> as 4223dbffed9f89596177ff2b256ef3258b20fa46.

Me too, I think that this family has some problems with this mode, however, without

hard evidence, I would put it softer.


I have discovered this bug by just monitoring of data to/from MDIO registers of LAN8700.
And HW issue is proven on 100 % by rare absence of ENERGYON bit when cable is plugged in.
Sometimes, it is required to make 2-20 tests to catch this issue.

The configuration of CPU pins, responsible for the MDIO interface, was checked carefully by oscilloscope and they are fine (no spikes, no garbage, good shape of edges).

> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous

> algorythm still not reliable on 100 % and sometimes skip cable plugging.

>

> Signed-off-by: Igor Plyatov <plyatov@xxxxxxxxx>

> ---

> drivers/net/phy/smsc.c | 15 ++++++++++++---

> 1 file changed, 12 insertions(+), 3 deletions(-)

>

> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c

> index c0f6479..8559ff1 100644

> --- a/drivers/net/phy/smsc.c

> +++ b/drivers/net/phy/smsc.c

> @@ -104,6 +104,7 @@ static int lan911x_config_init(struct phy_device *phydev)

> static int lan87xx_read_status(struct phy_device *phydev)

> {

> int err = genphy_read_status(phydev);

> + int i;

>

> if (!phydev->link) {

> /* Disable EDPD to wake up PHY */

> @@ -116,8 +117,16 @@ static int lan87xx_read_status(struct phy_device *phydev)

> if (rc < 0)

> return rc;

>

> - /* Sleep 64 ms to allow ~5 link test pulses to be sent */

> - msleep(64);

> + /* Wait max 640 ms to detect energy */

Why 640ms and not e.g. 650ms?

I'm no PHY expert, but this looks like an ugly workaround.


Such a value was adopted after many trial and probes. It allows to detect cable plugging on 100 %.
Ugly or not, but it works and reliable.

Maybe it would be better to avoid this power saving mode at all, when it is not

reliable, but this are just my 2cts. :-)


Power saving mode allow to save around 220 mW of energy consumed from power supply, when Ethernet cable is not plugged in.
This is a good value for embedded devices.
Better to keep power save mode on.

Anyway, I guess you should also update the explanation on top of the function to reflect

your new approach.


I propose following comment for the lan87xx_read_status():
/*
* The LAN87xx suffers from rare absence of the ENERGYON-bit when Ethernet cable
* plugs in while LAN87xx is in Energy Detect Power-Down mode. This leads to
* unstable detection of plugging in Ethernet cable.
* This workaround disables Energy Detect Power-Down mode and waiting for
* response on link pulses to detect presence of plugged Ethernet cable.
* The Energy Detect Power-Down mode enabled again in the end of procedure to
* save approximately 220 mW of power if cable is unplugged.
*/

> + for (i = 0; i < 64; i++) {

> + /* Sleep to allow link test pulses to be sent */

> + msleep(10);

> + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);

> + if (rc < 0)

> + return rc;

> + if (rc & MII_LAN83C185_ENERGYON)

> + break;

> + };

>

> /* Re-enable EDPD */

> rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);

> @@ -191,7 +200,7 @@ static struct phy_driver smsc_phy_driver[] = {

>

> /* basic functions */

> .config_aneg = genphy_config_aneg,

> - .read_status = genphy_read_status,

> + .read_status = lan87xx_read_status,

This one makes sense, since I really guess, that the whole PHY family behave very similar.

But this change alone does not solve your problem, right?


Yes, use of non modified lan87xx_read_status() only reduce amount of false cable detections, but does not resolve issue completely.

> .config_init = smsc_phy_config_init,

> .soft_reset = smsc_phy_reset,

>

>

Regards,

Michael


Best wishes.

--
Igor Plyatov

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/