| Message ID | 1439471556-13760-1-git-send-email-plyatov@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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; > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
* 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(-)