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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Apr 03, 2026 at 03:48:02PM +0000, Ovidiu Panait wrote:
> Hi,
> 
> > 
> > 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?
> > 
> 
> On my board the KSZ9131RNX PHY is paired to stmmac.
> 
> I could add phy_init_hw() before the phylink_prepare_resume() call, which
> does the phy_resume() and remove the ksz9131_config_rgmii_delay() call from
> ksz9131_resume(), as it is not correct/complete.

Yes, I think we should add phy_init_hw() before calling phy_resume()
in phylink's prepare_resume() path to ensure that the PHY state is
the same as when the PHY is resumed via the MDIO bus. Please prepare
a patch to that end, thanks.

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