Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Dec 13, 2024 at 10:59:54AM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote:
> > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> >  			phy_init_eee(phy, !(priv->plat->flags &
> >  				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
> >  		priv->eee_enabled = stmmac_eee_init(priv);
> > +		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
> >  		priv->tx_lpi_enabled = priv->eee_enabled;
> >  		stmmac_set_eee_pls(priv, priv->hw, true);
> >  	}
> 
> While looking deeper at stmmac, there's a bug in the above hunk -
> stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member
> needs to be set before calling this function. I'll post a v2 shortly.

I'm going to hold off v2, there's a lot more that can be cleaned up
here - the EEE code is rather horrid in stmmac, and there's definitely
one race, and one logical error in it (e.g. why mark software EEE mode
*enabled* when EEE mode is being disabled - which can lead to the EEE
timer being added back onto the timer list.)

There's also weirdness with dwmac4's EEE register fiddling.

The stmmac driver uses hardware timed LPI entry if the timer is small
enough to be programmed into hardware, otherwise it uses software mode.

When software mode wants to enter LPI mode, it sets both:

	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)

When software mode wants to exit LPI mode, it clears both of these
two bits.

In hardware mode, when enabling LPI generation, we set the hardware LPI
entry timer (separate register) to a non-zero value, and then set:

	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)
	GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable)

That seems logical. However, in hardware mode, when we want to then
disable hardware LPI generation, we set the hardware LPI entry timer to
zero, the following bits:

	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)

and clear:

	GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable)

So, hardware mode, disabled, ends up setting the same bits as
software mode wanting to generate LPI state on the transmit side.
This makes no sense to me, and looks like another bug in this driver.

Can anyone suggest any hardware that I could source which uses the
dwmac4 code and which supports EEE please, so that I have hardware to
run some tests on.

Thanks.

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