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

[Emanuele Ghidoli <ghidoliemanuele@gmail.com>]

On 27/10/2025 15:53, Russell King (Oracle) 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
> 
> Oh damn. I see why now:
> 
>         /* Some DT bindings do not set-up the PHY handle. Let's try to
>          * manually parse it
>          */
>         if (!phy_fwnode || IS_ERR(phy_fwnode)) {
>                 int addr = priv->plat->phy_addr;
> 		...
>                 if (priv->dma_cap.eee)
>                         phy_support_eee(phydev);
> 
>                 ret = phylink_connect_phy(priv->phylink, phydev);
>         } else {
>                 fwnode_handle_put(phy_fwnode);
>                 ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
>         }
> 
> The driver only considers calling phy_support_eee() when DT fails to
> describe the PHY (because in the other path, it doesn't have access to
> the struct phy_device to make this call.)
> 
> My commit makes it apply even to DT described PHYs, so now (as has been
> shown when you enable EEE manually) it's uncovering latent problems.
> 
> So now we understand why the change has occurred - this is important.
Good. Thanks.> Now the question becomes, what to do about it.
> 
> For your issue, given that we have statements from TI that indicate
> none of their gigabit PHYs support EEE, we really should not be
> reporting to userspace that there is any EEE support. Therefore,
> "Supported EEE link modes" should be completely empty.
> 
> I think I understand why we're getting EEE modes supported. In the
> DP83867 manual, it states for the DEVAD field of the C45 indirect
> access registers:
> 
> "Device Address: In general, these bits [4:0] are the device address
> DEVAD that directs any accesses of ADDAR register (0x000E) to the
> appropriate MMD. Specifically, the DP83867 uses the vendor specific
> DEVAD [4:0] = 11111 for accesses. All accesses through registers
> REGCR and ADDAR can use this DEVAD. Transactions with other
> DEVAD are ignored."
> 
> Specifically, that last sentence, and the use of "ignored". If this
> means the PHY does not drive the MDIO data line when registers are
> read, they will return 0xffff, which is actually against the IEEE
> requirements for C45 registers (unimplemented C45 registers are
> supposed to be zero.)
> 
> So, this needs to be tested - please modify phylib's
> genphy_c45_read_eee_cap1() to print the value read from the register.
> 
> If it is 0xffff, that confirms that theory.
It’s not 0xffff; I verified that the value read is:
TI DP83867 stmmac-0:02: Reading EEE capabilities from MDIO_PCS_EEE_ABLE: 0x0006

This indicates that EEE is reported as supported, according to:
#define MDIO_AN_EEE_ADV_100TX	0x0002	/* Advertise 100TX EEE cap */
#define MDIO_AN_EEE_ADV_1000T	0x0004	/* Advertise 1000T EEE cap */

So the PHY simply reports the capability as present.

I verified this behaviour before submitting the patch, which is why I wrote:
"While the DP83867 PHYs report EEE capability through their feature registers."

Anyway, if the value were 0xffff, the code already handles it as not supported.

Let me know if I should test anything else.