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

On 24.2.2026 11:17, Russell King (Oracle) wrote:

On Tue, Feb 24, 2026 at 10:18:19AM +0100, Nicolai Buchwitz wrote:

quoted

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:

Thanks for the quick feedback! I will address this in a v3 of this 
series.

quoted

+static void macb_tx_lpi_set(struct macb *bp, bool enable)

bool return type.

quoted

+{
+	unsigned long flags;
+	u32 ncr;

	u32 old, ncr;

quoted

+
+	spin_lock_irqsave(&bp->lock, flags);
+	ncr = macb_readl(bp, NCR);

	old = ncr;

quoted

+	if (enable)
+		ncr |= GEM_BIT(TXLPIEN);
+	else
+		ncr &= ~GEM_BIT(TXLPIEN);

	if (old != ncr)

quoted

+	macb_writel(bp, NCR, ncr);
+	bp->tx_lpi_enabled = enable;

No need for tx_lpi_enabled

quoted

+	spin_unlock_irqrestore(&bp->lock, flags);

	return old != ncr;

quoted

+}

...

quoted

+/* 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().

quoted

+
+	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.

quoted

+	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?

AFAIK, the race shouldn't be an issue here because macb_tx_lpi_wake()
is called from macb_start_xmit() after the TX descriptors have been
written and tx_head has been advanced (there's a wmb() before the
call to ensure visibility). So if macb_tx_lpi_work_fn() happens to
be running concurrently, macb_tx_all_queues_idle() will see
tx_head != tx_tail and return false, preventing LPI from being
re-enabled.

Thanks.

Nicolai