Re: [PATCH v1] net: phy: dp83867: Disable EEE support as not implemented

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Mon, Oct 27, 2025 at 02:25:12PM +0100, Andrew Lunn wrote:
> On Mon, Oct 27, 2025 at 01:57:48PM +0100, Emanuele Ghidoli wrote:
> > 
> > 
> > On 27/10/2025 00:45, Andrew Lunn wrote:
> > >> Since the introduction of phylink-managed EEE support in the stmmac driver,
> > >> EEE is now enabled by default, leading to issues on systems using the
> > >> DP83867 PHY.
> > > 
> > > Did you do a bisect to prove this?
> > Yes, I have done a bisect and the commit that introduced the behavior on our
> > board is 4218647d4556 ("net: stmmac: convert to phylink managed EEE support").
> > 
> > > 
> > >> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
> > > 
> > > What has this Fixes: tag got to do with phylink?
> > I think that the phylink commit is just enabling by default the EEE support,
> > and my commit is not really fixing that. It is why I didn't put a Fixes: tag
> > pointing to that.
> > 
> > I’ve tried to trace the behavior, but it’s quite complex. From my testing, I
> > can summarize the situation as follows:
> > 
> > - ethtool, after that patch, returns:
> > ethtool --show-eee end0
> > EEE settings for end0:
> >         EEE status: enabled - active
> >         Tx LPI: 1000000 (us)
> >         Supported EEE link modes:  100baseT/Full
> >                                    1000baseT/Full
> >         Advertised EEE link modes:  100baseT/Full
> >                                     1000baseT/Full
> >         Link partner advertised EEE link modes:  100baseT/Full
> >                                                  1000baseT/Full
> > - before that patch returns, after boot:
> > EEE settings for end0:
> >         EEE status: disabled
> >         Tx LPI: disabled
> >         Supported EEE link modes:  100baseT/Full
> >                                    1000baseT/Full
> >         Advertised EEE link modes:  Not reported
> >         Link partner advertised EEE link modes:  100baseT/Full
> >                                                  1000baseT/Full
> > - Enabling EEE manually using ethtool, triggers the problem too (and ethtool
> > -show-eee report eee status enabled):
> > ethtool --set-eee end0 eee on tx-lpi on
> > ethtool --show-eee end0
> > EEE settings for end0:
> >         EEE status: enabled - active
> >         Tx LPI: 1000000 (us)
> >         Supported EEE link modes:  100baseT/Full
> >                                    1000baseT/Full
> >         Advertised EEE link modes:  100baseT/Full
> >                                     1000baseT/Full
> >         Link partner advertised EEE link modes:  100baseT/Full
> >                                                  1000baseT/Full
> > 
> > I understand Russell point of view but from my point of view EEE is now
> > enabled by default, and before it wasn't, at least on my setup.
> 
> We like to try to understand what is going on, and give accurate
> descriptions. You have given us important information here, which at
> minimum should go into the commit message, but more likely, it will
> help lead us to the correct fix.
> 
> So, two things here. You say:
> 
> > I think that the phylink commit is just enabling by default the EEE support,
> 
> That needs confirming, because you are blaming the conversion to
> phylink, not that phylink now enabled EEE by default. Russell also
> tries to avoid behaviour change, which this clearly is. We want a
> better understanding what caused this behaviour change.
> 
> Also:
> 
> > - Enabling EEE manually using ethtool, triggers the problem too (and ethtool
> > -show-eee report eee status enabled):
> 
> This indicates EEE has always been broken. This brokenness has been
> somewhat hidden in the past, and it is the change in behaviour in
> phylink which exposed this brokenness. A commit message using these
> words would be much more factually correct, and it would also fit with
> the Fixes: tag you used.
> 
> So, please work with Russell. I see two things which would be good to
> understand before a new version of the patch is submitted:
> 
> What cause the behaviour change such that EEE is now enabled? Was it
> deliberate? Should something be change to revert that behaviour
> change?
> 
> Given that EEE has always been broken, do we understand it
> sufficiently to say it is not fixable? Is there an errata?

None of following TI Gbit PHYs claim EEE support:
dp83867cr/ir  https://www.ti.com/de/lit/gpn/dp83867cr
dp83867e/cs/is  https://www.ti.com/de/lit/gpn/dp83867cs
dp83869hm https://www.ti.com/lit/gpn/dp83869hm

For comparison, TI 100Mbit PHYs list EEE as supported:
dp83826a*  https://www.ti.com/de/lit/gpn/dp83826ae
dp83826i https://www.ti.com/de/lit/gpn/dp83826i

If vendor do not see it as selling point, or it is just broken beyond
repair, there is nothing we can do here. I guess it is ok to sync the
driver with vendors claim.

> Are we sure it is the PHY and not the MAC which is broken?

I personally still do not have suitable reference board for testing.
There are some with Realtek or TI PHYs. It will be good to find board
with iMX8MP + KSZ9131 on both MACs (FEC and STMMAC).
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |