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

[Russell King (Oracle)]

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.

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