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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

On 2/28/26 15:33, Claudiu Beznea wrote:
> 
> 
> On 2/26/26 11: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@bootlin.com>
>> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
>> ---
>>   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 43cd013bb70e..6b9c63195970 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>
>> @@ -589,6 +590,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) {
>          // ...
>      }
> 

Replied to the wrong version. Please ignore.

> Apart from that:
> 
> Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> 
> Thank you,
> Claudiu