Message-ID: <Z36WqNGpWWkHTjUE@shell.armlinux.org.uk>
Date: Wed, 8 Jan 2025 15:15:52 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	UNGLinuxDriver@...rochip.com, Phil Elwell <phil@...pberrypi.org>
Subject: Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support
 with phylink integration

On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote:
> > On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote:
> > > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
> > > to integrate with phylink. This includes the following changes:
> > > 
> > > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
> > >   EEE settings, aligning with the phylink API.
> > > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
> > >   request delay. Default it to 50 microseconds based on LAN7800 documentation
> > >   recommendations.
> > 
> > phylib maintains tx_lpi_timer for you. Please use that instead.
> 
> Just using this variable directly phydev->eee_cfg.tx_lpi_timer ?

Yes. We're already accessing phydev->enable_tx_lpi directly, and we
have no helpers for this. Maybe it's more a question for Andrew,
whether he wishes to see phylib helpers for this state rather than
directly dereferencing phydev.

> > In any case, I've been submitting phylink EEE support which will help
> > driver authors get this correct, but I think it needs more feedback.
> > Please can you look at my patch set previously posted which is now
> > a bit out of date, review, and think about how this driver can make
> > use of it.
> 
> Ack, will do. It looks like your port of lan743x to the new API
> looks exactly like what I need for this driver too.
> 
> > In particular, I'd like ideas on what phylink should be doing with
> > tx_lpi_timer values that are out of range of the hardware. Should it
> > limit the value itself?
> 
> Yes, otherwise every MAC driver will need to do it in the
> ethtool_set_eee() function.

I've had several solutions, and my latest patch set actually has a
mixture of them in there (which is why I'm eager to try and find a way
forward on this, so I can fix the patch set):

1. the original idea to address this in Marvell platforms was to limit
   the LPI timer to the maximum representable value in the hardware,
   which would be 255us. This ignores that the hardware uses a 1us
   tick rate for the timer at 1G speeds, and 10us for 100M speeds.
   (So it limits it to 260us, even though the hardware can do 2550us
   at 100M speed). This limit was applied by clamping the value passed
   in from userspace without erroring out.

2. another solution was added the mac_validate_tx_lpi() method, and
   implementations added _in addition_ to the above, with the idea
   of erroring out for values > 255us on Marvell hardware.

3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
   possible to allow e.g. falling back to a software timer (see stmmac
   comments below.) Another reason for erroring out applies to Marvell
   hardware, where PP2 hardware supports LPI on the GMAC but not the
   XGMAC - so it only works at speeds at or below 2.5G. However, that
   can be handled via the lpi_capabilities, so I don't think needs to
   be a concern.

> The other question is, should we allow absolute maximum values, or sane
> maximum? At some point will come the question, why the EEE is even
> enabled?

As referenced above, stmmac uses the hardware timer for LPI timeouts up
to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
normal kernel timer which is:

- disabled (and EEE mode reset) when we have a packet to transmit, or
  EEE is disabled
- is re-armed when cleaning up from packet transmission (although
  it looks like we attempt to immediately enter LPI mode, and would
  only wait for the timer if there are more packets to queue... maybe
  this is a bug in stmmac's implementation?) or when EEE mode is first
  enabled with a LPI timer longer than the above value.

So, should phylink have the capability to switch to a software LPI timer
implementation when the LPI timeout value exceeds what the hardware
supports? To put it another way, should the stmmac solution to this be
made generic?

Note that stmmac has this software timer implementation because not
only for the reason I've given above, but also because cores other than
GMAC4 that support LPI do not have support for the hardware timer.

> The same is about minimal value, too low value will cause strong speed
> degradation. Should we allow set insane minimum, but use sane default
> value?

We currently allow zero, and the behaviour of that depends on the
hardware. For example, in the last couple of days, it's been reported
that stmmac will never enter LPI with a value of zero.

Note that phylib defaults to zero, so imposing a minimum would cause
a read-modify-write of the EEE settings without setting the timer to
fail.

> > Should set_eee() error out?
> 
> Yes, please.

If we are to convert stmmac, then we need to consider what it's doing
(as per the above) and whether that should be generic - and if it isn't
what we want in generic code, then how do we allow drivers to do this if
they wish.

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