Skip to content
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

sam0_eth: wait until the current tx buffer is no longer in use #20534

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −0
Draft

sam0_eth: wait until the current tx buffer is no longer in use #20534

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions cpu/sam0_common/periph/eth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@
/* GMAC buffer descriptors */
#define GMAC_DESC_ALIGNMENT 8
#define GMAC_BUF_ALIGNMENT 32
static struct eth_buf_desc rx_desc[ETH_RX_BUFFER_COUNT] __attribute__((aligned(GMAC_DESC_ALIGNMENT)));

Check warning on line 81 in cpu/sam0_common/periph/eth.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
static struct eth_buf_desc tx_desc[ETH_TX_BUFFER_COUNT] __attribute__((aligned(GMAC_DESC_ALIGNMENT)));

Check warning on line 82 in cpu/sam0_common/periph/eth.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters

static struct eth_buf_desc *rx_curr;
static struct eth_buf_desc *tx_curr;
Expand All @@ -89,8 +89,8 @@
static uint8_t tx_idx;
static uint8_t rx_idx;

static uint8_t rx_buf[ETH_RX_BUFFER_COUNT][ETH_RX_BUFFER_SIZE] __attribute__((aligned(GMAC_BUF_ALIGNMENT)));

Check warning on line 92 in cpu/sam0_common/periph/eth.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
static uint8_t tx_buf[ETH_TX_BUFFER_COUNT][ETH_TX_BUFFER_SIZE] __attribute__((aligned(GMAC_BUF_ALIGNMENT)));

Check warning on line 93 in cpu/sam0_common/periph/eth.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
extern sam0_eth_netdev_t _sam0_eth_dev;

static bool _is_sleeping;
Expand Down Expand Up @@ -185,6 +185,7 @@
/* Initialize TX buffer descriptors */
for (i=0; i < ETH_TX_BUFFER_COUNT; i++) {
tx_desc[i].address = (uint32_t) tx_buf[i];
tx_desc[i].status = DESC_TX_STATUS_USED;
Copy link
Member

Choose a reason for hiding this comment

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

Why setting TX buffers to one here ? This is suppose to be done by the IP. Software should only clears this bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bit gets set by the hardware when it's done with the buffer.
So this will mark all buffers as unused to the driver.

Copy link
Member

Choose a reason for hiding this comment

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

But GMAC IP will not find an available buffer when it want to send data if they are all marked as used already.
GMAC IP is supposed to take an unused buffer (mark == 0) then take it (mark it to one to tell software this buffer is being used)
This is not software job to set it.

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot_20240403_121021

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bit is set to 0 before starting the transmission.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I missed the inversion in the if statement for the thread_yield()
That's a bit weird but it makes sense.

So do we want really want to block this or drop the new frames (like this is the case for RX) ?

}
/* Set WRAP flag to indicate last buffer */
tx_desc[i-1].status |= DESC_TX_STATUS_WRAP;
Expand Down Expand Up @@ -225,6 +226,11 @@
return -ENOTSUP;
}

/* wait until the current buffer is no longer in use */
while (!(tx_curr->status & DESC_TX_STATUS_USED)) {
thread_yield();
}

/* load packet data into TX buffer */
for (const iolist_t *iol = iolist; iol; iol = iol->iol_next) {
if (tx_len + iol->iol_len > ETHERNET_MAX_LEN) {
Expand Down
Loading