| Message ID | 20230301030126.18494-1-leoyang.li@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND,v2,1/2] net: phy: at803x: fix the wol setting functions | expand |
On Tue, 28 Feb 2023 21:01:25 -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 that part is removed. > > Fixes: 7beecaf7d507b ("net: phy: at803x: improve the WOL feature") Given the fixes tag Luo Jie <luoj@codeaurora.org> should be CCed. > 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, 16 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 22f4458274aa..2102279b3964 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, This line is now too long, unless there is a good reason please stick to the 80 char maximum. > + 0, AT803X_WOL_EN); while at it please fix the alignment, the continuation line should start under phydev (checkpatch will tell you) > + 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); same comments as above > + 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); Does phy_read() never fail? Why remove the error checking? > + if (value & AT803X_INTR_ENABLE_WOL) > wol->wolopts |= WAKE_MAGIC; > } >
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, March 3, 2023 7:29 PM > To: Leo Li <leoyang.li@nxp.com> > Cc: Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S . > Miller <davem@davemloft.net>; 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 RESEND v2 1/2] net: phy: at803x: fix the wol setting > functions > > On Tue, 28 Feb 2023 21:01:25 -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 that part is removed. > > > > Fixes: 7beecaf7d507b ("net: phy: at803x: improve the WOL feature") > > Given the fixes tag Luo Jie <luoj@codeaurora.org> should be CCed. Sorry for the late response, I missed this email. I tried to cc him, but the email bounced. > > > 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, 16 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index > > 22f4458274aa..2102279b3964 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, > > This line is now too long, unless there is a good reason please stick to the 80 > char maximum. > > > + 0, AT803X_WOL_EN); > > while at it please fix the alignment, the continuation line should start under > phydev (checkpatch will tell you) > > > + 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); > > same comments as above > > > + 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); > > Does phy_read() never fail? Why remove the error checking? > > > + if (value & AT803X_INTR_ENABLE_WOL) > > wol->wolopts |= WAKE_MAGIC; > > } > >
On Wed, 22 Mar 2023 19:55:56 +0000 Leo Li wrote: > > Given the fixes tag Luo Jie <luoj@codeaurora.org> should be CCed. > > Sorry for the late response, I missed this email. I tried to cc him, > but the email bounced. I see, let me add them to the ignored addresses list.
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 22f4458274aa..2102279b3964 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;
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 that part is removed. 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 | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)