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

From: Leo Li
Date: Wed Aug 02 2023 - 14:43:58 EST



> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: Wednesday, August 2, 2023 5:22 AM
> To: Leo Li <leoyang.li@xxxxxxx>
> Cc: Andrew Lunn <andrew@xxxxxxx>; Heiner Kallweit
> <hkallweit1@xxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; David Bauer <mail@xxxxxxxxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Viorel Suman
> <viorel.suman@xxxxxxx>; Wei Fang <wei.fang@xxxxxxx>
> 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