Skip to contentSkip to conversation

net: phy: do not read configuration register on reset

PATCH In review Stefan Agner 6 participants 7 messages Dec 9, 2015 – Feb 9, 2016

Subsystems (2)
Network drivers Ethernet PHY
lore
Share & tools
lore

Stefan Agner

author
#0 lore raw

PATCH 1/1 net: phy: do not read configuration register on reset

When doing a software reset, the reset flag should be written without other bits set. Writing the current state will lead to restoring the state of the PHY (e.g. Powerdown), which is not what is expected from a software reset.

Signed-off-by Stefan Agner <[email protected]>

---
This lead to the PHY staying powered down on a reboot when using a Micrel PHY and not using hardware reset...

It also aligns with the behavior of Linux:
http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c#L1122

--
Stefan

 drivers/net/phy/phy.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/net/phy/phy.c +2 -9
index 51b5746..ef9f13b 100644
@@ -717,15 +717,7 @@ int phy_reset(struct phy_device *phydev)
 	}
 #endif
 
-	reg = phy_read(phydev, devad, MII_BMCR);
-	if (reg < 0) {
-		debug("PHY status read failed\n");
-		return -1;
-	}
-
-	reg |= BMCR_RESET;
-
-	if (phy_write(phydev, devad, MII_BMCR, reg) < 0) {
+	if (phy_write(phydev, devad, MII_BMCR, BMCR_RESET) < 0) {
 		debug("PHY reset failed\n");
 		return -1;
 	}

@@ -738,6 +730,7 @@ int phy_reset(struct phy_device *phydev)
 	 * auto-clearing).  This should happen within 0.5 seconds per the
 	 * IEEE spec.
 	 */
+	reg = phy_read(phydev, devad, MII_BMCR);
 	while ((reg & BMCR_RESET) && timeout--) {
 		reg = phy_read(phydev, devad, MII_BMCR);
Signature · 1 line
2.6.3
#1 lore raw

PATCH 1/1 net: phy: do not read configuration register on reset

Quoted 6 lines · Stefan Agner

When doing a software reset, the reset flag should be written without other bits set. Writing the current state will lead to restoring the state of the PHY (e.g. Powerdown), which is not what is expected from a software reset.

Signed-off-by Stefan Agner <[email protected]>
Acked-by Michael Welling <[email protected]>
Quoted 49 lines

---
This lead to the PHY staying powered down on a reboot when using a Micrel PHY and not using hardware reset...

It also aligns with the behavior of Linux:
http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c#L1122

--
Stefan

 drivers/net/phy/phy.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/net/phy/phy.c +2 -9
index 51b5746..ef9f13b 100644
@@ -717,15 +717,7 @@ int phy_reset(struct phy_device *phydev)
 	}
 #endif
 
-	reg = phy_read(phydev, devad, MII_BMCR);
-	if (reg < 0) {
-		debug("PHY status read failed\n");
-		return -1;
-	}
-
-	reg |= BMCR_RESET;
-
-	if (phy_write(phydev, devad, MII_BMCR, reg) < 0) {
+	if (phy_write(phydev, devad, MII_BMCR, BMCR_RESET) < 0) {
 		debug("PHY reset failed\n");
 		return -1;
 	}

@@ -738,6 +730,7 @@ int phy_reset(struct phy_device *phydev)
 	 * auto-clearing).  This should happen within 0.5 seconds per the
 	 * IEEE spec.
 	 */
+	reg = phy_read(phydev, devad, MII_BMCR);
 	while ((reg & BMCR_RESET) && timeout--) {
 		reg = phy_read(phydev, devad, MII_BMCR);
Signature · 6 lines
2.6.3

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
#2 lore raw

PATCH 1/1 net: phy: do not read configuration register on reset

In reply to Stefan Agner
Quoted 10 lines · Stefan Agner

When doing a software reset, the reset flag should be written without other bits set. Writing the current state will lead to restoring the state of the PHY (e.g. Powerdown), which is not what is expected from a software reset.

Signed-off-by Stefan Agner <[email protected]>

---
This lead to the PHY staying powered down on a reboot when using a Micrel PHY and not using hardware reset...

Quoted 8 lines

It also aligns with the behavior of Linux:
http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c#L1122

--
Stefan

 drivers/net/phy/phy.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by Bin Meng <[email protected]>
#6 lore raw

PATCH 1/1 net: phy: do not read configuration register on reset

In reply to Stefan Agner
Quoted 41 lines · Stefan Agner

When doing a software reset, the reset flag should be written without other bits set. Writing the current state will lead to restoring the state of the PHY (e.g. Powerdown), which is not what is expected from a software reset.

Signed-off-by Stefan Agner <[email protected]>

---
This lead to the PHY staying powered down on a reboot when using a Micrel PHY and not using hardware reset...

It also aligns with the behavior of Linux:
http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c#L1122

--
Stefan

  drivers/net/phy/phy.c | 11 ++---------
  1 file changed, 2 insertions(+), 9 deletions(-)
drivers/net/phy/phy.c +1 -9
index 51b5746..ef9f13b 100644
@@ -717,15 +717,7 @@ int phy_reset(struct phy_device *phydev)
  	}
  #endif
 
-	reg = phy_read(phydev, devad, MII_BMCR);
-	if (reg < 0) {
-		debug("PHY status read failed\n");
-		return -1;
-	}
-
-	reg |= BMCR_RESET;
-
-	if (phy_write(phydev, devad, MII_BMCR, reg) < 0) {
+	if (phy_write(phydev, devad, MII_BMCR, BMCR_RESET) < 0) {
  		debug("PHY reset failed\n");
  		return -1;
  	}

drivers/net/phy/phy.c int phy_reset(struct phy_device *phydev) · line 735

Quoted 6 lines
drivers/net/phy/phy.c +1 -0
@@ -738,6 +730,7 @@ int phy_reset(struct phy_device *phydev)
  	 * auto-clearing).  This should happen within 0.5 seconds per the
  	 * IEEE spec.
  	 */
+	reg = phy_read(phydev, devad, MII_BMCR);
  	while ((reg & BMCR_RESET) && timeout--) {
  		reg = phy_read(phydev, devad, MII_BMCR);

This breaks PHY link auto negotiation support on Marvell Armada 388 ClearFog for me. As with this patch, bit 12 (A/N enable) will get cleared. And the link is not established correctly.

The problem here seems to be, that the Marvell PHY driver calls phy_reset() again at the end of m88e1111s_config(). Resulting in the loss of the configuration of the MII_BMCR register. I'm currently thinking if this patchbelow would be the correct fix for this problem?

drivers/net/phy/marvell.c +1 -2
index eab1558..8ede927 100644
@@ -270,8 +270,7 @@ static int m88e1111s_config(struct phy_device *phydev)
                printf("%s: phy soft reset timeout\n", __func__);
 
        genphy_config_aneg(phydev);
-
-       phy_reset(phydev);
+       genphy_restart_aneg(phydev);

Thanks,
Stefan