Message-ID: <Z1c-CL5KuwnAeW4G@shell.armlinux.org.uk>
Date: Mon, 9 Dec 2024 18:59:20 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Bryan Whitehead <bryan.whitehead@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Marcin Wojtas <marcin.s.wojtas@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next 00/10] net: add phylink managed EEE support

On Mon, Dec 09, 2024 at 07:35:11PM +0100, Christian Marangi wrote:
> On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Adding managed EEE support to phylink has been on the cards ever since
> > the idea in phylib was mooted. This overly large series attempts to do
> > so. I've included all the patches as it's important to get the driver
> > patches out there.
> > 
> > Patch 1 adds a definition for the clock stop capable bit in the PCS
> > MMD status register.
> > 
> > Patch 2 adds a phylib API to query whether the PHY allows the transmit
> > xMII clock to be stopped while in LPI mode. This capability is for MAC
> > drivers to save power when LPI is active, to allow them to stop their
> > transmit clock.
> > 
> > Patch 3 adds another phylib API to configure whether the receive xMII
> > clock may be disabled by the PHY. We do have an existing API,
> > phy_init_eee(), but... it only allows the control bit to be set which
> > is weird - what if a boot firmware or previous kernel has set this bit
> > and we want it clear?
> > 
> > Patch 4 starts on the phylink parts of this, extracting from
> > phylink_resolve() the detection of link-up. (Yes, okay, I could've
> > dropped this patch, but with 23 patches, it's not going to make that
> > much difference.)
> > 
> > Patch 5 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?
> 
> Maybe only to validate?

validate doesn't know what interface will be used - at set_eee() time
we can't know that.

> >  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
> >    support the interface mode?
> 
> I'm a bit confused by this... Following principle with other OPs
> shouldn't this never happen? Supported interface are validated by
> capabilities hence mac_enable_tx_lpi() should never be reached (if not
> supported). Or I'm missing something by this idea?

This question was asked in the RFC, and before I added lpi_interfaces
which now prevents calling this unless the interface bit is set in
lpi_interfaces. However, does it still make sense to pass the
interface in case e.g. the MAC block that needs to be configured is
dependent on it?

Some network interfaces are made up of multiple MACs for different
speeds, and the MAC is selected by interface. E.g. mvpp2.

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