| Message ID | 20240221062107.778661-7-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On 2/20/2024 10:21 PM, Oleksij Rempel wrote: > From: Andrew Lunn <andrew@lunn.ch> > > In order for EEE to operate, both the MAC and the PHY need to support > it, similar to how pause works. Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in order to provide some EEE-like power savings with non-EEE capable MACs. Oleksij did not you have a patch series at some point that introduced a smarteee field in the phy_device structure to reflect that? I thought that had been accepted, but maybe not. > Copy the pause concept and add the > call phy_support_eee() which the MAC makes after connecting the PHY to > indicate it supports EEE. phylib will then advertise EEE when auto-neg > is performed. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/phy_device.c | 18 ++++++++++++++++++ > include/linux/phy.h | 3 ++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 2eefee970851..269d3c7f0849 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev) > } > EXPORT_SYMBOL_GPL(phy_advertise_eee_all); > > +/** > + * phy_support_eee - Enable support of EEE > + * @phydev: target phy_device struct > + * > + * Description: Called by the MAC to indicate is supports Energy > + * Efficient Ethernet. This should be called before phy_start() in > + * order that EEE is negotiated when the link comes up as part of > + * phy_start(). EEE is enabled by default when the hardware supports > + * it. That comment is a bit confusing without mentioning how the hardware default state wrt. EEE is being factored in, can we have some details here?
On Thu, Feb 22, 2024 at 08:52:25PM -0800, Florian Fainelli wrote: > > > On 2/20/2024 10:21 PM, Oleksij Rempel wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > In order for EEE to operate, both the MAC and the PHY need to support > > it, similar to how pause works. > > Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in > order to provide some EEE-like power savings with non-EEE capable MACs. Will reword it. > Oleksij did not you have a patch series at some point that introduced a > smarteee field in the phy_device structure to reflect that? I thought that > had been accepted, but maybe not. Ack. They are pending at the end of EEE refactoring queue :) > > Copy the pause concept and add the > > call phy_support_eee() which the MAC makes after connecting the PHY to > > indicate it supports EEE. phylib will then advertise EEE when auto-neg > > is performed. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/phy/phy_device.c | 18 ++++++++++++++++++ > > include/linux/phy.h | 3 ++- > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 2eefee970851..269d3c7f0849 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev) > > } > > EXPORT_SYMBOL_GPL(phy_advertise_eee_all); > > +/** > > + * phy_support_eee - Enable support of EEE > > + * @phydev: target phy_device struct > > + * > > + * Description: Called by the MAC to indicate is supports Energy > > + * Efficient Ethernet. This should be called before phy_start() in > > + * order that EEE is negotiated when the link comes up as part of > > + * phy_start(). EEE is enabled by default when the hardware supports > > + * it. > > That comment is a bit confusing without mentioning how the hardware default > state wrt. EEE is being factored in, can we have some details here? If I see it correctly, this function set initial EEE policy for the PHY. It should be called only once at PHY registration by the MAC and/or by the PHY in case of SmartEEE or AutoGrEEEn PHY. The advertisement configuration will be based on already filtered set of supported modes.
On Thu, Feb 22, 2024 at 08:52:25PM -0800, Florian Fainelli wrote: > > > On 2/20/2024 10:21 PM, Oleksij Rempel wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > In order for EEE to operate, both the MAC and the PHY need to support > > it, similar to how pause works. > > Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in > order to provide some EEE-like power savings with non-EEE capable MACs. > > Oleksij did not you have a patch series at some point that introduced a > smarteee field in the phy_device structure to reflect that? I thought that > had been accepted, but maybe not. I have some similar hacks for the Atheros SmartEEE in my tree that I've never published. For SmartEEE, we need two things to happen: 1) MAC drivers must not fail set_eee()/get_eee() just because they themselves do not support EEE. 2) MAC drivers must not attempt to modify the EEE parameters passed to phylib. Whether a MAC driver should be configuring the hardware in set_eee() at all is another question - because in the case of using SmartEEE the MAC side is irrelevant. So maybe phylib should have a callback to set the EEE TX LPI parameters? In phylink, my model was to add two new functions (one to enable and another to disable TX LPI) and the enable function always gets passed the TX LPI timeout. If we did the same in phylib, we would eliminate the need for MAC drivers to conditionalise based on SmartEEE - that could be handled entirely within phylib.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 2eefee970851..269d3c7f0849 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev) } EXPORT_SYMBOL_GPL(phy_advertise_eee_all); +/** + * phy_support_eee - Enable support of EEE + * @phydev: target phy_device struct + * + * Description: Called by the MAC to indicate is supports Energy + * Efficient Ethernet. This should be called before phy_start() in + * order that EEE is negotiated when the link comes up as part of + * phy_start(). EEE is enabled by default when the hardware supports + * it. + */ +void phy_support_eee(struct phy_device *phydev) +{ + linkmode_copy(phydev->advertising_eee, phydev->supported_eee); + phydev->eee_cfg.tx_lpi_enabled = true; + phydev->eee_cfg.eee_enabled = true; +} +EXPORT_SYMBOL(phy_support_eee); + /** * phy_support_sym_pause - Enable support of symmetrical pause * @phydev: target phy_device struct diff --git a/include/linux/phy.h b/include/linux/phy.h index 356916695a26..b6c5dee143d1 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -706,7 +706,7 @@ struct phy_device { __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); /* used with phy_speed_down */ __ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old); - /* used for eee validation */ + /* used for eee validation and configuration*/ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee); __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee); bool eee_enabled; @@ -1973,6 +1973,7 @@ void phy_advertise_supported(struct phy_device *phydev); void phy_advertise_eee_all(struct phy_device *phydev); void phy_support_sym_pause(struct phy_device *phydev); void phy_support_asym_pause(struct phy_device *phydev); +void phy_support_eee(struct phy_device *phydev); void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg); void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);