Re: [PATCH net-next 05/10] net: phylink: add EEE management

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sun, Dec 15, 2024 at 12:38:24AM +0100, Heiner Kallweit wrote:
> On 09.12.2024 15:23, Russell King (Oracle) wrote:
> > Add EEE management to phylink, making use of the phylib implementation.
> > This will only be used where a MAC driver populates the methods and
> > capabilities bitfield, otherwise we keep our old behaviour.
> > 
> > Phylink will keep track of the EEE configuration, including the clock
> > stop abilities at each end of the MAC to PHY link, programming the PHY
> > appropriately and preserving the EEE configuration should the PHY go
> > away.
> > 
> > It will also call into the MAC driver when LPI needs to be enabled or
> > disabled, with the expectation that the MAC have LPI disabled during
> > probe.
> > 
> > Support for phylink managed EEE is enabled by populating both tx_lpi
> > MAC operations method pointers.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
> >  include/linux/phylink.h   |  44 ++++++++++++++
> >  2 files changed, 163 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 03509fdaa1ec..750356b6a2e9 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -81,12 +81,19 @@ struct phylink {
> >  	unsigned int pcs_state;
> >  
> >  	bool link_failed;
> > +	bool mac_supports_eee;
> > +	bool phy_enable_tx_lpi;
> > +	bool mac_enable_tx_lpi;
> > +	bool mac_tx_clk_stop;
> > +	u32 mac_tx_lpi_timer;
> >  
> >  	struct sfp_bus *sfp_bus;
> >  	bool sfp_may_have_phy;
> >  	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> >  	u8 sfp_port;
> > +
> > +	struct eee_config eee_cfg;
> >  };
> >  
> >  #define phylink_printk(level, pl, fmt, ...) \
> > @@ -1563,6 +1570,47 @@ static const char *phylink_pause_to_str(int pause)
> >  	}
> >  }
> >  
> > +static void phylink_deactivate_lpi(struct phylink *pl)
> > +{
> > +	if (pl->mac_enable_tx_lpi) {
> > +		pl->mac_enable_tx_lpi = false;
> > +
> > +		phylink_dbg(pl, "disabling LPI\n");
> > +
> > +		pl->mac_ops->mac_disable_tx_lpi(pl->config);
> > +	}
> > +}
> > +
> > +static void phylink_activate_lpi(struct phylink *pl)
> > +{
> > +	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
> > +		phylink_dbg(pl, "MAC does not support LPI with %s\n",
> > +			    phy_modes(pl->cur_interface));
> > +		return;
> > +	}
> > +
> > +	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
> > +		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
> > +
> > +	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
> > +				       pl->mac_tx_clk_stop);
> > +
> > +	pl->mac_enable_tx_lpi = true;
> > +}
> > +
> > +static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
> > +{
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
> > +
> > +	/* Convert the MAC's LPI capabilities to linkmodes */
> > +	linkmode_zero(eee_supported);
> > +	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
> > +
> > +	/* Mask out EEE modes that are not supported */
> > +	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
> > +	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
> > +}
> > +
> 
> Something similar we may need in phylib too. An example is cpsw MAC driver which
> doesn't support EEE. Issues have been reported if the PHY's on both sides negotiate
> EEE, workaround is to use property eee-broken-xxx in the respective DT's to disable
> PHY EEE advertisement. I'm thinking of adding a simple phy_disable_eee() which can
> be called by MAC drivers to clear EEE supported and advertising bitmaps.
> 
> A similar case is enetc (using phylink) which doesn't support EEE. See following in
> enetc.c:
> 
> /* disable EEE autoneg, until ENETC driver supports it */
> memset(&edata, 0, sizeof(struct ethtool_keee));
> phylink_ethtool_set_eee(priv->phylink, &edata);
> 
> Russell, do you plan to change this driver too, based on phylink extensions?
> I think already now, based on the quoted code piece, several (all?) eee-broken-xxx
> properties can be removed under arch/arm64/boot/dts/freescale .

At the moment, phylink-managed EEE is opt-in, so what you get without
this patch is what you get with the patch but no driver changes. This
allows existing drivers that support EEE and that use phylink to
continue to support EEE, and those that don't to continue in their
current situation (if they use work-arounds to disable EEE, then
those will continue to work.)

Rather than adding something explicit to phylink to disable EEE, I
would much rather we work towards a situation where, if a driver
uses phylink, then if EEE is supported, the EEE methods and other
configuration get populated. If not, then phylink disables EEE on
the PHY.

However, we can only get there once we have EEE implemented on all
the phylink-using drivers that currently support EEE.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!