Skip to content

MCXN ethernet fixes and improvements #93533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 6, 2025
+91 −65
Merged

MCXN ethernet fixes and improvements #93533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d77ed6
drivers: eth_nxp_enet_qos: Fix deadlock in system workqueue
decsny Jul 22, 2025
df42b12
drivers: eth_nxp_enet_qos: Don't block TX send
decsny Jul 23, 2025
8d6a1ef
eth_nxp_enet_qos: Turn on interrupt after init
decsny Jul 25, 2025
9e90136
drivers: eth_nxp_enet_qos: Update as many desc as possible
decsny Jul 17, 2025
5841264
drivers: eth_nxp_enet_qos: Move RX underrun code
decsny Jul 18, 2025
28537a8
eth_nxp_enet_qos: Fixup logging in driver
decsny Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 91 additions & 64 deletions drivers/ethernet/eth_nxp_enet_qos/eth_nxp_enet_qos_mac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ static int eth_nxp_enet_qos_tx(const struct device *dev, struct net_pkt *pkt)

struct net_buf *fragment = pkt->frags;
int frags_count = 0, total_bytes = 0;
int ret;

/* Only allow send of the maximum normal packet size */
while (fragment != NULL) {
Expand All @@ -91,7 +92,14 @@ static int eth_nxp_enet_qos_tx(const struct device *dev, struct net_pkt *pkt)


/* One TX at a time in the current implementation */
k_sem_take(&data->tx.tx_sem, K_FOREVER);
ret = k_sem_take(&data->tx.tx_sem, K_NO_WAIT);
if (ret) {
LOG_DBG("%s TX busy, rejected thread %s", dev->name,
k_thread_name_get(k_current_get()));
return ret;
}
LOG_DBG("Took driver TX sem %p by thread %s", &data->tx.tx_sem,
k_thread_name_get(k_current_get()));

net_pkt_ref(pkt);

Expand Down Expand Up @@ -141,16 +149,19 @@ static int eth_nxp_enet_qos_tx(const struct device *dev, struct net_pkt *pkt)
return 0;
}

static void tx_dma_done(struct k_work *work)
static void tx_dma_done(const struct device *dev)
{
struct nxp_enet_qos_tx_data *tx_data =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is avoiding using a workq to do packet reference freeing in the ISR. It is unfortunate that we can't use the workq for something that is truly a background low priority activity.

Copy link
Member Author

@decsny decsny Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative which I considered if the optimization is important is to make another workqueue for the TX, but it seems a bit like overkill to have a whole thread just to do a bit of code that only does a few memory frees.

I think the http server is really more at fault here, as it was the thing blocking the system workqueue. It seems like it shouldn't be an issue to give a semaphore from a sysworkq item, but I can see the argument now for sure that it could lead to confusing deadlocks in case of other programming mistakes. Seeing as how I spent basically a whole week trying to debug a memory leak in the RX code until realizing there was no memory leak and it was actually a deadlock that had something to do with the TX side. So for the sake of avoiding more situations like this for people to debug, I think we might as well avoid it by not giving a semaphore from the wild west that could become of the system work queue.

CONTAINER_OF(work, struct nxp_enet_qos_tx_data, tx_done_work);
struct nxp_enet_qos_mac_data *data =
CONTAINER_OF(tx_data, struct nxp_enet_qos_mac_data, tx);
struct nxp_enet_qos_mac_data *data = dev->data;
struct nxp_enet_qos_tx_data *tx_data = &data->tx;
struct net_pkt *pkt = tx_data->pkt;
struct net_buf *fragment = pkt->frags;

LOG_DBG("TX DMA completed on packet %p", pkt);
if (pkt == NULL) {
LOG_WRN("%s TX DMA done on nonexistent packet?", dev->name);
goto skip;
} else {
LOG_DBG("TX DMA completed on packet %p", pkt);
}

/* Returning the buffers and packet to the pool */
while (fragment != NULL) {
Expand All @@ -163,63 +174,91 @@ static void tx_dma_done(struct k_work *work)

eth_stats_update_pkts_tx(data->iface);

skip:
/* Allows another send */
k_sem_give(&data->tx.tx_sem);
LOG_DBG("Gave driver TX sem %p by thread %s", &data->tx.tx_sem,
k_thread_name_get(k_current_get()));
}

static enum ethernet_hw_caps eth_nxp_enet_qos_get_capabilities(const struct device *dev)
{
return ETHERNET_LINK_100BASE | ETHERNET_LINK_10BASE | ENET_MAC_PACKET_FILTER_PM_MASK;
}

static bool software_owns_descriptor(volatile union nxp_enet_qos_rx_desc *desc)
{
return (desc->write.control3 & OWN_FLAG) != OWN_FLAG;
}

/* this function resumes the rx dma in case of underrun */
static void enet_qos_dma_rx_resume(const struct device *dev)
{
const struct nxp_enet_qos_mac_config *config = dev->config;
enet_qos_t *base = config->base;
struct nxp_enet_qos_mac_data *data = dev->data;
struct nxp_enet_qos_rx_data *rx_data = &data->rx;

if (!atomic_cas(&rx_data->rbu_flag, 1, 0)) {
/* no RBU flag means no underrun happened */
return;
}

LOG_DBG("Handle RX underrun");

/* We need to just touch the tail ptr to make sure it resumes if underrun happened.
* Then it will resume if there are DMA owned descriptors queued up in the ring properly.
*/
base->DMA_CH[0].DMA_CHX_RXDESC_TAIL_PTR = ENET_QOS_REG_PREP(
DMA_CH_DMA_CHX_RXDESC_TAIL_PTR, RDTP,
ENET_QOS_ALIGN_ADDR_SHIFT((uint32_t)&rx_data->descriptors[NUM_RX_BUFDESC]));
}


static void eth_nxp_enet_qos_rx(struct k_work *work)
{
struct nxp_enet_qos_rx_data *rx_data =
CONTAINER_OF(work, struct nxp_enet_qos_rx_data, rx_work);
struct nxp_enet_qos_mac_data *data =
CONTAINER_OF(rx_data, struct nxp_enet_qos_mac_data, rx);
const struct device *dev = data->iface->if_dev->dev;
volatile union nxp_enet_qos_rx_desc *desc_arr = data->rx.descriptors;
volatile union nxp_enet_qos_rx_desc *desc;
uint32_t desc_idx;
uint32_t desc_idx = rx_data->next_desc_idx;
volatile union nxp_enet_qos_rx_desc *desc = &desc_arr[desc_idx];
struct net_pkt *pkt = NULL;
struct net_buf *new_buf;
struct net_buf *buf;
size_t pkt_len;
size_t processed_len;

LOG_DBG("iteration work:%p, rx_data:%p, data:%p", work, rx_data, data);
/* We are going to find all of the descriptors we own and update them */
for (int i = 0; i < NUM_RX_BUFDESC; i++) {
desc_idx = rx_data->next_desc_idx;
desc = &desc_arr[desc_idx];

if (desc->write.control3 & OWN_FLAG) {
/* The DMA owns the descriptor, we have processed all */
break;
}
LOG_DBG("RX work start: %p", work);

/* Walk through the descriptor ring and refresh the descriptors we own so that the
* DMA can use them for receiving again. We stop when we reach DMA owned part of ring.
*/
while (software_owns_descriptor(desc)) {
rx_data->next_desc_idx = (desc_idx + 1U) % NUM_RX_BUFDESC;

if (pkt == NULL) {
if ((desc->write.control3 & FIRST_DESCRIPTOR_FLAG) !=
FIRST_DESCRIPTOR_FLAG) {
LOG_DBG("receive packet mask %X ",
(desc->write.control3 >> 28) & 0x0f);
LOG_ERR("Rx descriptor does not have first descriptor flag, drop");
desc->read.control = rx_desc_refresh_flags;
/* Error statistics for this packet already updated earlier */
continue;
LOG_DBG("dropping frame from errored packet");
desc->read.control = rx_desc_refresh_flags;
goto next;
}

/* Otherwise, we found a packet that we need to process */
pkt = net_pkt_rx_alloc(K_NO_WAIT);

if (!pkt) {
LOG_ERR("Could not alloc new RX pkt");
LOG_WRN("Could not alloc new RX pkt");
/* error: no new buffer, reuse previous immediately */
desc->read.control = rx_desc_refresh_flags;
eth_stats_update_errors_rx(data->iface);
continue;
goto next;
}

processed_len = 0U;
Expand All @@ -236,13 +275,13 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
pkt_len = desc->write.control3 & DESC_RX_PKT_LEN;
if ((pkt_len < processed_len) ||
((pkt_len - processed_len) > ENET_QOS_RX_BUFFER_SIZE)) {
LOG_ERR("Invalid packet length in descriptor: pkt_len=%u, processed_len=%u",
LOG_WRN("Invalid packet length in descriptor: pkt_len=%u, processed_len=%u",
pkt_len, processed_len);
net_pkt_unref(pkt);
pkt = NULL;
desc->read.control = rx_desc_refresh_flags;
eth_stats_update_errors_rx(data->iface);
continue;
goto next;
}

/* We need to know if we can replace the reserved fragment in advance.
Expand All @@ -257,12 +296,12 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
* we don't know what the upper layer will do to our poor buffer.
* drop this buffer, reuse allocated DMA buffer
*/
LOG_ERR("No new RX buf available");
LOG_WRN("No new RX buf available");
net_pkt_unref(pkt);
pkt = NULL;
desc->read.control = rx_desc_refresh_flags;
eth_stats_update_errors_rx(data->iface);
continue;
goto next;
}

/* Append buffer to a packet */
Expand All @@ -275,7 +314,7 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
/* Propagate completed packet to network stack */
LOG_DBG("Receiving RX packet");
if (net_recv_data(data->iface, pkt)) {
LOG_ERR("RECV failed");
LOG_WRN("RECV failed on pkt %p", pkt);
/* Error during processing, we continue with new buffer */
net_pkt_unref(pkt);
eth_stats_update_errors_rx(data->iface);
Expand All @@ -286,11 +325,15 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
pkt = NULL;
}

LOG_DBG("Swap RX buf");
LOG_DBG("Swap RX buf %p for %p", data->rx.reserved_bufs[desc_idx], new_buf);
/* Allow receive into a new buffer */
data->rx.reserved_bufs[desc_idx] = new_buf;
desc->read.buf1_addr = (uint32_t)new_buf->data;
desc->read.control = rx_desc_refresh_flags;

next:
desc_idx = rx_data->next_desc_idx;
desc = &desc_arr[desc_idx];
}

if (pkt != NULL) {
Expand All @@ -303,29 +346,18 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
eth_stats_update_errors_rx(data->iface);
}

/* try to restart if halted */
const struct device *dev = net_if_get_device(data->iface);

if (atomic_cas(&rx_data->rbu_flag, 1, 0)) {
LOG_DBG("handle RECEIVE BUFFER UNDERRUN in worker");

/* When the DMA reaches the tail pointer, it suspends. Set to last descriptor */
const struct nxp_enet_qos_mac_config *config = dev->config;
enet_qos_t *const base = config->base;
/* now that we updated the descriptors, resume in case we are suspended */
enet_qos_dma_rx_resume(dev);

base->DMA_CH[0].DMA_CHX_RXDESC_TAIL_PTR = ENET_QOS_REG_PREP(
DMA_CH_DMA_CHX_RXDESC_TAIL_PTR, RDTP,
ENET_QOS_ALIGN_ADDR_SHIFT((uint32_t)&rx_data->descriptors[NUM_RX_BUFDESC]));
}

LOG_DBG("end loop normally");
LOG_DBG("End RX work normally");
return;
}

static void eth_nxp_enet_qos_mac_isr(const struct device *dev)
{
const struct nxp_enet_qos_mac_config *config = dev->config;
struct nxp_enet_qos_mac_data *data = dev->data;
struct nxp_enet_qos_rx_data *rx_data = &data->rx;
enet_qos_t *base = config->base;

/* cleared on read */
Expand All @@ -346,28 +378,26 @@ static void eth_nxp_enet_qos_mac_isr(const struct device *dev)
if (ENET_QOS_REG_GET(DMA_INTERRUPT_STATUS, DC0IS, dma_interrupts)) {
if (ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, TI, dma_ch0_interrupts)) {
/* add pending tx to queue */
k_work_submit(&data->tx.tx_done_work);
tx_dma_done(dev);
}

if (ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, RI, dma_ch0_interrupts)) {
/* add pending rx to queue */
k_work_submit_to_queue(&rx_work_queue, &data->rx.rx_work);
k_work_submit_to_queue(&rx_work_queue, &rx_data->rx_work);
}
if (ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, FBE, dma_ch0_interrupts)) {
LOG_ERR("fatal bus error: RX:%x, TX:%x", (dma_ch0_interrupts >> 19) & 0x07,
LOG_ERR("Fatal bus error: RX:%x, TX:%x", (dma_ch0_interrupts >> 19) & 0x07,
(dma_ch0_interrupts >> 16) & 0x07);
}
if (ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, RBU, dma_ch0_interrupts)) {
LOG_ERR("RECEIVE BUFFER UNDERRUN in ISR");
LOG_WRN("RX buffer underrun");
if (!ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, RI, dma_ch0_interrupts)) {
/* RBU migth happen without RI, schedule worker */
k_work_submit_to_queue(&rx_work_queue, &data->rx.rx_work);
/* RBU might happen without RI, schedule worker */
k_work_submit_to_queue(&rx_work_queue, &rx_data->rx_work);
}
}
if (ENET_QOS_REG_GET(DMA_CH_DMA_CHX_STAT, TBU, dma_ch0_interrupts)) {
/* actually this is not an error
* LOG_ERR("TRANSMIT BUFFER UNDERRUN");
*/
/* by design for now */
}
}
}
Expand Down Expand Up @@ -659,13 +689,6 @@ static int eth_nxp_enet_qos_mac_init(const struct device *dev)
break;
}

/* This driver cannot work without interrupts. */
if (config->irq_config_func) {
config->irq_config_func();
} else {
return -ENOSYS;
}

/* Effectively reset of the peripheral */
ret = enet_qos_dma_reset(base);
if (ret) {
Expand Down Expand Up @@ -714,8 +737,12 @@ static int eth_nxp_enet_qos_mac_init(const struct device *dev)
/* Work upon a reception of a packet to a buffer */
k_work_init(&data->rx.rx_work, eth_nxp_enet_qos_rx);

/* Work upon a complete transmission by a channel's TX DMA */
k_work_init(&data->tx.tx_done_work, tx_dma_done);
/* This driver cannot work without interrupts. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test moved to the bottom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see commit message

if (config->irq_config_func) {
config->irq_config_func();
} else {
return -ENOSYS;
}

return ret;
}
Expand Down Expand Up @@ -757,7 +784,7 @@ static int eth_nxp_enet_qos_set_config(const struct device *dev,
net_if_set_link_addr(data->iface, data->mac_addr.addr,
sizeof(data->mac_addr.addr),
NET_LINK_ETHERNET);
LOG_DBG("%s MAC set to %02x:%02x:%02x:%02x:%02x:%02x",
LOG_INF("%s MAC set to %02x:%02x:%02x:%02x:%02x:%02x",
dev->name,
data->mac_addr.addr[0], data->mac_addr.addr[1],
data->mac_addr.addr[2], data->mac_addr.addr[3],
Expand Down
1 change: 0 additions & 1 deletion drivers/ethernet/eth_nxp_enet_qos/nxp_enet_qos_priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ struct nxp_enet_qos_mac_config {
struct nxp_enet_qos_tx_data {
struct k_sem tx_sem;
struct net_pkt *pkt;
struct k_work tx_done_work;
struct net_buf *tx_header;
volatile union nxp_enet_qos_tx_desc descriptors[NUM_TX_BUFDESC];
};
Expand Down