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

Andrew Lunn
2025-03-24T18:36:59Z

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

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