Message-ID: <Z0Xz0pQGXMIcG4jB@shell.armlinux.org.uk>
Date: Tue, 26 Nov 2024 16:14:10 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Bryan Whitehead <bryan.whitehead@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Jakub Kicinski <kuba@...nel.org>, Jose Abreu <joabreu@...opsys.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Marcin Wojtas <marcin.s.wojtas@...il.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 00/23] net: phylink managed EEE support

On Tue, Nov 26, 2024 at 03:21:26PM +0100, Oleksij Rempel wrote:
> Hi Russell,
> 
> On Tue, Nov 26, 2024 at 01:01:11PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 26, 2024 at 12:51:36PM +0000, Russell King (Oracle) wrote:
> > > Patch 11 adds phylink managed EEE support. Two new MAC APIs are added,
> > > to enable and disable LPI. The enable method is passed the LPI timer
> > > setting which it is expected to program into the hardware, and also a
> > > flag ehther the transmit clock should be stopped.
> > > 
> > >  *** There are open questions here. Eagle eyed reviewers will notice
> > >    pl->config->lpi_interfaces. There are MACs out there which only
> > >    support LPI signalling on a subset of their interface types. Phylib
> > >    doesn't understand this. I'm handling this at the moment by simply
> > >    not activating LPI at the MAC, but that leads to ethtool --show-eee
> > >    suggesting that EEE is active when it isn't.
> > >  *** Should we pass the phy_interface_t to these functions?
> > >  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
> > >    support the interface mode?
> > 
> > There is another point to raise here - should we have a "validate_eee"
> > method in struct phylink_mac_ops so that MAC drivers can validate
> > settings such as the tx_lpi_timer value can be programmed into the
> > hardware?
> > 
> > We do have the situation on Marvell platforms where the programmed
> > value depends on the MAC speed, and is only 8 bit, which makes
> > validating its value rather difficult - at 1G speeds, it's a
> > resolution of 1us so we can support up to 255us. At 100M speeds,
> > it's 10us, supporting up to 2.55ms. This makes it awkward to be able
> > to validate the set_eee() settings are sane for the hardware. Should
> > Marvell platforms instead implement a hrtimer above this? That sounds
> > a bit problematical to manage sanely.
> 
> I agree that tx_lpi_timer can be a problem, and this is not just a
> Marvell issue.  For example, I think the FEC MAC on i.MX8MP might also
> be affected.  But I can't confirm this because I don't have an i.MX8MP
> board with a PHY that supports MAC-controlled EEE mode. The Realtek PHY
> I have uses PHY-controlled EEE (SmartEEE).
> 
> Except for this, I think there should be sane default values for
> tx_lpi_timer.  The IEEE 802.3-2022 standard (Section 78.2) describes EEE
> timing, but it doesn’t give a clear recommendation for tx_lpi_timer.

There are of course some parameters of EEE that should be fixed, and
IEEE specifies them in that section. These are Ts, Tq and Tr, and IEEE
gives a range of values for these which need to be conformed with in a
compliant implementation.

Please don't get confused by the mvneta/mvpp2 implementation, there are
parameters for Ts and Tw, but the Ts value is not the same as Ts in
IEEE. IEEE defines it as the period of time between the PHY transmitting
sleep and turning all transmitters off. Marvell define it as the minimum
time from the Tx FIFO being empty to asserting LPI - quite different!

Other parameters depend on the implementation, such as the propagation
delay of data through the PHY. These, we don't currently take account
of. I don't recall off-hand whether there's any standards defined
registers describing these parameters in the PHY (I need to delve into
802.3...) That's all needed for computing Tw.

> IMO, the best value for tx_lpi_timer should be the sum of the time
> needed to put the full chain (MAC -> PHY -> remote PHY) into sleep mode
> and the time needed to wake the chain. These times are link-speed
> specific, so defaults should consider PHY timings for each link speed.

One can only make a set of defaults that depend on the speed if we also
give the user the ability to set the tx_lpi_timer values on a per-speed
basis, otherwise how do we update the values when set_eee() gets called?

Also how do we know what the requirements of the remote PHY are? I think
the only way that could be known is by exchanging the EEE TLV with the
link partner as described in section 78.

> Except for tx_lpi_timer, some MACs also allow configuration of the Twake
> timer.  For example, the FEC driver uses the lpi_timer value to
> configure the Twake timer, and the LAN78xx driver also provides a
> configurable Twake timer register.
> 
> If the Twake timer is not configured properly, or if the system has
> quirks causing the actual Twake time to be longer than expected, it can
> result in frame corruption. 

I have been aware of it, and to me it sounds like another can of worms
that right now I'd rather not open until we have solved the basics of
EEE and got MAC drivers into better shape for the basics. I had been
wondering whether we would eventually need phylink to pass Tw along
with the LPI timer, and I think eventually we would need to - and we
also need some infrastructure for the EEE TLV, both sending it at the
appropriate time, and processing it when received. I don't think we
have any of that at the moment, so it would be another chunk of
development.

-- 
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