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

Hi Igor,

Am Freitag, 14. August 2015, 11:03:04 schrieb Igor Plyatov:

Dear Michael,

quoted

Hi Igor,

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

quoted

* Due to HW bug, LAN8700 sometimes does not detect presence of 

energy in the

quoted

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

bit is

quoted

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

bug of

quoted

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.

Long time ago, I worked on a custom device with a PHY of the same family.
Errata sheet existed but was only available by signing a NDA. So I simply
wondered whether this changed since SMSC is now Microchip or if they keep
it still so covered...

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 [off-list ref] as b629820d18fa65cc598390e4b9712fd5f83ee693.
  * Patrick Trantham [off-list ref] as 
4223dbffed9f89596177ff2b256ef3258b20fa46.

quoted

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).

quoted

quoted

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

Its previous

quoted

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

quoted

quoted

Signed-off-by: Igor Plyatov <redacted>

quoted

---

quoted

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

quoted

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

quoted

quoted

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

quoted

index c0f6479..8559ff1 100644

quoted

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

quoted

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

quoted

@@ -104,6 +104,7 @@ static int lan911x_config_init(struct phy_device 

*phydev)

quoted

static int lan87xx_read_status(struct phy_device *phydev)

quoted

{

quoted

int err = genphy_read_status(phydev);

quoted

+ int i;

quoted

quoted

if (!phydev->link) {

quoted

/* Disable EDPD to wake up PHY */

quoted

@@ -116,8 +117,16 @@ static int lan87xx_read_status(struct 

phy_device *phydev)

quoted

if (rc < 0)

quoted

return rc;

quoted

quoted

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

quoted

- msleep(64);

quoted

+ /* 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.

quoted

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.

Ok, I was not aware, that this is so much.

quoted

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.
  */

Nice. Only one nitpick: ... _is_ enabled again...

quoted

quoted

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

quoted

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

quoted

+ msleep(10);

quoted

+ rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);

quoted

+ if (rc < 0)

quoted

+ return rc;

quoted

+ if (rc & MII_LAN83C185_ENERGYON)

quoted

+ break;

quoted

+ };

quoted

quoted

/* Re-enable EDPD */

quoted

rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);

quoted

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

quoted

quoted

/* basic functions */

quoted

.config_aneg = genphy_config_aneg,

quoted

- .read_status = genphy_read_status,

quoted

+ .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.

quoted

quoted

.config_init = smsc_phy_config_init,

quoted

.soft_reset = smsc_phy_reset,

quoted

quoted

Regards,

Michael

Best wishes.

Thanks, Michael