drivers: ethernet: Add Xilinx AXI Enet driver #73986
Conversation
zephyrbot
added
area: Devicetree Binding
zephyrbot
requested review from
decsny,
galak,
henrikbrixandersen,
ibirnbaum,
lmajewski and
pdgendt
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
2 times, most recently
from
bce8faf
to
b8dde37
Compare
|
Note that the Ethernet MAC driver depends on the Ethernet MDIO and the AXI DMA drivers. |
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
2 times, most recently
from
75a28a2
to
e59f91a
Compare
| bool "Xilinx AXI Ethernet Driver" | ||
| default y | ||
| depends on DT_HAS_XLNX_AXI_ETHERNET_7_2_ENABLED || DT_HAS_XLNX_AXI_ETHERNET_1_00_A_ENABLED | ||
| depends on DT_HAS_XLNX_ETH_DMA_ENABLED && XILINX_AXI_DMA |
There was a problem hiding this comment.
Is XILINX_AXI_DMA supposed to be DMA_XILINX_AXI_DMA? @WorldofJARcraft
There was a problem hiding this comment.
Yes, it seems like I forgot to change this in this pull request.
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
from
e59f91a
to
189d3b1
Compare
| depends on DT_HAS_XLNX_ETH_DMA_ENABLED && DMA_XILINX_AXI_DMA | ||
| help | ||
| Enable Xilinx 1G / 2.5G AXI Ethernet driver, | ||
| commonly found on FPGAs. Depends on Xilinx AXI DMA. |
There was a problem hiding this comment.
very minor nit but the dependency should already be visible in the menu, don't think it's needed to clarify in the help string.
There was a problem hiding this comment.
OK, removed it.
| @@ -0,0 +1,577 @@ | |||
|
|
|||
There was a problem hiding this comment.
missing license (and potentially copyright, but license is the big thing)
There was a problem hiding this comment.
License added.
| #define LOG_MODULE_NAME eth_xilinx_axienet | ||
| #define LOG_LEVEL CONFIG_ETHERNET_LOG_LEVEL | ||
| #include <zephyr/logging/log.h> | ||
| LOG_MODULE_REGISTER(LOG_MODULE_NAME); |
There was a problem hiding this comment.
nit but please do the log module register as one line (it can take two arguments for the name and level, dont need to define macros)
| #define XILINX_AXIENET_UNICAST_ADDRESS_WORD_0_OFFSET 0x00000700 | ||
| #define XILINX_AXIENET_UNICAST_ADDRESS_WORD_1_OFFSET 0x00000704 | ||
|
|
||
| #define ETH_ALEN 6 |
There was a problem hiding this comment.
NET_ETH_ADDR_LEN macro already exists, please use that
There was a problem hiding this comment.
Done, must have overlooked this.
| (void)ethdev; | ||
| if (status < 0) { | ||
| LOG_ERR("DMA RX error: %d", status); | ||
| k_panic(); |
There was a problem hiding this comment.
is a dma error code worth causing a kernel panic?
There was a problem hiding this comment.
I do not insist on the panic.
The Xilinx DMA only errors when it was configured incorrectly (e.g., invalid address), so I found this helpful in debugging.
Is there some define I can guard this with?
|
|
||
| if (!pkt) { | ||
| LOG_ERR("Could not allocate a packet!"); | ||
| } else { |
There was a problem hiding this comment.
same comment about the else block
There was a problem hiding this comment.
Panic removed.
| LOG_ERR("Could not write RX buffer into packet!"); | ||
| k_panic(); | ||
| net_pkt_unref(pkt); | ||
| } else { |
There was a problem hiding this comment.
Guard with ifdef?
| if (net_recv_data(data->interface, pkt) < 0) { | ||
| LOG_ERR("Coult not receive packet data!"); | ||
| net_pkt_unref(pkt); | ||
| } else { |
There was a problem hiding this comment.
Guard with ifdef?
| } else { | ||
| if (net_pkt_write(pkt, data->rx_buffer, packet_size)) { | ||
| LOG_ERR("Could not write RX buffer into packet!"); | ||
| k_panic(); |
There was a problem hiding this comment.
ditto kernel panic question, and for all other occurrences in the file
| struct device *ethdev = (struct device *)user_data; | ||
| struct xilinx_axienet_data *data = ethdev->data; | ||
|
|
||
| (void)data; |
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
from
189d3b1
to
34ea455
Compare
decsny
dismissed
their stale review
removing change request because the comments mostly addressed, the panic still seems like a bad idea to me but I don't have a stake in this driver so it's up to author
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
2 times, most recently
from
789595f
to
b1c0676
Compare
|
@WorldofJARcraft Is it on purpose that I'm fairly new to zephyr so forgive if I am missing something. |
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
2 times, most recently
from
f7de377
to
6150685
Compare
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
from
6150685
to
0d3f93e
Compare
|
CI failure is expected as long as the DMA (#73926) has not been merged. As the DMA has uses outside of the Ethernet subsystem (e.g., with custom AXI-Stream IP), I think it should be a separate PR. |
The Xilinx AXI Ethernet subsystem is commonly found in FPGA designs. This patch adds a driver and device tree bindings for the Ethernet MAC core and its MDIO controller. The driver was tested on a RISC-V softcore in an FPGA design, with an RGMII phy and Ethernet subsystem version 7.2 Rev. 14. Device tree bindings match the device tree generated by Vitis hsi. Note that Vitis generates one of the two included compatible strings depending on version. Signed-off-by: Eric Ackermann <eric.ackermann@cispa.de>
WorldofJARcraft
force-pushed
the
xilinx-axienet-mac
branch
from
0d3f93e
to
3ae091e
Compare
|
I have removed the kernel panics from the driver, relying on error messages and appropriate return codes instead. |
The Xilinx AXI Ethernet subsystem is commonly found in FPGA designs. This patch adds a driver and device tree bindings for the Ethernet MAC core.
The driver was tested on a RISC-V softcore in an FPGA design, with an RGMII phy and Ethernet subsystem version 7.2 Rev. 14. Device tree bindings match the device tree generated by Vitis hsi. Note that Vitis generates one of the two included compatible strings depending on version.
Please not that TX checksum offloading depends on #73985.