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

[Igor Plyatov]

* 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.
* 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 <ply...@gmail.com>
---
drivers/net/phy/smsc.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index c0f6479..a380958 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -104,10 +104,12 @@ 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 rc;
+ int i;

if (!phydev->link) {
/* Disable EDPD to wake up PHY */
- int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
+ rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
if (rc < 0)
return rc;

@@ -116,8 +118,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 */
+ 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 +201,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,
.config_init = smsc_phy_config_init,
.soft_reset = smsc_phy_reset,

--
1.7.9.5

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

[Joe Perches]

On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
> * 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.
> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous
> algorythm still not reliable on 100 % and sometimes skip cable plugging.

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

[]
> @@ -104,10 +104,12 @@ 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 rc;

Is there a reason to move this declaration?

> + int i;
>
> if (!phydev->link) {
> /* Disable EDPD to wake up PHY */
> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> if (rc < 0)
> return rc;
>

[Igor Plyatov]

Dear Joe,

> On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
>> * 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.
>> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous
>> algorythm still not reliable on 100 % and sometimes skip cable plugging.
> []
>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> []
>> @@ -104,10 +104,12 @@ 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 rc;
> Is there a reason to move this declaration?

There is no strict requirement to move declaration of the rc.
It was made just to have all declarations easily visible.

>> + int i;
>>
>> if (!phydev->link) {
>> /* Disable EDPD to wake up PHY */
>> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>> + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>> if (rc < 0)
>> return rc;
>>
>

Best wishes
--
Igor Plyatov

[Joe Perches]

On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:
> > On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
> >> * 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.
> >> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous
> >> algorythm still not reliable on 100 % and sometimes skip cable plugging.
> > []
> >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > []
> >> @@ -104,10 +104,12 @@ 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 rc;
> > Is there a reason to move this declaration?
>
> There is no strict requirement to move declaration of the rc.
> It was made just to have all declarations easily visible.

Generally it's better to have declarations
in the minimal/narrowest scope possible.

[David Miller]

From: Joe Perches <j...@perches.com>
Date: Thu, 13 Aug 2015 10:15:15 -0700

> On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:
>> > On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
>> >> * 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.
>> >> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous
>> >> algorythm still not reliable on 100 % and sometimes skip cable plugging.
>> > []
>> >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> > []
>> >> @@ -104,10 +104,12 @@ 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 rc;
>> > Is there a reason to move this declaration?
>>
>> There is no strict requirement to move declaration of the rc.
>> It was made just to have all declarations easily visible.
>
> Generally it's better to have declarations
> in the minimal/narrowest scope possible.

Agreed, and it's %100 unrelated to the purpose of this patch so not
should be included for that reason as well.

You will need to respin this patch with the variable moving elided.

[Igor Plyatov]

* 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.
* 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 <ply...@gmail.com>
---

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 */
+ 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,
.config_init = smsc_phy_config_init,
.soft_reset = smsc_phy_reset,

--
1.7.9.5

[Igor Plyatov]

Dear David and Joe,

thank you for patch review!

Please look at email with subject
"[PATCH v2] net: phy: workaround for buggy cable detection by LAN8700
after cable plugging"

Best wishes.
--
Igor Plyatov

[Igor Plyatov]

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 <ma...@denx.de> as b629820d18fa65cc598390e4b9712fd5f83ee693.
* Patrick Trantham <patrick....@fuel7.com> 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).

> 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

[Michael Heimpold]

Hi Igor,

Am Freitag, 14. August 2015, 11:03:04 schrieb Igor Plyatov:
> 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.

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

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

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

Thanks, Michael

[Igor Plyatov]

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

* 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 <ply...@gmail.com>
---

drivers/net/phy/smsc.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index c0f6479..d64f016 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -91,19 +91,18 @@ static int lan911x_config_init(struct phy_device *phydev)
}

/*
- * The LAN8710/LAN8720 requires a minimum of 2 link pulses within 64ms of each
- * other in order to set the ENERGYON bit and exit EDPD mode. If a link partner
- * does send the pulses within this interval, the PHY will remained powered
- * down.
- *
- * This workaround will manually toggle the PHY on/off upon calls to read_status
- * in order to generate link test pulses if the link is down. If a link partner
- * is present, it will respond to the pulses, which will cause the ENERGYON bit
- * to be set and will cause the EDPD mode to be exited.
+ * 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 is enabled again in the end of procedure to
+ * save approximately 220 mW of power if cable is unplugged.
*/

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 +115,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 */

+ 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 +198,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,

.config_init = smsc_phy_config_init,
.soft_reset = smsc_phy_reset,

--
1.7.9.5

[Igor Plyatov]

Dear Michael,

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

The Microchip web-site does not contain Errata sheet for LAN87xx devices.
While it contains many Errata sheets for PIC and dsPIC devices.
So, situation is same as many years ago.

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

Changed in [PATCH v3].

Best wishes.
--
Igor Plyatov

[David Miller]

From: Igor Plyatov <ply...@gmail.com>
Date: Fri, 14 Aug 2015 20:11:02 +0300

> * 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.
> * 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 <ply...@gmail.com>

Applied, thanks.