Date: Wed, 7 Jun 2023 09:45:58 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, o.rempel@...gutronix.de, andrew@...n.ch,
 hkallweit1@...il.com, Doug Berger <opendmb@...il.com>,
 Florian Fainelli <f.fainelli@...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>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: bcmgenet: Fix EEE implementation

On 6/7/23 01:46, Russell King (Oracle) wrote:
> On Tue, Jun 06, 2023 at 03:16:03PM -0700, Florian Fainelli wrote:
>> On 6/6/23 15:02, Russell King (Oracle) wrote:
>>> On Tue, Jun 06, 2023 at 02:43:47PM -0700, Florian Fainelli wrote:
>>>> We had a number of short comings:
>>>>
>>>> - EEE must be re-evaluated whenever the state machine detects a link
>>>>     change as wight be switching from a link partner with EEE
>>>>     enabled/disabled
>>>>
>>>> - tx_lpi_enabled controls whether EEE should be enabled/disabled for the
>>>>     transmit path, which applies to the TBUF block
>>>>
>>>> - We do not need to forcibly enable EEE upon system resume, as the PHY
>>>>     state machine will trigger a link event that will do that, too
>>>>
>>>> Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support")
>>>> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
>>>> ---
>>>> netdev maintainers, please do not apply without Andrew, Russell and
>>>> Oleksij reviewing first since this relates to the on-going EEE rework
>>>> from Andrew.
>>>
>>> Hi Florian,
>>>
>>> Please could you include some information on the UMAC_EEE_CTRL EEE_EN
>>> bit - is this like the main switch for EEE which needs to be set
>>> along with the bits in the tbuf register for the transmit side to
>>> signal LPI?
>>
>> EEE_EN is described as:
>>
>> If set, the TX LPI policy control engine is enabled and the MAC inserts
>> LPI_idle codes if the link is idle. The rx_lpi_detect assertion is
>> independent of this configuration.
>>
>> in the RBUF, EEE_EN is described as:
>>
>> 1: to enable Energy Efficient feature between Unimac and PHY for Rx Path
>>
>> and in the TBUF, EEE_EN is described as:
>>
>> 1: to enable Energy Efficient feature between Unimac and PHY for Tx Path
>>
>> The documentation is unfortunately scare about how these two signals connect
>> :/
> 
> Thanks for the clarification. Squaring this with my understanding of
> EEE, the transmit side makes sense. LPI on the transmit side is only
> asserted only when EEE_EN and TBUF_EEE_EN are both set, so this is
> the behaviour we want. If we were evaluating this in software, my
> understanding is it would be:
> 
> 	if (eee_enabled && eee_active && tx_lpi_enabled)
> 		enable LPI generation at MAC;
> 	else
> 		disable LPI generation at MAC;
> 
> and the code here treats eee_enabled && eee_active as the "enabled"
> flag controlling EEE_EN, and tx_lpi_enabled controls TBUF_EEE_EN.
> The hardware effectively does the last && operation for us. So
> this all seems fine.
> 
> On the receive side, if the link partner successfully negotiates
> EEE, then it can assert LPI, and the local end will see that
> assertion (hence, rx_lpi_detect may become true.) If the transmit
> side doesn't generate LPI, then this won't have any effect other
> than maybe setting status bits, so I don't see that setting
> RBUF_EEE_EN when eee_enabled && eee_active would be wrong.
> 
> Moving the phy_init_eee() (as it currently stands) into the adjust_link
> path is definitely the right thing, since it provides the resolution
> of the negotiated EEE state.
> 
> So, all round, I think your patch makes complete sense as far as the
> logic goes.
> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> 
> However, one thing I will ask is whether the hardware has any
> configuration of the various timers for EEE operation, and if it does,
> are they dependent on the negotiated speed of the interface? In
> Marvell's neta and pp2 drivers, the timers scale with link speed and
> thus need reprogramming accordingly. In any case, 802.3 specifies
> different timer settings depending on link speed and media type.

There are a couple of timers that are available:

- LPI timer (EEE_LPI_TIMER register)
- WAKE_TIMER (EEE_WAKE_TIMER register)

both are dependent upon the EEE_REF_COUNT register which is described as:

Clock divider for 1 us quanta count in EEE. This field controls clock 
divider used to generate ~1us reference pulses used by EEE timers.
It specifies integer number of system clock cycles contained within 1us.

the value is currently set to 0x7d (125) which does make some sense 
given that the system clock is considered stable and is provided at the 
same frequency irrespective of the link speed.

Hope this helps.
-- 
Florian


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

Powered by blists - more mailing lists