Re: [PATCH RFC net-next 2/4] net: phylink: add EEE management

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Marcin Wojtas <mw@semihalf.com>,
	netdev@vger.kernel.org, Paolo Abeni <pabeni@redhat.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH RFC net-next 2/4] net: phylink: add EEE management
Date: Tue, 13 Jun 2023 10:13:53 +0100	[thread overview]
Message-ID: <ZIgzUZSKW0WsA0AC@shell.armlinux.org.uk> (raw)
In-Reply-To: <50a42dc7-02df-4052-abeb-7d7b9cd7225e@lunn.ch>

On Mon, Jun 12, 2023 at 12:25:29AM +0200, Andrew Lunn wrote:
> On Sun, Jun 11, 2023 at 10:37:55PM +0100, Russell King (Oracle) wrote:
> > On Sun, Jun 11, 2023 at 11:28:32PM +0200, Andrew Lunn wrote:
> > > On Fri, Jun 09, 2023 at 10:11:21AM +0100, Russell King (Oracle) wrote:
> > > > Add EEE management to phylink.
> > > 
> > > Hi Russell
> > > 
> > > I've been working on my EEE patches. I have all pure phylib drivers
> > > converted. I've incorporated these four patches as well, and make use
> > > of the first patch in phylib.
> > > 
> > > Looking at this patch, i don't see a way for the MAC to indicate it
> > > actually does support EEE. Am i missing it?
> > 
> > If a MAC doesn't support EEE, it won't have the necessary calls to
> > phylink_*_eee() in its ethtool ops. As can be seen from the mvpp2
> > patch, mvpp2_ethtool_get_eee() and mvpp2_ethtool_set_eee() are
> > needed to call the phylink methods.
> 
> This is a bit messy for stmmac. Some versions of stmmac have EEE
> support, some don't. Same is true for r8169, but that is a phylib
> driver. I expect there are other examples.

I see what you mean, but I think really what's going on there is
whether the MAC supports LPI or not coupled with whether the PHY has
any "smartEEE" feature meaning overall the system doesn't support
any kind of EEE.

The reason I pick that level of detail is when one comes to a setup
like i.MX6 FEC coupled with an AR8035 with it's SmartEEE, the system
as a whole supports EEE but the MAC doesn't. So, while the FEC would
have a MAC_CAP_EEE flag clear, we don't actually want to disable EEE
support with that setup.

> We also have the same problem with DSA. There is currently one
> phylink_mac_ops structure which has generic methods for all the
> callbacks which then call into the switch driver if the switch driver
> implements the call. And at least with the mv88e6xxx driver, when we
> get around to adding EEE support, some switches will support it, some
> won't, so will we need two different switch op structures?

I'd rather not end up with multiple mac_ops.

> Adding to the mac_capabilities just seems easier.

It does, but I suspect MAC_CAP_EEE is not sufficient because:

a) the issue of PHYs with SmartEEE like features such as AR803x.
b) an interface which has multiple MACs that it switches between
   depending on speed may have differing levels of LPI support,
   so a single property doesn't seem to be suitable.
c) rate adapting PHYs that may connect only with e.g. a 10G MAC that
   doesn't support LPI, but the MAC setup does have a 1G MAC that
   does.

I'm wondering if, rather than adding a bit to mac_capabilities, whether
instead:

1) add eee_capabilities and re-use the existing MAC_CAP_* definitions
   to indicate what speeds the MAC supports LPI. This doesn't seem to
   solve (c).
2) add a phy interface bitmap indicating which interface modes support
   LPI generation.

Phylib already has similar with its supported_eee link mode bitmap,
which presumably MACs can knock out link modes that they know they
wouldn't support.

Hmm.

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