Re: [PATCH net-next v2 08/18] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)

[Sean Anderson <sean.anderson@seco.com>]

On 6/27/25 05:08, Théo Lebrun wrote:
> Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
> Many codepaths are made simpler by dropping conditional compilation.
> 
> This implies three changes:
>  - Always compile related structure definitions inside <macb.h>.
>  - Make the field hw_dma_cap in struct macb always present.
>  - MACB_EXT_DESC can be dropped as it is useless now.
> 
> The common case is:
> 
> 	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> 		struct macb_dma_desc_64 *desc_64;
> 		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> 			desc_64 = macb_64b_desc(bp, desc);
> 			// ...
> 		}
> 	#endif
> 
> And replaced by:
> 
> 	struct macb_dma_desc_64 *desc_64;
> 	if (macb_dma_is_64b(bp)) {
> 		desc_64 = macb_64b_desc(bp, desc);
> 		// ...
> 	}
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |   8 ---
>  drivers/net/ethernet/cadence/macb_main.c | 113 +++++++++++--------------------
>  2 files changed, 40 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3b43cb9468e3618754ff2bc6c5f360447bdeeed0..707b3286a6b8408a3bc4bbbcb1335ae8c3cd95ad 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -15,10 +15,6 @@
>  #include <linux/phy/phy.h>
>  #include <linux/workqueue.h>
>  
> -#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
> -#define MACB_EXT_DESC
> -#endif
> -
>  #define MACB_GREGS_NBR 16
>  #define MACB_GREGS_VERSION 2
>  #define MACB_MAX_QUEUES 8
> @@ -823,7 +819,6 @@ struct macb_dma_desc {
>  	u32	ctrl;
>  };
>  
> -#ifdef MACB_EXT_DESC
>  #define HW_DMA_CAP_32B		0
>  #define HW_DMA_CAP_64B		(1 << 0)
>  #define HW_DMA_CAP_PTP		(1 << 1)
> @@ -838,7 +833,6 @@ struct macb_dma_desc_ptp {
>  	u32	ts_1;
>  	u32	ts_2;
>  };
> -#endif
>  
>  /* DMA descriptor bitfields */
>  #define MACB_RX_USED_OFFSET			0
> @@ -1316,9 +1310,7 @@ struct macb {
>  
>  	struct phy		*sgmii_phy;	/* for ZynqMP SGMII mode */
>  
> -#ifdef MACB_EXT_DESC
>  	uint8_t hw_dma_cap;
> -#endif

Should this member really be here? Seems like it would eat at least
7 bytes of padding. It probably makes sense to move this somewhere else
in the struct. Or maybe even fold this into MACB_CAPS? Not sure what
that does to text size.

>  	spinlock_t tsu_clk_lock; /* gem tsu clock locking */
>  	unsigned int tsu_rate;
>  	struct ptp_clock *ptp_clock;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6926ea11d71adae7a25d5ee329c148f5882d4184..be667cb5acce85a9d11aaea1f5081a3380b60ef2 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -95,6 +95,18 @@ struct sifive_fu540_macb_mgmt {
>  
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
> +static bool macb_dma_is_64b(struct macb *bp)

TBH I would prefer just `macb_dma64` and `macb_dma_ptp` since the "_is_"
isn't really adding much.

> +{
> +	return IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> +	       bp->hw_dma_cap & HW_DMA_CAP_64B;
> +}
> +
> +static bool macb_dma_is_ptp(struct macb *bp)
> +{
> +	return IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) &&
> +	       bp->hw_dma_cap & HW_DMA_CAP_PTP;
> +}
> +
>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -124,56 +136,31 @@ struct sifive_fu540_macb_mgmt {
>   */
>  static unsigned int macb_dma_desc_get_size(struct macb *bp)
>  {
> -#ifdef MACB_EXT_DESC
> -	unsigned int desc_size;
> +	unsigned int desc_size = sizeof(struct macb_dma_desc);
> +
> +	if (macb_dma_is_64b(bp))
> +		desc_size += sizeof(struct macb_dma_desc_64);
> +	if (macb_dma_is_ptp(bp))
> +		desc_size += sizeof(struct macb_dma_desc_ptp);
>  
> -	switch (bp->hw_dma_cap) {
> -	case HW_DMA_CAP_64B:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_64);
> -		break;
> -	case HW_DMA_CAP_PTP:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_ptp);
> -		break;
> -	case HW_DMA_CAP_64B_PTP:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_64)
> -			+ sizeof(struct macb_dma_desc_ptp);
> -		break;
> -	default:
> -		desc_size = sizeof(struct macb_dma_desc);
> -	}
>  	return desc_size;
> -#endif
> -	return sizeof(struct macb_dma_desc);
>  }
>  
>  static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int desc_idx)
>  {
> -#ifdef MACB_EXT_DESC
> -	switch (bp->hw_dma_cap) {
> -	case HW_DMA_CAP_64B:
> -	case HW_DMA_CAP_PTP:
> -		desc_idx <<= 1;
> -		break;
> -	case HW_DMA_CAP_64B_PTP:
> -		desc_idx *= 3;
> -		break;
> -	default:
> -		break;
> -	}
> -#endif
> -	return desc_idx;
> +	if (macb_dma_is_64b(bp) && macb_dma_is_ptp(bp))
> +		return desc_idx * 3;
> +	else if (macb_dma_is_64b(bp) || macb_dma_is_ptp(bp))
> +		return desc_idx << 1;
> +	else
> +		return desc_idx;

Can this be

return desc_idx * (1 + 64() + ptp())

? Not sure what the assembly looks like.

>  }
>  
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
>  {
>  	return (struct macb_dma_desc_64 *)((void *)desc
>  		+ sizeof(struct macb_dma_desc));
>  }
> -#endif
>  
>  /* Ring buffer accessors */
>  static unsigned int macb_tx_ring_wrap(struct macb *bp, unsigned int index)
> @@ -497,17 +484,13 @@ static void macb_init_buffers(struct macb *bp)
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  		queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			queue_writel(queue, RBQPH,
>  				     upper_32_bits(queue->rx_ring_dma));
> -#endif
>  		queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			queue_writel(queue, TBQPH,
>  				     upper_32_bits(queue->tx_ring_dma));
> -#endif
>  	}
>  }
>  
> @@ -1028,10 +1011,9 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budge
>  
>  static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_t addr)
>  {
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -	struct macb_dma_desc_64 *desc_64;
> +	if (macb_dma_is_64b(bp)) {
> +		struct macb_dma_desc_64 *desc_64;
>  
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>  		desc_64 = macb_64b_desc(bp, desc);
>  		desc_64->addrh = upper_32_bits(addr);
>  		/* The low bits of RX address contain the RX_USED bit, clearing
> @@ -1040,26 +1022,23 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
>  		 */
>  		dma_wmb();
>  	}
> -#endif
> +
>  	desc->addr = lower_32_bits(addr);
>  }
>  
>  static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc)
>  {
>  	dma_addr_t addr = 0;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -	struct macb_dma_desc_64 *desc_64;
>  
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +	if (macb_dma_is_64b(bp)) {
> +		struct macb_dma_desc_64 *desc_64;
> +
>  		desc_64 = macb_64b_desc(bp, desc);
>  		addr = ((u64)(desc_64->addrh) << 32);
>  	}
> -#endif
>  	addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -	if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> +	if (macb_dma_is_ptp(bp))
>  		addr &= ~GEM_BIT(DMA_RXVALID);
> -#endif
>  	return addr;
>  }
>  
> @@ -1166,10 +1145,8 @@ static void macb_tx_error_task(struct work_struct *work)
>  
>  	/* Reinitialize the TX desc queue */
>  	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +	if (macb_dma_is_64b(bp))
>  		queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
> -#endif
>  	/* Make TX ring reflect state of hardware */
>  	queue->tx_head = 0;
>  	queue->tx_tail = 0;
> @@ -2321,11 +2298,9 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		return ret;
>  	}
>  
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> -	    (bp->hw_dma_cap & HW_DMA_CAP_PTP))
> +	if (macb_dma_is_ptp(bp) &&
> +	    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> -#endif
>  
>  	is_lso = (skb_shinfo(skb)->gso_size != 0);
>  
> @@ -2785,14 +2760,10 @@ static void macb_configure_dma(struct macb *bp)
>  			dmacfg &= ~GEM_BIT(TXCOEN);
>  
>  		dmacfg &= ~GEM_BIT(ADDR64);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			dmacfg |= GEM_BIT(ADDR64);
> -#endif
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -		if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> +		if (macb_dma_is_ptp(bp))
>  			dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
> -#endif
>  		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
>  			   dmacfg);
>  		gem_writel(bp, DMACFG, dmacfg);
> @@ -4298,12 +4269,10 @@ static int macb_init(struct platform_device *pdev)
>  			queue->TBQP = GEM_TBQP(hw_q - 1);
>  			queue->RBQP = GEM_RBQP(hw_q - 1);
>  			queue->RBQS = GEM_RBQS(hw_q - 1);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +			if (macb_dma_is_64b(bp)) {
>  				queue->TBQPH = GEM_TBQPH(hw_q - 1);
>  				queue->RBQPH = GEM_RBQPH(hw_q - 1);
>  			}
> -#endif
>  		} else {
>  			/* queue0 uses legacy registers */
>  			queue->ISR  = MACB_ISR;
> @@ -4312,12 +4281,10 @@ static int macb_init(struct platform_device *pdev)
>  			queue->IMR  = MACB_IMR;
>  			queue->TBQP = MACB_TBQP;
>  			queue->RBQP = MACB_RBQP;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +			if (macb_dma_is_64b(bp)) {
>  				queue->TBQPH = MACB_TBQPH;
>  				queue->RBQPH = MACB_RBQPH;
>  			}
> -#endif
>  		}
>  
>  		/* get irq: here we use the linux queue index, not the hardware
>