net: phy: micrel: Fix MMD register access during SPD in ksz9131_resume()

[PATCH net] net: phy: micrel: Fix MMD register access during SPD in ksz9131_resume()

Posted by Ovidiu Panait 2 months, 2 weeks ago

During system suspend, phy_suspend() puts the PHY into Software Power-Down
(SPD) by setting the BMCR_PDOWN bit in MII_BMCR. According to the KSZ9131
datasheet, MMD register access is restricted during SPD:

  - Only access to the standard registers (0 through 31) is supported.
  - Access to MMD address spaces other than MMD address space 1 is
    possible if the spd_clock_gate_override bit is set.
  - Access to MMD address space 1 is not possible.

However, ksz9131_resume() calls ksz9131_config_rgmii_delay() before
kszphy_resume() clears BMCR_PDOWN. This means MMD registers are accessed
while the PHY is still in SPD, contrary to the datasheet.

Additionally, on platforms where the PHY loses power during suspend
(e.g. RZ/G3E), all settings from ksz9131_config_init(), not just the
RGMII delays, are lost and need to be restored. When the MAC driver
sets mac_managed_pm (e.g. stmmac), mdio_bus_phy_resume() is skipped,
so phy_init_hw() (which calls config_init to restore all PHY settings)
is never invoked during resume.

Fix this by replacing the RGMII delay restoration with a call to
phy_init_hw(), which takes the PHY out of SPD and performs full
reinitialization.

Fixes: f25a7eaa897f ("net: phy: micrel: Add ksz9131_resume()")
Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>
---
 drivers/net/phy/micrel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2aa1dedd21b8..4236dbf4ad6b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -6016,8 +6016,13 @@ static int lan8841_suspend(struct phy_device *phydev)
 
 static int ksz9131_resume(struct phy_device *phydev)
 {
-	if (phydev->suspended && phy_interface_is_rgmii(phydev))
-		ksz9131_config_rgmii_delay(phydev);
+	int ret;
+
+	if (phydev->suspended) {
+		ret = phy_init_hw(phydev);
+		if (ret)
+			return ret;
+	}
 
 	return kszphy_resume(phydev);
 }
-- 
2.51.0

Re: [PATCH net] net: phy: micrel: Fix MMD register access during SPD in ksz9131_resume()

Posted by Geert Uytterhoeven 2 months, 2 weeks ago

Hi Ovidiu,

On Fri, 3 Apr 2026 at 13:18, Ovidiu Panait <ovidiu.panait.rb@renesas.com> wrote:
> During system suspend, phy_suspend() puts the PHY into Software Power-Down
> (SPD) by setting the BMCR_PDOWN bit in MII_BMCR. According to the KSZ9131
> datasheet, MMD register access is restricted during SPD:
>
>   - Only access to the standard registers (0 through 31) is supported.
>   - Access to MMD address spaces other than MMD address space 1 is
>     possible if the spd_clock_gate_override bit is set.
>   - Access to MMD address space 1 is not possible.
>
> However, ksz9131_resume() calls ksz9131_config_rgmii_delay() before
> kszphy_resume() clears BMCR_PDOWN. This means MMD registers are accessed
> while the PHY is still in SPD, contrary to the datasheet.
>
> Additionally, on platforms where the PHY loses power during suspend
> (e.g. RZ/G3E), all settings from ksz9131_config_init(), not just the
> RGMII delays, are lost and need to be restored. When the MAC driver
> sets mac_managed_pm (e.g. stmmac), mdio_bus_phy_resume() is skipped,
> so phy_init_hw() (which calls config_init to restore all PHY settings)
> is never invoked during resume.
>
> Fix this by replacing the RGMII delay restoration with a call to
> phy_init_hw(), which takes the PHY out of SPD and performs full
> reinitialization.
>
> Fixes: f25a7eaa897f ("net: phy: micrel: Add ksz9131_resume()")
> Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>

Thanks for your patch!

> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -6016,8 +6016,13 @@ static int lan8841_suspend(struct phy_device *phydev)
>
>  static int ksz9131_resume(struct phy_device *phydev)
>  {
> -       if (phydev->suspended && phy_interface_is_rgmii(phydev))
> -               ksz9131_config_rgmii_delay(phydev);
> +       int ret;
> +
> +       if (phydev->suspended) {
> +               ret = phy_init_hw(phydev);
> +               if (ret)
> +                       return ret;
> +       }
>
>         return kszphy_resume(phydev);
>  }

This function is now no longer KSZ9131-specific.
I am wondering if this should be done for other Micrel PHYs, too,
e.g. by moving the phy_init_hw() call into kszphy_resume()?

Ethernet after resume has always been flaky on Salvator-X with KSZ9031
and R-Car M3-W ES1.0 (this seems to be specific to R-Car M3-W, as
boards with R-Car H3 or M3-N do not seem to suffer from this; don't
ask me why).

I have just tried:

-       .resume         = kszphy_resume,
+       .resume         = ksz9131_resume,

in the KSZ9031 entry, and ... surprise! Ethernet on R-Car M3-W now
works much better after resume!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net] net: phy: micrel: Fix MMD register access during SPD in ksz9131_resume()

Posted by Russell King (Oracle) 2 months, 2 weeks ago

On Fri, Apr 03, 2026 at 11:17:38AM +0000, Ovidiu Panait wrote:
> During system suspend, phy_suspend() puts the PHY into Software Power-Down
> (SPD) by setting the BMCR_PDOWN bit in MII_BMCR. According to the KSZ9131
> datasheet, MMD register access is restricted during SPD:
> 
>   - Only access to the standard registers (0 through 31) is supported.
>   - Access to MMD address spaces other than MMD address space 1 is
>     possible if the spd_clock_gate_override bit is set.
>   - Access to MMD address space 1 is not possible.
> 
> However, ksz9131_resume() calls ksz9131_config_rgmii_delay() before
> kszphy_resume() clears BMCR_PDOWN. This means MMD registers are accessed
> while the PHY is still in SPD, contrary to the datasheet.
> 
> Additionally, on platforms where the PHY loses power during suspend
> (e.g. RZ/G3E), all settings from ksz9131_config_init(), not just the
> RGMII delays, are lost and need to be restored. When the MAC driver
> sets mac_managed_pm (e.g. stmmac), mdio_bus_phy_resume() is skipped,
> so phy_init_hw() (which calls config_init to restore all PHY settings)
> is never invoked during resume.
> 
> Fix this by replacing the RGMII delay restoration with a call to
> phy_init_hw(), which takes the PHY out of SPD and performs full
> reinitialization.
> 
> Fixes: f25a7eaa897f ("net: phy: micrel: Add ksz9131_resume()")
> Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>
> ---
>  drivers/net/phy/micrel.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2aa1dedd21b8..4236dbf4ad6b 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -6016,8 +6016,13 @@ static int lan8841_suspend(struct phy_device *phydev)
>  
>  static int ksz9131_resume(struct phy_device *phydev)
>  {
> -	if (phydev->suspended && phy_interface_is_rgmii(phydev))
> -		ksz9131_config_rgmii_delay(phydev);
> +	int ret;
> +
> +	if (phydev->suspended) {
> +		ret = phy_init_hw(phydev);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return kszphy_resume(phydev);
>  }

mdio_bus_phy_resume():

        ret = phy_init_hw(phydev);
        if (ret < 0)
                return ret;

        ret = phy_resume(phydev);
        if (ret < 0)
                return ret;

where phy_resume() calls your resume function.

If a MAC driver is handling suspend/resume by setting
phydev->mac_managed_pm then maybe the MAC driver should also be
issuing phy_init_hw() before calling phy_resume() ?

Which MAC driver are you seeing a problem with?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: phy: micrel: Fix MMD register access during SPD in ksz9131_resume()

Posted by Nicolai Buchwitz 2 months, 2 weeks ago

On 3.4.2026 13:17, Ovidiu Panait wrote:
> During system suspend, phy_suspend() puts the PHY into Software 
> Power-Down
> (SPD) by setting the BMCR_PDOWN bit in MII_BMCR. According to the 
> KSZ9131
> datasheet, MMD register access is restricted during SPD:
> 
>   - Only access to the standard registers (0 through 31) is supported.
>   - Access to MMD address spaces other than MMD address space 1 is
>     possible if the spd_clock_gate_override bit is set.
>   - Access to MMD address space 1 is not possible.
> 
> However, ksz9131_resume() calls ksz9131_config_rgmii_delay() before
> kszphy_resume() clears BMCR_PDOWN. This means MMD registers are 
> accessed
> while the PHY is still in SPD, contrary to the datasheet.
> 
> Additionally, on platforms where the PHY loses power during suspend
> (e.g. RZ/G3E), all settings from ksz9131_config_init(), not just the
> RGMII delays, are lost and need to be restored. When the MAC driver
> sets mac_managed_pm (e.g. stmmac), mdio_bus_phy_resume() is skipped,
> so phy_init_hw() (which calls config_init to restore all PHY settings)
> is never invoked during resume.
> 
> Fix this by replacing the RGMII delay restoration with a call to
> phy_init_hw(), which takes the PHY out of SPD and performs full
> reinitialization.
> 
> Fixes: f25a7eaa897f ("net: phy: micrel: Add ksz9131_resume()")
> Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>
> ---
>  drivers/net/phy/micrel.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2aa1dedd21b8..4236dbf4ad6b 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -6016,8 +6016,13 @@ static int lan8841_suspend(struct phy_device 
> *phydev)
> 
>  static int ksz9131_resume(struct phy_device *phydev)
>  {
> -	if (phydev->suspended && phy_interface_is_rgmii(phydev))
> -		ksz9131_config_rgmii_delay(phydev);
> +	int ret;
> +
> +	if (phydev->suspended) {
> +		ret = phy_init_hw(phydev);
> +		if (ret)
> +			return ret;
> +	}

Fix looks correct.

nit: phy_init_hw() already clears PDOWN, so the PDOWN clear and
1ms sleep in kszphy_resume() become redundant. But this is probably
something for a future cleanup.

> 
>  	return kszphy_resume(phydev);
>  }

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>