drivers: dma: Add Xilinx AXI DMA driver #73926

WorldofJARcraft · 2024-06-07T14:50:13Z

The Xilinx AXI DMA Controller is commonly used in FPGA designs. For example, it is a part of the 1G/2.5G AXI Ethernet subsystem. This patch adds a driver for the Xilinx AXI DMA that supports single MM2S and S2MM channels as well as the control and status streams used by the AXI Ethernet subsystem.
I will send a pull request for MAC and MDIO drivers for the AXI Ethernet subsystem shortly.

github-actions · 2024-06-07T14:50:54Z

Hello @WorldofJARcraft, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

decsny · 2024-06-07T17:30:37Z

dts/bindings/dma/xilinx,eth-dma.yaml

why is a different compatible needed?

This comes down to an idiosyncrasy in the Xilinx tools: Vitis can automatically generate a device tree from an (Vivado) FPGA project. This is useful, because MMIO addresses are assigned in the FPGA project and need to be reflected in the device tree. I have noticed that when the DMA is paired with any core other than the AXI Ethernet MAC core from the AXI Ethernet subsystem in the FPGA project, Vitis will generate a compatible string xlnx,axi-dma-1.00.a. However, if the DMA core is used with the MAC core, Vitis will generate a compatible xlnx,eth-dma. The main difference between using the DMA with the Ethernet MAC vs. with any other AXI-Stream module is that in the Ethernet subsystem, the DMA has additional control and status streams that are currently only used for checksum offloading. I hope that gets more clear when I send pull requests for the MAC driver next week.
Whether these additional streams are present is also encoded in the axistream-control-connected property, making the different compatible strings redundant. However, currently (at least until version 2023.2), Vitis will still generate two different compatible strings for the DMA.
I elected to allow both compatibles, because especially on the Zynq MPSOC, someone might use the Vitis-generated device tree for both the first-stage bootloader or u-boot (which is necessary for programming the DMA) and in Zephyr.

I have submitted a pull request for the MAC driver: #73986

decsny · 2024-08-01T14:38:11Z

dts/bindings/dma/xilinx,axi-dma-base.yaml

+  xlnx,addrwidth:
+    type: int
+    default: 32
+    description: DMA address width (64 or 32 bit), defaults to 32 bit


why does it default to 32 bit? should it just be required: true?

Good point, I have changed this accordingly.

saizen408 · 2024-08-27T23:09:43Z

@WorldofJARcraft Is it on purpose that zephyr_library_sources_ifdef(CONFIG_XILINX_AXI_DMA dma_xilinx_axi_dma.c) is not included in zephyr/drivers/dma/CMakeLists.txt?

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

teburd

Some small changes could make this driver a bit more Zephyr-like.

k_oops() is pretty rare to see in a driver, generally if its a programming error __ASSERT(), if its a runtime error propogate the error code up the call stack along with a reasonable log message to inform whats going on.

teburd · 2024-10-01T16:55:07Z

drivers/dma/Kconfig.xilinx_axi_dma

+config DMA_XILINX_AXI_DMA_SG_DESCRIPTOR_NUM_TX
+	int "Number of transfer descriptors allocated for transmission (TX)."
+	depends on DMA_XILINX_AXI_DMA
+	default 1024


This seems like a very large number of scatter gather descriptors as a default, this could use some justification as a comment here.

I changed it to 16, which is what I use on my test platform.
As the DMA is used with a variety of different peripherals, I expect users of this driver to overwrite the value with a value suitable for their specific application anyway.

teburd · 2024-10-01T16:58:39Z

drivers/dma/dma_xilinx_axi_dma.c

+	bool halted = false;
+
+	/* running ISR in parallel could cause issues with the metadata */
+	const int irq_key = irq_lock();


Do you really need to lock all interrupts or is masking the dma interrupt enough?

Depending on the application, this function might be called from interrupt handlers triggered by different devices, periodically using threads or timers or in any other possible context. Thus, I think locking all IRQs generally is the safest action.
I will wrap this into a configuration variable, such that users of the driver can choose to only mask the DMA interrupt on the same or both channels, respectively, if their application allows it.

teburd · 2024-10-01T17:01:52Z

drivers/dma/dma_xilinx_axi_dma.c

+	if (channel >= cfg->channels) {
+		LOG_ERR("Invalid channel %" PRIu32 " - must be < %" PRIu32 "!", channel,
+			cfg->channels);
+		k_oops();


configuration errors shouldn't be a k_oops(), the error should be propogated up the callstack to the application which can do what it likes, perhaps asserting or panicking (k_oops())

I have removed the oops() and use appropriate error messages / return values instead.

teburd · 2024-10-01T17:02:40Z

drivers/dma/dma_xilinx_axi_dma.c

+		}
+		/* SG descriptors have 64-byte alignment requirements */
+		/* we check this here, were each descriptor */
+		if (nextdesc & XILINX_AXI_DMA_SG_DESCRIPTOR_ADDRESS_MASK) {


__ASSERT() instead, this is a programming error

teburd · 2024-10-01T17:04:54Z

drivers/dma/dma_xilinx_axi_dma.c

+	return 0;
+}
+
+static int dma_xilinx_axi_dma_suspend(const struct device *dev, uint32_t channel)


If this is unimplemented (along with resume) set the function pointers to NULL in the API struct instead, this is dealt with at an API level for you already

e.g. https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/dma.h#L501

WorldofJARcraft · 2024-10-07T08:46:03Z

I have noticed that the DMA in conjunction with the Ethernet driver (#73986) stops receiving Ethernet packets at some point when CONFIG_NET_TC_RX_COUNT is not 0. Please do not merge while I investigate this.

WorldofJARcraft · 2024-10-07T09:37:12Z

I have noticed that the DMA in conjunction with the Ethernet driver (#73986) stops receiving Ethernet packets at some point when CONFIG_NET_TC_RX_COUNT is not 0. Please do not merge while I investigate this.

This is fixed; the cause was resetting the DMA multiple times.

decsny · 2024-10-07T17:46:42Z

dts/bindings/dma/xilinx,eth-dma.yaml

+# SPDX-License-Identifier: Apache-2.0
+
+description: |
+  Xilinx AXI DMA LogiCORE IP controller with compatibility string


why can't the same string be used?

Xilinx' Vitis tool can automatically generate device trees. This feature is often used as a starting point before further customization. The output generated by Vitis is context-dependent; for DMAs used together with the Ethernet subsystem, it generates an eth-dma compatible, for other uses it generates the other, more generic, compatible.
Linux actually binds different drivers for the DMA depending on the compatible: the entirety of the Ethernet subsystem including DMA has one driver, and there is an independent driver for the stand-alone DMA (e.g., useful for custom IPs connected to the DMA).

Small correction: Upstream Linux actually does not actively use the eth-dma compatible. Instead, the Ethernet driver resolves the dma using the axistream-connected handle. Still, using the eth-dma handle prevents the generic DMA driver from claiming the device.
References:

Ethernet driver: https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

DMA driver: https://github.com/torvalds/linux/blob/master/drivers/dma/xilinx/xilinx_dma.c

My other PR soc: add OpenHW Group CVA6 SoC #77732 has an exemplary device tree based on what Vitis generated automatically (notice the eth-dma compatible): https://github.com/zephyrproject-rtos/zephyr/blob/eda4bedfb96b2f07dd65142dc335d247e680b971/dts/riscv/openhwgroup/cv64a6.dtsi

WorldofJARcraft · 2024-10-08T16:35:50Z

I would like to do additional testing before this is eventually merged.
Should I convert this pull request back to a draft, or is it better to add a Do Not Merge label?

decsny · 2024-10-08T17:51:11Z

I would like to do additional testing before this is eventually merged. Should I convert this pull request back to a draft, or is it better to add a Do Not Merge label?

either works

The Xilinx AXI DMA Controller is commonly used in FPGA designs. For example, it is a part of the 1G/2.5G AXI Ethernet subsystem. This patch adds a driver for the Xilinx AXI DMA that supports single MM2S and S2MM channels as well as the control and status streams used by the AXI Ethernet subsystem. Signed-off-by: Eric Ackermann <eric.ackermann@cispa.de>

WorldofJARcraft · 2024-10-30T17:38:30Z

I found a small issue in the Ethernet driver (it allocated a buffer that was slightly too small).
I am confident that the DMA driver works.

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: DMA Direct Memory Access labels Jun 7, 2024

zephyrbot requested review from decsny, galak and teburd June 7, 2024 14:50

zephyrbot assigned teburd Jun 7, 2024

decsny reviewed Jun 7, 2024

View reviewed changes

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch 3 times, most recently from 1279281 to edaca6d Compare June 17, 2024 08:32

WorldofJARcraft mentioned this pull request Jun 17, 2024

drivers: ethernet: Add Xilinx AXI Enet driver #73986

Open

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch from edaca6d to 500e561 Compare June 17, 2024 08:36

teburd previously approved these changes Jul 31, 2024

View reviewed changes

decsny reviewed Aug 1, 2024

View reviewed changes

WorldofJARcraft dismissed teburd’s stale review via 1d7260b August 1, 2024 15:26

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch from 500e561 to 1d7260b Compare August 1, 2024 15:26

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch 2 times, most recently from 3017083 to 5e9675d Compare August 29, 2024 13:14

teburd previously approved these changes Aug 29, 2024

View reviewed changes

WorldofJARcraft mentioned this pull request Sep 2, 2024

soc: add OpenHW Group CVA6 SoC #77732

Open

WorldofJARcraft dismissed teburd’s stale review via b0f1bfd September 24, 2024 08:48

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch 2 times, most recently from b0f1bfd to 2c7871f Compare September 30, 2024 12:15

decsny requested a review from teburd October 1, 2024 16:30

teburd requested changes Oct 1, 2024

View reviewed changes

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch from 2c7871f to d9cf04e Compare October 7, 2024 07:54

WorldofJARcraft marked this pull request as draft October 7, 2024 08:44

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch from d9cf04e to 89b98d2 Compare October 7, 2024 09:33

WorldofJARcraft marked this pull request as ready for review October 7, 2024 09:37

teburd previously approved these changes Oct 7, 2024

View reviewed changes

decsny requested changes Oct 7, 2024

View reviewed changes

WorldofJARcraft dismissed teburd’s stale review via 3f7c836 October 8, 2024 10:05

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch 2 times, most recently from 3f7c836 to 0f49ae5 Compare October 8, 2024 15:01

WorldofJARcraft marked this pull request as draft October 9, 2024 06:16

WorldofJARcraft force-pushed the xilinx_axi_dma_driver branch from 0f49ae5 to 0369124 Compare October 17, 2024 15:13

WorldofJARcraft marked this pull request as ready for review October 30, 2024 17:33

zephyrbot added the platform: Xilinx Xilinx label Oct 30, 2024

zephyrbot requested review from henrikbrixandersen, ibirnbaum and michalsimek October 30, 2024 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: dma: Add Xilinx AXI DMA driver #73926

drivers: dma: Add Xilinx AXI DMA driver #73926

WorldofJARcraft commented Jun 7, 2024

github-actions bot commented Jun 7, 2024

decsny Jun 7, 2024

WorldofJARcraft Jun 8, 2024

WorldofJARcraft Jun 10, 2024

decsny Aug 1, 2024

WorldofJARcraft Aug 1, 2024

saizen408 commented Aug 27, 2024

teburd left a comment

teburd Oct 1, 2024

WorldofJARcraft Oct 7, 2024

teburd Oct 1, 2024

WorldofJARcraft Oct 7, 2024

teburd Oct 1, 2024

WorldofJARcraft Oct 7, 2024

teburd Oct 1, 2024

WorldofJARcraft Oct 7, 2024

teburd Oct 1, 2024

WorldofJARcraft Oct 7, 2024

WorldofJARcraft commented Oct 7, 2024

WorldofJARcraft commented Oct 7, 2024

decsny Oct 7, 2024

WorldofJARcraft Oct 8, 2024

WorldofJARcraft Oct 8, 2024

WorldofJARcraft commented Oct 8, 2024

decsny commented Oct 8, 2024

WorldofJARcraft commented Oct 30, 2024

drivers: dma: Add Xilinx AXI DMA driver #73926

Are you sure you want to change the base?

drivers: dma: Add Xilinx AXI DMA driver #73926

Conversation

WorldofJARcraft commented Jun 7, 2024

github-actions bot commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saizen408 commented Aug 27, 2024

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldofJARcraft commented Oct 7, 2024

WorldofJARcraft commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldofJARcraft commented Oct 8, 2024

decsny commented Oct 8, 2024

WorldofJARcraft commented Oct 30, 2024