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

[Katakam, Harini]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Theo,

> -----Original Message-----
> From: Andrew Lunn <andrew at lunn.ch>
> Sent: Tuesday, March 25, 2025 12:06 AM
> To: Théo Lebrun <theo.lebrun at bootlin.com>
> Cc: Andrew Lunn <andrew+netdev at lunn.ch>; David S. Miller
> <davem at davemloft.net>; Eric Dumazet <edumazet at google.com>; Jakub Kicinski
> <kuba at kernel.org>; Paolo Abeni <pabeni at redhat.com>; Rob Herring
> <robh at kernel.org>; Krzysztof Kozlowski <krzk+dt at kernel.org>; Conor Dooley
> <conor+dt at kernel.org>; Nicolas Ferre <nicolas.ferre at microchip.com>; Claudiu
> Beznea <claudiu.beznea at tuxon.dev>; Paul Walmsley
> <paul.walmsley at sifive.com>; Palmer Dabbelt <palmer at dabbelt.com>; Albert Ou
> <aou at eecs.berkeley.edu>; Alexandre Ghiti <alex at ghiti.fr>; Samuel Holland
> <samuel.holland at sifive.com>; Richard Cochran <richardcochran at gmail.com>;
> Russell King <linux at armlinux.org.uk>; Thomas Bogendoerfer
> <tsbogend at alpha.franken.de>; Vladimir Kondratiev
> <vladimir.kondratiev at mobileye.com>; Gregory CLEMENT
> <gregory.clement at bootlin.com>; netdev at vger.kernel.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
> riscv at lists.infradead.org; linux-mips at vger.kernel.org; Thomas Petazzoni
> <thomas.petazzoni at bootlin.com>; Tawfik Bayouk <tawfik.bayouk at 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)

Regards,
Harini

>
> I'm not sure this table is useful. What might be more interesting is
>
>      Compatible             |  architecture |  hw_ip_align | value of NET_IP_ALIGN
>
> We can then see if there are cases when the 3rd and 4th column differ.
>
>       Andrew