Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Harini, Andrew,

On Wed Mar 26, 2025 at 6:01 AM CET, Katakam, Harini wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Theo,
>
>> -----Original Message-----
>> From: Andrew Lunn <andrew@lunn.ch>
>> Sent: Tuesday, March 25, 2025 12:06 AM
>> To: Théo Lebrun <theo.lebrun@bootlin.com>
>> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
>> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
>> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Rob Herring
>> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
>> <conor+dt@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>; Claudiu
>> Beznea <claudiu.beznea@tuxon.dev>; Paul Walmsley
>> <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>; Albert Ou
>> <aou@eecs.berkeley.edu>; Alexandre Ghiti <alex@ghiti.fr>; Samuel Holland
>> <samuel.holland@sifive.com>; Richard Cochran <richardcochran@gmail.com>;
>> Russell King <linux@armlinux.org.uk>; Thomas Bogendoerfer
>> <tsbogend@alpha.franken.de>; Vladimir Kondratiev
>> <vladimir.kondratiev@mobileye.com>; Gregory CLEMENT
>> <gregory.clement@bootlin.com>; netdev@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> riscv@lists.infradead.org; linux-mips@vger.kernel.org; Thomas Petazzoni
>> <thomas.petazzoni@bootlin.com>; Tawfik Bayouk <tawfik.bayouk@mobileye.com>
>> Subject: Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to
>> macb_config
>>
>> On Mon, Mar 24, 2025 at 06:49:05PM +0100, Théo Lebrun wrote:
>> > Hello Andrew,
>> >
>> > On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
>> > > On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
>> > >> The controller does IP alignment (two bytes).
>> > >
>> > > I'm a bit confused here. Is this hard coded, baked into the silicon?
>> > > It will always do IP alignment? It cannot be turned off?
>> >
>> > Yes, the alignment is baked inside the silicon.
>> > I looked but haven't seen any register to configure the alignment.
>> >
>> > Sorry the commit message isn't clear, it needs improvements.
>> >
>> > >>  skb_reserve(skb, NET_IP_ALIGN);
>> > >
>> > > Why not just replace this with
>> > >
>> > >         skb_reserve(skb, 2);
>> >
>> > On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current
>> > code is telling us that the silicon doesn't do alignment on those:
>>
>> This is part of the confusion. You say the hardware does alignment, and then say it
>> does not....
>>
>> >    skb = netdev_alloc_skb(...);
>> >    paddr = dma_map_single(..., skb->data, ...);
>> >    macb_set_addr(..., paddr);
>> >
>> >    // arm   => NET_IP_ALIGN=2 => silicon does alignment
>> >    // arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
>> >    skb_reserve(skb, NET_IP_ALIGN);
>> >
>> > The platform we introduce is the first one where the silicon alignment
>> > (0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).
>>
>> This is starting to make it clearer. So the first statement that the controller does IP
>> alignment (two bytes) is not the full story. I would start there, explain the full story,
>> otherwise readers get the wrong idea.
>>
>> > >>     Compatible             |  DTS folders              |  hw_ip_align
>> > >>    ------------------------|---------------------------|----------------
>> > >>    cdns,at91sam9260-macb   | arch/arm/                 | 2
>> > >>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
>> > >>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
>> > >>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
>> > >>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
>> > >>    cdns,sam9x60-macb       | arch/arm/                 | 2
>> > >>    atmel,sama5d2-gem       | arch/arm/                 | 2
>> > >>    atmel,sama5d29-gem      | arch/arm/                 | 2
>> > >>    atmel,sama5d3-gem       | arch/arm/                 | 2
>> > >>    atmel,sama5d3-macb      | arch/arm/                 | 2
>> > >>    atmel,sama5d4-gem       | arch/arm/                 | 2
>> > >>    cdns,at91rm9200-emac    | arch/arm/                 | 2
>> > >>    cdns,emac               | arch/arm/                 | 2
>> > >>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
>> > >>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
>> > >>    sifive,fu540-c000-gem   | arch/riscv/               | 2
>> > >>    microchip,mpfs-macb     | arch/riscv/               | 2
>> > >>    microchip,sama7g5-gem   | arch/arm/                 | 2
>> > >>    microchip,sama7g5-emac  | arch/arm/                 | 2
>> > >>    xlnx,zynqmp-gem         | arch/arm64/               | 0
>> > >>    xlnx,zynq-gem           | arch/arm/                 | 2
>> > >>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN
>
> Thanks for the patch. xlnx,versal-gem is arm64 and NET_IP_ALIGN is 0.
>
> AFAIK, IP alignment is controlled by the register field " receive buffer offset "
> in the NW config register. The only exception is when " gem_pbuf_rsc " i.e.
> receive coalescing is enabled in the RTL in the IP. In that case, the Cadenc
> specification states that these bits are ignored.
> So to summarize, if RSC is not enabled (see bit 26 of designcfg_debug6),
> then the current implementation works for all architectures i.e. these two
> statements are in sync:
> config |= MACB_BF(RBOF, NET_IP_ALIGN);  /* Make eth data aligned */
> skb_reserve(skb, NET_IP_ALIGN);
>
> Hope this helps simplify the patch (and also fill up the table that Andrew suggested)

Well, big thanks! That'll make the patch much simpler. Either EyeQ5
is the first compatible with RSC enabled, or others with RSC enabled
have NET_IP_ALIGN=0.

Below is what the patch could look like for V2.
 - We detect at probe if the HW is RSC-capable.
 - If it isn't, we keep the code the same.
 - If it is, that means the alignment feature isn't available.
   We can't respect our arch alignment request.

That removes all the macb_config->hw_ip_align mess. Much better.

Note: I tried checking if the RBOF field is read-only when the "receive
buffer offset" isn't available but it isn't.

-

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d8ee7878e144..478152f70563 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -525,6 +525,8 @@
 /* Bitfields in DCFG6. */
 #define GEM_PBUF_LSO_OFFSET        27
 #define GEM_PBUF_LSO_SIZE       1
+#define GEM_PBUF_RSC_OFFSET        26
+#define GEM_PBUF_RSC_SIZE       1
 #define GEM_PBUF_CUTTHRU_OFFSET       25
 #define GEM_PBUF_CUTTHRU_SIZE         1
 #define GEM_DAW64_OFFSET        23
@@ -736,6 +738,7 @@
 #define MACB_CAPS_NEED_TSUCLK         BIT(10)
 #define MACB_CAPS_QUEUE_DISABLE       BIT(11)
 #define MACB_CAPS_NO_LSO        BIT(12)
+#define MACB_CAPS_RSC_CAPABLE         BIT(13)
 #define MACB_CAPS_PCS           BIT(24)
 #define MACB_CAPS_HIGH_SPEED       BIT(25)
 #define MACB_CAPS_CLK_HW_CHG       BIT(26)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db8da8590fe0..51e82d66403b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1329,8 +1329,19 @@ static void gem_rx_refill(struct macb_queue *queue)
         dma_wmb();
         macb_set_addr(bp, desc, paddr);

-        /* Properly align Ethernet header. */
-        skb_reserve(skb, NET_IP_ALIGN);
+        /* Properly align Ethernet header.
+         *
+         * Hardware can add dummy bytes if asked using the RBOF
+         * field inside the NCFGR register. That feature isn't
+         * available if hardware is RSC capable.
+         *
+         * We cannot fallback to doing the 2-byte shift before
+         * DMA mapping because the address field does not allow
+         * setting the low 2/3 bits.
+         * It is 3 bits if HW_DMA_CAP_PTP.
+         */
+        if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
+           skb_reserve(skb, NET_IP_ALIGN);
      } else {
         desc->ctrl = 0;
         dma_wmb();
@@ -2788,7 +2799,9 @@ static void macb_init_hw(struct macb *bp)
   macb_set_hwaddr(bp);

   config = macb_mdc_clk_div(bp);
-  config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
+  /* Make eth data aligned. If RSC capable, that offset is ignored. */
+  if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
+     config |= MACB_BF(RBOF, NET_IP_ALIGN);
   config |= MACB_BIT(DRFCS);    /* Discard Rx FCS */
   if (bp->caps & MACB_CAPS_JUMBO)
      config |= MACB_BIT(JFRAME);   /* Enable jumbo frames */
@@ -4109,6 +4122,8 @@ static void macb_configure_caps(struct macb *bp,
      dcfg = gem_readl(bp, DCFG2);
      if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
         bp->caps |= MACB_CAPS_FIFO_MODE;
+     if (GEM_BFEXT(PBUF_RSC, gem_readl(bp, DCFG6)))
+        bp->caps |= MACB_CAPS_RSC_CAPABLE;
      if (gem_has_ptp(bp)) {
         if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
            dev_err(&bp->pdev->dev,

Thanks Andrew & Harini,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com