[net-next 0/6] ravb: Align Rx descriptor setup and maintenance

[net-next 0/6] ravb: Align Rx descriptor setup and maintenance

[Niklas Söderlund]

Hello,

When RZ/G2L support was added the Rx code path was split in two, one to 
support R-Car and one to support RZ/G2L. One reason for this is that 
R-Car uses the extended Rx descriptor format, while RZ/G2L uses the 
normal descriptor format.

In many aspects this is not needed as the extended descriptor format is 
just a normal descriptor with extra metadata (timestamsp) appended. And 
the R-Car SoCs can also use normal descriptors if hardware timestamps 
where not desired. This split has lead to RZ/G2L gaining support for 
split descriptors in the Rx path while R-Car still lacks this.

This series is the first step in trying to merge the R-Car and RZ/G2L Rx 
paths so features and bugs corrected in one will benefit the other.

The first patch in the series clarify that the driver now supports 
either normal or extended descriptors, not both at the same time by 
grouping them in a union. This is the foundation that later patches will 
build on the align the two Rx paths.

Patch 2-5 deals with correcting small issues in the Rx frame and 
descriptor sizes that either where incorrect at the time they were added 
in 2017 (my bad) or concepts built on-top of this initial incorrect 
design.

While finally patch 6 merges the R-Car and RZ/G2L for Rx descriptor 
setup and maintenance.

When this work has landed I plan to follow up with more work aligning 
the rest of the Rx code paths and hopefully bring split descriptor 
support to the R-Car SoCs.

Niklas Söderlund (6):
  ravb: Group descriptor types used in Rx ring
  ravb: Make it clear the information relates to maximum frame size
  ravb: Create helper to allocate skb and align it
  ravb: Use the max frame size from hardware info for RZ/G2L
  ravb: Move maximum Rx descriptor data usage to info struct
  ravb: Unify Rx ring maintenance code paths

 drivers/net/ethernet/renesas/ravb.h      |  20 +--
 drivers/net/ethernet/renesas/ravb_main.c | 205 ++++++++---------------
 2 files changed, 79 insertions(+), 146 deletions(-)

-- 
2.43.2

[net-next 1/6] ravb: Group descriptor types used in Rx ring

[Niklas Söderlund]

The Rx ring can either be made up of normal or extended descriptors, not
a mix of the two at the same time. Make this explicitly by grouping the
two variables in a rx_ring union.

The extension of the storage for more than one queue of normal
descriptors from a single to NUM_RX_QUEUE queues have no practical
effect. But aids in making the code readable as the code that uses it
already piggyback on other members of struct ravb_private that are
arrays of max length NUM_RX_QUEUE, e.g. rx_desc_dma. This will also make
further refactoring easier.

While at it rename the normal descriptor Rx ring to make it clear it's
not strictly related to the GbEthernet E-MAC IP found in RZ/G2L, normal
descriptors could be used on R-Car SoCs too.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      |  6 ++-
 drivers/net/ethernet/renesas/ravb_main.c | 57 ++++++++++++------------
 2 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 35e642fc4b2a..aecc98282c7e 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1092,8 +1092,10 @@ struct ravb_private {
 	struct ravb_desc *desc_bat;
 	dma_addr_t rx_desc_dma[NUM_RX_QUEUE];
 	dma_addr_t tx_desc_dma[NUM_TX_QUEUE];
-	struct ravb_rx_desc *gbeth_rx_ring;
-	struct ravb_ex_rx_desc *rx_ring[NUM_RX_QUEUE];
+	union {
+		struct ravb_rx_desc *desc;
+		struct ravb_ex_rx_desc *ex_desc;
+	} rx_ring[NUM_RX_QUEUE];
 	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
 	void *tx_align[NUM_TX_QUEUE];
 	struct sk_buff *rx_1st_skb;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index f9fb772b05c7..c25a80f4d3b9 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -241,11 +241,11 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 	unsigned int ring_size;
 	unsigned int i;
 
-	if (!priv->gbeth_rx_ring)
+	if (!priv->rx_ring[q].desc)
 		return;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
+		struct ravb_rx_desc *desc = &priv->rx_ring[q].desc[i];
 
 		if (!dma_mapping_error(ndev->dev.parent,
 				       le32_to_cpu(desc->dptr)))
@@ -255,9 +255,9 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 					 DMA_FROM_DEVICE);
 	}
 	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
-	dma_free_coherent(ndev->dev.parent, ring_size, priv->gbeth_rx_ring,
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].desc,
 			  priv->rx_desc_dma[q]);
-	priv->gbeth_rx_ring = NULL;
+	priv->rx_ring[q].desc = NULL;
 }
 
 static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
@@ -266,11 +266,11 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
 	unsigned int ring_size;
 	unsigned int i;
 
-	if (!priv->rx_ring[q])
+	if (!priv->rx_ring[q].ex_desc)
 		return;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
+		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q].ex_desc[i];
 
 		if (!dma_mapping_error(ndev->dev.parent,
 				       le32_to_cpu(desc->dptr)))
@@ -281,9 +281,9 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
 	}
 	ring_size = sizeof(struct ravb_ex_rx_desc) *
 		    (priv->num_rx_ring[q] + 1);
-	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].ex_desc,
 			  priv->rx_desc_dma[q]);
-	priv->rx_ring[q] = NULL;
+	priv->rx_ring[q].ex_desc = NULL;
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
@@ -335,11 +335,11 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 	unsigned int i;
 
 	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
-	memset(priv->gbeth_rx_ring, 0, rx_ring_size);
+	memset(priv->rx_ring[q].desc, 0, rx_ring_size);
 	/* Build RX ring buffer */
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
-		rx_desc = &priv->gbeth_rx_ring[i];
+		rx_desc = &priv->rx_ring[q].desc[i];
 		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  GBETH_RX_BUFF_MAX,
@@ -352,7 +352,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 		rx_desc->dptr = cpu_to_le32(dma_addr);
 		rx_desc->die_dt = DT_FEMPTY;
 	}
-	rx_desc = &priv->gbeth_rx_ring[i];
+	rx_desc = &priv->rx_ring[q].desc[i];
 	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
 	rx_desc->die_dt = DT_LINKFIX; /* type */
 }
@@ -365,11 +365,11 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 	dma_addr_t dma_addr;
 	unsigned int i;
 
-	memset(priv->rx_ring[q], 0, rx_ring_size);
+	memset(priv->rx_ring[q].ex_desc, 0, rx_ring_size);
 	/* Build RX ring buffer */
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
-		rx_desc = &priv->rx_ring[q][i];
+		rx_desc = &priv->rx_ring[q].ex_desc[i];
 		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  RX_BUF_SZ,
@@ -382,7 +382,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 		rx_desc->dptr = cpu_to_le32(dma_addr);
 		rx_desc->die_dt = DT_FEMPTY;
 	}
-	rx_desc = &priv->rx_ring[q][i];
+	rx_desc = &priv->rx_ring[q].ex_desc[i];
 	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
 	rx_desc->die_dt = DT_LINKFIX; /* type */
 }
@@ -437,10 +437,10 @@ static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
 
 	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
 
-	priv->gbeth_rx_ring = dma_alloc_coherent(ndev->dev.parent, ring_size,
-						 &priv->rx_desc_dma[q],
-						 GFP_KERNEL);
-	return priv->gbeth_rx_ring;
+	priv->rx_ring[q].desc = dma_alloc_coherent(ndev->dev.parent, ring_size,
+						   &priv->rx_desc_dma[q],
+						   GFP_KERNEL);
+	return priv->rx_ring[q].desc;
 }
 
 static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
@@ -450,10 +450,11 @@ static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
 
 	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
 
-	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
-					      &priv->rx_desc_dma[q],
-					      GFP_KERNEL);
-	return priv->rx_ring[q];
+	priv->rx_ring[q].ex_desc = dma_alloc_coherent(ndev->dev.parent,
+						      ring_size,
+						      &priv->rx_desc_dma[q],
+						      GFP_KERNEL);
+	return priv->rx_ring[q].ex_desc;
 }
 
 /* Init skb and descriptor buffer for Ethernet AVB */
@@ -830,7 +831,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
 	stats = &priv->stats[q];
 
-	desc = &priv->gbeth_rx_ring[entry];
+	desc = &priv->rx_ring[q].desc[entry];
 	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
@@ -901,13 +902,13 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 		}
 
 		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
-		desc = &priv->gbeth_rx_ring[entry];
+		desc = &priv->rx_ring[q].desc[entry];
 	}
 
 	/* Refill the RX ring buffers. */
 	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
 		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
-		desc = &priv->gbeth_rx_ring[entry];
+		desc = &priv->rx_ring[q].desc[entry];
 		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
 
 		if (!priv->rx_skb[q][entry]) {
@@ -957,7 +958,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 
 	boguscnt = min(boguscnt, *quota);
 	limit = boguscnt;
-	desc = &priv->rx_ring[q][entry];
+	desc = &priv->rx_ring[q].ex_desc[entry];
 	while (desc->die_dt != DT_FEMPTY) {
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
@@ -1017,13 +1018,13 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 		}
 
 		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
-		desc = &priv->rx_ring[q][entry];
+		desc = &priv->rx_ring[q].ex_desc[entry];
 	}
 
 	/* Refill the RX ring buffers. */
 	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
 		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
-		desc = &priv->rx_ring[q][entry];
+		desc = &priv->rx_ring[q].ex_desc[entry];
 		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
 
 		if (!priv->rx_skb[q][entry]) {
-- 
2.43.2

Re: [net-next 1/6] ravb: Group descriptor types used in Rx ring

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 9418 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The Rx ring can either be made up of normal or extended descriptors, not
> a mix of the two at the same time. Make this explicitly by grouping the
> two variables in a rx_ring union.
> 
> The extension of the storage for more than one queue of normal
> descriptors from a single to NUM_RX_QUEUE queues have no practical
> effect. But aids in making the code readable as the code that uses it
> already piggyback on other members of struct ravb_private that are
> arrays of max length NUM_RX_QUEUE, e.g. rx_desc_dma. This will also make
> further refactoring easier.
> 
> While at it rename the normal descriptor Rx ring to make it clear it's
> not strictly related to the GbEthernet E-MAC IP found in RZ/G2L, normal
> descriptors could be used on R-Car SoCs too.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  6 ++-
>  drivers/net/ethernet/renesas/ravb_main.c | 57 ++++++++++++------------
>  2 files changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 35e642fc4b2a..aecc98282c7e 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1092,8 +1092,10 @@ struct ravb_private {
>  	struct ravb_desc *desc_bat;
>  	dma_addr_t rx_desc_dma[NUM_RX_QUEUE];
>  	dma_addr_t tx_desc_dma[NUM_TX_QUEUE];
> -	struct ravb_rx_desc *gbeth_rx_ring;
> -	struct ravb_ex_rx_desc *rx_ring[NUM_RX_QUEUE];
> +	union {
> +		struct ravb_rx_desc *desc;
> +		struct ravb_ex_rx_desc *ex_desc;
> +	} rx_ring[NUM_RX_QUEUE];
>  	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
>  	void *tx_align[NUM_TX_QUEUE];
>  	struct sk_buff *rx_1st_skb;
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index f9fb772b05c7..c25a80f4d3b9 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -241,11 +241,11 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  	unsigned int ring_size;
>  	unsigned int i;
>  
> -	if (!priv->gbeth_rx_ring)
> +	if (!priv->rx_ring[q].desc)
>  		return;
>  
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
> +		struct ravb_rx_desc *desc = &priv->rx_ring[q].desc[i];
>  
>  		if (!dma_mapping_error(ndev->dev.parent,
>  				       le32_to_cpu(desc->dptr)))
> @@ -255,9 +255,9 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  					 DMA_FROM_DEVICE);
>  	}
>  	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> -	dma_free_coherent(ndev->dev.parent, ring_size, priv->gbeth_rx_ring,
> +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].desc,
>  			  priv->rx_desc_dma[q]);
> -	priv->gbeth_rx_ring = NULL;
> +	priv->rx_ring[q].desc = NULL;
>  }
>  
>  static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
> @@ -266,11 +266,11 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
>  	unsigned int ring_size;
>  	unsigned int i;
>  
> -	if (!priv->rx_ring[q])
> +	if (!priv->rx_ring[q].ex_desc)
>  		return;
>  
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q].ex_desc[i];
>  
>  		if (!dma_mapping_error(ndev->dev.parent,
>  				       le32_to_cpu(desc->dptr)))
> @@ -281,9 +281,9 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
>  	}
>  	ring_size = sizeof(struct ravb_ex_rx_desc) *
>  		    (priv->num_rx_ring[q] + 1);
> -	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].ex_desc,
>  			  priv->rx_desc_dma[q]);
> -	priv->rx_ring[q] = NULL;
> +	priv->rx_ring[q].ex_desc = NULL;
>  }
>  
>  /* Free skb's and DMA buffers for Ethernet AVB */
> @@ -335,11 +335,11 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  	unsigned int i;
>  
>  	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> -	memset(priv->gbeth_rx_ring, 0, rx_ring_size);
> +	memset(priv->rx_ring[q].desc, 0, rx_ring_size);
>  	/* Build RX ring buffer */
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		/* RX descriptor */
> -		rx_desc = &priv->gbeth_rx_ring[i];
> +		rx_desc = &priv->rx_ring[q].desc[i];
>  		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>  					  GBETH_RX_BUFF_MAX,
> @@ -352,7 +352,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  		rx_desc->dptr = cpu_to_le32(dma_addr);
>  		rx_desc->die_dt = DT_FEMPTY;
>  	}
> -	rx_desc = &priv->gbeth_rx_ring[i];
> +	rx_desc = &priv->rx_ring[q].desc[i];
>  	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>  	rx_desc->die_dt = DT_LINKFIX; /* type */
>  }
> @@ -365,11 +365,11 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  	dma_addr_t dma_addr;
>  	unsigned int i;
>  
> -	memset(priv->rx_ring[q], 0, rx_ring_size);
> +	memset(priv->rx_ring[q].ex_desc, 0, rx_ring_size);
>  	/* Build RX ring buffer */
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		/* RX descriptor */
> -		rx_desc = &priv->rx_ring[q][i];
> +		rx_desc = &priv->rx_ring[q].ex_desc[i];
>  		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>  					  RX_BUF_SZ,
> @@ -382,7 +382,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  		rx_desc->dptr = cpu_to_le32(dma_addr);
>  		rx_desc->die_dt = DT_FEMPTY;
>  	}
> -	rx_desc = &priv->rx_ring[q][i];
> +	rx_desc = &priv->rx_ring[q].ex_desc[i];
>  	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>  	rx_desc->die_dt = DT_LINKFIX; /* type */
>  }
> @@ -437,10 +437,10 @@ static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
>  
>  	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
>  
> -	priv->gbeth_rx_ring = dma_alloc_coherent(ndev->dev.parent, ring_size,
> -						 &priv->rx_desc_dma[q],
> -						 GFP_KERNEL);
> -	return priv->gbeth_rx_ring;
> +	priv->rx_ring[q].desc = dma_alloc_coherent(ndev->dev.parent, ring_size,
> +						   &priv->rx_desc_dma[q],
> +						   GFP_KERNEL);
> +	return priv->rx_ring[q].desc;
>  }
>  
>  static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
> @@ -450,10 +450,11 @@ static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
>  
>  	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
>  
> -	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
> -					      &priv->rx_desc_dma[q],
> -					      GFP_KERNEL);
> -	return priv->rx_ring[q];
> +	priv->rx_ring[q].ex_desc = dma_alloc_coherent(ndev->dev.parent,
> +						      ring_size,
> +						      &priv->rx_desc_dma[q],
> +						      GFP_KERNEL);
> +	return priv->rx_ring[q].ex_desc;
>  }
>  
>  /* Init skb and descriptor buffer for Ethernet AVB */
> @@ -830,7 +831,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>  	stats = &priv->stats[q];
>  
> -	desc = &priv->gbeth_rx_ring[entry];
> +	desc = &priv->rx_ring[q].desc[entry];
>  	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
> @@ -901,13 +902,13 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  		}
>  
>  		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> -		desc = &priv->gbeth_rx_ring[entry];
> +		desc = &priv->rx_ring[q].desc[entry];
>  	}
>  
>  	/* Refill the RX ring buffers. */
>  	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
>  		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
> -		desc = &priv->gbeth_rx_ring[entry];
> +		desc = &priv->rx_ring[q].desc[entry];
>  		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
>  
>  		if (!priv->rx_skb[q][entry]) {
> @@ -957,7 +958,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  
>  	boguscnt = min(boguscnt, *quota);
>  	limit = boguscnt;
> -	desc = &priv->rx_ring[q][entry];
> +	desc = &priv->rx_ring[q].ex_desc[entry];
>  	while (desc->die_dt != DT_FEMPTY) {
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
> @@ -1017,13 +1018,13 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  		}
>  
>  		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> -		desc = &priv->rx_ring[q][entry];
> +		desc = &priv->rx_ring[q].ex_desc[entry];
>  	}
>  
>  	/* Refill the RX ring buffers. */
>  	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
>  		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
> -		desc = &priv->rx_ring[q][entry];
> +		desc = &priv->rx_ring[q].ex_desc[entry];
>  		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
>  
>  		if (!priv->rx_skb[q][entry]) {

Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[net-next 2/6] ravb: Make it clear the information relates to maximum frame size

[Niklas Söderlund]

The struct member rx_max_buf_size was added before split descriptor
support where added. It is unclear if the value describes the full skb
frame buffer or the data descriptor buffer which can be combined into a
single skb.

Rename it to make it clear it referees to the maximum frame size and can
cover multiple descriptors.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      |  2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index aecc98282c7e..7f9e8b2c012a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1059,7 +1059,7 @@ struct ravb_hw_info {
 	int stats_len;
 	size_t max_rx_len;
 	u32 tccr_mask;
-	u32 rx_max_buf_size;
+	u32 rx_max_frame_size;
 	unsigned aligned_tx: 1;
 
 	/* hardware features */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c25a80f4d3b9..3c59e2c317c7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2684,7 +2684,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
-	.rx_max_buf_size = SZ_2K,
+	.rx_max_frame_size = SZ_2K,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2710,7 +2710,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
-	.rx_max_buf_size = SZ_2K,
+	.rx_max_frame_size = SZ_2K,
 	.aligned_tx = 1,
 	.gptp = 1,
 	.nc_queues = 1,
@@ -2733,7 +2733,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
-	.rx_max_buf_size = SZ_2K,
+	.rx_max_frame_size = SZ_2K,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
 	.gptp = 1,
@@ -2758,7 +2758,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tccr_mask = TCCR_TSRQ0,
-	.rx_max_buf_size = SZ_8K,
+	.rx_max_frame_size = SZ_8K,
 	.aligned_tx = 1,
 	.tx_counters = 1,
 	.carrier_counters = 1,
@@ -2967,7 +2967,7 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+	ndev->max_mtu = info->rx_max_frame_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
 	/* FIXME: R-Car Gen2 has 4byte alignment restriction for tx buffer
-- 
2.43.2

Re: [net-next 2/6] ravb: Make it clear the information relates to maximum frame size

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 3367 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The struct member rx_max_buf_size was added before split descriptor
> support where added. It is unclear if the value describes the full skb
> frame buffer or the data descriptor buffer which can be combined into a
> single skb.
> 
> Rename it to make it clear it referees to the maximum frame size and can
> cover multiple descriptors.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  2 +-
>  drivers/net/ethernet/renesas/ravb_main.c | 10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index aecc98282c7e..7f9e8b2c012a 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1059,7 +1059,7 @@ struct ravb_hw_info {
>  	int stats_len;
>  	size_t max_rx_len;
>  	u32 tccr_mask;
> -	u32 rx_max_buf_size;
> +	u32 rx_max_frame_size;
>  	unsigned aligned_tx: 1;
>  
>  	/* hardware features */
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c25a80f4d3b9..3c59e2c317c7 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2684,7 +2684,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> -	.rx_max_buf_size = SZ_2K,
> +	.rx_max_frame_size = SZ_2K,
>  	.internal_delay = 1,
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
> @@ -2710,7 +2710,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> -	.rx_max_buf_size = SZ_2K,
> +	.rx_max_frame_size = SZ_2K,
>  	.aligned_tx = 1,
>  	.gptp = 1,
>  	.nc_queues = 1,
> @@ -2733,7 +2733,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> -	.rx_max_buf_size = SZ_2K,
> +	.rx_max_frame_size = SZ_2K,
>  	.multi_irqs = 1,
>  	.err_mgmt_irqs = 1,
>  	.gptp = 1,
> @@ -2758,7 +2758,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
>  	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
>  	.tccr_mask = TCCR_TSRQ0,
> -	.rx_max_buf_size = SZ_8K,
> +	.rx_max_frame_size = SZ_8K,
>  	.aligned_tx = 1,
>  	.tx_counters = 1,
>  	.carrier_counters = 1,
> @@ -2967,7 +2967,7 @@ static int ravb_probe(struct platform_device *pdev)
>  	priv->avb_link_active_low =
>  		of_property_read_bool(np, "renesas,ether-link-active-low");
>  
> -	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> +	ndev->max_mtu = info->rx_max_frame_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
>  	ndev->min_mtu = ETH_MIN_MTU;
>  
>  	/* FIXME: R-Car Gen2 has 4byte alignment restriction for tx buffer

Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[net-next 3/6] ravb: Create helper to allocate skb and align it

[Niklas Söderlund]

The RAVB device requires the SKB data to be aligned to 128 bytes. The
alignment is done by allocating a skb 128 bytes larger than the maximum
frame size supported by the device and adjusting the headroom to fit the
requirement.

This code has been refactored a few times and small issues have been
added along the way. The issues are not harmful but prevents merging
parts of the Rx code which have been split in two implementations with
the addition of RZ/G2L support, a device that supports larger frame
sizes.

This change removes the need for duplicated and somewhat inaccurate
hardware alignment constrains stored in the hardware information struct
by creating a helper to handle the allocation of a skb and alignment of
a skb data.

For the R-Car class of devices the maximum frame size is 4K and each
descriptor is limited to 2K of data. The current implementation does not
support split descriptors, this limits the frame size to 2K. The
current hardware information however records the descriptor size just
under 2K due to bad understanding of the device when larger MTUs where
added.

For the RZ/G2L device the maximum frame size is 8K and each descriptor
is limited to 4K of data. The current hardware information records this
correctly, but it gets the alignment constrains wrong as just aligns it
by 128, it does not extend it by 128 bytes to allow the full frame to be
stored. This works because the RZ/G2L device supports split descriptors
and allocates each skb to 8K and aligns each 4K descriptor in this
space.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 -
 drivers/net/ethernet/renesas/ravb_main.c | 41 +++++++++++++-----------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 7f9e8b2c012a..751bb29cd488 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1057,7 +1057,6 @@ struct ravb_hw_info {
 	netdev_features_t net_hw_features;
 	netdev_features_t net_features;
 	int stats_len;
-	size_t max_rx_len;
 	u32 tccr_mask;
 	u32 rx_max_frame_size;
 	unsigned aligned_tx: 1;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 3c59e2c317c7..6e39d498936f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -113,12 +113,21 @@ static void ravb_set_rate_rcar(struct net_device *ndev)
 	}
 }
 
-static void ravb_set_buffer_align(struct sk_buff *skb)
+static struct sk_buff *
+ravb_alloc_skb(struct net_device *ndev, const struct ravb_hw_info *info)
 {
-	u32 reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
+	struct sk_buff *skb;
+	u32 reserve;
 
+	skb = netdev_alloc_skb(ndev, info->rx_max_frame_size + RAVB_ALIGN - 1);
+	if (!skb)
+		return NULL;
+
+	reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
 	if (reserve)
 		skb_reserve(skb, RAVB_ALIGN - reserve);
+
+	return skb;
 }
 
 /* Get MAC address from the MAC address registers
@@ -251,7 +260,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 				       le32_to_cpu(desc->dptr)))
 			dma_unmap_single(ndev->dev.parent,
 					 le32_to_cpu(desc->dptr),
-					 GBETH_RX_BUFF_MAX,
+					 priv->info->rx_max_frame_size,
 					 DMA_FROM_DEVICE);
 	}
 	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
@@ -276,7 +285,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
 				       le32_to_cpu(desc->dptr)))
 			dma_unmap_single(ndev->dev.parent,
 					 le32_to_cpu(desc->dptr),
-					 RX_BUF_SZ,
+					 priv->info->rx_max_frame_size,
 					 DMA_FROM_DEVICE);
 	}
 	ring_size = sizeof(struct ravb_ex_rx_desc) *
@@ -342,7 +351,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 		rx_desc = &priv->rx_ring[q].desc[i];
 		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
-					  GBETH_RX_BUFF_MAX,
+					  priv->info->rx_max_frame_size,
 					  DMA_FROM_DEVICE);
 		/* We just set the data size to 0 for a failed mapping which
 		 * should prevent DMA from happening...
@@ -372,7 +381,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 		rx_desc = &priv->rx_ring[q].ex_desc[i];
 		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
-					  RX_BUF_SZ,
+					  priv->info->rx_max_frame_size,
 					  DMA_FROM_DEVICE);
 		/* We just set the data size to 0 for a failed mapping which
 		 * should prevent DMA from happening...
@@ -476,10 +485,9 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 		goto error;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
+		skb = ravb_alloc_skb(ndev, info);
 		if (!skb)
 			goto error;
-		ravb_set_buffer_align(skb);
 		priv->rx_skb[q][i] = skb;
 	}
 
@@ -805,7 +813,8 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
 	skb = priv->rx_skb[RAVB_BE][entry];
 	priv->rx_skb[RAVB_BE][entry] = NULL;
 	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
-			 ALIGN(GBETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
+			 ALIGN(priv->info->rx_max_frame_size, 16),
+			 DMA_FROM_DEVICE);
 
 	return skb;
 }
@@ -912,13 +921,12 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
 
 		if (!priv->rx_skb[q][entry]) {
-			skb = netdev_alloc_skb(ndev, info->max_rx_len);
+			skb = ravb_alloc_skb(ndev, info);
 			if (!skb)
 				break;
-			ravb_set_buffer_align(skb);
 			dma_addr = dma_map_single(ndev->dev.parent,
 						  skb->data,
-						  GBETH_RX_BUFF_MAX,
+						  priv->info->rx_max_frame_size,
 						  DMA_FROM_DEVICE);
 			skb_checksum_none_assert(skb);
 			/* We just set the data size to 0 for a failed mapping
@@ -992,7 +1000,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 			skb = priv->rx_skb[q][entry];
 			priv->rx_skb[q][entry] = NULL;
 			dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
-					 RX_BUF_SZ,
+					 priv->info->rx_max_frame_size,
 					 DMA_FROM_DEVICE);
 			get_ts &= (q == RAVB_NC) ?
 					RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
@@ -1028,10 +1036,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
 
 		if (!priv->rx_skb[q][entry]) {
-			skb = netdev_alloc_skb(ndev, info->max_rx_len);
+			skb = ravb_alloc_skb(ndev, info);
 			if (!skb)
 				break;	/* Better luck next round. */
-			ravb_set_buffer_align(skb);
 			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
 						  le16_to_cpu(desc->ds_cc),
 						  DMA_FROM_DEVICE);
@@ -2682,7 +2689,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.net_hw_features = NETIF_F_RXCSUM,
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
-	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.internal_delay = 1,
@@ -2708,7 +2714,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.net_hw_features = NETIF_F_RXCSUM,
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
-	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.aligned_tx = 1,
@@ -2731,7 +2736,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.net_hw_features = NETIF_F_RXCSUM,
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
-	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.multi_irqs = 1,
@@ -2756,7 +2760,6 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.net_hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
-	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tccr_mask = TCCR_TSRQ0,
 	.rx_max_frame_size = SZ_8K,
 	.aligned_tx = 1,
-- 
2.43.2

Re: [net-next 3/6] ravb: Create helper to allocate skb and align it

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 2549 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The RAVB device requires the SKB data to be aligned to 128 bytes. The
> alignment is done by allocating a skb 128 bytes larger than the maximum
> frame size supported by the device and adjusting the headroom to fit the
> requirement.
> 
> This code has been refactored a few times and small issues have been
> added along the way. The issues are not harmful but prevents merging
> parts of the Rx code which have been split in two implementations with
> the addition of RZ/G2L support, a device that supports larger frame
> sizes.
> 
> This change removes the need for duplicated and somewhat inaccurate
> hardware alignment constrains stored in the hardware information struct
> by creating a helper to handle the allocation of a skb and alignment of
> a skb data.
> 
> For the R-Car class of devices the maximum frame size is 4K and each
> descriptor is limited to 2K of data. The current implementation does not
> support split descriptors, this limits the frame size to 2K. The
> current hardware information however records the descriptor size just
> under 2K due to bad understanding of the device when larger MTUs where
> added.
> 
> For the RZ/G2L device the maximum frame size is 8K and each descriptor
> is limited to 4K of data. The current hardware information records this
> correctly, but it gets the alignment constrains wrong as just aligns it
> by 128, it does not extend it by 128 bytes to allow the full frame to be
> stored. This works because the RZ/G2L device supports split descriptors
> and allocates each skb to 8K and aligns each 4K descriptor in this
> space.

This change tidies up code which I re-wrote last week but I haven't sent
patches yet... I was holding off sending further patches until we've
completed gPTP testing, but maybe I can get an updated RFC series sent
first then continue with testing.

To get the best RX performance, we should allocate plain RX buffers
using a page pool, then call build_skb() when a packet is received.
Reducing RX buffer size from 8kB to 2kB for RZ/G2L gets me a 50%
improvement in TCP RX throughput and a 200% improvement in UDP RX
throughput. Using a page pool gets me a further 10% or so in both TCP &
UDP RX throughput. So, if we merge this I'll be trying to remove
ravb_alloc_skb() shortly anyway.

So NACK on this for now, but let's co-ordinate the improvements we're
both making so there aren't any merge conflicts for the maintainers to
resolve.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [net-next 3/6] ravb: Create helper to allocate skb and align it

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 9688 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The RAVB device requires the SKB data to be aligned to 128 bytes. The
> alignment is done by allocating a skb 128 bytes larger than the maximum
> frame size supported by the device and adjusting the headroom to fit the
> requirement.
> 
> This code has been refactored a few times and small issues have been
> added along the way. The issues are not harmful but prevents merging
> parts of the Rx code which have been split in two implementations with
> the addition of RZ/G2L support, a device that supports larger frame
> sizes.
> 
> This change removes the need for duplicated and somewhat inaccurate
> hardware alignment constrains stored in the hardware information struct
> by creating a helper to handle the allocation of a skb and alignment of
> a skb data.
> 
> For the R-Car class of devices the maximum frame size is 4K and each
> descriptor is limited to 2K of data. The current implementation does not
> support split descriptors, this limits the frame size to 2K. The
> current hardware information however records the descriptor size just
> under 2K due to bad understanding of the device when larger MTUs where
> added.
> 
> For the RZ/G2L device the maximum frame size is 8K and each descriptor
> is limited to 4K of data. The current hardware information records this
> correctly, but it gets the alignment constrains wrong as just aligns it
> by 128, it does not extend it by 128 bytes to allow the full frame to be
> stored. This works because the RZ/G2L device supports split descriptors
> and allocates each skb to 8K and aligns each 4K descriptor in this
> space.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

After some discussion with Niklas on IRC, I'm dropping my NACK so that
this can hopefully get in to v6.9. I'll have to re-do some of my work,
but it was unlikely that would be ready to go in for v6.9 anyway. So,
here's some review...

> ---
>  drivers/net/ethernet/renesas/ravb.h      |  1 -
>  drivers/net/ethernet/renesas/ravb_main.c | 41 +++++++++++++-----------
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 7f9e8b2c012a..751bb29cd488 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1057,7 +1057,6 @@ struct ravb_hw_info {
>  	netdev_features_t net_hw_features;
>  	netdev_features_t net_features;
>  	int stats_len;
> -	size_t max_rx_len;
>  	u32 tccr_mask;
>  	u32 rx_max_frame_size;
>  	unsigned aligned_tx: 1;
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 3c59e2c317c7..6e39d498936f 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -113,12 +113,21 @@ static void ravb_set_rate_rcar(struct net_device *ndev)
>  	}
>  }
>  
> -static void ravb_set_buffer_align(struct sk_buff *skb)
> +static struct sk_buff *
> +ravb_alloc_skb(struct net_device *ndev, const struct ravb_hw_info *info)

This function should take an extra `gfp_t gfp_mask` argument since it is
called from two contexts: RX ring initialization where we want regular
allocation, and RX ring refill where we need atomic allocation.

>  {
> -	u32 reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
> +	struct sk_buff *skb;
> +	u32 reserve;
>  
> +	skb = netdev_alloc_skb(ndev, info->rx_max_frame_size + RAVB_ALIGN - 1);

Call __netdev_alloc_skb() instead with the gfp_mask argument.

> +	if (!skb)
> +		return NULL;
> +
> +	reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
>  	if (reserve)
>  		skb_reserve(skb, RAVB_ALIGN - reserve);
> +
> +	return skb;
>  }
>  
>  /* Get MAC address from the MAC address registers
> @@ -251,7 +260,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  				       le32_to_cpu(desc->dptr)))
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
> -					 GBETH_RX_BUFF_MAX,
> +					 priv->info->rx_max_frame_size,
>  					 DMA_FROM_DEVICE);
>  	}
>  	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> @@ -276,7 +285,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
>  				       le32_to_cpu(desc->dptr)))
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
> -					 RX_BUF_SZ,
> +					 priv->info->rx_max_frame_size,
>  					 DMA_FROM_DEVICE);
>  	}
>  	ring_size = sizeof(struct ravb_ex_rx_desc) *
> @@ -342,7 +351,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  		rx_desc = &priv->rx_ring[q].desc[i];
>  		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> -					  GBETH_RX_BUFF_MAX,
> +					  priv->info->rx_max_frame_size,
>  					  DMA_FROM_DEVICE);
>  		/* We just set the data size to 0 for a failed mapping which
>  		 * should prevent DMA from happening...
> @@ -372,7 +381,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  		rx_desc = &priv->rx_ring[q].ex_desc[i];
>  		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> -					  RX_BUF_SZ,
> +					  priv->info->rx_max_frame_size,
>  					  DMA_FROM_DEVICE);
>  		/* We just set the data size to 0 for a failed mapping which
>  		 * should prevent DMA from happening...
> @@ -476,10 +485,9 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  		goto error;
>  
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
> +		skb = ravb_alloc_skb(ndev, info);

Add GFP_KERNEL as the gfp_mask argument.

>  		if (!skb)
>  			goto error;
> -		ravb_set_buffer_align(skb);
>  		priv->rx_skb[q][i] = skb;
>  	}
>  
> @@ -805,7 +813,8 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
>  	skb = priv->rx_skb[RAVB_BE][entry];
>  	priv->rx_skb[RAVB_BE][entry] = NULL;
>  	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> -			 ALIGN(GBETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
> +			 ALIGN(priv->info->rx_max_frame_size, 16),
> +			 DMA_FROM_DEVICE);
>  
>  	return skb;
>  }
> @@ -912,13 +921,12 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
>  
>  		if (!priv->rx_skb[q][entry]) {
> -			skb = netdev_alloc_skb(ndev, info->max_rx_len);
> +			skb = ravb_alloc_skb(ndev, info);

Add GFP_ATOMIC as the gfp_mask argument.

>  			if (!skb)
>  				break;
> -			ravb_set_buffer_align(skb);
>  			dma_addr = dma_map_single(ndev->dev.parent,
>  						  skb->data,
> -						  GBETH_RX_BUFF_MAX,
> +						  priv->info->rx_max_frame_size,
>  						  DMA_FROM_DEVICE);
>  			skb_checksum_none_assert(skb);
>  			/* We just set the data size to 0 for a failed mapping
> @@ -992,7 +1000,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  			skb = priv->rx_skb[q][entry];
>  			priv->rx_skb[q][entry] = NULL;
>  			dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> -					 RX_BUF_SZ,
> +					 priv->info->rx_max_frame_size,
>  					 DMA_FROM_DEVICE);
>  			get_ts &= (q == RAVB_NC) ?
>  					RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> @@ -1028,10 +1036,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
>  
>  		if (!priv->rx_skb[q][entry]) {
> -			skb = netdev_alloc_skb(ndev, info->max_rx_len);
> +			skb = ravb_alloc_skb(ndev, info);


Add GFP_ATOMIC as the gfp_mask argument.

>  			if (!skb)
>  				break;	/* Better luck next round. */
> -			ravb_set_buffer_align(skb);
>  			dma_addr = dma_map_single(ndev->dev.parent, skb->data,
>  						  le16_to_cpu(desc->ds_cc),
>  						  DMA_FROM_DEVICE);
> @@ -2682,7 +2689,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.net_hw_features = NETIF_F_RXCSUM,
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> -	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.internal_delay = 1,
> @@ -2708,7 +2714,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.net_hw_features = NETIF_F_RXCSUM,
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> -	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.aligned_tx = 1,
> @@ -2731,7 +2736,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.net_hw_features = NETIF_F_RXCSUM,
>  	.net_features = NETIF_F_RXCSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> -	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.multi_irqs = 1,
> @@ -2756,7 +2760,6 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.net_hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
>  	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
> -	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
>  	.tccr_mask = TCCR_TSRQ0,
>  	.rx_max_frame_size = SZ_8K,
>  	.aligned_tx = 1,

Looks ok other than the above comments. I'll try to do some testing
tomorrow.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[net-next 4/6] ravb: Use the max frame size from hardware info for RZ/G2L

[Niklas Söderlund]

Remove the define describing the RZ/G2L maximum frame size and only use
the information in the hardware information struct. This will make it
easier to merge the R-Car and RZ/G2L code paths.

There is no functional change as both the define and the maximum frame
length in the hardware information is set to 8K.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      | 1 -
 drivers/net/ethernet/renesas/ravb_main.c | 5 +++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 751bb29cd488..7fa60fccb6ea 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1017,7 +1017,6 @@ enum CSR2_BIT {
 
 #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
 
-#define GBETH_RX_BUFF_MAX 8192
 #define GBETH_RX_DESC_DATA_SIZE 4080
 
 struct ravb_tstamp_skb {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 6e39d498936f..b309ca23f5b6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -566,7 +566,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 	}
 
 	/* Receive frame limit set register */
-	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
+	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
 
 	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through */
 	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
@@ -627,6 +627,7 @@ static void ravb_emac_init(struct net_device *ndev)
 
 static int ravb_dmac_init_gbeth(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
 	int error;
 
 	error = ravb_ring_init(ndev, RAVB_BE);
@@ -640,7 +641,7 @@ static int ravb_dmac_init_gbeth(struct net_device *ndev)
 	ravb_write(ndev, 0x60000000, RCR);
 
 	/* Set Max Frame Length (RTC) */
-	ravb_write(ndev, 0x7ffc0000 | GBETH_RX_BUFF_MAX, RTC);
+	ravb_write(ndev, 0x7ffc0000 | priv->info->rx_max_frame_size, RTC);
 
 	/* Set FIFO size */
 	ravb_write(ndev, 0x00222200, TGC);
-- 
2.43.2

Re: [net-next 4/6] ravb: Use the max frame size from hardware info for RZ/G2L

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 2444 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> Remove the define describing the RZ/G2L maximum frame size and only use
> the information in the hardware information struct. This will make it
> easier to merge the R-Car and RZ/G2L code paths.
> 
> There is no functional change as both the define and the maximum frame
> length in the hardware information is set to 8K.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb.h      | 1 -
>  drivers/net/ethernet/renesas/ravb_main.c | 5 +++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 751bb29cd488..7fa60fccb6ea 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1017,7 +1017,6 @@ enum CSR2_BIT {
>  
>  #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
>  
> -#define GBETH_RX_BUFF_MAX 8192
>  #define GBETH_RX_DESC_DATA_SIZE 4080
>  
>  struct ravb_tstamp_skb {
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 6e39d498936f..b309ca23f5b6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -566,7 +566,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>  	}
>  
>  	/* Receive frame limit set register */
> -	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
> +	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
>  
>  	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through */
>  	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
> @@ -627,6 +627,7 @@ static void ravb_emac_init(struct net_device *ndev)
>  
>  static int ravb_dmac_init_gbeth(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
>  	int error;
>  
>  	error = ravb_ring_init(ndev, RAVB_BE);
> @@ -640,7 +641,7 @@ static int ravb_dmac_init_gbeth(struct net_device *ndev)
>  	ravb_write(ndev, 0x60000000, RCR);
>  
>  	/* Set Max Frame Length (RTC) */
> -	ravb_write(ndev, 0x7ffc0000 | GBETH_RX_BUFF_MAX, RTC);
> +	ravb_write(ndev, 0x7ffc0000 | priv->info->rx_max_frame_size, RTC);
>  
>  	/* Set FIFO size */
>  	ravb_write(ndev, 0x00222200, TGC);

Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[net-next 5/6] ravb: Move maximum Rx descriptor data usage to info struct

[Niklas Söderlund]

To make it possible to merge the R-Car and RZ/G2L code paths move the
maximum usable size of a single Rx descriptor data slice in to the
hardware information instead of using two different defines in the two
different code paths.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      |  5 +----
 drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 7fa60fccb6ea..b12b379baf5a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1015,10 +1015,6 @@ enum CSR2_BIT {
 #define NUM_RX_QUEUE	2
 #define NUM_TX_QUEUE	2
 
-#define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
-
-#define GBETH_RX_DESC_DATA_SIZE 4080
-
 struct ravb_tstamp_skb {
 	struct list_head list;
 	struct sk_buff *skb;
@@ -1058,6 +1054,7 @@ struct ravb_hw_info {
 	int stats_len;
 	u32 tccr_mask;
 	u32 rx_max_frame_size;
+	u32 rx_max_desc_use;
 	unsigned aligned_tx: 1;
 
 	/* hardware features */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b309ca23f5b6..dee51a78cf36 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -349,7 +349,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
 		rx_desc = &priv->rx_ring[q].desc[i];
-		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
+		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  priv->info->rx_max_frame_size,
 					  DMA_FROM_DEVICE);
@@ -379,7 +379,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
 		rx_desc = &priv->rx_ring[q].ex_desc[i];
-		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
+		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  priv->info->rx_max_frame_size,
 					  DMA_FROM_DEVICE);
@@ -919,7 +919,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
 		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
 		desc = &priv->rx_ring[q].desc[entry];
-		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
+		desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
 
 		if (!priv->rx_skb[q][entry]) {
 			skb = ravb_alloc_skb(ndev, info);
@@ -1034,7 +1034,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
 		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
 		desc = &priv->rx_ring[q].ex_desc[entry];
-		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
+		desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
 
 		if (!priv->rx_skb[q][entry]) {
 			skb = ravb_alloc_skb(ndev, info);
@@ -2692,6 +2692,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
+	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
+	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
 	.aligned_tx = 1,
 	.gptp = 1,
 	.nc_queues = 1,
@@ -2739,6 +2741,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
+	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
 	.gptp = 1,
@@ -2763,6 +2766,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.tccr_mask = TCCR_TSRQ0,
 	.rx_max_frame_size = SZ_8K,
+	.rx_max_desc_use = 4080,
 	.aligned_tx = 1,
 	.tx_counters = 1,
 	.carrier_counters = 1,
-- 
2.43.2

Re: [net-next 5/6] ravb: Move maximum Rx descriptor data usage to info struct

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 4930 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> To make it possible to merge the R-Car and RZ/G2L code paths move the
> maximum usable size of a single Rx descriptor data slice in to the
> hardware information instead of using two different defines in the two
> different code paths.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  5 +----
>  drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++----
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 7fa60fccb6ea..b12b379baf5a 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1015,10 +1015,6 @@ enum CSR2_BIT {
>  #define NUM_RX_QUEUE	2
>  #define NUM_TX_QUEUE	2
>  
> -#define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
> -
> -#define GBETH_RX_DESC_DATA_SIZE 4080
> -
>  struct ravb_tstamp_skb {
>  	struct list_head list;
>  	struct sk_buff *skb;
> @@ -1058,6 +1054,7 @@ struct ravb_hw_info {
>  	int stats_len;
>  	u32 tccr_mask;
>  	u32 rx_max_frame_size;
> +	u32 rx_max_desc_use;
>  	unsigned aligned_tx: 1;
>  
>  	/* hardware features */
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index b309ca23f5b6..dee51a78cf36 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -349,7 +349,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		/* RX descriptor */
>  		rx_desc = &priv->rx_ring[q].desc[i];
> -		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
> +		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>  					  priv->info->rx_max_frame_size,
>  					  DMA_FROM_DEVICE);
> @@ -379,7 +379,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		/* RX descriptor */
>  		rx_desc = &priv->rx_ring[q].ex_desc[i];
> -		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
> +		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>  					  priv->info->rx_max_frame_size,
>  					  DMA_FROM_DEVICE);
> @@ -919,7 +919,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
>  		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
>  		desc = &priv->rx_ring[q].desc[entry];
> -		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
> +		desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>  
>  		if (!priv->rx_skb[q][entry]) {
>  			skb = ravb_alloc_skb(ndev, info);
> @@ -1034,7 +1034,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
>  		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
>  		desc = &priv->rx_ring[q].ex_desc[entry];
> -		desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
> +		desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>  
>  		if (!priv->rx_skb[q][entry]) {
>  			skb = ravb_alloc_skb(ndev, info);
> @@ -2692,6 +2692,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
> +	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),

I'd prefer that we use SZ_2K here instead of 2048 for consistency with the line above.

>  	.internal_delay = 1,
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
> @@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
> +	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),

As above.

>  	.aligned_tx = 1,
>  	.gptp = 1,
>  	.nc_queues = 1,
> @@ -2739,6 +2741,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
> +	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),

As above.

>  	.multi_irqs = 1,
>  	.err_mgmt_irqs = 1,
>  	.gptp = 1,
> @@ -2763,6 +2766,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
>  	.tccr_mask = TCCR_TSRQ0,
>  	.rx_max_frame_size = SZ_8K,
> +	.rx_max_desc_use = 4080,
>  	.aligned_tx = 1,
>  	.tx_counters = 1,
>  	.carrier_counters = 1,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[net-next 6/6] ravb: Unify Rx ring maintenance code paths

[Niklas Söderlund]

The R-Car and RZ/G2L Rx code paths was split in two separate
implementations when support for RZ/G2L was added due to the fact that
R-Car uses the extended descriptor format while RZ/G2L uses normal
descriptors. This has lead to a duplication of Rx logic with the only
difference being the different Rx descriptors types used. The
implementation however neglects to take into account that extended
descriptors are normal descriptors with additional metadata at the end
to carry hardware timestamp information.

The hardware timestamps information is only consumed in the R-Car Rx
loop and all the maintenance code around the Rx ring can be shared
between the two implementations if the difference in descriptor length
is carefully considered.

This change merges the two implementations for Rx ring maintenance by
adding a method to access both types of descriptors as normal
descriptors, as this part covers all the fields needed for Rx ring
maintenance the only difference between using normal or extended
descriptor is the size of the memory region to allocate/free and the
step size between each descriptor in the ring.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      |   5 +-
 drivers/net/ethernet/renesas/ravb_main.c | 132 ++++++-----------------
 2 files changed, 32 insertions(+), 105 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b12b379baf5a..b48935ec7e28 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1039,9 +1039,6 @@ struct ravb_ptp {
 };
 
 struct ravb_hw_info {
-	void (*rx_ring_free)(struct net_device *ndev, int q);
-	void (*rx_ring_format)(struct net_device *ndev, int q);
-	void *(*alloc_rx_desc)(struct net_device *ndev, int q);
 	bool (*receive)(struct net_device *ndev, int *quota, int q);
 	void (*set_rate)(struct net_device *ndev);
 	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
@@ -1055,6 +1052,7 @@ struct ravb_hw_info {
 	u32 tccr_mask;
 	u32 rx_max_frame_size;
 	u32 rx_max_desc_use;
+	u32 rx_desc_size;
 	unsigned aligned_tx: 1;
 
 	/* hardware features */
@@ -1090,6 +1088,7 @@ struct ravb_private {
 	union {
 		struct ravb_rx_desc *desc;
 		struct ravb_ex_rx_desc *ex_desc;
+		void *raw;
 	} rx_ring[NUM_RX_QUEUE];
 	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
 	void *tx_align[NUM_TX_QUEUE];
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index dee51a78cf36..2702455b6cc6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -200,6 +200,13 @@ static const struct mdiobb_ops bb_ops = {
 	.get_mdio_data = ravb_get_mdio_data,
 };
 
+static struct ravb_rx_desc *
+ravb_rx_get_desc(struct ravb_private *priv, unsigned int q,
+		 unsigned int i)
+{
+	return priv->rx_ring[q].raw + priv->info->rx_desc_size * i;
+}
+
 /* Free TX skb function for AVB-IP */
 static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 {
@@ -244,17 +251,17 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 	return free_num;
 }
 
-static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
+static void ravb_rx_ring_free(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned int ring_size;
 	unsigned int i;
 
-	if (!priv->rx_ring[q].desc)
+	if (!priv->rx_ring[q].raw)
 		return;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		struct ravb_rx_desc *desc = &priv->rx_ring[q].desc[i];
+		struct ravb_rx_desc *desc = ravb_rx_get_desc(priv, q, i);
 
 		if (!dma_mapping_error(ndev->dev.parent,
 				       le32_to_cpu(desc->dptr)))
@@ -263,48 +270,21 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 					 priv->info->rx_max_frame_size,
 					 DMA_FROM_DEVICE);
 	}
-	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
-	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].desc,
+	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].raw,
 			  priv->rx_desc_dma[q]);
-	priv->rx_ring[q].desc = NULL;
-}
-
-static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	unsigned int ring_size;
-	unsigned int i;
-
-	if (!priv->rx_ring[q].ex_desc)
-		return;
-
-	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q].ex_desc[i];
-
-		if (!dma_mapping_error(ndev->dev.parent,
-				       le32_to_cpu(desc->dptr)))
-			dma_unmap_single(ndev->dev.parent,
-					 le32_to_cpu(desc->dptr),
-					 priv->info->rx_max_frame_size,
-					 DMA_FROM_DEVICE);
-	}
-	ring_size = sizeof(struct ravb_ex_rx_desc) *
-		    (priv->num_rx_ring[q] + 1);
-	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].ex_desc,
-			  priv->rx_desc_dma[q]);
-	priv->rx_ring[q].ex_desc = NULL;
+	priv->rx_ring[q].raw = NULL;
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
 static void ravb_ring_free(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
 	unsigned int num_tx_desc = priv->num_tx_desc;
 	unsigned int ring_size;
 	unsigned int i;
 
-	info->rx_ring_free(ndev, q);
+	ravb_rx_ring_free(ndev, q);
 
 	if (priv->tx_ring[q]) {
 		ravb_tx_free(ndev, q, false);
@@ -335,7 +315,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	priv->tx_skb[q] = NULL;
 }
 
-static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
+static void ravb_rx_ring_format(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	struct ravb_rx_desc *rx_desc;
@@ -344,11 +324,11 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 	unsigned int i;
 
 	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
-	memset(priv->rx_ring[q].desc, 0, rx_ring_size);
+	memset(priv->rx_ring[q].raw, 0, rx_ring_size);
 	/* Build RX ring buffer */
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
-		rx_desc = &priv->rx_ring[q].desc[i];
+		rx_desc = ravb_rx_get_desc(priv, q, i);
 		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
 		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
 					  priv->info->rx_max_frame_size,
@@ -361,37 +341,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 		rx_desc->dptr = cpu_to_le32(dma_addr);
 		rx_desc->die_dt = DT_FEMPTY;
 	}
-	rx_desc = &priv->rx_ring[q].desc[i];
-	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
-	rx_desc->die_dt = DT_LINKFIX; /* type */
-}
-
-static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	struct ravb_ex_rx_desc *rx_desc;
-	unsigned int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
-	dma_addr_t dma_addr;
-	unsigned int i;
-
-	memset(priv->rx_ring[q].ex_desc, 0, rx_ring_size);
-	/* Build RX ring buffer */
-	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		/* RX descriptor */
-		rx_desc = &priv->rx_ring[q].ex_desc[i];
-		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
-		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
-					  priv->info->rx_max_frame_size,
-					  DMA_FROM_DEVICE);
-		/* We just set the data size to 0 for a failed mapping which
-		 * should prevent DMA from happening...
-		 */
-		if (dma_mapping_error(ndev->dev.parent, dma_addr))
-			rx_desc->ds_cc = cpu_to_le16(0);
-		rx_desc->dptr = cpu_to_le32(dma_addr);
-		rx_desc->die_dt = DT_FEMPTY;
-	}
-	rx_desc = &priv->rx_ring[q].ex_desc[i];
+	rx_desc = ravb_rx_get_desc(priv, q, i);
 	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
 	rx_desc->die_dt = DT_LINKFIX; /* type */
 }
@@ -400,7 +350,6 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 static void ravb_ring_format(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
 	unsigned int num_tx_desc = priv->num_tx_desc;
 	struct ravb_tx_desc *tx_desc;
 	struct ravb_desc *desc;
@@ -413,7 +362,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	priv->dirty_rx[q] = 0;
 	priv->dirty_tx[q] = 0;
 
-	info->rx_ring_format(ndev, q);
+	ravb_rx_ring_format(ndev, q);
 
 	memset(priv->tx_ring[q], 0, tx_ring_size);
 	/* Build TX ring buffer */
@@ -439,31 +388,18 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
 }
 
-static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
+static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned int ring_size;
 
-	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
 
-	priv->rx_ring[q].desc = dma_alloc_coherent(ndev->dev.parent, ring_size,
-						   &priv->rx_desc_dma[q],
-						   GFP_KERNEL);
-	return priv->rx_ring[q].desc;
-}
+	priv->rx_ring[q].raw = dma_alloc_coherent(ndev->dev.parent, ring_size,
+						  &priv->rx_desc_dma[q],
+						  GFP_KERNEL);
 
-static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	unsigned int ring_size;
-
-	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
-
-	priv->rx_ring[q].ex_desc = dma_alloc_coherent(ndev->dev.parent,
-						      ring_size,
-						      &priv->rx_desc_dma[q],
-						      GFP_KERNEL);
-	return priv->rx_ring[q].ex_desc;
+	return priv->rx_ring[q].raw;
 }
 
 /* Init skb and descriptor buffer for Ethernet AVB */
@@ -500,7 +436,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	}
 
 	/* Allocate all RX descriptors. */
-	if (!info->alloc_rx_desc(ndev, q))
+	if (!ravb_alloc_rx_desc(ndev, q))
 		goto error;
 
 	priv->dirty_rx[q] = 0;
@@ -2677,9 +2613,6 @@ static int ravb_mdio_release(struct ravb_private *priv)
 }
 
 static const struct ravb_hw_info ravb_gen3_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free_rcar,
-	.rx_ring_format = ravb_rx_ring_format_rcar,
-	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
@@ -2693,6 +2626,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
+	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2703,9 +2637,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 };
 
 static const struct ravb_hw_info ravb_gen2_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free_rcar,
-	.rx_ring_format = ravb_rx_ring_format_rcar,
-	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
@@ -2719,6 +2650,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
+	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.aligned_tx = 1,
 	.gptp = 1,
 	.nc_queues = 1,
@@ -2726,9 +2658,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 };
 
 static const struct ravb_hw_info ravb_rzv2m_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free_rcar,
-	.rx_ring_format = ravb_rx_ring_format_rcar,
-	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
@@ -2742,6 +2671,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_frame_size = SZ_2K,
 	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
+	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
 	.gptp = 1,
@@ -2751,9 +2681,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 };
 
 static const struct ravb_hw_info gbeth_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free_gbeth,
-	.rx_ring_format = ravb_rx_ring_format_gbeth,
-	.alloc_rx_desc = ravb_alloc_rx_desc_gbeth,
 	.receive = ravb_rx_gbeth,
 	.set_rate = ravb_set_rate_gbeth,
 	.set_feature = ravb_set_features_gbeth,
@@ -2767,6 +2694,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.tccr_mask = TCCR_TSRQ0,
 	.rx_max_frame_size = SZ_8K,
 	.rx_max_desc_use = 4080,
+	.rx_desc_size = sizeof(struct ravb_rx_desc),
 	.aligned_tx = 1,
 	.tx_counters = 1,
 	.carrier_counters = 1,
-- 
2.43.2

Re: [net-next 6/6] ravb: Unify Rx ring maintenance code paths

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 14026 bytes --]

On 27/02/2024 01:40, Niklas Söderlund wrote:
> The R-Car and RZ/G2L Rx code paths was split in two separate
> implementations when support for RZ/G2L was added due to the fact that
> R-Car uses the extended descriptor format while RZ/G2L uses normal
> descriptors. This has lead to a duplication of Rx logic with the only
> difference being the different Rx descriptors types used. The
> implementation however neglects to take into account that extended
> descriptors are normal descriptors with additional metadata at the end
> to carry hardware timestamp information.
> 
> The hardware timestamps information is only consumed in the R-Car Rx
> loop and all the maintenance code around the Rx ring can be shared
> between the two implementations if the difference in descriptor length
> is carefully considered.
> 
> This change merges the two implementations for Rx ring maintenance by
> adding a method to access both types of descriptors as normal
> descriptors, as this part covers all the fields needed for Rx ring
> maintenance the only difference between using normal or extended
> descriptor is the size of the memory region to allocate/free and the
> step size between each descriptor in the ring.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |   5 +-
>  drivers/net/ethernet/renesas/ravb_main.c | 132 ++++++-----------------
>  2 files changed, 32 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b12b379baf5a..b48935ec7e28 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1039,9 +1039,6 @@ struct ravb_ptp {
>  };
>  
>  struct ravb_hw_info {
> -	void (*rx_ring_free)(struct net_device *ndev, int q);
> -	void (*rx_ring_format)(struct net_device *ndev, int q);
> -	void *(*alloc_rx_desc)(struct net_device *ndev, int q);
>  	bool (*receive)(struct net_device *ndev, int *quota, int q);
>  	void (*set_rate)(struct net_device *ndev);
>  	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
> @@ -1055,6 +1052,7 @@ struct ravb_hw_info {
>  	u32 tccr_mask;
>  	u32 rx_max_frame_size;
>  	u32 rx_max_desc_use;
> +	u32 rx_desc_size;
>  	unsigned aligned_tx: 1;
>  
>  	/* hardware features */
> @@ -1090,6 +1088,7 @@ struct ravb_private {
>  	union {
>  		struct ravb_rx_desc *desc;
>  		struct ravb_ex_rx_desc *ex_desc;
> +		void *raw;
>  	} rx_ring[NUM_RX_QUEUE];
>  	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
>  	void *tx_align[NUM_TX_QUEUE];
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index dee51a78cf36..2702455b6cc6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -200,6 +200,13 @@ static const struct mdiobb_ops bb_ops = {
>  	.get_mdio_data = ravb_get_mdio_data,
>  };
>  
> +static struct ravb_rx_desc *
> +ravb_rx_get_desc(struct ravb_private *priv, unsigned int q,
> +		 unsigned int i)
> +{
> +	return priv->rx_ring[q].raw + priv->info->rx_desc_size * i;
> +}
> +
>  /* Free TX skb function for AVB-IP */
>  static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  {
> @@ -244,17 +251,17 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  	return free_num;
>  }
>  
> -static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
> +static void ravb_rx_ring_free(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned int ring_size;
>  	unsigned int i;
>  
> -	if (!priv->rx_ring[q].desc)
> +	if (!priv->rx_ring[q].raw)
>  		return;
>  
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		struct ravb_rx_desc *desc = &priv->rx_ring[q].desc[i];
> +		struct ravb_rx_desc *desc = ravb_rx_get_desc(priv, q, i);
>  
>  		if (!dma_mapping_error(ndev->dev.parent,
>  				       le32_to_cpu(desc->dptr)))
> @@ -263,48 +270,21 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  					 priv->info->rx_max_frame_size,
>  					 DMA_FROM_DEVICE);
>  	}
> -	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> -	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].desc,
> +	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
> +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].raw,
>  			  priv->rx_desc_dma[q]);
> -	priv->rx_ring[q].desc = NULL;
> -}
> -
> -static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	unsigned int ring_size;
> -	unsigned int i;
> -
> -	if (!priv->rx_ring[q].ex_desc)
> -		return;
> -
> -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q].ex_desc[i];
> -
> -		if (!dma_mapping_error(ndev->dev.parent,
> -				       le32_to_cpu(desc->dptr)))
> -			dma_unmap_single(ndev->dev.parent,
> -					 le32_to_cpu(desc->dptr),
> -					 priv->info->rx_max_frame_size,
> -					 DMA_FROM_DEVICE);
> -	}
> -	ring_size = sizeof(struct ravb_ex_rx_desc) *
> -		    (priv->num_rx_ring[q] + 1);
> -	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].ex_desc,
> -			  priv->rx_desc_dma[q]);
> -	priv->rx_ring[q].ex_desc = NULL;
> +	priv->rx_ring[q].raw = NULL;
>  }
>  
>  /* Free skb's and DMA buffers for Ethernet AVB */
>  static void ravb_ring_free(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	const struct ravb_hw_info *info = priv->info;
>  	unsigned int num_tx_desc = priv->num_tx_desc;
>  	unsigned int ring_size;
>  	unsigned int i;
>  
> -	info->rx_ring_free(ndev, q);
> +	ravb_rx_ring_free(ndev, q);
>  
>  	if (priv->tx_ring[q]) {
>  		ravb_tx_free(ndev, q, false);
> @@ -335,7 +315,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	priv->tx_skb[q] = NULL;
>  }
>  
> -static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
> +static void ravb_rx_ring_format(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	struct ravb_rx_desc *rx_desc;
> @@ -344,11 +324,11 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  	unsigned int i;
>  
>  	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];

I think `sizeof(*rx_desc)` should be changed to
`priv->info->rx_desc_size`.

> -	memset(priv->rx_ring[q].desc, 0, rx_ring_size);
> +	memset(priv->rx_ring[q].raw, 0, rx_ring_size);
>  	/* Build RX ring buffer */
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		/* RX descriptor */
> -		rx_desc = &priv->rx_ring[q].desc[i];
> +		rx_desc = ravb_rx_get_desc(priv, q, i);
>  		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>  					  priv->info->rx_max_frame_size,
> @@ -361,37 +341,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
>  		rx_desc->dptr = cpu_to_le32(dma_addr);
>  		rx_desc->die_dt = DT_FEMPTY;
>  	}
> -	rx_desc = &priv->rx_ring[q].desc[i];
> -	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> -	rx_desc->die_dt = DT_LINKFIX; /* type */
> -}
> -
> -static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ravb_ex_rx_desc *rx_desc;
> -	unsigned int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> -	dma_addr_t dma_addr;
> -	unsigned int i;
> -
> -	memset(priv->rx_ring[q].ex_desc, 0, rx_ring_size);
> -	/* Build RX ring buffer */
> -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -		/* RX descriptor */
> -		rx_desc = &priv->rx_ring[q].ex_desc[i];
> -		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> -		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> -					  priv->info->rx_max_frame_size,
> -					  DMA_FROM_DEVICE);
> -		/* We just set the data size to 0 for a failed mapping which
> -		 * should prevent DMA from happening...
> -		 */
> -		if (dma_mapping_error(ndev->dev.parent, dma_addr))
> -			rx_desc->ds_cc = cpu_to_le16(0);
> -		rx_desc->dptr = cpu_to_le32(dma_addr);
> -		rx_desc->die_dt = DT_FEMPTY;
> -	}
> -	rx_desc = &priv->rx_ring[q].ex_desc[i];
> +	rx_desc = ravb_rx_get_desc(priv, q, i);
>  	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>  	rx_desc->die_dt = DT_LINKFIX; /* type */
>  }
> @@ -400,7 +350,6 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  static void ravb_ring_format(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	const struct ravb_hw_info *info = priv->info;
>  	unsigned int num_tx_desc = priv->num_tx_desc;
>  	struct ravb_tx_desc *tx_desc;
>  	struct ravb_desc *desc;
> @@ -413,7 +362,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  	priv->dirty_rx[q] = 0;
>  	priv->dirty_tx[q] = 0;
>  
> -	info->rx_ring_format(ndev, q);
> +	ravb_rx_ring_format(ndev, q);
>  
>  	memset(priv->tx_ring[q], 0, tx_ring_size);
>  	/* Build TX ring buffer */
> @@ -439,31 +388,18 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  	desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
>  }
>  
> -static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
> +static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned int ring_size;
>  
> -	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> +	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
>  
> -	priv->rx_ring[q].desc = dma_alloc_coherent(ndev->dev.parent, ring_size,
> -						   &priv->rx_desc_dma[q],
> -						   GFP_KERNEL);
> -	return priv->rx_ring[q].desc;
> -}
> +	priv->rx_ring[q].raw = dma_alloc_coherent(ndev->dev.parent, ring_size,
> +						  &priv->rx_desc_dma[q],
> +						  GFP_KERNEL);
>  
> -static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	unsigned int ring_size;
> -
> -	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
> -
> -	priv->rx_ring[q].ex_desc = dma_alloc_coherent(ndev->dev.parent,
> -						      ring_size,
> -						      &priv->rx_desc_dma[q],
> -						      GFP_KERNEL);
> -	return priv->rx_ring[q].ex_desc;
> +	return priv->rx_ring[q].raw;
>  }
>  
>  /* Init skb and descriptor buffer for Ethernet AVB */
> @@ -500,7 +436,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  	}
>  
>  	/* Allocate all RX descriptors. */
> -	if (!info->alloc_rx_desc(ndev, q))
> +	if (!ravb_alloc_rx_desc(ndev, q))
>  		goto error;
>  
>  	priv->dirty_rx[q] = 0;
> @@ -2677,9 +2613,6 @@ static int ravb_mdio_release(struct ravb_private *priv)
>  }
>  
>  static const struct ravb_hw_info ravb_gen3_hw_info = {
> -	.rx_ring_free = ravb_rx_ring_free_rcar,
> -	.rx_ring_format = ravb_rx_ring_format_rcar,
> -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
>  	.receive = ravb_rx_rcar,
>  	.set_rate = ravb_set_rate_rcar,
>  	.set_feature = ravb_set_features_rcar,
> @@ -2693,6 +2626,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
>  	.internal_delay = 1,
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
> @@ -2703,9 +2637,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  };
>  
>  static const struct ravb_hw_info ravb_gen2_hw_info = {
> -	.rx_ring_free = ravb_rx_ring_free_rcar,
> -	.rx_ring_format = ravb_rx_ring_format_rcar,
> -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
>  	.receive = ravb_rx_rcar,
>  	.set_rate = ravb_set_rate_rcar,
>  	.set_feature = ravb_set_features_rcar,
> @@ -2719,6 +2650,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
>  	.aligned_tx = 1,
>  	.gptp = 1,
>  	.nc_queues = 1,
> @@ -2726,9 +2658,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  };
>  
>  static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> -	.rx_ring_free = ravb_rx_ring_free_rcar,
> -	.rx_ring_format = ravb_rx_ring_format_rcar,
> -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
>  	.receive = ravb_rx_rcar,
>  	.set_rate = ravb_set_rate_rcar,
>  	.set_feature = ravb_set_features_rcar,
> @@ -2742,6 +2671,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
>  	.rx_max_frame_size = SZ_2K,
>  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
>  	.multi_irqs = 1,
>  	.err_mgmt_irqs = 1,
>  	.gptp = 1,
> @@ -2751,9 +2681,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  };
>  
>  static const struct ravb_hw_info gbeth_hw_info = {
> -	.rx_ring_free = ravb_rx_ring_free_gbeth,
> -	.rx_ring_format = ravb_rx_ring_format_gbeth,
> -	.alloc_rx_desc = ravb_alloc_rx_desc_gbeth,
>  	.receive = ravb_rx_gbeth,
>  	.set_rate = ravb_set_rate_gbeth,
>  	.set_feature = ravb_set_features_gbeth,
> @@ -2767,6 +2694,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.tccr_mask = TCCR_TSRQ0,
>  	.rx_max_frame_size = SZ_8K,
>  	.rx_max_desc_use = 4080,
> +	.rx_desc_size = sizeof(struct ravb_rx_desc),
>  	.aligned_tx = 1,
>  	.tx_counters = 1,
>  	.carrier_counters = 1,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [net-next 6/6] ravb: Unify Rx ring maintenance code paths

[Niklas Söderlund]

Hi Paul,

Thanks for your feedback,

On 2024-02-27 21:11:34 +0000, Paul Barker wrote:
> On 27/02/2024 01:40, Niklas Söderlund wrote:
> > The R-Car and RZ/G2L Rx code paths was split in two separate
> > implementations when support for RZ/G2L was added due to the fact that
> > R-Car uses the extended descriptor format while RZ/G2L uses normal
> > descriptors. This has lead to a duplication of Rx logic with the only
> > difference being the different Rx descriptors types used. The
> > implementation however neglects to take into account that extended
> > descriptors are normal descriptors with additional metadata at the end
> > to carry hardware timestamp information.
> > 
> > The hardware timestamps information is only consumed in the R-Car Rx
> > loop and all the maintenance code around the Rx ring can be shared
> > between the two implementations if the difference in descriptor length
> > is carefully considered.
> > 
> > This change merges the two implementations for Rx ring maintenance by
> > adding a method to access both types of descriptors as normal
> > descriptors, as this part covers all the fields needed for Rx ring
> > maintenance the only difference between using normal or extended
> > descriptor is the size of the memory region to allocate/free and the
> > step size between each descriptor in the ring.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |   5 +-
> >  drivers/net/ethernet/renesas/ravb_main.c | 132 ++++++-----------------
> >  2 files changed, 32 insertions(+), 105 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > index b12b379baf5a..b48935ec7e28 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -1039,9 +1039,6 @@ struct ravb_ptp {
> >  };
> >  
> >  struct ravb_hw_info {
> > -	void (*rx_ring_free)(struct net_device *ndev, int q);
> > -	void (*rx_ring_format)(struct net_device *ndev, int q);
> > -	void *(*alloc_rx_desc)(struct net_device *ndev, int q);
> >  	bool (*receive)(struct net_device *ndev, int *quota, int q);
> >  	void (*set_rate)(struct net_device *ndev);
> >  	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
> > @@ -1055,6 +1052,7 @@ struct ravb_hw_info {
> >  	u32 tccr_mask;
> >  	u32 rx_max_frame_size;
> >  	u32 rx_max_desc_use;
> > +	u32 rx_desc_size;
> >  	unsigned aligned_tx: 1;
> >  
> >  	/* hardware features */
> > @@ -1090,6 +1088,7 @@ struct ravb_private {
> >  	union {
> >  		struct ravb_rx_desc *desc;
> >  		struct ravb_ex_rx_desc *ex_desc;
> > +		void *raw;
> >  	} rx_ring[NUM_RX_QUEUE];
> >  	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
> >  	void *tx_align[NUM_TX_QUEUE];
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index dee51a78cf36..2702455b6cc6 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -200,6 +200,13 @@ static const struct mdiobb_ops bb_ops = {
> >  	.get_mdio_data = ravb_get_mdio_data,
> >  };
> >  
> > +static struct ravb_rx_desc *
> > +ravb_rx_get_desc(struct ravb_private *priv, unsigned int q,
> > +		 unsigned int i)
> > +{
> > +	return priv->rx_ring[q].raw + priv->info->rx_desc_size * i;
> > +}
> > +
> >  /* Free TX skb function for AVB-IP */
> >  static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
> >  {
> > @@ -244,17 +251,17 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
> >  	return free_num;
> >  }
> >  
> > -static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
> > +static void ravb_rx_ring_free(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	unsigned int ring_size;
> >  	unsigned int i;
> >  
> > -	if (!priv->rx_ring[q].desc)
> > +	if (!priv->rx_ring[q].raw)
> >  		return;
> >  
> >  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > -		struct ravb_rx_desc *desc = &priv->rx_ring[q].desc[i];
> > +		struct ravb_rx_desc *desc = ravb_rx_get_desc(priv, q, i);
> >  
> >  		if (!dma_mapping_error(ndev->dev.parent,
> >  				       le32_to_cpu(desc->dptr)))
> > @@ -263,48 +270,21 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
> >  					 priv->info->rx_max_frame_size,
> >  					 DMA_FROM_DEVICE);
> >  	}
> > -	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> > -	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].desc,
> > +	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
> > +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].raw,
> >  			  priv->rx_desc_dma[q]);
> > -	priv->rx_ring[q].desc = NULL;
> > -}
> > -
> > -static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
> > -{
> > -	struct ravb_private *priv = netdev_priv(ndev);
> > -	unsigned int ring_size;
> > -	unsigned int i;
> > -
> > -	if (!priv->rx_ring[q].ex_desc)
> > -		return;
> > -
> > -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > -		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q].ex_desc[i];
> > -
> > -		if (!dma_mapping_error(ndev->dev.parent,
> > -				       le32_to_cpu(desc->dptr)))
> > -			dma_unmap_single(ndev->dev.parent,
> > -					 le32_to_cpu(desc->dptr),
> > -					 priv->info->rx_max_frame_size,
> > -					 DMA_FROM_DEVICE);
> > -	}
> > -	ring_size = sizeof(struct ravb_ex_rx_desc) *
> > -		    (priv->num_rx_ring[q] + 1);
> > -	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q].ex_desc,
> > -			  priv->rx_desc_dma[q]);
> > -	priv->rx_ring[q].ex_desc = NULL;
> > +	priv->rx_ring[q].raw = NULL;
> >  }
> >  
> >  /* Free skb's and DMA buffers for Ethernet AVB */
> >  static void ravb_ring_free(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	const struct ravb_hw_info *info = priv->info;
> >  	unsigned int num_tx_desc = priv->num_tx_desc;
> >  	unsigned int ring_size;
> >  	unsigned int i;
> >  
> > -	info->rx_ring_free(ndev, q);
> > +	ravb_rx_ring_free(ndev, q);
> >  
> >  	if (priv->tx_ring[q]) {
> >  		ravb_tx_free(ndev, q, false);
> > @@ -335,7 +315,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
> >  	priv->tx_skb[q] = NULL;
> >  }
> >  
> > -static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
> > +static void ravb_rx_ring_format(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	struct ravb_rx_desc *rx_desc;
> > @@ -344,11 +324,11 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
> >  	unsigned int i;
> >  
> >  	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> 
> I think `sizeof(*rx_desc)` should be changed to
> `priv->info->rx_desc_size`.

Nice catch, thanks!

> 
> > -	memset(priv->rx_ring[q].desc, 0, rx_ring_size);
> > +	memset(priv->rx_ring[q].raw, 0, rx_ring_size);
> >  	/* Build RX ring buffer */
> >  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >  		/* RX descriptor */
> > -		rx_desc = &priv->rx_ring[q].desc[i];
> > +		rx_desc = ravb_rx_get_desc(priv, q, i);
> >  		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> >  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> >  					  priv->info->rx_max_frame_size,
> > @@ -361,37 +341,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
> >  		rx_desc->dptr = cpu_to_le32(dma_addr);
> >  		rx_desc->die_dt = DT_FEMPTY;
> >  	}
> > -	rx_desc = &priv->rx_ring[q].desc[i];
> > -	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> > -	rx_desc->die_dt = DT_LINKFIX; /* type */
> > -}
> > -
> > -static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
> > -{
> > -	struct ravb_private *priv = netdev_priv(ndev);
> > -	struct ravb_ex_rx_desc *rx_desc;
> > -	unsigned int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> > -	dma_addr_t dma_addr;
> > -	unsigned int i;
> > -
> > -	memset(priv->rx_ring[q].ex_desc, 0, rx_ring_size);
> > -	/* Build RX ring buffer */
> > -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > -		/* RX descriptor */
> > -		rx_desc = &priv->rx_ring[q].ex_desc[i];
> > -		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> > -		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> > -					  priv->info->rx_max_frame_size,
> > -					  DMA_FROM_DEVICE);
> > -		/* We just set the data size to 0 for a failed mapping which
> > -		 * should prevent DMA from happening...
> > -		 */
> > -		if (dma_mapping_error(ndev->dev.parent, dma_addr))
> > -			rx_desc->ds_cc = cpu_to_le16(0);
> > -		rx_desc->dptr = cpu_to_le32(dma_addr);
> > -		rx_desc->die_dt = DT_FEMPTY;
> > -	}
> > -	rx_desc = &priv->rx_ring[q].ex_desc[i];
> > +	rx_desc = ravb_rx_get_desc(priv, q, i);
> >  	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> >  	rx_desc->die_dt = DT_LINKFIX; /* type */
> >  }
> > @@ -400,7 +350,6 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
> >  static void ravb_ring_format(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	const struct ravb_hw_info *info = priv->info;
> >  	unsigned int num_tx_desc = priv->num_tx_desc;
> >  	struct ravb_tx_desc *tx_desc;
> >  	struct ravb_desc *desc;
> > @@ -413,7 +362,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >  	priv->dirty_rx[q] = 0;
> >  	priv->dirty_tx[q] = 0;
> >  
> > -	info->rx_ring_format(ndev, q);
> > +	ravb_rx_ring_format(ndev, q);
> >  
> >  	memset(priv->tx_ring[q], 0, tx_ring_size);
> >  	/* Build TX ring buffer */
> > @@ -439,31 +388,18 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >  	desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
> >  }
> >  
> > -static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
> > +static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	unsigned int ring_size;
> >  
> > -	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> > +	ring_size = priv->info->rx_desc_size * (priv->num_rx_ring[q] + 1);
> >  
> > -	priv->rx_ring[q].desc = dma_alloc_coherent(ndev->dev.parent, ring_size,
> > -						   &priv->rx_desc_dma[q],
> > -						   GFP_KERNEL);
> > -	return priv->rx_ring[q].desc;
> > -}
> > +	priv->rx_ring[q].raw = dma_alloc_coherent(ndev->dev.parent, ring_size,
> > +						  &priv->rx_desc_dma[q],
> > +						  GFP_KERNEL);
> >  
> > -static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
> > -{
> > -	struct ravb_private *priv = netdev_priv(ndev);
> > -	unsigned int ring_size;
> > -
> > -	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
> > -
> > -	priv->rx_ring[q].ex_desc = dma_alloc_coherent(ndev->dev.parent,
> > -						      ring_size,
> > -						      &priv->rx_desc_dma[q],
> > -						      GFP_KERNEL);
> > -	return priv->rx_ring[q].ex_desc;
> > +	return priv->rx_ring[q].raw;
> >  }
> >  
> >  /* Init skb and descriptor buffer for Ethernet AVB */
> > @@ -500,7 +436,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> >  	}
> >  
> >  	/* Allocate all RX descriptors. */
> > -	if (!info->alloc_rx_desc(ndev, q))
> > +	if (!ravb_alloc_rx_desc(ndev, q))
> >  		goto error;
> >  
> >  	priv->dirty_rx[q] = 0;
> > @@ -2677,9 +2613,6 @@ static int ravb_mdio_release(struct ravb_private *priv)
> >  }
> >  
> >  static const struct ravb_hw_info ravb_gen3_hw_info = {
> > -	.rx_ring_free = ravb_rx_ring_free_rcar,
> > -	.rx_ring_format = ravb_rx_ring_format_rcar,
> > -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
> >  	.receive = ravb_rx_rcar,
> >  	.set_rate = ravb_set_rate_rcar,
> >  	.set_feature = ravb_set_features_rcar,
> > @@ -2693,6 +2626,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> >  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> >  	.rx_max_frame_size = SZ_2K,
> >  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> > +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> >  	.internal_delay = 1,
> >  	.tx_counters = 1,
> >  	.multi_irqs = 1,
> > @@ -2703,9 +2637,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> >  };
> >  
> >  static const struct ravb_hw_info ravb_gen2_hw_info = {
> > -	.rx_ring_free = ravb_rx_ring_free_rcar,
> > -	.rx_ring_format = ravb_rx_ring_format_rcar,
> > -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
> >  	.receive = ravb_rx_rcar,
> >  	.set_rate = ravb_set_rate_rcar,
> >  	.set_feature = ravb_set_features_rcar,
> > @@ -2719,6 +2650,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> >  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> >  	.rx_max_frame_size = SZ_2K,
> >  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> > +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> >  	.aligned_tx = 1,
> >  	.gptp = 1,
> >  	.nc_queues = 1,
> > @@ -2726,9 +2658,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> >  };
> >  
> >  static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> > -	.rx_ring_free = ravb_rx_ring_free_rcar,
> > -	.rx_ring_format = ravb_rx_ring_format_rcar,
> > -	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
> >  	.receive = ravb_rx_rcar,
> >  	.set_rate = ravb_set_rate_rcar,
> >  	.set_feature = ravb_set_features_rcar,
> > @@ -2742,6 +2671,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> >  	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> >  	.rx_max_frame_size = SZ_2K,
> >  	.rx_max_desc_use = 2048 - ETH_FCS_LEN + sizeof(__sum16),
> > +	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> >  	.multi_irqs = 1,
> >  	.err_mgmt_irqs = 1,
> >  	.gptp = 1,
> > @@ -2751,9 +2681,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> >  };
> >  
> >  static const struct ravb_hw_info gbeth_hw_info = {
> > -	.rx_ring_free = ravb_rx_ring_free_gbeth,
> > -	.rx_ring_format = ravb_rx_ring_format_gbeth,
> > -	.alloc_rx_desc = ravb_alloc_rx_desc_gbeth,
> >  	.receive = ravb_rx_gbeth,
> >  	.set_rate = ravb_set_rate_gbeth,
> >  	.set_feature = ravb_set_features_gbeth,
> > @@ -2767,6 +2694,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
> >  	.tccr_mask = TCCR_TSRQ0,
> >  	.rx_max_frame_size = SZ_8K,
> >  	.rx_max_desc_use = 4080,
> > +	.rx_desc_size = sizeof(struct ravb_rx_desc),
> >  	.aligned_tx = 1,
> >  	.tx_counters = 1,
> >  	.carrier_counters = 1,
> 
> -- 
> Paul Barker






-- 
Kind Regards,
Niklas Söderlund