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

[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 <plyatov@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

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

[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;
>  

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

[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

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

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

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

[David Miller]

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

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

[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

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