Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 09:46:36 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Florian Fainelli <florian.fainelli@broadcom.com>
Cc:     netdev@vger.kernel.org, o.rempel@pengutronix.de, andrew@lunn.ch,
        hkallweit1@gmail.com, Doug Berger <opendmb@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net] net: bcmgenet: Fix EEE implementation
Message-ID: <ZIBD7B8cwSjjFWuq@shell.armlinux.org.uk>
References: <20230606214348.2408018-1-florian.fainelli@broadcom.com>
 <ZH+tAKg4hqlosb2N@shell.armlinux.org.uk>
 <1074c668-3e75-dff7-9d23-d43fbeb98d84@broadcom.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1074c668-3e75-dff7-9d23-d43fbeb98d84@broadcom.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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@broadcom.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@armlinux.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.

Thanks!

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