Messages in this thread | | | From | Leo Li <> | Subject | RE: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions | Date | Wed, 2 Aug 2023 18:43:44 +0000 |
| |
> -----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
| |