Message-ID: <cb4695ba-8107-4810-b859-da102dd2bfba@broadcom.com>
Date: Thu, 11 Apr 2024 10:04:13 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrew Lunn <andrew@...n.ch>, Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Doug Berger <opendmb@...il.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: genet: Fixup EEE

On 4/11/24 07:35, Andrew Lunn wrote:
> On Thu, Apr 11, 2024 at 07:17:36AM +0200, Oleksij Rempel wrote:
>> Hi Florian,
>>
>> On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote:
>>
>>> I am seeing a functional difference with and without your patch however, and
>>> also, there appears to be something wrong within the bcmgenet driver after
>>> PHYLIB having absorbed the EEE configuration. Both cases we start on boot
>>> with:
>>>
>>> # ethtool --show-eee eth0
>>> EEE settings for eth0:
>>>          EEE status: disabled
>>>          Tx LPI: disabled
>>>          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 would expect the EEE status to be enabled, that's how I remember it
>>> before.
>>
>> Yes, current default kernel implementation is to use EEE if available.
> 
> We do however seem to be inconsistent in this example. EEE seems to be
> disabled, yet it is advertising? Or is it showing what we would
> advertise, when EEE is enabled?

What I consider to be the "canonical" behavior is the following on boot:

# ethtool --show-eee eth0
EEE settings for eth0:
         EEE status: enabled - active
         Tx LPI: disabled
         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

whereby we advertise EEE, the link partner does too, and the adjust_link 
callback determined that we could EEE as a result via phy_init_eee(). 
This is seen on 6.8.

Starting with 6.9-rc and Andrew's series to rework EEE, I have the 
behavior provided before:

# ethtool --show-eee eth0
EEE settings for eth0:
         EEE status: disabled
         Tx LPI: disabled
         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

whereby we need user intervention to opt-in and have EEE enabled with:

ethtool --set-eee eth0 eee on

This presents users with a difference in behavior which we might get 
regression reports for.

> 
>>> Now, with your patch, once I turn on EEE with:
>>>
>>> # ethtool --set-eee eth0 eee on
>>> # ethtool --show-eee eth0
>>> EEE settings for eth0:
>>>          EEE status: enabled - active
>>>          Tx LPI: disabled
>>>          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
>>> #
>>>
>>> there is no change to the EEE_CTRL register to set the EEE_EN, this only
>>> happens when doing:
>>>
>>> # ethtool --set-eee eth0 eee on tx-lpi on
>>>
>>> which is consistent with the patch, but I don't think this is quite correct
>>> as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi
>>> on" meant enable EEE for the TX path?
>>
>> Yes. More precisely, with "eee on" we allow the PHY to advertise EEE
>> link modes. On link_up, if both sides are agreed to use EEE, MAC is
>> configured to process LPI opcodes from the PHY and send LPI opcodes to
>> the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in
>> case of "eee off".
> 
> Florian seems to be suggesting the RX and TX path could have different
> configurations? RX EEE could be enabled but TX EEE disabled? That i
> don't understand, in terms of auto-neg. auto-neg is for the link as a
> whole, it does not appear to allow different results for each
> direction. Does tx-lpi only make sense when EEE is forced, not
> auto-neg'ed?

To me the 'tx-lpi' parameter allows for an additional level of local 
control of whether TX path should sent LPI or not, irrespective of 
forced versus auto-negotiated EEE capability.

I am not sure why the API was defined like it was in the first place and 
what was the rationale for offering a separate 'tx-lpi', this might have 
been based upon a real or hypothetical use case.

If we are to honor the separate controls, we would have to agree on 
their meaning and we had discussed this before with Oleksij.

EEE_EN means that the Ethernet MAC (called UNIMAC in this block) enables 
clock gating signaling from the PHY up to DMA interface, this is how the 
power savings are actually realized on the digital logic side.
-- 
Florian


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists