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

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

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.

[]

[]

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;
 

Dear Joe,

On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:

quoted

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

[]

quoted

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

[]

quoted

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

quoted

+	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

On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:

quoted

On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:

quoted

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

[]

quoted

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

[]

quoted

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

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

On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:

quoted

quoted

On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:

quoted

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

[]

quoted

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

[]

quoted

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

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

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

quoted

On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:

quoted

quoted

On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:

quoted

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

[]

quoted

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

[]

quoted

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