diff mbox series

[net-next,1/5] net: ethernet: ti: am65-cpsw: remove am65_cpsw_nuss_tx_compl_packets_2g()

Message ID 20250217-am65-cpsw-zc-prep-v1-1-ce450a62d64f@kernel.org (mailing list archive)
State Accepted
Commit 9a369ae3d1431a83589dde57323a04692dd7fc12
Delegated to: Netdev Maintainers
Headers show
Series net: ethernet: ti: am65-cpsw: drop multiple functions and code cleanup | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: jpanis@baylibre.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 148 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-02-18--00-00 (tests: 891)

Commit Message

Roger Quadros Feb. 17, 2025, 7:31 a.m. UTC 
The only difference between am65_cpsw_nuss_tx_compl_packets_2g() and
am65_cpsw_nuss_tx_compl_packets() is the usage of spin_lock() and
netdev_tx_completed_queue() + am65_cpsw_nuss_tx_wake at every packet
in the latter.

Insted of having 2 separate functions for TX completion, merge them
into one. This will reduce code duplication and make maintenance easier.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 98 ++++++++------------------------
 1 file changed, 25 insertions(+), 73 deletions(-)

Comments

Simon Horman Feb. 18, 2025, 6:01 p.m. UTC | #1 
On Mon, Feb 17, 2025 at 09:31:46AM +0200, Roger Quadros wrote:
> The only difference between am65_cpsw_nuss_tx_compl_packets_2g() and
> am65_cpsw_nuss_tx_compl_packets() is the usage of spin_lock() and
> netdev_tx_completed_queue() + am65_cpsw_nuss_tx_wake at every packet
> in the latter.
> 
> Insted of having 2 separate functions for TX completion, merge them

nit, in case there is a v2 for some other reason: Instead

> into one. This will reduce code duplication and make maintenance easier.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>

...

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c

...

> @@ -1533,23 +1538,35 @@ static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
>  		if (buf_type == AM65_CPSW_TX_BUF_TYPE_SKB) {
>  			skb = am65_cpsw_nuss_tx_compl_packet_skb(tx_chn, desc_dma);
>  			ndev = skb->dev;
> -			total_bytes = skb->len;
> +			pkt_len = skb->len;
>  			napi_consume_skb(skb, budget);
>  		} else {
>  			xdpf = am65_cpsw_nuss_tx_compl_packet_xdp(common, tx_chn,
>  								  desc_dma, &ndev);
> -			total_bytes = xdpf->len;
> +			pkt_len = xdpf->len;
>  			if (buf_type == AM65_CPSW_TX_BUF_TYPE_XDP_TX)
>  				xdp_return_frame_rx_napi(xdpf);
>  			else
>  				xdp_return_frame(xdpf);
>  		}
> +
> +		total_bytes += pkt_len;
>  		num_tx++;
>  
> -		netif_txq = netdev_get_tx_queue(ndev, chn);
> +		if (!single_port) {
> +			/* as packets from multi ports can be interleaved
> +			 * on the same channel, we have to figure out the
> +			 * port/queue at every packet and report it/wake queue.
> +			 */
> +			netif_txq = netdev_get_tx_queue(ndev, chn);
> +			netdev_tx_completed_queue(netif_txq, 1, pkt_len);
> +			am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
> +		}
> +	}
>  
> +	if (single_port) {
> +		netif_txq = netdev_get_tx_queue(ndev, chn);
>  		netdev_tx_completed_queue(netif_txq, num_tx, total_bytes);
> -
>  		am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
>  	}

Maybe it's not worth it, but it seems that a helper could
avoid duplication of the netif_txq handling immediately above (twice).

Regardless, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index a2b6a30918f6..75b402132e3f 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1502,6 +1502,7 @@  static void am65_cpsw_nuss_tx_wake(struct am65_cpsw_tx_chn *tx_chn, struct net_d
 static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
 					   int chn, unsigned int budget, bool *tdown)
 {
+	bool single_port = AM65_CPSW_IS_CPSW2G(common);
 	enum am65_cpsw_tx_buf_type buf_type;
 	struct device *dev = common->dev;
 	struct am65_cpsw_tx_chn *tx_chn;
@@ -1509,6 +1510,7 @@  static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
 	unsigned int total_bytes = 0;
 	struct net_device *ndev;
 	struct xdp_frame *xdpf;
+	unsigned int pkt_len;
 	struct sk_buff *skb;
 	dma_addr_t desc_dma;
 	int res, num_tx = 0;
@@ -1516,9 +1518,12 @@  static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
 	tx_chn = &common->tx_chns[chn];
 
 	while (true) {
-		spin_lock(&tx_chn->lock);
+		if (!single_port)
+			spin_lock(&tx_chn->lock);
 		res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma);
-		spin_unlock(&tx_chn->lock);
+		if (!single_port)
+			spin_unlock(&tx_chn->lock);
+
 		if (res == -ENODATA)
 			break;
 
@@ -1533,23 +1538,35 @@  static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
 		if (buf_type == AM65_CPSW_TX_BUF_TYPE_SKB) {
 			skb = am65_cpsw_nuss_tx_compl_packet_skb(tx_chn, desc_dma);
 			ndev = skb->dev;
-			total_bytes = skb->len;
+			pkt_len = skb->len;
 			napi_consume_skb(skb, budget);
 		} else {
 			xdpf = am65_cpsw_nuss_tx_compl_packet_xdp(common, tx_chn,
 								  desc_dma, &ndev);
-			total_bytes = xdpf->len;
+			pkt_len = xdpf->len;
 			if (buf_type == AM65_CPSW_TX_BUF_TYPE_XDP_TX)
 				xdp_return_frame_rx_napi(xdpf);
 			else
 				xdp_return_frame(xdpf);
 		}
+
+		total_bytes += pkt_len;
 		num_tx++;
 
-		netif_txq = netdev_get_tx_queue(ndev, chn);
+		if (!single_port) {
+			/* as packets from multi ports can be interleaved
+			 * on the same channel, we have to figure out the
+			 * port/queue at every packet and report it/wake queue.
+			 */
+			netif_txq = netdev_get_tx_queue(ndev, chn);
+			netdev_tx_completed_queue(netif_txq, 1, pkt_len);
+			am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
+		}
+	}
 
+	if (single_port) {
+		netif_txq = netdev_get_tx_queue(ndev, chn);
 		netdev_tx_completed_queue(netif_txq, num_tx, total_bytes);
-
 		am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
 	}
 
@@ -1558,66 +1575,6 @@  static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
 	return num_tx;
 }
 
-static int am65_cpsw_nuss_tx_compl_packets_2g(struct am65_cpsw_common *common,
-					      int chn, unsigned int budget, bool *tdown)
-{
-	enum am65_cpsw_tx_buf_type buf_type;
-	struct device *dev = common->dev;
-	struct am65_cpsw_tx_chn *tx_chn;
-	struct netdev_queue *netif_txq;
-	unsigned int total_bytes = 0;
-	struct net_device *ndev;
-	struct xdp_frame *xdpf;
-	struct sk_buff *skb;
-	dma_addr_t desc_dma;
-	int res, num_tx = 0;
-
-	tx_chn = &common->tx_chns[chn];
-
-	while (true) {
-		res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma);
-		if (res == -ENODATA)
-			break;
-
-		if (cppi5_desc_is_tdcm(desc_dma)) {
-			if (atomic_dec_and_test(&common->tdown_cnt))
-				complete(&common->tdown_complete);
-			*tdown = true;
-			break;
-		}
-
-		buf_type = am65_cpsw_nuss_buf_type(tx_chn, desc_dma);
-		if (buf_type == AM65_CPSW_TX_BUF_TYPE_SKB) {
-			skb = am65_cpsw_nuss_tx_compl_packet_skb(tx_chn, desc_dma);
-			ndev = skb->dev;
-			total_bytes += skb->len;
-			napi_consume_skb(skb, budget);
-		} else {
-			xdpf = am65_cpsw_nuss_tx_compl_packet_xdp(common, tx_chn,
-								  desc_dma, &ndev);
-			total_bytes += xdpf->len;
-			if (buf_type == AM65_CPSW_TX_BUF_TYPE_XDP_TX)
-				xdp_return_frame_rx_napi(xdpf);
-			else
-				xdp_return_frame(xdpf);
-		}
-		num_tx++;
-	}
-
-	if (!num_tx)
-		return 0;
-
-	netif_txq = netdev_get_tx_queue(ndev, chn);
-
-	netdev_tx_completed_queue(netif_txq, num_tx, total_bytes);
-
-	am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
-
-	dev_dbg(dev, "%s:%u pkt:%d\n", __func__, chn, num_tx);
-
-	return num_tx;
-}
-
 static enum hrtimer_restart am65_cpsw_nuss_tx_timer_callback(struct hrtimer *timer)
 {
 	struct am65_cpsw_tx_chn *tx_chns =
@@ -1633,13 +1590,8 @@  static int am65_cpsw_nuss_tx_poll(struct napi_struct *napi_tx, int budget)
 	bool tdown = false;
 	int num_tx;
 
-	if (AM65_CPSW_IS_CPSW2G(tx_chn->common))
-		num_tx = am65_cpsw_nuss_tx_compl_packets_2g(tx_chn->common, tx_chn->id,
-							    budget, &tdown);
-	else
-		num_tx = am65_cpsw_nuss_tx_compl_packets(tx_chn->common,
-							 tx_chn->id, budget, &tdown);
-
+	num_tx = am65_cpsw_nuss_tx_compl_packets(tx_chn->common,
+						 tx_chn->id, budget, &tdown);
 	if (num_tx >= budget)
 		return budget;