Re: [PATCH net-next v2 3/5] net: cadence: macb: implement EEE TX LPI support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!