net: phy: marvell: Fix up reset ordering

PATCH New Nathan Rossi 3 participants 5 messages Jun 1, 2016 – Jun 3, 2016

Subsystems (2)

Network drivers Ethernet PHY

lore

Share & tools

lore

Nathan Rossi

author

2016-06-01 16:22 UTC #0 lore raw

PATCH 1/1 net: phy: marvell: Fix up reset ordering

Commit a058052c "net: phy: do not read configuration register on reset", changes the behaviour of the phy_reset function such that the state of the BMCR register is not preserved during reset.

Reorder the phy_reset and genphy_config_aneg calls for some of the marvell phy drivers so that auto-negotiation occurs after reset.

Signed-off-by Nathan Rossi <[email protected]>

Cc Joe Hershberger <[email protected]>

Cc Michal Simek <[email protected]>

Cc Stefan Roese <[email protected]>

---
 drivers/net/phy/marvell.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/net/phy/marvell.c +6 -5

index d2e68d4..40284a5 100644
@@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device *phydev)
 			MIIM_M88E1145_RGMII_TX_DELAY;
 	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
 
-	genphy_config_aneg(phydev);
-
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
 	return 0;
 }

@@ -443,10 +443,10 @@ static int m88e1149_config(struct phy_device *phydev)
 	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x0);
 	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x100);
 
-	genphy_config_aneg(phydev);
-
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
 	return 0;
 }

@@ -476,9 +476,10 @@ static int m88e1310_config(struct phy_device *phydev)
 	/* Ensure to return to page 0 */
 	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0000);
 
-	genphy_config_aneg(phydev);
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
 	return 0;
 }

Signature · 1 line

2.8.1

Michal Simek

2016-06-02 05:31 UTC #1 lore raw

PATCH 1/1 net: phy: marvell: Fix up reset ordering

Hi Nathan,

Quoted 20 lines · Nathan Rossi

Commit a058052c "net: phy: do not read configuration register on reset", changes the behaviour of the phy_reset function such that the state of the BMCR register is not preserved during reset.
Reorder the phy_reset and genphy_config_aneg calls for some of the marvell phy drivers so that auto-negotiation occurs after reset.
Signed-off-by Nathan Rossi <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Michal Simek <[email protected]>
Cc Stefan Roese <[email protected]>
---
 drivers/net/phy/marvell.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/marvell.c +0 -0
index d2e68d4..40284a5 100644

drivers/net/phy/marvell.c static int m88e1145_config(struct phy_device *phydev) · line 420

Quoted 9 lines

drivers/net/phy/marvell.c +2 -2

@@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device *phydev)
 			MIIM_M88E1145_RGMII_TX_DELAY;
 	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
 
-	genphy_config_aneg(phydev);
-
 	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+

As you see from my patch 1b008fdb06848c7c84e7c1a4a9b2b76239550555
you should return value from genphy_config_aneg() which should be fixed everywhere.

Also as you see above you do some writes to the phy and the question is if you should run phy_reset here.
Based on my patch above and investigating I found that phy_reset is called before this function is called and not sure if phy should be reset twice.

Thanks,
Michal

Stefan Roese

2016-06-02 05:42 UTC #2 lore raw

PATCH 1/1 net: phy: marvell: Fix up reset ordering

Hi Michal,

Quoted 36 lines · Michal Simek

Quoted 20 lines · Nathan Rossi
Commit a058052c "net: phy: do not read configuration register on reset", changes the behaviour of the phy_reset function such that the state of the BMCR register is not preserved during reset.
Reorder the phy_reset and genphy_config_aneg calls for some of the marvell phy drivers so that auto-negotiation occurs after reset.
Signed-off-by Nathan Rossi <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Michal Simek <[email protected]>
Cc Stefan Roese <[email protected]>
---
  drivers/net/phy/marvell.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/marvell.c +0 -0
index d2e68d4..40284a5 100644
drivers/net/phy/marvell.c static int m88e1145_config(struct phy_device *phydev) · line 420
Quoted 9 lines
drivers/net/phy/marvell.c +2 -2
@@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device *phydev)
  			MIIM_M88E1145_RGMII_TX_DELAY;
  	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
 
-	genphy_config_aneg(phydev);
-
  	phy_reset(phydev);
 
+	genphy_config_aneg(phydev);
+
As you see from my patch 1b008fdb06848c7c84e7c1a4a9b2b76239550555
you should return value from genphy_config_aneg() which should be fixed everywhere.

Quoted 5 lines

Also as you see above you do some writes to the phy and the question is if you should run phy_reset here.
Based on my patch above and investigating I found that phy_reset is called before this function is called and not sure if phy should be reset twice.

Some changes to the PHY registers need a soft-reset to occur before these changes to be effective. Not sure if this is the case here though.

But I share your thoughts about this phy_reset() being called now in some of the xxx_config() functions and not in others. Your patch mentions that phy_reset() is already called in phy_connect_dev(), which not all ethernet drivers do right now AFAICT.

We should perhaps take a look at the Linux Marvell PHY driver to check how it is done there.

Thanks,
Stefan

Michal Simek

2016-06-02 05:59 UTC #3 lore raw

PATCH 1/1 net: phy: marvell: Fix up reset ordering

Quoted 50 lines · Stefan Roese

Hi Michal,
Quoted 37 lines · Michal Simek
Quoted 20 lines · Nathan Rossi
Commit a058052c "net: phy: do not read configuration register on reset", changes the behaviour of the phy_reset function such that the state of the BMCR register is not preserved during reset.
Reorder the phy_reset and genphy_config_aneg calls for some of the marvell phy drivers so that auto-negotiation occurs after reset.
Signed-off-by Nathan Rossi <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Michal Simek <[email protected]>
Cc Stefan Roese <[email protected]>
---
  drivers/net/phy/marvell.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/marvell.c +0 -0
index d2e68d4..40284a5 100644
drivers/net/phy/marvell.c static int m88e1145_config(struct phy_device · line 421
Quoted 10 lines
drivers/net/phy/marvell.c +2 -2
@@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device
 *phydev)
              MIIM_M88E1145_RGMII_TX_DELAY;
      phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
 
-    genphy_config_aneg(phydev);
-
      phy_reset(phydev);
 
+    genphy_config_aneg(phydev);
+
As you see from my patch 1b008fdb06848c7c84e7c1a4a9b2b76239550555
you should return value from genphy_config_aneg() which should be fixed everywhere.
Quoted 5 lines
Also as you see above you do some writes to the phy and the question is if you should run phy_reset here.
Based on my patch above and investigating I found that phy_reset is called before this function is called and not sure if phy should be reset twice.
Some changes to the PHY registers need a soft-reset to occur before these changes to be effective. Not sure if this is the case here though.

Quoted 7 lines

But I share your thoughts about this phy_reset() being called now in some of the xxx_config() functions and not in others. Your patch mentions that phy_reset() is already called in phy_connect_dev(), which not all ethernet drivers do right now AFAICT.
We should perhaps take a look at the Linux Marvell PHY driver to check how it is done there.

That was also the part of the reason that I have fixed just one particular phy which I have access to. I expect that some phy can require this reset and some of them not.
But definitely this should be properly tested.

Thanks,
Michal

Nathan Rossi

author

2016-06-03 13:15 UTC #4 lore raw

PATCH 1/1 net: phy: marvell: Fix up reset ordering

Quoted 51 lines · Michal Simek

Quoted 50 lines · Stefan Roese
Hi Michal,
Quoted 37 lines · Michal Simek
Quoted 20 lines · Nathan Rossi
Commit a058052c "net: phy: do not read configuration register on reset", changes the behaviour of the phy_reset function such that the state of the BMCR register is not preserved during reset.
Reorder the phy_reset and genphy_config_aneg calls for some of the marvell phy drivers so that auto-negotiation occurs after reset.
Signed-off-by Nathan Rossi <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Michal Simek <[email protected]>
Cc Stefan Roese <[email protected]>
---
  drivers/net/phy/marvell.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/marvell.c +0 -0
index d2e68d4..40284a5 100644
drivers/net/phy/marvell.c static int m88e1145_config(struct phy_device · line 421
Quoted 10 lines
drivers/net/phy/marvell.c +2 -2
@@ -414,10 +414,10 @@ static int m88e1145_config(struct phy_device
 *phydev)
              MIIM_M88E1145_RGMII_TX_DELAY;
      phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1145_PHY_EXT_CR, reg);
 
-    genphy_config_aneg(phydev);
-
      phy_reset(phydev);
 
+    genphy_config_aneg(phydev);
+
As you see from my patch 1b008fdb06848c7c84e7c1a4a9b2b76239550555
you should return value from genphy_config_aneg() which should be fixed everywhere.
Quoted 5 lines
Also as you see above you do some writes to the phy and the question is if you should run phy_reset here.
Based on my patch above and investigating I found that phy_reset is called before this function is called and not sure if phy should be reset twice.
Some changes to the PHY registers need a soft-reset to occur before these changes to be effective. Not sure if this is the case here though.

Quoted 7 lines

Quoted 7 lines
But I share your thoughts about this phy_reset() being called now in some of the xxx_config() functions and not in others. Your patch mentions that phy_reset() is already called in phy_connect_dev(), which not all ethernet drivers do right now AFAICT.
We should perhaps take a look at the Linux Marvell PHY driver to check how it is done there.

The m88e131(8/0) in linux does not reset during config. The m88e1145 in linux does not reset during config. The m88e1149 in linux does reset during config.

Also there are differences in what is configured between linux and u-boot, so it is a bit hard to determine if the reset is required based on the registers being configured.

Quoted 6 lines

That was also the part of the reason that I have fixed just one particular phy which I have access to. I expect that some phy can require this reset and some of them not.
But definitely this should be properly tested.

I am only using the 88e1318 (same as 88e1310 but 1.8V variant). In the configuration it is used on the board I am testing the phy reset does not appear to be needed, behaviour is consistent with and or without the reset (given the config_aneg being after the reset). With the config_aneg being before the reset, it causes the phy to disable autoneg.

I will send a separate patch that only applies to that device, removing the phy reset and making the return value that of genphy_config_aneg.

Regards,
Nathan