| Message ID | 20230221224031.7244-1-leoyang.li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: at803x: fix the wol setting functions | expand |
On Tue, Feb 21, 2023 at 04:40:31PM -0600, Li Yang wrote: > In 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 > d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer > needed. So it is removed. Rather than remove it, please git revert it, and explain in the commit message why. > > Also remove the set_wol()/get_wol() callbacks from AR8032 which doesn't > support WoL. This change was part of 5800091a2061 ("net: phy: at803x: add support for AR8032 PHY") Please break this patch up into individual fixes. The different fixes might need different levels of backporting etc. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, February 21, 2023 4:57 PM > 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>; Luo Jie <luoj@codeaurora.org>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman > <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com> > Subject: Re: [PATCH] net: phy: at803x: fix the wol setting functions > > On Tue, Feb 21, 2023 at 04:40:31PM -0600, Li Yang wrote: > > In 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 d7cd5e06c9dd ("net: > > phy: at803x: disable WOL at probe") is no longer needed. So it is > > removed. > > Rather than remove it, please git revert it, and explain in the commit > message why. Not all changes from that patch is removed. The remaining part might still be useful. > > > > > Also remove the set_wol()/get_wol() callbacks from AR8032 which > > doesn't support WoL. > > This change was part of 5800091a2061 ("net: phy: at803x: add support for > AR8032 PHY") > > Please break this patch up into individual fixes. The different fixes might > need different levels of backporting etc. Ok. I can split this out. Regards, Leo
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 22f4458274aa..ac47a1d681b2 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -461,21 +461,25 @@ 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) @@ -510,11 +514,8 @@ 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); - if (value < 0) - return; - - if (value & AT803X_WOL_EN) + value = phy_read(phydev, AT803X_INTR_ENABLE); + if (value & AT803X_INTR_ENABLE_WOL) wol->wolopts |= WAKE_MAGIC; } @@ -866,9 +867,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) { ret = ccr; @@ -887,12 +885,6 @@ static int at803x_probe(struct phy_device *phydev) 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); - goto err; - } } return 0; @@ -2087,8 +2079,6 @@ static struct phy_driver at803x_driver[] = { .flags = PHY_POLL_CABLE_TEST, .config_init = at803x_config_init, .link_change_notify = at803x_link_change_notify, - .set_wol = at803x_set_wol, - .get_wol = at803x_get_wol, .suspend = at803x_suspend, .resume = at803x_resume, /* PHY_BASIC_FEATURES */
In 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 d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer needed. So it is removed. Also remove the set_wol()/get_wol() callbacks from AR8032 which doesn't support WoL. Fixes: 7beecaf7d507b ("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 | 42 +++++++++++++++------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)