[PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[Oleksij Rempel]

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

Re: [PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[Oleksij Rempel]

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
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[Lukas Wunner]

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

Re: [PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[Andrew Lunn]

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

Re: [PATCH net-next v1 1/1] net: phy: ax88772a: fix lost pause advertisement configuration

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |