axi_dmac: Add support for DMA Scatter-Gather #1198

podgori · 2023-10-12T11:13:31Z

This commit introduces a different interface to submit transfers, using DMA descriptors.

The structure of the DMA descriptor is as follows:

struct dma_desc {
u32 flags,
u32 id,
u64 dest_addr,
u64 src_addr,
u64 next_sg_addr,
u32 y_len,
u32 x_len,
u32 src_stride,
u32 dst_stride,
};

The 'flags' field currently offers two control bits:

bit 0: if set, the transfer will complete after this last descriptor is processed, and the DMA core will go back to idle state; if cleared, the next DMA descriptor pointed to by 'next_sg_addr' will be loaded.
bit 1: if set, an end-of-transfer interrupt will be raised after the memory segment pointed to by this descriptor has been transferred.

The 'id' field corresponds to an identifier of the descriptor.

The 'dest_addr' and 'src_addr' contain the destination and source addresses to use for the transfer, respectively.

The 'x_len' field contains the number of bytes to transfer, minus one.

The 'y_len', 'src_stride' and 'dst_stride' fields are only useful for 2D transfers, and should be set to zero if 2D transfers are not required.

To start a transfer, the address of the first DMA descriptor must be written to register 0x47c and the HWDESC bit of CONTROL register must be set. The Scatter-Gather transfer is queued similarly to the simple transfers, by writing 1 in TRANSFER_SUBMIT.

The Scatter-Gather interface has a dedicated AXI-MM bus configured for read transfers, with its own dedicated clock, which can be asynchronous.

The SG reset is generated by the reset manager to reset the logic after completing any pending transactions on the bus.

When the Scatter-Gather is enabled during runtime, the legacy cyclic functionality of the DMA is disabled.

The Scatter-Gather DMA currently uses a custom DMABUF interface for software control, since the ADI-specific interface only uses contiguous buffers. The software needs to be updated.

PR Type

Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

I have followed the code style guidelines
I have performed a self-review of changes
I have compiled all hdl projects and libraries affected by this PR
I have tested in hardware affected projects, at least on relevant boards
I have commented my code, at least hard-to-understand parts
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe files, Copyright etc)
I have not introduced new Warnings/Critical Warnings on compilation
I have added new hdl testbenches or updated existing ones

podgori · 2023-10-19T07:32:47Z

Added testbench: https://github.com/analogdevicesinc/testbenches/tree/dma_test_sg

gastmaier

The logic matches what's written in the register map and is well explained.
The SG IP options in the Vivado GUI works.
The fmcomms2 project updated to use the SG successfully synthesizes in OCC and non-OCC (tested zed), and there are no warnings related to the SG.
I'm still synthesizing the intel arradio project.

gastmaier · 2023-11-01T12:49:18Z

library/axi_dmac/axi_dmac.v

+  output [ 1:0]                            m_sg_axi_arburst,
+  output [ 2:0]                            m_sg_axi_arprot,
+  output [ 3:0]                            m_sg_axi_arcache,
+  output [AXI_ID_WIDTH_SG-1:0]             m_sg_axi_arid,


Is there a reason for *_axi_arid and *_axi_arlock being outside the "Unused write interface" sections, even though they are always set to 'h0 ?
I would suggest moving these from all interfaces (m_src and m_sg) to their respective "Unused write interface" sections.

I disagree, these signals are from the read interface and should be integrated into it, even though they are not currently used. The m_src and m_sg are dedicated read interfaces, therefore the write section of the AXI interface is not used.

gastmaier · 2023-11-01T12:58:08Z

library/axi_dmac/axi_dmac.v

@@ -257,6 +310,14 @@ module axi_dmac #(
    DMA_DATA_WIDTH_SRC > 32 ? 3 :
    DMA_DATA_WIDTH_SRC > 16 ? 2 :
    DMA_DATA_WIDTH_SRC > 8 ? 1 : 0;
+  localparam BYTES_PER_BEAT_WIDTH_SG = DMA_DATA_WIDTH_SG > 1024 ? 8 :


Here we have some logic for values for width != 64, even though we only support 64 right now, right?

gastmaier · 2023-11-01T17:48:27Z

library/axi_dmac/axi_dmac_hw.tcl

@@ -127,6 +129,22 @@ foreach {suffix group} { \
 # FIFO interface
 set_parameter_property DMA_TYPE_SRC DEFAULT_VALUE 2

+# Scatter-Gather interface
+add_parameter  DMA_AXI_PROTOCOL_SG INTEGER 1


Remove extra space after add_parameter

gastmaier · 2023-11-01T18:24:53Z

library/axi_dmac/bd/bd.tcl

 		set old [get_property "CONFIG.DMA_TYPE_${dir}" $ip]
 		set_property "CONFIG.DMA_TYPE_${dir}" "0" $ip
 		set_property "CONFIG.DMA_AXI_PROTOCOL_${dir}" $axi_protocol $ip
 		set_property "CONFIG.DMA_TYPE_${dir}" $old $ip
 	}

+	# Change the protocol by first enabling the parameter - enabling the SG transfers 
+	set old [get_property "CONFIG.DMA_SG_TRANSFER" $ip]


Why is this called DMA_SG_TRANSFER instead of following the pattern, like DMA_TYPE_SG ?

Because the Scatter-Gather interface has only one type of AXI interface (AXI-MM), unlike the Source interface, which has 3 types selectable through DMA_TYPE_SRC. The parameter is similar to DMA_2D_TRANSFER.

gastmaier · 2023-11-01T18:38:29Z

library/axi_dmac/axi_dmac_response_manager.v

@@ -155,7 +155,7 @@ module axi_dmac_response_manager #(
    end
  end

-  assign response_eot = (state == STATE_WRITE_RESPR) ? req_eot : 1'b1;
+  assign response_eot = (state == STATE_WRITE_RESPR) ? req_eot : 1'b0;


Here you changed the behavior to only tick at the end of the transfer.
Previously, if state was anything but [STATE_WRITE_RESPR and not eot], it was 1'b1.
This change makes sense.

gastmaier · 2023-11-01T19:02:23Z

library/axi_dmac/axi_dmac_transfer.v

@@ -250,8 +306,114 @@ module axi_dmac_transfer #(
  assign req_valid_gated = req_enable & req_valid;
  assign req_ready = req_enable & req_ready_gated;

+  assign req_eot = ctrl_hwdesc == 1'b1 ? (dma_eot & dma_sg_hwdesc_eot) : dma_eot;


dma_sg_hwdesc_eot stays high during the period of the last sg transfers, while dma_eot ticks during 1 clock cycle.

This is to gate the dma_eot requested interrupts with the EOT flag of the descriptor, thus triggering the EOT interrupt only when the descriptor flag is set.

gastmaier · 2023-11-01T19:08:12Z

library/axi_dmac/dmac_sg.v

+  assign sg_ext_resetn = m_axi_aresetn;
+  assign sg_enabled = sg_enable | ~sg_in_ready;
+
+  assign sg_in_ready = hwdesc_state == STATE_IDLE ? 1'b1 : 1'b0;


For those can't we just

assign sg_in_ready = hwdesc_state == STATE_IDLE;

gastmaier · 2023-11-01T19:08:46Z

library/axi_dmac/dmac_sg.v

+  localparam STATE_RECV_DESC = 2;
+  localparam STATE_DESC_READY = 3;
+
+  localparam FLAG_IS_LAST_HWDESC = 1 << 0;


I would name it like MASK or something

gastmaier · 2023-11-01T19:13:16Z

library/axi_dmac/dmac_sg.v

+      src_stride <= 'h00;
+      dest_stride <= 'h00;
+    end else begin
+      if (sg_in_valid && sg_in_ready) begin


Not in love with the sg_in_ready here, I think is better to have the sm condition itself, state == STATE_IDLE.

I wouldn't change it, I think it makes sense to refer to the valid and ready signals together as a condition. Besides, you can easily find the logic behind sg_in_ready.

gastmaier · 2023-11-01T19:24:42Z

library/axi_dmac/dmac_sg.v

+    }),
+    .m_ready({
+      sg_out_ready,
+      fifo_in_ready


The closing parenthesis of the ports list must be right next to the last parenthesis of the last port.

This commit introduces a different interface to submit transfers, using DMA descriptors. The structure of the DMA descriptor is as follows: struct dma_desc { u32 flags, u32 id, u64 dest_addr, u64 src_addr, u64 next_sg_addr, u32 y_len, u32 x_len, u32 src_stride, u32 dst_stride, }; The 'flags' field currently offers two control bits: - bit 0: if set, the transfer will complete after this last descriptor is processed, and the DMA core will go back to idle state; if cleared, the next DMA descriptor pointed to by 'next_sg_addr' will be loaded. - bit 1: if set, an end-of-transfer interrupt will be raised after the memory segment pointed to by this descriptor has been transferred. The 'id' field corresponds to an identifier of the descriptor. The 'dest_addr' and 'src_addr' contain the destination and source addresses to use for the transfer, respectively. The 'x_len' field contains the number of bytes to transfer, minus one. The 'y_len', 'src_stride' and 'dst_stride' fields are only useful for 2D transfers, and should be set to zero if 2D transfers are not required. To start a transfer, the address of the first DMA descriptor must be written to register 0x47c and the HWDESC bit of CONTROL register must be set. The Scatter-Gather transfer is queued similarly to the simple transfers, by writing 1 in TRANSFER_SUBMIT. The Scatter-Gather interface has a dedicated AXI-MM bus configured for read transfers, with its own dedicated clock, which can be asynchronous. The Scatter-Gather reset is generated by the reset manager to reset the logic after completing any pending transactions on the bus. When the Scatter-Gather is enabled during runtime, the legacy cyclic functionality of the DMA is disabled. Signed-off-by: Ionut Podgoreanu <[email protected]>

Signed-off-by: Ionut Podgoreanu <[email protected]>

podgori · 2023-11-22T12:00:57Z

V2:

Cosmetic updates, addressing the requested changes
Updated the DMA Block Diagram with the SG Interface

pcercuei · 2023-12-04T12:11:27Z

Is that good to merge?

podgori requested a review from acostina October 12, 2023 11:13

podgori requested a review from FilipG24 as a code owner October 12, 2023 11:13

podgori added the new feature label Oct 19, 2023

gastmaier self-requested a review October 30, 2023 13:13

podgori force-pushed the dev_dmac_sg_pr branch from f2120d4 to 77f3363 Compare October 30, 2023 22:28

podgori requested review from AndreiGrozav and PopPaul2021 as code owners October 30, 2023 22:28

podgori requested a review from dbogdan October 31, 2023 06:03

gastmaier requested changes Nov 1, 2023

View reviewed changes

podgori added 5 commits November 22, 2023 13:19

util_hbm: Add the SG interface in DMA instances

d44cfab

Signed-off-by: Ionut Podgoreanu <[email protected]>

docs: Include the DMA SG documentation

25a7936

Signed-off-by: Ionut Podgoreanu <[email protected]>

fmcomms2: Enable the scatter-gather DMA core

f27fdb7

Signed-off-by: Ionut Podgoreanu <[email protected]>

arradio: Enable the scatter-gather DMA core

39fcc3a

Signed-off-by: Ionut Podgoreanu <[email protected]>

podgori force-pushed the dev_dmac_sg_pr branch from 77f3363 to 39fcc3a Compare November 22, 2023 11:22

podgori requested review from IuliaCMoldovan, StancaPop and LBFFilho as code owners November 22, 2023 11:22

gastmaier approved these changes Nov 22, 2023

View reviewed changes

acostina approved these changes Dec 4, 2023

View reviewed changes

FilipG24 approved these changes Dec 4, 2023

View reviewed changes

podgori merged commit 9f2a03f into main Dec 4, 2023

podgori deleted the dev_dmac_sg_pr branch December 4, 2023 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

axi_dmac: Add support for DMA Scatter-Gather #1198

axi_dmac: Add support for DMA Scatter-Gather #1198

podgori commented Oct 12, 2023 •

edited

Loading

podgori commented Oct 19, 2023

gastmaier left a comment

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

gastmaier Nov 1, 2023

podgori Nov 22, 2023

podgori commented Nov 22, 2023

pcercuei commented Dec 4, 2023

axi_dmac: Add support for DMA Scatter-Gather #1198

axi_dmac: Add support for DMA Scatter-Gather #1198

Conversation

podgori commented Oct 12, 2023 • edited Loading

PR Type

PR Checklist

podgori commented Oct 19, 2023

gastmaier 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

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

podgori commented Nov 22, 2023

pcercuei commented Dec 4, 2023

podgori commented Oct 12, 2023 •

edited

Loading