-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Add Xilinx Axi Ethernet Lite #95073
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.
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
base: main
Are you sure you want to change the base?
Add Xilinx Axi Ethernet Lite #95073
Conversation
affe6b3
to
491c31a
Compare
FYI @venodela |
if (mdio_xilinx_axi_ethernet_lite_check_busy(config)) { | ||
LOG_ERR("MDIO bus busy!"); | ||
return -ENOSYS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be blocking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This serves as an error check: In case the device is completely stuck, there is no point in starting a new transaction. So we throw an error in this case.
If the device is not stuck, the device should never be busy when read/write are called, as they block until the transaction completes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well if you added a mutex or a semaphore, that would also already stop hinder starting another transfer, while another is ongoing, as it would only be cleared after it finishes. Also how could a mdio controller get stuck? with fixed timings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transactions can time out, which releases the mutex.
The busy state can then be encountered by the next - if any - transaction.
Generally, the core is not supposed to get stuck in an MDIO transaction. I only expect this to happen, e.g., when it is not correctly wired up (this is commonly used in FPGA designs, so this can happen).
I figured printing a clear error message here would help the FPGA engineer notice the issue.
491c31a
to
ffebac8
Compare
ffebac8
to
3cc6a47
Compare
do { | ||
int frag_len = cursor->buf->len; | ||
const uint8_t *frag_data = cursor->buf->data; | ||
|
||
xilinx_axi_ethernet_lite_write_transmit_buffer(config, buffer_addr, frag_data, | ||
frag_len); | ||
|
||
buffer_addr += frag_len; | ||
} while (xilinx_axi_ethernet_lite_cursor_advance(cursor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you are just copying the packet at a specific location, just use net_pkt_read()
directly. in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/ethernet/eth_litex_liteeth.c it is used similar. You don't have to handle each buffer by its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is not guaranteed to work for this IP:
net_pkt_read
uses memcpy()
internally to move data.
memcpy()
uses byte-width loads and stores (e.g., lbu, sbu
on RISC-V) for the last <sizeof(long)
bytes and (in low-footprint configurations) even for the entire copy operation.
Depending on the CPU implementation, this causes narrow AXI bursts to be issued. Narrow bursts are not supported by the IP (see product guide). I am not sure what would happen in this scenario; my best guess is that you will end up with 4 times the last byte instead of the four last bytes in the transmitted packet.
LiteEth does not have the same problem, as it uses a Wishbone bus which does not support narrow bursts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it right now, replacing only the code on the RX side:
static inline int axi_eth_lite_read_to_pkt(const struct axi_eth_lite_config *config,
struct net_pkt *pkt, mem_addr_t buffer_addr,
size_t bytes_to_read)
{
return net_pkt_write(pkt, (void*)buffer_addr, bytes_to_read);
}
The original function was:
static inline int axi_eth_lite_read_to_pkt(const struct axi_eth_lite_config *config,
struct net_pkt *pkt, mem_addr_t buffer_addr,
size_t bytes_to_read)
{
int ret = 0;
for (size_t read_bytes = 0; read_bytes < bytes_to_read; read_bytes += sizeof(uint32_t)) {
uint32_t current_data = axi_eth_lite_read_reg(config, buffer_addr);
size_t bytes_to_write_now = read_bytes + sizeof(uint32_t) > bytes_to_read
? bytes_to_read - read_bytes
: sizeof(uint32_t);
ret += net_pkt_write(pkt, ¤t_data, bytes_to_write_now);
if (ret < 0) {
LOG_ERR("Write error bytes %zu/%zu (%zu)", read_bytes, bytes_to_read,
bytes_to_write_now);
} else {
LOG_DBG("Write OK bytes %zu/%zu (%zu) cursor %p remaining %d", read_bytes,
bytes_to_read, bytes_to_write_now, pkt->cursor.buf,
pkt->cursor.buf ? pkt->cursor.buf->size - pkt->cursor.buf->len : 0);
}
buffer_addr += sizeof(uint32_t);
}
return ret;
}
The results were interesting: using net_pkt_write
directly instead of in a loop causes no problems for some packets, but breaks reception for other packets:
My assumption is that - as I explained above - packets with a length that is a multiple of sizeof(long)
like ARP can be copied with no issue, but packets with different lengths cause issues due to byte-wise read and write operations.
Again, the results might be different depending on the implementation of the CPU, bus, minor revisions of the IP, ..., so I would be hesistant to use this approach even if it did work on my machine.
Especially as this can cause reception to fail silently, I believe this is a hard-to-troubleshoot bug waiting to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about rounding the size up to the next value dividable by 4. Saw something like that in eth_smsc911x.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would still break if memcpy is implemented something like this:
void *memcpy(void *dst, const void *src, size_t n){
uint8_t *dst_ptr = dst;
const uint8_t *src_ptr = src;
while(n--){
*dst_ptr++ = *src_ptr++;
}
return dst;
}
This is actually the size-optimized implementation of memcpy
in picolibc.
Generally, using any wrapper function for copying to/from the FIFO in the device makes assumptions about how the copying is implemented internally, which I would like to avoid.
Again, the AXI bus exacerbates the problem here. Other buses do not behave like this, so it is a none-issue for many other devices.
hdr = (struct net_eth_hdr *)header_buf; | ||
len = hdr->type; | ||
/* | ||
* AXI Ethernet Lite cannot tell us the length of the received packet, so we try to parse it | ||
* Also, FCS is not used by Zephyr stack | ||
*/ | ||
switch (ntohs(len)) { | ||
case NET_ETH_PTYPE_ARP: | ||
/* fixed length */ | ||
packet_size = | ||
sizeof(struct net_eth_hdr) + XILINX_AXI_ETHERNET_LITE_ARP_PACKET_LENGTH; | ||
break; | ||
case NET_ETH_PTYPE_IP: { | ||
const struct net_ipv4_hdr *ip4_hdr = | ||
(const struct net_ipv4_hdr *)&header_buf[sizeof(*hdr)]; | ||
len = ip4_hdr->len; | ||
packet_size = ntohs(len); | ||
/* length includes ipv4 header length */ | ||
packet_size += sizeof(struct net_eth_hdr); | ||
break; | ||
} | ||
case NET_ETH_PTYPE_IPV6: { | ||
const struct net_ipv6_hdr *ip6_hdr = | ||
(const struct net_ipv6_hdr *)&header_buf[sizeof(*hdr)]; | ||
/* payload + any optional extension headers */ | ||
len = ip6_hdr->len; | ||
packet_size = ntohs(len); | ||
packet_size += sizeof(struct net_eth_hdr) + sizeof(*ip6_hdr); | ||
break; | ||
} | ||
default: | ||
/* use the full MTU... */ | ||
break; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the len of a ethernet packet is always at the same location, independent of independent of the protocol (ARP, ipv4, ipv6) in it.
use that.
https://docs.amd.com/r/en-US/pg135-axi-ethernetlite/Type/Length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type/length field is a part of the Ethernet MAC header.
This only indicates the packet length for the LLC (link-layer control) L2 protocol, a predecessor of modern Ethernet, which is not commonly used any more.
Modern networks generally use this field as a Type indicator (EtherType) instead.
For example, zephyr's network stack always interprets this as a type instead of a length, which is always the same for each protocol irrespective of the packet length.
For that reason, the IP has a separate transmit size MMIO register and does not rely on the type/length field for transmissions.
I have no idea why the same does not exist for RX; this was probably a design oversight.
static inline int | ||
xilinx_axi_ethernet_lite_read_to_pkt(const struct xilinx_axi_ethernet_lite_config *config, | ||
struct net_pkt *pkt, mem_addr_t buffer_addr, | ||
size_t bytes_to_read) | ||
{ | ||
int ret = 0; | ||
|
||
for (size_t read_bytes = 0; read_bytes < bytes_to_read; read_bytes += sizeof(uint32_t)) { | ||
uint32_t current_data = xilinx_axi_ethernet_lite_read_reg(config, buffer_addr); | ||
size_t bytes_to_write_now = read_bytes + sizeof(uint32_t) > bytes_to_read | ||
? bytes_to_read - read_bytes | ||
: sizeof(uint32_t); | ||
|
||
ret += net_pkt_write(pkt, ¤t_data, bytes_to_write_now); | ||
|
||
if (ret < 0) { | ||
LOG_ERR("Write error bytes %zu/%zu (%zu)", read_bytes, bytes_to_read, | ||
bytes_to_write_now); | ||
} else { | ||
LOG_DBG("Write OK bytes %zu/%zu (%zu) cursor %p remaining %d", read_bytes, | ||
bytes_to_read, bytes_to_write_now, pkt->cursor.buf, | ||
pkt->cursor.buf ? pkt->cursor.buf->size - pkt->cursor.buf->len : 0); | ||
} | ||
|
||
buffer_addr += sizeof(uint32_t); | ||
} | ||
return ret; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just read it at one. take a look at https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/ethernet/eth_litex_liteeth.c for reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with byte-size transfers.
struct k_sem tx_sem; | ||
/* used to trigger the RX ISR from time to time */ | ||
struct k_timer rx_timer; | ||
/* used to offload copying an RX packet outside of ISR context */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not copying it in the isr? Would be less overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workqueues are scheduled. Hence, higher-priority threads can preempt them.
ISRs cannot be preempted.
As copying the packet takes many cycles, and the Ethernet Lite is a low-performance core anyway, I figured it made sense to copy packets in a preemptible context.
This commit adds support for the Xilinx AXI Ethernet Lite device, also known as the emaclite. The emaclite is a light-weight 10/100 MII Ethernet device. It can optionally be configured to include an MDIO. The MMIO interface is controlled via MMIO registers and requires the software to busy-wait until completion. Signed-off-by: Eric Ackermann <eric.ackermann@cispa.de>
This commit adds support for the Xilinx AXI Ethernet Lite device, also known as the emaclite. The emaclite is a light-weight 10/100 MII Ethernet device. It foregoes a DMA to reduce chip area. Instead, it uses memory-mapped transmit/receive buffers. A selection of features can optionally be enabled: - a second ("pong") RX/TX Buffer - MDIO support - Interrupt support This driver handles the MAC functionality of the core; a driver for the MDIO part is introduced separately as an MDIO driver. Signed-off-by: Eric Ackermann <eric.ackermann@cispa.de>
3cc6a47
to
634b7e0
Compare
|
This commit adds support for the Xilinx AXI Ethernet Lite device, also known as the emaclite.
The emaclite is a light-weight 10/100 MII Ethernet device commonly found in Xilinx FPGA / SoC designs, as it does not require a separate license.
It foregoes a DMA to reduce chip area. Instead, it uses memory-mapped transmit/receive buffers.
A selection of features can optionally be enabled:
This driver handles the MAC functionality of the core; a driver for the MDIO part is introduced separately as an MDIO driver.
Documentation for the device is available online.
This driver has been tested in an out-of-tree variant of the OpenHW Group cva6 SoC (RISC-V 64) on a Digilent Arty A7 board. In particular, the most commonly used version with all optional features enabled has been tested.
Here is an exemplary device tree entry for this device: