| Message ID | 20240605101611.18791-4-Raju.Lakkaraju@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan743x: Fixes for multiple WOL related issues | expand |
On 05.06.2024 12:16, Raju Lakkaraju wrote: > When the system resumes from sleep, the phy_init_hw() function invokes > config_init(), which clears all interrupt masks and causes wake events to be > lost in subsequent wake sequences. Remove interrupt mask clearing from > config_init() and preserve relevant masks in config_intr() > > Fixes: 7d901a1e878a ("net: phy: add Maxlinear GPY115/21x/24x driver") > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > --- One nit, other than that: Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Change List: > ------------ > V0 -> V3: > - Address the https://lore.kernel.org/lkml/4a565d54-f468-4e32-8a2c-102c1203f72c@lunn.ch/T/ > review comments > > drivers/net/phy/mxl-gpy.c | 58 +++++++++++++++++++++++++-------------- > 1 file changed, 38 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c > index b2d36a3a96f1..e5f8ac4b4604 100644 > --- a/drivers/net/phy/mxl-gpy.c > +++ b/drivers/net/phy/mxl-gpy.c > @@ -107,6 +107,7 @@ struct gpy_priv { > > u8 fw_major; > u8 fw_minor; > + u32 wolopts; > > /* It takes 3 seconds to fully switch out of loopback mode before > * it can safely re-enter loopback mode. Record the time when > @@ -221,6 +222,15 @@ static int gpy_hwmon_register(struct phy_device *phydev) > } > #endif > > +static int gpy_ack_interrupt(struct phy_device *phydev) > +{ > + int ret; > + > + /* Clear all pending interrupts */ > + ret = phy_read(phydev, PHY_ISTAT); > + return ret < 0 ? ret : 0; Can we just return phy_read? > +} > + > static int gpy_mbox_read(struct phy_device *phydev, u32 addr) > { > struct gpy_priv *priv = phydev->priv; > @@ -262,16 +272,8 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr) > > static int gpy_config_init(struct phy_device *phydev) > { > - int ret; > - > - /* Mask all interrupts */ > - ret = phy_write(phydev, PHY_IMASK, 0); > - if (ret) > - return ret; > - > - /* Clear all pending interrupts */ > - ret = phy_read(phydev, PHY_ISTAT); > - return ret < 0 ? ret : 0; > + /* Nothing to configure. Configuration Requirement Placeholder */ > + return 0; > } > > static int gpy21x_config_init(struct phy_device *phydev) > @@ -627,11 +629,23 @@ static int gpy_read_status(struct phy_device *phydev) > > static int gpy_config_intr(struct phy_device *phydev) > { > + struct gpy_priv *priv = phydev->priv; > u16 mask = 0; > + int ret; > + > + ret = gpy_ack_interrupt(phydev); > + if (ret) > + return ret; > > if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > mask = PHY_IMASK_MASK; > > + if (priv->wolopts & WAKE_MAGIC) > + mask |= PHY_IMASK_WOL; > + > + if (priv->wolopts & WAKE_PHY) > + mask |= PHY_IMASK_LSTC; > + > return phy_write(phydev, PHY_IMASK, mask); > } > > @@ -678,6 +692,7 @@ static int gpy_set_wol(struct phy_device *phydev, > struct ethtool_wolinfo *wol) > { > struct net_device *attach_dev = phydev->attached_dev; > + struct gpy_priv *priv = phydev->priv; > int ret; > > if (wol->wolopts & WAKE_MAGIC) { > @@ -725,6 +740,8 @@ static int gpy_set_wol(struct phy_device *phydev, > ret = phy_read(phydev, PHY_ISTAT); > if (ret < 0) > return ret; > + > + priv->wolopts |= WAKE_MAGIC; > } else { > /* Disable magic packet matching */ > ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, > @@ -732,6 +749,13 @@ static int gpy_set_wol(struct phy_device *phydev, > WOL_EN); > if (ret < 0) > return ret; > + > + /* Disable the WOL interrupt */ > + ret = phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_WOL); > + if (ret < 0) > + return ret; > + > + priv->wolopts &= ~WAKE_MAGIC; > } > > if (wol->wolopts & WAKE_PHY) { > @@ -748,9 +772,11 @@ static int gpy_set_wol(struct phy_device *phydev, > if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC)) > phy_trigger_machine(phydev); > > + priv->wolopts |= WAKE_PHY; > return 0; > } > > + priv->wolopts &= ~WAKE_PHY; > /* Disable the link state change interrupt */ > return phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC); > } > @@ -758,18 +784,10 @@ static int gpy_set_wol(struct phy_device *phydev, > static void gpy_get_wol(struct phy_device *phydev, > struct ethtool_wolinfo *wol) > { > - int ret; > + struct gpy_priv *priv = phydev->priv; > > wol->supported = WAKE_MAGIC | WAKE_PHY; > - wol->wolopts = 0; > - > - ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, VPSPEC2_WOL_CTL); > - if (ret & WOL_EN) > - wol->wolopts |= WAKE_MAGIC; > - > - ret = phy_read(phydev, PHY_IMASK); > - if (ret & PHY_IMASK_LSTC) > - wol->wolopts |= WAKE_PHY; > + wol->wolopts = priv->wolopts; > } > > static int gpy_loopback(struct phy_device *phydev, bool enable)
Hi Wojciech, The 06/05/2024 13:24, Wojciech Drewek wrote: > [Some people who received this message don't often get email from wojciech.drewek@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 05.06.2024 12:16, Raju Lakkaraju wrote: > > When the system resumes from sleep, the phy_init_hw() function invokes > > config_init(), which clears all interrupt masks and causes wake events to be > > lost in subsequent wake sequences. Remove interrupt mask clearing from > > config_init() and preserve relevant masks in config_intr() > > > > Fixes: 7d901a1e878a ("net: phy: add Maxlinear GPY115/21x/24x driver") > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > > --- > > One nit, other than that: > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > > Change List: > > ------------ > > V0 -> V3: > > - Address the https://lore.kernel.org/lkml/4a565d54-f468-4e32-8a2c-102c1203f72c@lunn.ch/T/ > > review comments > > > > drivers/net/phy/mxl-gpy.c | 58 +++++++++++++++++++++++++-------------- > > 1 file changed, 38 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c > > index b2d36a3a96f1..e5f8ac4b4604 100644 > > --- a/drivers/net/phy/mxl-gpy.c > > +++ b/drivers/net/phy/mxl-gpy.c > > @@ -107,6 +107,7 @@ struct gpy_priv { > > > > u8 fw_major; > > u8 fw_minor; > > + u32 wolopts; > > > > /* It takes 3 seconds to fully switch out of loopback mode before > > * it can safely re-enter loopback mode. Record the time when > > @@ -221,6 +222,15 @@ static int gpy_hwmon_register(struct phy_device *phydev) > > } > > #endif > > > > +static int gpy_ack_interrupt(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + /* Clear all pending interrupts */ > > + ret = phy_read(phydev, PHY_ISTAT); > > + return ret < 0 ? ret : 0; > > Can we just return phy_read? > No. PHY_ISTAT (i.e.16 bit) is Max Linear's GPY211 PHY interrupt status register. These bits are ROSC (i.e. "Read Only, Self-Clearing) bits. Read Status gives us the interrupts details Any negative number is indicate the error in accessing PHY registgers. Return either success (i.e. 0) or Error ( < 0) > > +} > > + > > static int gpy_mbox_read(struct phy_device *phydev, u32 addr) > > { > > struct gpy_priv *priv = phydev->priv; > > @@ -262,16 +272,8 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr) > >
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c index b2d36a3a96f1..e5f8ac4b4604 100644 --- a/drivers/net/phy/mxl-gpy.c +++ b/drivers/net/phy/mxl-gpy.c @@ -107,6 +107,7 @@ struct gpy_priv { u8 fw_major; u8 fw_minor; + u32 wolopts; /* It takes 3 seconds to fully switch out of loopback mode before * it can safely re-enter loopback mode. Record the time when @@ -221,6 +222,15 @@ static int gpy_hwmon_register(struct phy_device *phydev) } #endif +static int gpy_ack_interrupt(struct phy_device *phydev) +{ + int ret; + + /* Clear all pending interrupts */ + ret = phy_read(phydev, PHY_ISTAT); + return ret < 0 ? ret : 0; +} + static int gpy_mbox_read(struct phy_device *phydev, u32 addr) { struct gpy_priv *priv = phydev->priv; @@ -262,16 +272,8 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr) static int gpy_config_init(struct phy_device *phydev) { - int ret; - - /* Mask all interrupts */ - ret = phy_write(phydev, PHY_IMASK, 0); - if (ret) - return ret; - - /* Clear all pending interrupts */ - ret = phy_read(phydev, PHY_ISTAT); - return ret < 0 ? ret : 0; + /* Nothing to configure. Configuration Requirement Placeholder */ + return 0; } static int gpy21x_config_init(struct phy_device *phydev) @@ -627,11 +629,23 @@ static int gpy_read_status(struct phy_device *phydev) static int gpy_config_intr(struct phy_device *phydev) { + struct gpy_priv *priv = phydev->priv; u16 mask = 0; + int ret; + + ret = gpy_ack_interrupt(phydev); + if (ret) + return ret; if (phydev->interrupts == PHY_INTERRUPT_ENABLED) mask = PHY_IMASK_MASK; + if (priv->wolopts & WAKE_MAGIC) + mask |= PHY_IMASK_WOL; + + if (priv->wolopts & WAKE_PHY) + mask |= PHY_IMASK_LSTC; + return phy_write(phydev, PHY_IMASK, mask); } @@ -678,6 +692,7 @@ static int gpy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) { struct net_device *attach_dev = phydev->attached_dev; + struct gpy_priv *priv = phydev->priv; int ret; if (wol->wolopts & WAKE_MAGIC) { @@ -725,6 +740,8 @@ static int gpy_set_wol(struct phy_device *phydev, ret = phy_read(phydev, PHY_ISTAT); if (ret < 0) return ret; + + priv->wolopts |= WAKE_MAGIC; } else { /* Disable magic packet matching */ ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, @@ -732,6 +749,13 @@ static int gpy_set_wol(struct phy_device *phydev, WOL_EN); if (ret < 0) return ret; + + /* Disable the WOL interrupt */ + ret = phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_WOL); + if (ret < 0) + return ret; + + priv->wolopts &= ~WAKE_MAGIC; } if (wol->wolopts & WAKE_PHY) { @@ -748,9 +772,11 @@ static int gpy_set_wol(struct phy_device *phydev, if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC)) phy_trigger_machine(phydev); + priv->wolopts |= WAKE_PHY; return 0; } + priv->wolopts &= ~WAKE_PHY; /* Disable the link state change interrupt */ return phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC); } @@ -758,18 +784,10 @@ static int gpy_set_wol(struct phy_device *phydev, static void gpy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) { - int ret; + struct gpy_priv *priv = phydev->priv; wol->supported = WAKE_MAGIC | WAKE_PHY; - wol->wolopts = 0; - - ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, VPSPEC2_WOL_CTL); - if (ret & WOL_EN) - wol->wolopts |= WAKE_MAGIC; - - ret = phy_read(phydev, PHY_IMASK); - if (ret & PHY_IMASK_LSTC) - wol->wolopts |= WAKE_PHY; + wol->wolopts = priv->wolopts; } static int gpy_loopback(struct phy_device *phydev, bool enable)
When the system resumes from sleep, the phy_init_hw() function invokes config_init(), which clears all interrupt masks and causes wake events to be lost in subsequent wake sequences. Remove interrupt mask clearing from config_init() and preserve relevant masks in config_intr() Fixes: 7d901a1e878a ("net: phy: add Maxlinear GPY115/21x/24x driver") Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> --- Change List: ------------ V0 -> V3: - Address the https://lore.kernel.org/lkml/4a565d54-f468-4e32-8a2c-102c1203f72c@lunn.ch/T/ review comments drivers/net/phy/mxl-gpy.c | 58 +++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 20 deletions(-)