| Message ID | 20220624075558.3141464-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/1] net: phy: ax88772a: fix lost pause advertisement configuration | expand |
I forgot to add PHY maintainers. CCing Andrew and Heiner. On Fri, Jun 24, 2022 at 09:55:58AM +0200, Oleksij Rempel wrote: > In case of asix_ax88772a_link_change_notify() workaround, we run soft > reset which will automatically clear MII_ADVERTISE configuration. The > PHYlib framework do not know about changed configuration state of the > PHY, so we need to save and restore all needed configuration registers. > > Fixes: dde258469257 ("net: usb/phy: asix: add support for ax88772A/C PHYs") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/ax88796b.c | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c > index 457896337505..6971d0196917 100644 > --- a/drivers/net/phy/ax88796b.c > +++ b/drivers/net/phy/ax88796b.c > @@ -18,6 +18,11 @@ MODULE_DESCRIPTION("Asix PHY driver"); > MODULE_AUTHOR("Michael Schmitz <schmitzmic@gmail.com>"); > MODULE_LICENSE("GPL"); > > +struct asix_context { > + u16 bmcr; > + u16 advertise; > +}; > + > /** > * asix_soft_reset - software reset the PHY via BMCR_RESET bit > * @phydev: target phy_device struct > @@ -83,13 +88,43 @@ static int asix_ax88772a_read_status(struct phy_device *phydev) > return 0; > } > > +/* save relevant PHY registers to private copy */ > +static void asix_context_save(struct phy_device *phydev, > + struct asix_context *context) > +{ > + context->bmcr = phy_read(phydev, MII_BMCR); > + context->advertise = phy_read(phydev, MII_ADVERTISE); > +} > + > +/* restore relevant PHY registers from private copy */ > +static void asix_context_restore(struct phy_device *phydev, > + const struct asix_context *context) > +{ > + u16 bmcr = context->bmcr; > + > + phy_write(phydev, MII_ADVERTISE, context->advertise); > + > + /* after all settings are restored, restart autoneg */ > + if (phydev->autoneg == AUTONEG_ENABLE) > + bmcr |= BMCR_ANRESTART; > + > + phy_write(phydev, MII_BMCR, bmcr); > +} > + > static void asix_ax88772a_link_change_notify(struct phy_device *phydev) > { > /* Reset PHY, otherwise MII_LPA will provide outdated information. > * This issue is reproducible only with some link partner PHYs > */ > - if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) > + if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) { > + struct asix_context context; > + > + asix_context_save(phydev, &context); > + > phydev->drv->soft_reset(phydev); > + > + asix_context_restore(phydev, &context); > + } > } > > static struct phy_driver asix_driver[] = { > -- > 2.30.2 > > >
On Fri, Jun 24, 2022 at 09:55:58AM +0200, Oleksij Rempel wrote: > In case of asix_ax88772a_link_change_notify() workaround, we run soft > reset which will automatically clear MII_ADVERTISE configuration. The > PHYlib framework do not know about changed configuration state of the > PHY, so we need to save and restore all needed configuration registers. [...] > static void asix_ax88772a_link_change_notify(struct phy_device *phydev) > { > /* Reset PHY, otherwise MII_LPA will provide outdated information. > * This issue is reproducible only with some link partner PHYs > */ > - if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) > + if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) { > + struct asix_context context; > + > + asix_context_save(phydev, &context); > + > phydev->drv->soft_reset(phydev); > + > + asix_context_restore(phydev, &context); > + } > } Hm, how about just calling phy_init_hw()? That will perform a ->soft_reset() and also restore the configuration, including interrupts (which the above does not, but I guess that's irrelevant as long as the driver uses polling). Does phy_init_hw() do too much or too little compared to the above and is hence not a viable solution? Thanks, Lukas
On Sat, Jun 25, 2022 at 09:17:31AM +0200, Lukas Wunner wrote: > On Fri, Jun 24, 2022 at 09:55:58AM +0200, Oleksij Rempel wrote: > > In case of asix_ax88772a_link_change_notify() workaround, we run soft > > reset which will automatically clear MII_ADVERTISE configuration. The > > PHYlib framework do not know about changed configuration state of the > > PHY, so we need to save and restore all needed configuration registers. > [...] > > static void asix_ax88772a_link_change_notify(struct phy_device *phydev) > > { > > /* Reset PHY, otherwise MII_LPA will provide outdated information. > > * This issue is reproducible only with some link partner PHYs > > */ > > - if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) > > + if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) { > > + struct asix_context context; > > + > > + asix_context_save(phydev, &context); > > + > > phydev->drv->soft_reset(phydev); > > + > > + asix_context_restore(phydev, &context); > > + } > > } > > Hm, how about just calling phy_init_hw()? That will perform a > ->soft_reset() and also restore the configuration, including > interrupts (which the above does not, but I guess that's > irrelevant as long as the driver uses polling). > > Does phy_init_hw() do too much or too little compared to the above > and is hence not a viable solution? at803x.c has: /* After changing the smart speed settings, we need to perform a * software reset, use phy_init_hw() to make sure we set the * reapply any values which might got lost during software reset. */ if (ret == 1) ret = phy_init_hw(phydev); Andrew
On Sun, Jun 26, 2022 at 10:58:24AM +0200, Andrew Lunn wrote: > On Sat, Jun 25, 2022 at 09:17:31AM +0200, Lukas Wunner wrote: > > On Fri, Jun 24, 2022 at 09:55:58AM +0200, Oleksij Rempel wrote: > > > In case of asix_ax88772a_link_change_notify() workaround, we run soft > > > reset which will automatically clear MII_ADVERTISE configuration. The > > > PHYlib framework do not know about changed configuration state of the > > > PHY, so we need to save and restore all needed configuration registers. > > [...] > > > static void asix_ax88772a_link_change_notify(struct phy_device *phydev) > > > { > > > /* Reset PHY, otherwise MII_LPA will provide outdated information. > > > * This issue is reproducible only with some link partner PHYs > > > */ > > > - if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) > > > + if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) { > > > + struct asix_context context; > > > + > > > + asix_context_save(phydev, &context); > > > + > > > phydev->drv->soft_reset(phydev); > > > + > > > + asix_context_restore(phydev, &context); > > > + } > > > } > > > > Hm, how about just calling phy_init_hw()? That will perform a > > ->soft_reset() and also restore the configuration, including > > interrupts (which the above does not, but I guess that's > > irrelevant as long as the driver uses polling). > > > > Does phy_init_hw() do too much or too little compared to the above > > and is hence not a viable solution? > > > at803x.c has: > > /* After changing the smart speed settings, we need to perform a > * software reset, use phy_init_hw() to make sure we set the > * reapply any values which might got lost during software reset. > */ > if (ret == 1) > ret = phy_init_hw(phydev); > Hm, this is not enough to restore/reconfigure advertisement register. Should I change PHY state to UP and trigger the state machine? Regards, Oleksij
diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c index 457896337505..6971d0196917 100644 --- a/drivers/net/phy/ax88796b.c +++ b/drivers/net/phy/ax88796b.c @@ -18,6 +18,11 @@ MODULE_DESCRIPTION("Asix PHY driver"); MODULE_AUTHOR("Michael Schmitz <schmitzmic@gmail.com>"); MODULE_LICENSE("GPL"); +struct asix_context { + u16 bmcr; + u16 advertise; +}; + /** * asix_soft_reset - software reset the PHY via BMCR_RESET bit * @phydev: target phy_device struct @@ -83,13 +88,43 @@ static int asix_ax88772a_read_status(struct phy_device *phydev) return 0; } +/* save relevant PHY registers to private copy */ +static void asix_context_save(struct phy_device *phydev, + struct asix_context *context) +{ + context->bmcr = phy_read(phydev, MII_BMCR); + context->advertise = phy_read(phydev, MII_ADVERTISE); +} + +/* restore relevant PHY registers from private copy */ +static void asix_context_restore(struct phy_device *phydev, + const struct asix_context *context) +{ + u16 bmcr = context->bmcr; + + phy_write(phydev, MII_ADVERTISE, context->advertise); + + /* after all settings are restored, restart autoneg */ + if (phydev->autoneg == AUTONEG_ENABLE) + bmcr |= BMCR_ANRESTART; + + phy_write(phydev, MII_BMCR, bmcr); +} + static void asix_ax88772a_link_change_notify(struct phy_device *phydev) { /* Reset PHY, otherwise MII_LPA will provide outdated information. * This issue is reproducible only with some link partner PHYs */ - if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) + if (phydev->state == PHY_NOLINK && phydev->drv->soft_reset) { + struct asix_context context; + + asix_context_save(phydev, &context); + phydev->drv->soft_reset(phydev); + + asix_context_restore(phydev, &context); + } } static struct phy_driver asix_driver[] = {
In case of asix_ax88772a_link_change_notify() workaround, we run soft reset which will automatically clear MII_ADVERTISE configuration. The PHYlib framework do not know about changed configuration state of the PHY, so we need to save and restore all needed configuration registers. Fixes: dde258469257 ("net: usb/phy: asix: add support for ax88772A/C PHYs") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/ax88796b.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)