[U-Boot] [PATCH] 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 nathan@nathanrossi.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Michal Simek michal.simek@xilinx.com Cc: Stefan Roese sr@denx.de --- drivers/net/phy/marvell.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index d2e68d4..40284a5 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -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; }
Hi Nathan,
On 1.6.2016 18:22, Nathan Rossi wrote:
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 nathan@nathanrossi.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Michal Simek michal.simek@xilinx.com Cc: Stefan Roese sr@denx.de
drivers/net/phy/marvell.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index d2e68d4..40284a5 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -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
Hi Michal,
On 02.06.2016 07:31, Michal Simek wrote:
On 1.6.2016 18:22, Nathan Rossi wrote:
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 nathan@nathanrossi.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Michal Simek michal.simek@xilinx.com Cc: Stefan Roese sr@denx.de
drivers/net/phy/marvell.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index d2e68d4..40284a5 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -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.
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
On 2.6.2016 07:42, Stefan Roese wrote:
Hi Michal,
On 02.06.2016 07:31, Michal Simek wrote:
On 1.6.2016 18:22, Nathan Rossi wrote:
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 nathan@nathanrossi.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Michal Simek michal.simek@xilinx.com Cc: Stefan Roese sr@denx.de
drivers/net/phy/marvell.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index d2e68d4..40284a5 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -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.
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.
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
On Thu, Jun 2, 2016 at 3:59 PM, Michal Simek michal.simek@xilinx.com wrote:
On 2.6.2016 07:42, Stefan Roese wrote:
Hi Michal,
On 02.06.2016 07:31, Michal Simek wrote:
On 1.6.2016 18:22, Nathan Rossi wrote:
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 nathan@nathanrossi.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Michal Simek michal.simek@xilinx.com Cc: Stefan Roese sr@denx.de
drivers/net/phy/marvell.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index d2e68d4..40284a5 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -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.
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.
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.
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
participants (3)
-
Michal Simek -
Nathan Rossi -
Stefan Roese