| Message ID | 20200423163947.18313-2-dmurphy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | WoL fixes for DP83822 and DP83tc811 | expand |
On 23.04.2020 18:39, Dan Murphy wrote: > The WoL feature should be disabled when config_init is called and the > feature should turned on or off when set_wol is called. > > In addition updated the calls to modify the registers to use the set_bit > and clear_bit function calls. > > Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission") > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/net/phy/dp83822.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c > index fe9aa3ad52a7..40fdfd043947 100644 > --- a/drivers/net/phy/dp83822.c > +++ b/drivers/net/phy/dp83822.c > @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev, > value &= ~DP83822_WOL_SECURE_ON; > } > > - value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | > - DP83822_WOL_CLR_INDICATION); > - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, > - value); > + /* Clear any pending WoL interrupt */ > + phy_read(phydev, MII_DP83822_MISR2); > + > + value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | > + DP83822_WOL_CLR_INDICATION; > + > + return phy_set_bits_mmd(phydev, DP83822_DEVADDR, > + MII_DP83822_WOL_CFG, value); The switch to phy_set_bits_mmd() doesn't seem to be correct here. So far bit DP83822_WOL_MAGIC_EN is cleared if WAKE_MAGIC isn't set. Similar for bit DP83822_WOL_SECURE_ON. With your change they don't get cleared any longer. > } else { > - value = phy_read_mmd(phydev, DP83822_DEVADDR, > - MII_DP83822_WOL_CFG); > - value &= ~DP83822_WOL_EN; > - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, > - value); > + value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION; > + You clear one bit more than before. The reason for this may be worth a note in the commit message. > + return phy_clear_bits_mmd(phydev, DP83822_DEVADDR, > + MII_DP83822_WOL_CFG, value); > } > > return 0; > @@ -258,12 +261,11 @@ static int dp83822_config_intr(struct phy_device *phydev) > > static int dp83822_config_init(struct phy_device *phydev) > { > - int value; > - > - value = DP83822_WOL_MAGIC_EN | DP83822_WOL_SECURE_ON | DP83822_WOL_EN; > + int value = DP83822_WOL_EN | DP83822_WOL_MAGIC_EN | > + DP83822_WOL_SECURE_ON; > > - return phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, > - value); > + return phy_clear_bits_mmd(phydev, DP83822_DEVADDR, > + MII_DP83822_WOL_CFG, value); > } > > static int dp83822_phy_reset(struct phy_device *phydev) >
Heiner On 4/23/20 12:02 PM, Heiner Kallweit wrote: > On 23.04.2020 18:39, Dan Murphy wrote: >> The WoL feature should be disabled when config_init is called and the >> feature should turned on or off when set_wol is called. >> >> In addition updated the calls to modify the registers to use the set_bit >> and clear_bit function calls. >> >> Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission") >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/net/phy/dp83822.c | 30 ++++++++++++++++-------------- >> 1 file changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c >> index fe9aa3ad52a7..40fdfd043947 100644 >> --- a/drivers/net/phy/dp83822.c >> +++ b/drivers/net/phy/dp83822.c >> @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev, >> value &= ~DP83822_WOL_SECURE_ON; >> } >> >> - value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | >> - DP83822_WOL_CLR_INDICATION); >> - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, >> - value); >> + /* Clear any pending WoL interrupt */ >> + phy_read(phydev, MII_DP83822_MISR2); >> + >> + value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | >> + DP83822_WOL_CLR_INDICATION; >> + >> + return phy_set_bits_mmd(phydev, DP83822_DEVADDR, >> + MII_DP83822_WOL_CFG, value); > The switch to phy_set_bits_mmd() doesn't seem to be correct here. > So far bit DP83822_WOL_MAGIC_EN is cleared if WAKE_MAGIC isn't set. > Similar for bit DP83822_WOL_SECURE_ON. With your change they don't > get cleared any longer. Thanks for the review. I was not watching those bits during my testing I will revert back to the original code. >> } else { >> - value = phy_read_mmd(phydev, DP83822_DEVADDR, >> - MII_DP83822_WOL_CFG); >> - value &= ~DP83822_WOL_EN; >> - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, >> - value); >> + value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION; >> + > You clear one bit more than before. The reason for this may be worth a note > in the commit message. Thanks this was an artifact from debugging. I can remove the CLR as it was cleared in set_wol enablement. Dan
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c index fe9aa3ad52a7..40fdfd043947 100644 --- a/drivers/net/phy/dp83822.c +++ b/drivers/net/phy/dp83822.c @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev, value &= ~DP83822_WOL_SECURE_ON; } - value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | - DP83822_WOL_CLR_INDICATION); - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, - value); + /* Clear any pending WoL interrupt */ + phy_read(phydev, MII_DP83822_MISR2); + + value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | + DP83822_WOL_CLR_INDICATION; + + return phy_set_bits_mmd(phydev, DP83822_DEVADDR, + MII_DP83822_WOL_CFG, value); } else { - value = phy_read_mmd(phydev, DP83822_DEVADDR, - MII_DP83822_WOL_CFG); - value &= ~DP83822_WOL_EN; - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, - value); + value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION; + + return phy_clear_bits_mmd(phydev, DP83822_DEVADDR, + MII_DP83822_WOL_CFG, value); } return 0; @@ -258,12 +261,11 @@ static int dp83822_config_intr(struct phy_device *phydev) static int dp83822_config_init(struct phy_device *phydev) { - int value; - - value = DP83822_WOL_MAGIC_EN | DP83822_WOL_SECURE_ON | DP83822_WOL_EN; + int value = DP83822_WOL_EN | DP83822_WOL_MAGIC_EN | + DP83822_WOL_SECURE_ON; - return phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, - value); + return phy_clear_bits_mmd(phydev, DP83822_DEVADDR, + MII_DP83822_WOL_CFG, value); } static int dp83822_phy_reset(struct phy_device *phydev)
The WoL feature should be disabled when config_init is called and the feature should turned on or off when set_wol is called. In addition updated the calls to modify the registers to use the set_bit and clear_bit function calls. Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission") Signed-off-by: Dan Murphy <dmurphy@ti.com> --- drivers/net/phy/dp83822.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)