Message-ID: <CAAzmgs6MuoLSn=MNmKxio=MVkQq7NDs12Wwu=Sh3SeeU7YiWTA@mail.gmail.com>
Date:   Fri, 11 Aug 2023 10:26:30 +0000
From:   Luke Lu <luke.lu@...re.computer>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Da Xue <da@...re.computer>
Subject: Re: [PATCH net v4] net: phy: meson-gxl: implement meson_gxl_phy_resume()

Hi Heiner:

On Fri, Aug 11, 2023 at 6:07 AM Heiner Kallweit <hkallweit1@...il.com> wrote:
>
> On 09.08.2023 23:49, Luke Lu wrote:
> > From: Da Xue <da@...re.computer>
> >
> > While testing the suspend/resume function, we found the ethernet
> > is broken if using internal PHY of Amlogic meson GXL SoC.
> > After system resume back, the ethernet is down, no carrier found.
> >
> >       eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state
> >               DOWN group default qlen 1000
> >
> > In this patch, we re-initialize the internal PHY to fix this problem.
> >
>
> It's not an unusual case that system cuts power to the PHY during
> system suspend. So the PHY needs to be re-initialized on resume.
> That's why we call phy_init_hw() in mdio_bus_phy_resume().
>
Calling phy_init_hw() sounds a good idea, and should also fix this issue

But in the case of using stmmac in Amlogic GXL based SoC,
the phy_init_hw() will be skipped due to mac_managed_pm is true

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
static int stmmac_phy_setup(struct stmmac_priv *priv)
{
        ...
        priv->phylink_config.mac_managed_pm = true;
}

drivers/net/phy/phy_device.c
static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
{
        struct phy_device *phydev = to_phy_device(dev);
        int ret;

        if (phydev->mac_managed_pm)
                return 0;
        ...
}

> If going your way we would be better off calling .config_init()
> in genphy_resume().
I'm not sure if it's safe to go this way, which will change the generic code,
or question - does all phy devices need to call .config_init() in resume() path?

> Please check the MAC driver, maybe it's better
> to re-initialize the PHY in the resume path of the MAC driver.
>
Do you mean do the re-initialization in stmmac_main.c: stmmac_resume()?
It sounds like a feasible way to solve this, but as I'm not really
familiar with stmmac driver,
so, do you have some more detailed suggestions on how we should adapt
the code to fix this?

> >       eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
> >               group default qlen 1000
> >
> > Fixes: 7334b3e47aee ("net: phy: Add Meson GXL Internal PHY driver")
> > Signed-off-by: Da Xue <da@...re.computer>
> > Signed-off-by: Luke Lu <luke.lu@...re.computer>
> >
> > ---
> > Note, we don't Cc stable kernel tree in this patch intentionally, since
> > there will be a cherry-pick failure if apply this patch from kernel version
> > less than v6.2, it's not a logic failure but due to the changes too close.
> >
> > Please check commit 69ff53e4a4c9 ("net: phy: meson-gxl: use MMD access dummy stubs for GXL, internal PHY")
> > We plan to slightly rework the patch, and send it to stable tree separately
> > once this patch is accepted into mainline.
> >
> > v4:
> >  - refactor commit message to better explain the problem & fix
> >  - check return value of genphy_resume()
> >  - add 'net' annotation
> >  - add Fixes tag
> >
> > v3: https://lore.kernel.org/netdev/20230808050016.1911447-1-da@libre.computer
> >  - fix missing parameter of genphy_resume()
> >
> > v2: https://lore.kernel.org/netdev/20230804201903.1303713-1-da@libre.computer
> >  - call generic genphy_resume()
> >
> > v1: https://lore.kernel.org/all/CACqvRUZRyXTVQyy9bUviQZ+_moLQBjPc6nin_NQC+CJ37yNnLw@mail.gmail.com
> > ---
> >  drivers/net/phy/meson-gxl.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
> > index bb9b33b6bce2..9ebe09b0cd8c 100644
> > --- a/drivers/net/phy/meson-gxl.c
> > +++ b/drivers/net/phy/meson-gxl.c
> > @@ -132,6 +132,21 @@ static int meson_gxl_config_init(struct phy_device *phydev)
> >       return 0;
> >  }
> >
> > +static int meson_gxl_phy_resume(struct phy_device *phydev)
> > +{
> > +     int ret;
> > +
> > +     ret = genphy_resume(phydev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = meson_gxl_config_init(phydev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> >  /* This function is provided to cope with the possible failures of this phy
> >   * during aneg process. When aneg fails, the PHY reports that aneg is done
> >   * but the value found in MII_LPA is wrong:
> > @@ -196,7 +211,7 @@ static struct phy_driver meson_gxl_phy[] = {
> >               .config_intr    = smsc_phy_config_intr,
> >               .handle_interrupt = smsc_phy_handle_interrupt,
> >               .suspend        = genphy_suspend,
> > -             .resume         = genphy_resume,
> > +             .resume         = meson_gxl_phy_resume,
> >               .read_mmd       = genphy_read_mmd_unsupported,
> >               .write_mmd      = genphy_write_mmd_unsupported,
> >       }, {
>

Powered by blists - more mailing lists