Skip to content

Conversation

@WorldofJARcraft
Copy link
Contributor

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.

@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Xilinx AMD Xilinx labels Jun 10, 2024
@WorldofJARcraft WorldofJARcraft marked this pull request as draft June 10, 2024 09:40
@WorldofJARcraft WorldofJARcraft force-pushed the xilinx-axienet-mac branch 2 times, most recently from bce8faf to b8dde37 Compare June 10, 2024 16:00
@WorldofJARcraft WorldofJARcraft marked this pull request as ready for review June 17, 2024 08:34
@WorldofJARcraft
Copy link
Contributor Author

Note that the Ethernet MAC driver depends on the Ethernet MDIO and the AXI DMA drivers.

@WorldofJARcraft WorldofJARcraft force-pushed the xilinx-axienet-mac branch 2 times, most recently from 75a28a2 to e59f91a Compare June 17, 2024 08:49

Choose a reason for hiding this comment

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

Is XILINX_AXI_DMA supposed to be DMA_XILINX_AXI_DMA? @WorldofJARcraft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seems like I forgot to change this in this pull request.

decsny
decsny previously requested changes Aug 1, 2024
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, removed it.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

NET_ETH_ADDR_LEN macro already exists, please use that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, must have overlooked this.

Copy link
Member

Choose a reason for hiding this comment

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

is a dma error code worth causing a kernel panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

same comment about the else block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Panic removed.

Copy link
Member

Choose a reason for hiding this comment

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

ditto else block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guard with ifdef?

Copy link
Member

Choose a reason for hiding this comment

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

ditto else block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guard with ifdef?

Copy link
Member

Choose a reason for hiding this comment

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

ditto kernel panic question, and for all other occurrences in the file

Copy link
Member

Choose a reason for hiding this comment

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

what does this accomplish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@decsny decsny dismissed their stale review August 14, 2024 20:29

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

@henrikbrixandersen henrikbrixandersen removed their request for review August 20, 2024 08:10
@WorldofJARcraft WorldofJARcraft force-pushed the xilinx-axienet-mac branch 2 times, most recently from 789595f to b1c0676 Compare August 20, 2024 16:10
@saizen408
Copy link

saizen408 commented Aug 27, 2024

@WorldofJARcraft Is it on purpose that zephyr_library_sources_ifdef(CONFIG_XILINX_AXI_ENET eth_xilinx_axi_enet.c) is not included in zephyr/drivers/ethernet/CMakeLists.txt ?

I'm fairly new to zephyr so forgive if I am missing something.

@WorldofJARcraft
Copy link
Contributor Author

@robhancocksed I will fix the merge conflicts and make this PR ready for review.
I was actually waiting for feedback on my proposed API change (#82349). However, I am also happy to merge the branch as-is and change the API later.

@WorldofJARcraft WorldofJARcraft marked this pull request as ready for review March 20, 2025 07:46
@zephyrbot zephyrbot requested a review from maass-hamburg March 20, 2025 07:47
@WorldofJARcraft
Copy link
Contributor Author

I have force-pushed an untested commit with the changes suggested by @maass-hamburg.
I will build and test the driver manually later today.

@WorldofJARcraft
Copy link
Contributor Author

I have fixed a minor issue with the phy_link_state_changed callback and tested the driver on my RISC-V testbed.

@WorldofJARcraft WorldofJARcraft force-pushed the xilinx-axienet-mac branch 2 times, most recently from dceb394 to 2912c53 Compare March 21, 2025 13:49
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 <[email protected]>
(data->rx_completed_buffer_index + 1) % CONFIG_ETH_XILINX_AXIENET_BUFFER_NUM_RX;
size_t current_descriptor = data->rx_completed_buffer_index;

if (!net_if_is_up(data->interface)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to do this after updating rx_completed_buffer_index, otherwise the indexes won't be tracked properly across a down/up cycle..

@kartben kartben merged commit 4342d71 into zephyrproject-rtos:main Mar 24, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Ethernet area: MDIO platform: Xilinx AMD Xilinx

Projects

None yet

Development

Successfully merging this pull request may close these issues.