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,
quoted
-----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
[off-list ref]; Eric Dumazet [off-list ref]; Jakub Kicinski
[off-list ref]; Paolo Abeni [off-list ref]; Rob Herring
[off-list ref]; Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Nicolas Ferre [off-list ref]; Claudiu
Beznea [off-list ref]; Paul Walmsley
[off-list ref]; Palmer Dabbelt [off-list ref]; Albert Ou
[off-list ref]; Alexandre Ghiti [off-list ref]; Samuel Holland
[off-list ref]; Richard Cochran [off-list ref];
Russell King [off-list ref]; Thomas Bogendoerfer
[off-list ref]; Vladimir Kondratiev
[off-list ref]; Gregory CLEMENT
[off-list ref]; 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
[off-list ref]; Tawfik Bayouk [off-list ref]
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:
quoted
Hello Andrew,
On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
quoted
On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
quoted
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.
quoted
quoted
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....
quoted
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.
quoted
quoted
quoted
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