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

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!