| Message ID | 20221205153328.503576-2-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: macb: fix connectivity after resume | expand |
On Mon, Dec 05, 2022 at 05:33:27PM +0200, Claudiu Beznea wrote: > Add helper to initialize phydev embedded in a phylink object. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/net/phy/phylink.c | 10 ++++++++++ > include/linux/phylink.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 09cc65c0da93..1e2478b8cd5f 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee) > } > EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee); > > +/** > + * phylink_init_phydev() - initialize phydev associated to phylink > + * @pl: a pointer to a &struct phylink returned from phylink_create() > + */ > +int phylink_init_phydev(struct phylink *pl) > +{ > + return phy_init_hw(pl->phydev); > +} > +EXPORT_SYMBOL_GPL(phylink_init_phydev); I'd guess this is something that many MAC drivers will need to do when resuming if the PHY has lost power. Maybe a better solution would be to integrate it into phylink_resume(), when we know that the PHY has lost power - maybe the MAC driver can tell phylink that detail, and be updated to use phylink_suspend() and phylink_resume() ? macb_set_wol() sets bp->wol's MACB_WOL_ENABLED flag depending on whether WAKE_MAGIC is set in wolopts. No other wolopts are supported. Generic code sets netdev->wol_enabled if set_wol() was successful and wolopts is nonzero, indicating that WoL is enabled, and thus phylink_stop() won't be called if WoL is enabled (similar to what macb_suspend() is doing.) Given that the macb MAC seems to be implementing WoL, it should call phylink_suspend() with mac_wol=true. Please can you look into this, thanks.
On 05.12.2022 17:57, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Dec 05, 2022 at 05:33:27PM +0200, Claudiu Beznea wrote: >> Add helper to initialize phydev embedded in a phylink object. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/net/phy/phylink.c | 10 ++++++++++ >> include/linux/phylink.h | 1 + >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c >> index 09cc65c0da93..1e2478b8cd5f 100644 >> --- a/drivers/net/phy/phylink.c >> +++ b/drivers/net/phy/phylink.c >> @@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee) >> } >> EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee); >> >> +/** >> + * phylink_init_phydev() - initialize phydev associated to phylink >> + * @pl: a pointer to a &struct phylink returned from phylink_create() >> + */ >> +int phylink_init_phydev(struct phylink *pl) >> +{ >> + return phy_init_hw(pl->phydev); >> +} >> +EXPORT_SYMBOL_GPL(phylink_init_phydev); > > I'd guess this is something that many MAC drivers will need to do when > resuming if the PHY has lost power. > > Maybe a better solution would be to integrate it into phylink_resume(), OK, I'll look into this. > when we know that the PHY has lost power - maybe the MAC driver can > tell phylink that detail, and be updated to use phylink_suspend() and > phylink_resume() ? Cutting the power is arch specific and it may depends on the PM mode that system will go (at least for AT91 architecture). At the moment there is no way for drivers to know about architecture specific power management mode. There was an attempt to implement this (few years ago, see [1]) but it wasn't accepted (from what I can see in the source code at the moment). So, in case we choose to move it to phylink_resume() we will have to reinitialize the PHY unconditionally (see below). Would this be OK? [1] https://lore.kernel.org/lkml/20170623010837.11199-1-f.fainelli@gmail.com/ > > macb_set_wol() sets bp->wol's MACB_WOL_ENABLED flag depending on > whether WAKE_MAGIC is set in wolopts. No other wolopts are supported. > Generic code sets netdev->wol_enabled if set_wol() was successful and > wolopts is nonzero, indicating that WoL is enabled, and thus > phylink_stop() won't be called if WoL is enabled (similar to what > macb_suspend() is doing.) > > Given that the macb MAC seems to be implementing WoL, it should call > phylink_suspend() with mac_wol=true. In AT91 BSR (backup and self-refresh) could coexist with other PM modes (that doesn't cut power). And they are mapped to Linux standard PM specific modes. So, whenever one would execute: echo mem > /sys/power/state # or echo standby > /sys/power/state BSR or other PM mode could be executed. MACB driver could know only if system goes to mem Linux PM mode or standby Linux PM mode. But BSR could be mapped either to mem or standby. We can't decide from driver if we go to BSR. In BSR there are minimum wakeup sources (some reserved pins and RTC alarm). There is no way to resume from WoL. Arch specific PM code could decide to not go to BSR if MACB may wakeup thus on MACB driver we could decide to run phylink_suspend()/phylink_resume() based on not having the MACB driver configured as a wakeup source. But it will not mean in all cases that we go to BSR. And imposing on arch specific code to not go to BSR if MACB may wakeup may be a pain for users (in case they switch from one PM mode to another as they will need to reconfigure the wakeup sources every time). Hope I was clear. Thank you for your review, Claudiu Beznea > > Please can you look into this, thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi, On Wed, Dec 07, 2022 at 10:49:39AM +0000, Claudiu.Beznea@microchip.com wrote: > On 05.12.2022 17:57, Russell King (Oracle) wrote: > > when we know that the PHY has lost power - maybe the MAC driver can > > tell phylink that detail, and be updated to use phylink_suspend() and > > phylink_resume() ? > > Cutting the power is arch specific and it may depends on the PM mode that > system will go (at least for AT91 architecture). At the moment there is no > way for drivers to know about architecture specific power management mode. > There was an attempt to implement this (few years ago, see [1]) but it > wasn't accepted (from what I can see in the source code at the moment). > > So, in case we choose to move it to phylink_resume() we will have to > reinitialize the PHY unconditionally (see below). Would this be OK? I guess it would - off the top of my head, I can't think why a call to phy_init_hw() would cause an issue, but maybe my fellow phylib maintainers have a different opinion. Thanks.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 09cc65c0da93..1e2478b8cd5f 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee) } EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee); +/** + * phylink_init_phydev() - initialize phydev associated to phylink + * @pl: a pointer to a &struct phylink returned from phylink_create() + */ +int phylink_init_phydev(struct phylink *pl) +{ + return phy_init_hw(pl->phydev); +} +EXPORT_SYMBOL_GPL(phylink_init_phydev); + /* This emulates MII registers for a fixed-mode phy operating as per the * passed in state. "aneg" defines if we report negotiation is possible. * diff --git a/include/linux/phylink.h b/include/linux/phylink.h index c492c26202b5..6a969aa75c7f 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -609,6 +609,7 @@ int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *); int phylink_mii_ioctl(struct phylink *, struct ifreq *, int); int phylink_speed_down(struct phylink *pl, bool sync); int phylink_speed_up(struct phylink *pl); +int phylink_init_phydev(struct phylink *pl); #define phylink_zero(bm) \ bitmap_zero(bm, __ETHTOOL_LINK_MODE_MASK_NBITS)
Add helper to initialize phydev embedded in a phylink object. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/net/phy/phylink.c | 10 ++++++++++ include/linux/phylink.h | 1 + 2 files changed, 11 insertions(+)