drivers: eth_mcux: remove TX thread and allow for multiple TX buffers

remove the TX thread, as this thread can become starved and unable to
reclaim TX buffers from the hardware. Instead reclaim buffers in ISR.
Change eth_tx function to first take from the tx_buf_sem, so that multiple
TX buffer descriptors can be used effectively.

Performance change (as tested with iperf on RT1050)
TCP RX: 44.6Mbps->48.6Mbps
TCP TX: 38.1Mbps->40.7Mbps

Signed-off-by: Daniel DeGrasse <[email protected]>

Author: danieldegrasse

drivers/ethernet/eth_mcux.c

@@ -165,16 +165,15 @@ struct eth_context {
 #endif
 	enet_handle_t enet_handle;
 #if defined(CONFIG_PTP_CLOCK_MCUX)
-	struct net_pkt *ts_tx_pkt;
 	const struct device *ptp_clock;
 	enet_ptp_config_t ptp_config;
 	double clk_ratio;
 	struct k_mutex ptp_mutex;
+	struct k_sem ptp_ts_sem;
 #endif
 	struct k_sem tx_buf_sem;
 	phy_handle_t *phy_handle;
 	struct k_sem rx_thread_sem;
-	struct k_sem tx_thread_sem;
 	enum eth_mcux_phy_state phy_state;
 	bool enabled;
 	bool link_up;
@@ -191,9 +190,6 @@ struct eth_context {
 	K_KERNEL_STACK_MEMBER(rx_thread_stack, ETH_MCUX_RX_THREAD_STACK_SIZE);
 	struct k_thread rx_thread;
 
-	K_KERNEL_STACK_MEMBER(tx_thread_stack, ETH_MCUX_TX_THREAD_STACK_SIZE);
-	struct k_thread tx_thread;
-
 	/* TODO: FIXME. This Ethernet frame sized buffer is used for
 	 * interfacing with MCUX. How it works is that hardware uses
 	 * DMA scatter buffers to receive a frame, and then public
@@ -710,6 +706,9 @@ static int eth_tx(const struct device *dev, struct net_pkt *pkt)
 	bool timestamped_frame;
 #endif
 
+	/* Wait for a TX buffer descriptor to be available */
+	k_sem_take(&context->tx_buf_sem, K_FOREVER);
+
 	k_mutex_lock(&context->tx_frame_buf_mutex, K_FOREVER);
 
 	if (net_pkt_read(pkt, context->tx_frame_buf, total_len)) {
@@ -722,11 +721,16 @@ static int eth_tx(const struct device *dev, struct net_pkt *pkt)
 	timestamped_frame = eth_get_ptp_data(net_pkt_iface(pkt), pkt);
 	if (timestamped_frame) {
 		status = ENET_SendFrame(context->base, &context->enet_handle,
-					  context->tx_frame_buf, total_len, RING_ID, true, NULL);
+					  context->tx_frame_buf, total_len, RING_ID, true, pkt);
 		if (!status) {
-			context->ts_tx_pkt = net_pkt_ref(pkt);
-		} else {
-			context->ts_tx_pkt = NULL;
+			net_pkt_ref(pkt);
+			/*
+			 * Network stack will modify the packet upon return,
+			 * so wait for the packet to be timestamped,
+			 * which will occur within the TX ISR, before
+			 * returning
+			 */
+			k_sem_take(&context->ptp_ts_sem, K_FOREVER);
 		}
 
 	} else
@@ -745,7 +749,6 @@ static int eth_tx(const struct device *dev, struct net_pkt *pkt)
 	}
 
 	k_mutex_unlock(&context->tx_frame_buf_mutex);
-	k_sem_take(&context->tx_buf_sem, K_FOREVER);
 
 	return 0;
 }
@@ -892,7 +895,7 @@ static inline void ts_register_tx_event(struct eth_context *context,
 {
 	struct net_pkt *pkt;
 
-	pkt = context->ts_tx_pkt;
+	pkt = frameinfo->context;
 	if (pkt && atomic_get(&pkt->atomic_ref) > 0) {
 		if (eth_get_ptp_data(net_pkt_iface(pkt), pkt)) {
 			if (frameinfo->isTsAvail) {
@@ -904,6 +907,7 @@ static inline void ts_register_tx_event(struct eth_context *context,
 					frameinfo->timeStamp.second;
 
 				net_if_add_tx_timestamp(pkt);
+				k_sem_give(&context->ptp_ts_sem);
 				k_mutex_unlock(&context->ptp_mutex);
 			}
 		}
@@ -915,7 +919,6 @@ static inline void ts_register_tx_event(struct eth_context *context,
 		}
 	}
 
-	context->ts_tx_pkt = NULL;
 }
 #endif /* CONFIG_PTP_CLOCK_MCUX && CONFIG_NET_L2_PTP */
 
@@ -932,15 +935,12 @@ static void eth_callback(ENET_Type *base, enet_handle_t *handle,
 		k_sem_give(&context->rx_thread_sem);
 		break;
 	case kENET_TxEvent:
-		if (!k_is_in_isr()) {
 #if defined(CONFIG_PTP_CLOCK_MCUX) && defined(CONFIG_NET_L2_PTP)
-			/* Register event */
-			ts_register_tx_event(context, frameinfo);
+		/* Register event */
+		ts_register_tx_event(context, frameinfo);
 #endif /* CONFIG_PTP_CLOCK_MCUX && CONFIG_NET_L2_PTP */
-
-			/* Free the TX buffer. */
-			k_sem_give(&context->tx_buf_sem);
-		}
+		/* Free the TX buffer. */
+		k_sem_give(&context->tx_buf_sem);
 		break;
 	case kENET_ErrEvent:
 		/* Error event: BABR/BABT/EBERR/LC/RL/UN/PLR.  */
@@ -975,37 +975,6 @@ static void eth_rx_thread(void *arg1, void *unused1, void *unused2)
 	}
 }
 
-static void eth_tx_thread(void *arg1, void *unused1, void *unused2)
-{
-	struct eth_context *context = (struct eth_context *)arg1;
-
-	while (1) {
-		if (k_sem_take(&context->tx_thread_sem, K_FOREVER) == 0) {
-			if (context->enet_handle.txReclaimEnable[RING_ID]) {
-				ENET_ReclaimTxDescriptor(context->base,
-					&context->enet_handle, RING_ID);
-			} else {
-				enet_handle_t *handle = &context->enet_handle;
-
-				if (handle->callback != NULL) {
-#if FSL_FEATURE_ENET_QUEUE > 1
-					handle->callback(context->base,
-						handle, 0, kENET_TxEvent,
-						NULL, handle->userData);
-#else
-					handle->callback(context->base,
-						handle, kENET_TxEvent,
-						NULL, handle->userData);
-#endif
-				}
-			}
-			ENET_EnableInterrupts(context->base,
-			  kENET_TxBufferInterrupt | kENET_TxFrameInterrupt);
-		}
-	}
-}
-
-
 #if defined(CONFIG_ETH_MCUX_PHY_RESET)
 static int eth_phy_reset(const struct device *dev)
 {
@@ -1150,14 +1119,14 @@ static int eth_init(const struct device *dev)
 
 #if defined(CONFIG_PTP_CLOCK_MCUX)
 	k_mutex_init(&context->ptp_mutex);
+	k_sem_init(&context->ptp_ts_sem, 0, 1);
 #endif
 	k_mutex_init(&context->rx_frame_buf_mutex);
 	k_mutex_init(&context->tx_frame_buf_mutex);
 
 	k_sem_init(&context->rx_thread_sem, 0, CONFIG_ETH_MCUX_RX_BUFFERS);
-	k_sem_init(&context->tx_thread_sem, 0, CONFIG_ETH_MCUX_TX_BUFFERS);
 	k_sem_init(&context->tx_buf_sem,
-		   0, CONFIG_ETH_MCUX_TX_BUFFERS);
+		   CONFIG_ETH_MCUX_TX_BUFFERS, CONFIG_ETH_MCUX_TX_BUFFERS);
 	k_work_init(&context->phy_work, eth_mcux_phy_work);
 	k_work_init_delayable(&context->delayed_phy_work,
 			      eth_mcux_delayed_phy_work);
@@ -1169,13 +1138,6 @@ static int eth_init(const struct device *dev)
 			K_PRIO_COOP(2),
 			0, K_NO_WAIT);
 	k_thread_name_set(&context->rx_thread, "mcux_eth_rx");
-	k_thread_create(&context->tx_thread, context->tx_thread_stack,
-			K_KERNEL_STACK_SIZEOF(context->tx_thread_stack),
-			eth_tx_thread, (void *) context, NULL, NULL,
-			K_PRIO_COOP(3),
-			0, K_NO_WAIT);
-	k_thread_name_set(&context->tx_thread, "mcux_eth_tx");
-
 	if (context->generate_mac) {
 		context->generate_mac(context->mac_addr);
 	}
@@ -1355,11 +1317,11 @@ static void eth_mcux_common_isr(const struct device *dev)
 	}
 
 	if (EIR & kENET_TxFrameInterrupt) {
-		context->tx_irq_num++;
-		ENET_ClearInterruptStatus(context->base, kENET_TxFrameInterrupt);
-		ENET_DisableInterrupts(context->base, kENET_TxFrameInterrupt);
-		/* schedule tx thread back */
-		k_sem_give(&context->tx_thread_sem);
+#if FSL_FEATURE_ENET_QUEUE > 1
+		ENET_TransmitIRQHandler(context->base, &context->enet_handle, 0);
+#else
+		ENET_TransmitIRQHandler(context->base, &context->enet_handle);
+#endif
 	}
 
 	if (EIR | kENET_TxBufferInterrupt) {
@@ -1400,11 +1362,11 @@ static void eth_mcux_rx_isr(const struct device *dev)
 static void eth_mcux_tx_isr(const struct device *dev)
 {
 	struct eth_context *context = dev->data;
-
-	ENET_DisableInterrupts(context->base, kENET_TxBufferInterrupt | kENET_TxFrameInterrupt);
-	ENET_ClearInterruptStatus(context->base, kENET_TxBufferInterrupt | kENET_TxFrameInterrupt);
-	/* schedule tx thread back */
-	k_sem_give(&context->tx_thread_sem);
+#if FSL_FEATURE_ENET_QUEUE > 1
+	ENET_TransmitIRQHandler(context->base, &context->enet_handle, 0);
+#else
+	ENET_TransmitIRQHandler(context->base, &context->enet_handle);
+#endif
 }
 #endif