Re: [net-next PATCH v2 08/12] net: phy: at803x: move specific at8031 WOL bits to dedicated function

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

On Tue, Dec 05, 2023 at 10:32:55AM +0100, Paolo Abeni wrote:
> On Fri, 2023-12-01 at 01:14 +0100, Christian Marangi wrote:
> > Move specific at8031 WOL enable/disable to dedicated function to make
> > at803x_set_wol more generic.
> > 
> > This is needed in preparation for PHY driver split as qca8081 share the
> > same function to toggle WOL settings.
> > 
> > In this new implementation WOL module in at8031 is enabled after the
> > generic interrupt is setup. This should not cause any problem as the
> > WOL_INT has a separate implementation and only relay on MAC bits.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/phy/at803x.c | 42 ++++++++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index 02ac71f98466..2de7a59c0faa 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -466,27 +466,11 @@ static int at803x_set_wol(struct phy_device *phydev,
> >  			phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i],
> >  				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
> >  
> > -		/* Enable WOL function for 1588 */
> > -		if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > -			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > -					     AT803X_PHY_MMD3_WOL_CTRL,
> > -					     0, AT803X_WOL_EN);
> > -			if (ret)
> > -				return ret;
> > -		}
> >  		/* Enable WOL interrupt */
> >  		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
> >  		if (ret)
> >  			return ret;
> >  	} else {
> > -		/* Disable WoL function for 1588 */
> > -		if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > -			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > -					     AT803X_PHY_MMD3_WOL_CTRL,
> > -					     AT803X_WOL_EN, 0);
> > -			if (ret)
> > -				return ret;
> > -		}
> >  		/* Disable WOL interrupt */
> >  		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
> >  		if (ret)
> > @@ -1611,6 +1595,30 @@ static int at8031_config_init(struct phy_device *phydev)
> >  	return at803x_config_init(phydev);
> >  }
> >  
> > +static int at8031_set_wol(struct phy_device *phydev,
> > +			  struct ethtool_wolinfo *wol)
> > +{
> > +	int ret;
> > +
> > +	/* First setup MAC address and enable WOL interrupt */
> > +	ret = at803x_set_wol(phydev, wol);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (wol->wolopts & WAKE_MAGIC)
> > +		/* Enable WOL function for 1588 */
> > +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +				     AT803X_PHY_MMD3_WOL_CTRL,
> > +				     0, AT803X_WOL_EN);
> > +	else
> > +		/* Disable WoL function for 1588 */
> > +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +				     AT803X_PHY_MMD3_WOL_CTRL,
> > +				     AT803X_WOL_EN, 0);
> > +
> > +	return ret;
> 
> If I read correctly, the above changes the order of some WoL
> initialization steps: now WOL_CTRL is touched after
> AT803X_INTR_ENABLE_WOL. Is that correct?

It is fine.

AT803X_INTR_ENABLE_WOL enables or disables whether the INT pin (which
is used for any interrupt from the PHY) is used to signal WOL - it's
the interrupt enable for the WoL function.

The MMD3 WOL_EN bit controls whether the WoL function is enabled, and
thus whether the WOL_INT pin will signal WoL. WOL_EN should not be
set until we have initialised the WoL function, and thus that needs
to happen _after_ the MAC address has been programmed.

Clearing WOL_EN afterwards is not a problem because it will already
have been setup, or is in its power-on default state.

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