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

[Claudiu Beznea]

On 2/27/26 17:06, Nicolai Buchwitz wrote:
> The GEM MAC has hardware LPI registers (NCR bit 19: TXLPIEN) but no
> built-in idle timer, so asserting TXLPIEN blocks all TX immediately
> with no automatic wake. A software idle timer is required, as noted
> in Microchip documentation (section 40.6.19): "It is best to use
> firmware to control LPI."
>
> Implement phylink managed EEE using the mac_enable_tx_lpi and
> mac_disable_tx_lpi callbacks:
>
> - macb_tx_lpi_set(): atomically sets or clears TXLPIEN under the
>    existing bp->lock spinlock; returns bool indicating whether the
>    register actually changed, avoiding redundant writes.
>
> - macb_tx_lpi_work_fn(): delayed_work handler that enters LPI if all
>    TX queues are idle and EEE is still active.
>
> - macb_tx_lpi_schedule(): arms the work timer using the LPI timer
>    value provided by phylink (default 250 ms). Called from
>    macb_tx_complete() after each TX drain so the idle countdown
>    restarts whenever the ring goes quiet.
>
> - macb_tx_lpi_wake(): called from macb_start_xmit() before TSTART.
>    Clears TXLPIEN and applies a 50 us udelay for PHY wake (IEEE
>    802.3az Tw_sys_tx is 16.5 us for 1000BASE-T / 30 us for
>    100BASE-TX; GEM has no hardware enforcement). Only delays when
>    TXLPIEN was actually set, avoiding overhead on the common path.
>    The delay is placed after tx_head is advanced so the work_fn's
>    queue-idle check sees a non-empty ring and cannot race back into
>    LPI before the frame is transmitted.
>
> - mac_enable_tx_lpi: stores the timer and sets eee_active, then
>    defers the first LPI entry by 1 second per IEEE 802.3az section
>    22.7a.
>
> - mac_disable_tx_lpi: clears eee_active, cancels the work, and
>    deasserts TXLPIEN.
>
> Populate phylink_config lpi_interfaces (MII, GMII, RGMII variants)
> and lpi_capabilities (MAC_100FD | MAC_1000FD) so phylink can
> negotiate EEE with the PHY and call the callbacks appropriately.
> Set lpi_timer_default to 250000 us and eee_enabled_default to true.
>
> Reviewed-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> Signed-off-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>
> ---
>   drivers/net/ethernet/cadence/macb.h      |   8 ++
>   drivers/net/ethernet/cadence/macb_main.c | 112 +++++++++++++++++++++++
>   2 files changed, 120 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 19aa98d01c8c..c69828b27dae 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -309,6 +309,8 @@
>   #define MACB_IRXFCS_SIZE	1
>   
>   /* GEM specific NCR bitfields. */
> +#define GEM_TXLPIEN_OFFSET		19
> +#define GEM_TXLPIEN_SIZE		1
>   #define GEM_ENABLE_HS_MAC_OFFSET	31
>   #define GEM_ENABLE_HS_MAC_SIZE		1
>   
> @@ -783,6 +785,7 @@
>   #define MACB_CAPS_DMA_PTP			BIT(22)
>   #define MACB_CAPS_RSC				BIT(23)
>   #define MACB_CAPS_NO_LSO			BIT(24)
> +#define MACB_CAPS_EEE				BIT(25)
>   
>   /* LSO settings */
>   #define MACB_LSO_UFO_ENABLE			0x01
> @@ -1369,6 +1372,11 @@ struct macb {
>   
>   	struct work_struct	hresp_err_bh_work;
>   
> +	/* EEE / LPI state */
> +	bool			eee_active;
> +	struct delayed_work	tx_lpi_work;
> +	u32			tx_lpi_timer;
> +
>   	int	rx_bd_rd_prefetch;
>   	int	tx_bd_rd_prefetch;
>   
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 02eab26fd98b..c23485f049d3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -10,6 +10,7 @@
>   #include <linux/clk-provider.h>
>   #include <linux/clk.h>
>   #include <linux/crc32.h>
> +#include <linux/delay.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/etherdevice.h>
>   #include <linux/firmware/xlnx-zynqmp.h>
> @@ -621,6 +622,94 @@ static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>   	.pcs_config = macb_pcs_config,
>   };
>   
> +static bool macb_tx_lpi_set(struct macb *bp, bool enable)
> +{
> +	unsigned long flags;
> +	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);
> +	spin_unlock_irqrestore(&bp->lock, flags);
> +
> +	return old != ncr;
> +}
> +
> +static bool macb_tx_all_queues_idle(struct macb *bp)
> +{
> +	unsigned int q;
> +
> +	for (q = 0; q < bp->num_queues; q++) {
> +		struct macb_queue *queue = &bp->queues[q];

In case there will be another version, to have a unified approach across the 
driver, this loop can be written as all of the loops on queues in this driver:

    for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
        // ...
    }

Apart from that:

Reviewed-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>

Thank you,
Claudiu