From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [78.32.30.218]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3D72D37757D for ; Tue, 24 Feb 2026 10:17:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=78.32.30.218 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771928243; cv=none; b=NHajrpbmGC+7Wb2Qon5tA+ijMIYSx8Ud1V4VSOmiq9EAR44OEnVVPVrVOMN6V9UP5TJ3j9ztI3s2GUn84nODN1LzQafA3XXfIboh6Lnvcx+c8OC5Jnx8DR4P1MLY5tIhiBraAcD+xbeTOg1DRStulX2+cDPpeXh9A5ueDsXPwdk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771928243; c=relaxed/simple; bh=rFF4K8Zgf1BYVWvYDIyd09wkETxX/B9s2eyJGQxFkWw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=t6BhcBtzGK5DnMG+4QT2d3CcdrmsfODyMsTjwKuqyo37bd9U0rWHy6XlI5tJ3/9S6iuyxRSWh/bZps/WMT/1RISC6BE8FdgQEPyRBBPfhpS3XH7pR4TmcIM8T3GyogZy+ANXyYed5g5bkMEWyhuQ1LSR+UMY2SRsI/r+arJ5+v4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=armlinux.org.uk; spf=none smtp.mailfrom=armlinux.org.uk; dkim=pass (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b=b48dGVhM; arc=none smtp.client-ip=78.32.30.218 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=armlinux.org.uk Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=armlinux.org.uk Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b="b48dGVhM" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=4YULu+uW6OF//44jjpCFpruF8dDEiT3NVbgUKXDQoeo=; b=b48dGVhMPjMoM08ZEEUU/QM7fE C8DARAtpoZNoXE/PzyN0mcJUmKJipxT2Gmo6ObVnTUmby8DwSObRPAZVcFN4w/9bAWjKeTmnwA5st AUc7/QUNaf9FUtVYlvkcSr5mJbtR6pLRRZ2UadZWkn+qhy7ymyEaZzMHK+KDqBHdRiC97YAn5KcWW YOdQ5IU13i85wdF07/d6skemAdQbJwzJuqoyEB8lmqKq8ZJx9NZAO2snSHrh3dDA3xmZt2+QUio0N 4dedSkl20KMRCguCC287I2r21si+Z3sppmdq4vvUHTFtXc0Bmtczn5o8BEuGOYMynEm12xW5I81mm qd+GeF9Q==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:55642) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1vupTh-0000000055S-0ACt; Tue, 24 Feb 2026 10:17:17 +0000 Received: from linux by shell.armlinux.org.uk with local (Exim 4.98.2) (envelope-from ) id 1vupTe-000000008I5-3sCB; Tue, 24 Feb 2026 10:17:14 +0000 Date: Tue, 24 Feb 2026 10:17:14 +0000 From: "Russell King (Oracle)" To: Nicolai Buchwitz Cc: netdev@vger.kernel.org, andrew+netdev@lunn.ch, claudiu.beznea@tuxon.dev, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, nicolas.ferre@microchip.com, pabeni@redhat.com, phil@raspberrypi.com Subject: Re: [PATCH net-next v2 3/5] net: cadence: macb: implement EEE TX LPI support Message-ID: References: <20260224091821.47671-1-nb@tipi-net.de> <20260224091821.47671-4-nb@tipi-net.de> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260224091821.47671-4-nb@tipi-net.de> Sender: Russell King (Oracle) On Tue, Feb 24, 2026 at 10:18:19AM +0100, Nicolai Buchwitz wrote: > Implement Energy Efficient Ethernet TX Low Power Idle using phylink's > managed EEE framework. The Cadence GEM MAC has no built-in idle timer > - TXLPIEN (NCR bit 19) immediately blocks all TX when set and the MAC > does NOT auto-wake - so the driver uses a software delayed_work timer > for idle detection. > > The TX LPI lifecycle: > - phylink calls mac_enable_tx_lpi() after link-up with the negotiated > timer value. The driver defers the first LPI entry by 1 second per > IEEE 802.3az section 22.7a. > - macb_tx_complete() reschedules the idle timer after each TX drain. > - macb_start_xmit() wakes from LPI by clearing TXLPIEN, cancelling > the pending work, and waiting 50us (conservative Tw_sys) before > initiating the transmit. > - phylink calls mac_disable_tx_lpi() before link-down, which cancels > the work and clears TXLPIEN. > > The phylink_config is populated with LPI capabilities (MII, GMII, RGMII > modes; 100FD and 1000FD speeds) and a 250ms default idle timer, gated > on MACB_CAPS_EEE. Much better, thanks. One suggestion: > +static void macb_tx_lpi_set(struct macb *bp, bool enable) bool return type. > +{ > + unsigned long flags; > + u32 ncr; u32 old, ncr; > + > + spin_lock_irqsave(&bp->lock, flags); > + ncr = macb_readl(bp, NCR); old = ncr; > + if (enable) > + ncr |= GEM_BIT(TXLPIEN); > + else > + ncr &= ~GEM_BIT(TXLPIEN); if (old != ncr) > + macb_writel(bp, NCR, ncr); > + bp->tx_lpi_enabled = enable; No need for tx_lpi_enabled > + spin_unlock_irqrestore(&bp->lock, flags); return old != ncr; > +} ... > +/* Wake from LPI before transmitting. The MAC must deassert TXLPIEN > + * and wait for the PHY to exit LPI before any frame can be sent. > + * IEEE 802.3az Tw_sys is ~17us for 1000BASE-T, ~30us for 100BASE-TX; > + * we use a conservative 50us. > + */ > +static void macb_tx_lpi_wake(struct macb *bp) > +{ > + if (!bp->tx_lpi_enabled) > + return; No need for this if(). > + > + macb_tx_lpi_set(bp, false); instead: if (!macb_tx_lpi_set(bp, false)) return; The presence of the spinlock in macb_tx_lpi_set() suggests that there could be races, so keeping the state completely inside the spinlock region would be sensible. > + cancel_delayed_work(&bp->tx_lpi_work); I wonder whether this is reliable on its own. cancel_delayed_work() documentation says: * Note: * The work callback function may still be running on return, unless * it returns %true and the work doesn't re-arm itself. Explicitly flush or * use cancel_delayed_work_sync() to wait on it. That means macb_tx_lpi_work_fn() could have just been entered at the point that cancel_delayed_work() has been called. Would that cause a problem, e.g. setting LPI mode again, or would we be guaranteed that macb_tx_all_queues_idle() returns false preventing LPI mode being set again? Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!