[v3,1/2] net: phy: at803x: fix the wol setting functions

  In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"), it
seems not correct to use a wol_en bit in a 1588 Control Register which is
only available on AR8031/AR8033(share the same phy_id) to determine if WoL
is enabled.  Change it back to use AT803X_INTR_ENABLE_WOL for determining
the WoL status which is applicable on all chips supporting wol. Also update
the at803x_set_wol() function to only update the 1588 register on chips
having it.  After this change, disabling wol at probe from commit
d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer
needed.  So that part is removed.

Fixes: 7beecaf7d507 ("net: phy: at803x: improve the WOL feature")
Signed-off-by: Li Yang <leoyang.li@nxp.com>
Reviewed-by: Viorel Suman <viorel.suman@nxp.com>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/phy/at803x.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)
  

On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"), it
> seems not correct to use a wol_en bit in a 1588 Control Register which is
> only available on AR8031/AR8033(share the same phy_id) to determine if WoL
> is enabled.  Change it back to use AT803X_INTR_ENABLE_WOL for determining
> the WoL status which is applicable on all chips supporting wol. Also update
> the at803x_set_wol() function to only update the 1588 register on chips
> having it.

Do chips which do not have the 1588 register not have WoL? Or WoL
hardware is always enabled, but you still need to enable the
interrupt.

Have you tested on a range of PHY? It might be better to split this
patch up a bit. If it causes regressions, having smaller patches can
make it easier to find which change broken it.

     Andrew
  

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, July 29, 2023 3:14 AM
> To: Leo Li <leoyang.li@nxp.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; David Bauer <mail@david-bauer.net>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman
> <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com>
> Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
> 
> On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> > In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
> > it seems not correct to use a wol_en bit in a 1588 Control Register
> > which is only available on AR8031/AR8033(share the same phy_id) to
> > determine if WoL is enabled.  Change it back to use
> > AT803X_INTR_ENABLE_WOL for determining the WoL status which is
> > applicable on all chips supporting wol. Also update the
> > at803x_set_wol() function to only update the 1588 register on chips having
> it.
> 
> Do chips which do not have the 1588 register not have WoL? Or WoL
> hardware is always enabled, but you still need to enable the interrupt.

Some of them do and some don't, which is removed in the other patch from the series.  Since I don't find the register to enable it, I guess it always enabled.

> 
> Have you tested on a range of PHY? It might be better to split this patch up a
> bit. If it causes regressions, having smaller patches can make it easier to find
> which change broken it.

No, I only have AR8035 to test with.  Changes for other chips are according to the datasheet.  It would be good if others having the hardware can test it too.

The problem right now on our board with AR8035 is that it gets the wrong WoL setting and fails to enter sleep:
[  354.196156] Qualcomm Atheros AR8035 0x0000000008b96000:02: PM: dpm_run_callback(): mdio_bus_phy_suspend+0x0/0x110 returns -16
[  354.196172] Qualcomm Atheros AR8035 0x0000000008b96000:02: PM: failed to suspend: error -16

Regards,
Leo
  

On Mon, 2023-07-31 at 14:58 +0000, Leo Li wrote:
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Saturday, July 29, 2023 3:14 AM
> > To: Leo Li <leoyang.li@nxp.com>
> > Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> > <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Jakub
> > Kicinski <kuba@kernel.org>; David Bauer <mail@david-bauer.net>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman
> > <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com>
> > Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
> > 
> > On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> > > In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
> > > it seems not correct to use a wol_en bit in a 1588 Control Register
> > > which is only available on AR8031/AR8033(share the same phy_id) to
> > > determine if WoL is enabled.  Change it back to use
> > > AT803X_INTR_ENABLE_WOL for determining the WoL status which is
> > > applicable on all chips supporting wol. Also update the
> > > at803x_set_wol() function to only update the 1588 register on chips having
> > it.
> > 
> > Do chips which do not have the 1588 register not have WoL? Or WoL
> > hardware is always enabled, but you still need to enable the interrupt.
> 
> Some of them do and some don't, which is removed in the other patch
> from the series.  Since I don't find the register to enable it, I
> guess it always enabled.
> 
> > 
> > Have you tested on a range of PHY? It might be better to split this patch up a
> > bit. If it causes regressions, having smaller patches can make it easier to find
> > which change broken it.
> 
> No, I only have AR8035 to test with.  Changes for other chips are
> according to the datasheet.  It would be good if others having the
> hardware can test it too.

Adding Luo Jie for awareness.

@Luo Jie: do you have access to other chips handled by this driver
other then AR8035? could you please test this series:

https://patchwork.kernel.org/project/netdevbpf/list/?series=770734

?

Thanks!

Paolo
  

On 8/1/2023 5:16 PM, Paolo Abeni wrote:
> On Mon, 2023-07-31 at 14:58 +0000, Leo Li wrote:
>>> -----Original Message-----
>>> From: Andrew Lunn <andrew@lunn.ch>
>>> Sent: Saturday, July 29, 2023 3:14 AM
>>> To: Leo Li <leoyang.li@nxp.com>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
>>> <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Jakub
>>> Kicinski <kuba@kernel.org>; David Bauer <mail@david-bauer.net>;
>>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman
>>> <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com>
>>> Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
>>>
>>> On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
>>>> In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
>>>> it seems not correct to use a wol_en bit in a 1588 Control Register
>>>> which is only available on AR8031/AR8033(share the same phy_id) to
>>>> determine if WoL is enabled.  Change it back to use
>>>> AT803X_INTR_ENABLE_WOL for determining the WoL status which is
>>>> applicable on all chips supporting wol. Also update the
>>>> at803x_set_wol() function to only update the 1588 register on chips having
>>> it.
>>>
>>> Do chips which do not have the 1588 register not have WoL? Or WoL
>>> hardware is always enabled, but you still need to enable the interrupt.
>>
>> Some of them do and some don't, which is removed in the other patch
>> from the series.  Since I don't find the register to enable it, I
>> guess it always enabled.
>>
>>>
>>> Have you tested on a range of PHY? It might be better to split this patch up a
>>> bit. If it causes regressions, having smaller patches can make it easier to find
>>> which change broken it.
>>
>> No, I only have AR8035 to test with.  Changes for other chips are
>> according to the datasheet.  It would be good if others having the
>> hardware can test it too.
> 
> Adding Luo Jie for awareness.
> 
> @Luo Jie: do you have access to other chips handled by this driver
> other then AR8035? could you please test this series:
> 
> https://patchwork.kernel.org/project/netdevbpf/list/?series=770734
> 
> ?
> 
> Thanks!
> 
> Paolo
> 

Hi Paolo & Leo,
To make WoL feature working, we need to enable bit 
MDIO_MMD_PCS.AT803X_PHY_MMD3_WOL_CTRL.AT803X_WOL_EN on both PHY
qca8081 and at803x, which does not depend on the 1588 feature.

The bit AT803X_INTR_ENABLE.AT803X_INTR_ENABLE_WOL is just for triggering 
the external WoL interrupt PIN when the WOL interrupt occurs.

Thanks,
Jie
  

On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"), it
> seems not correct to use a wol_en bit in a 1588 Control Register which is
> only available on AR8031/AR8033(share the same phy_id) to determine if WoL
> is enabled.  Change it back to use AT803X_INTR_ENABLE_WOL for determining
> the WoL status which is applicable on all chips supporting wol. Also update
> the at803x_set_wol() function to only update the 1588 register on chips
> having it.  After this change, disabling wol at probe from commit
> d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer
> needed.  So that part is removed.

Okay, having been through the AR8031, AR8033, and AR8035 datasheets that
I have, this is what I've gathered:

AR8031 and AR8033 are identical as far as WoL is concerned:
	In terms of hardware, these have a WOL_INT pin that is separate
	from the normal interrupt.

	MMD3 0x8012 (1588 register) bit 5 controls whether the WoL
	function is enabled or disabled. Defaults to enabled.

	BMCR in copper/fiber can be used to save more power.

	AR8035 details below also apply.

AR8035:
	No WOL_INT pin.

	No MMD3 0x8012 register.

	WoL interrupt enable in C22 register 0x12 bit 0
	WoL interrupt status in C22 register 0x13 bit 0
	WoL MAC address programmed in MMD3 registers 0x804a (bits 47:32)
	0x804b (bits 31:16) and 0x804c (bits 15:0)

So, what this means is that AR8035, the only possibility for WoL is via
the INT pin and the C22 interrupt enable/status registers.

For AR8031 and AR8033, it depends how the hardware is wired.

If WOL_INT is used to wake the system, then MMD3 0x8012 has to be used to
enable or disable that functionality. From my reading of the datasheets,
WOL_INT is unaffected by the C22 interrupt enable register settings.

If INT is used to wake the system, then it behaves the same as AR8035.
However, the datasheet doesn't make it clear whether MMD3 0x8012 bit 5
also has an effect - although I would lean more towards it having an
effect.

So, given that:

> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index c1f307d90518..1d61f7190367 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -459,21 +459,27 @@ 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 */
> -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> -				0, AT803X_WOL_EN);
> -		if (ret)
> -			return ret;
> +		/* 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 */
> -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> -				AT803X_WOL_EN, 0);
> -		if (ret)
> -			return ret;
> +		/* 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)
> @@ -508,11 +514,11 @@ static void at803x_get_wol(struct phy_device *phydev,
>  	wol->supported = WAKE_MAGIC;
>  	wol->wolopts = 0;
>  
> -	value = phy_read_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL);
> +	value = phy_read(phydev, AT803X_INTR_ENABLE);
>  	if (value < 0)
>  		return;
>  
> -	if (value & AT803X_WOL_EN)
> +	if (value & AT803X_INTR_ENABLE_WOL)
>  		wol->wolopts |= WAKE_MAGIC;
>  }
>  

The above all looks correct to me.

> @@ -858,9 +864,6 @@ static int at803x_probe(struct phy_device *phydev)
>  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
>  		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
>  		int mode_cfg;
> -		struct ethtool_wolinfo wol = {
> -			.wolopts = 0,
> -		};
>  
>  		if (ccr < 0)
>  			return ccr;
> @@ -876,13 +879,6 @@ static int at803x_probe(struct phy_device *phydev)
>  			priv->is_fiber = true;
>  			break;
>  		}
> -
> -		/* Disable WOL by default */
> -		ret = at803x_set_wol(phydev, &wol);
> -		if (ret < 0) {
> -			phydev_err(phydev, "failed to disable WOL on probe: %d\n", ret);
> -			return ret;
> -		}
>  	}
>  
>  	return 0;

This doesn't look correct to me, because in the case of AR8031 or
AR8033 using WOL_INT, because MMD3 0x8012 bit 5 defaults on reset to
being set, if we don't want WoL enabled after the PHY has been probed,
we need to clear it.
  

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, August 2, 2023 5:22 AM
> To: Leo Li <leoyang.li@nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; David Bauer <mail@david-bauer.net>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman
> <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com>
> Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
> 
> On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> > In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
> > it seems not correct to use a wol_en bit in a 1588 Control Register
> > which is only available on AR8031/AR8033(share the same phy_id) to
> > determine if WoL is enabled.  Change it back to use
> > AT803X_INTR_ENABLE_WOL for determining the WoL status which is
> > applicable on all chips supporting wol. Also update the
> > at803x_set_wol() function to only update the 1588 register on chips
> > having it.  After this change, disabling wol at probe from commit
> > d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer
> needed.  So that part is removed.
> 
> Okay, having been through the AR8031, AR8033, and AR8035 datasheets that
> I have, this is what I've gathered:
> 
> AR8031 and AR8033 are identical as far as WoL is concerned:
> 	In terms of hardware, these have a WOL_INT pin that is separate
> 	from the normal interrupt.
> 
> 	MMD3 0x8012 (1588 register) bit 5 controls whether the WoL
> 	function is enabled or disabled. Defaults to enabled.
> 
> 	BMCR in copper/fiber can be used to save more power.
> 
> 	AR8035 details below also apply.
> 
> AR8035:
> 	No WOL_INT pin.
> 
> 	No MMD3 0x8012 register.
> 
> 	WoL interrupt enable in C22 register 0x12 bit 0
> 	WoL interrupt status in C22 register 0x13 bit 0
> 	WoL MAC address programmed in MMD3 registers 0x804a (bits 47:32)
> 	0x804b (bits 31:16) and 0x804c (bits 15:0)
> 
> So, what this means is that AR8035, the only possibility for WoL is via the INT
> pin and the C22 interrupt enable/status registers.
> 
> For AR8031 and AR8033, it depends how the hardware is wired.
> 
> If WOL_INT is used to wake the system, then MMD3 0x8012 has to be used
> to enable or disable that functionality. From my reading of the datasheets,
> WOL_INT is unaffected by the C22 interrupt enable register settings.
> 
> If INT is used to wake the system, then it behaves the same as AR8035.
> However, the datasheet doesn't make it clear whether MMD3 0x8012 bit 5
> also has an effect - although I would lean more towards it having an effect.
> 
> So, given that:
> 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index
> > c1f307d90518..1d61f7190367 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -459,21 +459,27 @@ 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 */
> > -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > -				0, AT803X_WOL_EN);
> > -		if (ret)
> > -			return ret;
> > +		/* 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 */
> > -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > -				AT803X_WOL_EN, 0);
> > -		if (ret)
> > -			return ret;
> > +		/* 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)
> > @@ -508,11 +514,11 @@ static void at803x_get_wol(struct phy_device
> *phydev,
> >  	wol->supported = WAKE_MAGIC;
> >  	wol->wolopts = 0;
> >
> > -	value = phy_read_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL);
> > +	value = phy_read(phydev, AT803X_INTR_ENABLE);
> >  	if (value < 0)
> >  		return;
> >
> > -	if (value & AT803X_WOL_EN)
> > +	if (value & AT803X_INTR_ENABLE_WOL)
> >  		wol->wolopts |= WAKE_MAGIC;
> >  }
> >
> 
> The above all looks correct to me.
> 
> > @@ -858,9 +864,6 @@ static int at803x_probe(struct phy_device *phydev)
> >  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> >  		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> >  		int mode_cfg;
> > -		struct ethtool_wolinfo wol = {
> > -			.wolopts = 0,
> > -		};
> >
> >  		if (ccr < 0)
> >  			return ccr;
> > @@ -876,13 +879,6 @@ static int at803x_probe(struct phy_device *phydev)
> >  			priv->is_fiber = true;
> >  			break;
> >  		}
> > -
> > -		/* Disable WOL by default */
> > -		ret = at803x_set_wol(phydev, &wol);
> > -		if (ret < 0) {
> > -			phydev_err(phydev, "failed to disable WOL on
> probe: %d\n", ret);
> > -			return ret;
> > -		}
> >  	}
> >
> >  	return 0;
> 
> This doesn't look correct to me, because in the case of AR8031 or
> AR8033 using WOL_INT, because MMD3 0x8012 bit 5 defaults on reset to
> being set, if we don't want WoL enabled after the PHY has been probed, we
> need to clear it.

The intent was to remove the code added by d7cd5e06c9d ("net: phy: at803x: disable WOL at probe") which claims to be needed after 7beecaf7d507b ("net: phy: at803x: improve the WOL feature").  Since I have changed the behavior back to before 7beecaf7d507b, it is natural that this part of d7cd5e06c9d is no longer needed.

But if we want MMD3 0x8012 bit 5 to be aligned with the WOL_INT on probe, we probably just need to clear the bit for AR8031/8033 instead of calling the set_wol for all chips.

Regards,
Leo