* [U-Boot] [PATCH] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
@ 2015-12-09 19:32 ` Michael Welling
2015-12-10 0:52 ` Bin Meng
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Michael Welling @ 2015-12-09 19:32 UTC (permalink / raw)
To: u-boot
On Wed, Dec 09, 2015 at 11:21:25AM -0800, Stefan Agner wrote:
> 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 <stefan@agner.ch>
Acked-by: Michael Welling <mwelling@ieee.org>
> ---
> 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(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 51b5746..ef9f13b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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);
>
> --
> 2.6.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
2015-12-09 19:32 ` Michael Welling
@ 2015-12-10 0:52 ` Bin Meng
2015-12-10 20:28 ` Joe Hershberger
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bin Meng @ 2015-12-10 0:52 UTC (permalink / raw)
To: u-boot
On Thu, Dec 10, 2015 at 3:21 AM, Stefan Agner <stefan@agner.ch> wrote:
> 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 <stefan@agner.ch>
> ---
> 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(-)
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
2015-12-09 19:32 ` Michael Welling
2015-12-10 0:52 ` Bin Meng
@ 2015-12-10 20:28 ` Joe Hershberger
2016-01-29 21:25 ` [U-Boot] " Joe Hershberger
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Joe Hershberger @ 2015-12-10 20:28 UTC (permalink / raw)
To: u-boot
Hi Stefan,
On Wed, Dec 9, 2015 at 1:21 PM, Stefan Agner <stefan@agner.ch> wrote:
> 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 <stefan@agner.ch>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
` (2 preceding siblings ...)
2015-12-10 20:28 ` Joe Hershberger
@ 2016-01-29 21:25 ` Joe Hershberger
2016-01-29 21:26 ` Joe Hershberger
2016-02-09 18:42 ` [U-Boot] [PATCH] " Stefan Roese
5 siblings, 0 replies; 7+ messages in thread
From: Joe Hershberger @ 2016-01-29 21:25 UTC (permalink / raw)
To: u-boot
Hi Stefan,
https://patchwork.ozlabs.org/patch/554780/ was applied to u-boot-net.git.
Thanks!
-Joe
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
` (3 preceding siblings ...)
2016-01-29 21:25 ` [U-Boot] " Joe Hershberger
@ 2016-01-29 21:26 ` Joe Hershberger
2016-02-09 18:42 ` [U-Boot] [PATCH] " Stefan Roese
5 siblings, 0 replies; 7+ messages in thread
From: Joe Hershberger @ 2016-01-29 21:26 UTC (permalink / raw)
To: u-boot
Hi Stefan,
https://patchwork.ozlabs.org/patch/554780/ was applied to u-boot-net.git.
Thanks!
-Joe
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] net: phy: do not read configuration register on reset
2015-12-09 19:21 [U-Boot] [PATCH] net: phy: do not read configuration register on reset Stefan Agner
` (4 preceding siblings ...)
2016-01-29 21:26 ` Joe Hershberger
@ 2016-02-09 18:42 ` Stefan Roese
5 siblings, 0 replies; 7+ messages in thread
From: Stefan Roese @ 2016-02-09 18:42 UTC (permalink / raw)
To: u-boot
On 09.12.2015 20:21, Stefan Agner wrote:
> 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 <stefan@agner.ch>
> ---
> 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(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 51b5746..ef9f13b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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);
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?
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index eab1558..8ede927 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -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
^ permalink raw reply related [flat|nested] 7+ messages in thread