Re: [PATCH net-next v4 6/7] net: ftgmac100: Add AST2700 upper DMA address support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The descriptor struct comments are now wrong.

--

net: ftgmac100: Add AST2700 upper DMA address support

This patch programs the high RX and TX descriptor ring base registers on
AST2700. It also stores DMA address bits 34:32 in bits 18:16 of txdes2 and
rxdes2, and requests a 35-bit DMA mask. All of the extended address
handling is gated by a new FTGMAC100_QUIRK_DMA_35BIT match-data quirk.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
> index 8c35c895b1465..0c244b1c0a351 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.h
> +++ b/drivers/net/ethernet/faraday/ftgmac100.h

[ ... ]

> @@ -226,6 +229,7 @@ struct ftgmac100_txdes {
>  #define FTGMAC100_TXDES1_TX2FIC		(1 << 30)
>  #define FTGMAC100_TXDES1_TXIC		(1 << 31)
>  
> +#define FTGMAC100_TXDES2_TXBUF_BADR_HI	GENMASK(18, 16)

[Severity: Low]
This isn't a bug, but the descriptor struct comments just above this hunk
no longer match the new layout:

struct ftgmac100_txdes {
	__le32	txdes0; /* Control & status bits */
	__le32	txdes1; /* Irq, checksum and vlan control */
	__le32	txdes2; /* Reserved */
	__le32	txdes3; /* DMA buffer address */
} __attribute__ ((aligned(16)));

With FTGMAC100_QUIRK_DMA_35BIT set, ftgmac100_hard_start_xmit() now writes
buffer address bits 34:32 into txdes2 for both head and fragment
descriptors. ftgmac100_free_tx_packet() reads them back to rebuild the
unmap address, and txdes3 only holds the low 32 bits.

struct ftgmac100_rxdes has the same problem. rxdes3 is still described as
the full "DMA buffer address", and rxdes2 is only described as
"length/type on AST2500".

Could the comments for txdes2/txdes3 and rxdes2/rxdes3 be updated? It
might also help to note on the new GENMASK defines that they only apply
to the 35-bit (AST2700) layout. Otherwise a future descriptor writer or
reset path could treat txdes2 as reserved and drop the upper address
bits.

>  /*
>   * Receive descriptor, aligned to 16 bytes
>   */
> @@ -272,4 +276,6 @@ struct ftgmac100_rxdes {
>  #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR	(1 << 26)
>  #define FTGMAC100_RXDES1_IP_CHKSUM_ERR	(1 << 27)
>  
> +#define FTGMAC100_RXDES2_RXBUF_BADR_HI	GENMASK(18, 16)
> +
>  #endif /* __FTGMAC100_H */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-support_ast2700-v4-0-4d36b40939b3%40aspeedtech.com