Message-ID: <ZHZQG+O9HkQ+5K62@shell.armlinux.org.uk>
Date: Tue, 30 May 2023 20:35:55 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Oleksij Rempel <linux@...pel-privat.de>
Subject: Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
> Hi Andrew, Russell,
> 
> On 3/30/23 17:54, Andrew Lunn wrote:
> > Most MAC drivers get EEE wrong. The API to the PHY is not very
> > obvious, which is probably why. Rework the API, pushing most of the
> > EEE handling into phylib core, leaving the MAC drivers to just
> > enable/disable support for EEE in there change_link call back, or
> > phylink mac_link_up callback.
> > 
> > MAC drivers are now expect to indicate to phylib/phylink if they
> > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > configured.
> 
> Thanks for doing this work, because it really is a happy mess out there. A
> few questions as I have been using mvneta as the reference for fixing GENET
> and its shortcomings.
> 
> In your new patches the decision to enable EEE is purely based upon the
> eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
> useed to do.
> 
> Russell, is there an use case for having eee_enabled while not having
> tx_lpi_enabled?

As I've been stating recently, LPI is entirely to do with the MAC, so
the LPI delay and the enable boolean are about controlling the MAC end
of things.

I've never really understood what "eee_enabled" is supposed to be doing,
since there's nowhere really to enable or disable EEE per-se. Through
recent patches, phylib has since defined "eee_enabled" to mean that the
advertisement is non-empty, which came as a surprise to me, but again
seems rather redundant and strange.

It's strange because as far as I can see from what documentation there
is, "eee_enabled" is supposed to be a control that users can set
independently of the advertisement, but with the phylib implementation,
eee_enabled read from get_eee() as I say is defined as whether the
advertisement is non-zero or not.

With fibre setups, there is no EEE advertisement, so in that situation
phylib's interpretation of eee_enabled via get_eee/set_eee would be
very much incorrect. The only control one has is whether LPI is enabled
and its timer setting.

That said, the current mvneta implementation doesn't actually enable
LPI for fibre... but there is a bug in that one can get the MAC to
enable LPI via ethtool's set_eee() !

I think for a fibre setup, eee_enabled && tx_lpi_enabled is reasonable,
given that eee_enabled is a seperate control from the advertisement
which will be zero in that case.

Going back to phylib, given this, things get even more "fun" if you have
a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
advertisement with a dual-media PHY that would only affect the copper
side, and EEE may still be possible in the fibre side... which makes
phylib's new interpretation of "eee_enabled" rather odd.

In any case, "eee_enabled" I don't think has much meaning for the fibre
case because there's definitely no control beyond what "tx_lpi_enabled"
already offers.

I think this is a classic case where the EEE interface has been designed
solely around copper without checking what the situation is for fibre!

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

Powered by blists - more mailing lists