Bug in 4K boundary crossing detection in DMA modules

[luyong6]

In axi_dma_rd.v and axi_dma_wr.v

Take axi_dma_rd.v as an example:

339         AXI_STATE_START: begin
340             // start state - initiate new AXI transfer
341             if (!m_axi_arvalid) begin
342                 if (op_word_count_reg <= AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK) || AXI_MAX_BURST_SIZE >= 4096) begin
343                     // packet smaller than max burst size
344                     if (addr_reg[12] != addr_plus_count[12]) begin
345                         // crosses 4k boundary
346                         tr_word_count_next = 13'h1000 - addr_reg[11:0];
347                     end else begin
348                         // does not cross 4k boundary
349                         tr_word_count_next = op_word_count_reg;
350                     end
351                 end else begin
352                     // packet larger than max burst size
353                     if (addr_reg[12] != addr_plus_max_burst[12]) begin
354                         // crosses 4k boundary
355                         tr_word_count_next = 13'h1000 - addr_reg[11:0];
356                     end else begin
357                         // does not cross 4k boundary
358                         tr_word_count_next = AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK);
359                     end
360                 end

Line 342, if setting AXI_MAX_BURST_SIZE = 4096, which is the maximum number defined in AXI4 spec, line 342 always returns "true", and then no matter what the remaining length is, the packet is treated as "smaller than max burst size". That means the last burst beat with possible invalid bytes masked by strobe (for write) or ignored (for read).

This issue can be fixed by changing ">=" to ">" or completely removing the expression of || AXI_MAX_BURST_SIZE >= 4096.

[alexforencich]

Not sure what the issue is. If max burst size is larger than 4096, then the only thing that can split a burst is a 4kb address boundary. If I remove that check, then the only line in the lower block that gets hit would be when a 4kb boundary is crossed, and this is handled by the top block in exactly the same way so doing a separate check based on the burst length is not necessary.

[alexforencich]

Think about it like this: this code is splitting the requested transfer into valid bursts. The burst must end either 1. when the configured max burst size is met, 2. when a 4 kb address boundary is reached, or 3. at the end of the transfer.

The max burst size is checked first (op_word_count_reg <= AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK)). If this check passes, then the requested transfer should fit into one burst. Then, the 4k boundary crossing is checked (addr_reg[12] != addr_plus_max_burst[12]). But here's the thing: if AXI_MAX_BURST_SIZE is 4096 or greater, the transfer will either fit into a burst or it will cross a 4kb address boundary, so in this case the max burst size check can be skipped completely because the 4kb address boundary check will ensure that the burst size is capped at 4096, which can only happen when the transfer starts on a 4kb address boundary.

If I remove || AXI_MAX_BURST_SIZE >= 4096, then when AXI_MAX_BURST_SIZE is 4096 or larger and the op_word_count_reg <= AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK) check fails, addr_reg[12] != addr_plus_max_burst[12] will also be true and the burst will end on the address boundary. The line tr_word_count_next = AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK); is unreachable. The whole point of || AXI_MAX_BURST_SIZE >= 4096 is to save some logic resources by removing the redundant op_word_count_reg <= AXI_MAX_BURST_SIZE - (addr_reg & OFFSET_MASK) comparison.

[luyong6]

Here is the problematic waveform:
AXI_MAX_BURST_SIZE = 4096
len = 0xfb1c

DATA_WIDTH = 128bit (16Bytes)
Starting Address = 0 (I have appended several higher bits)
There should be 15 full 4K bursts and 1 'end of the transfer'.
However, only two ARVALID were sent and the second ARLEN was wrong.

[alexforencich]

Alright, it looks like my 'optimized' check for 4k boundary crossings is problematic. I will rework that.

[alexforencich]

Try replacing

if (addr_reg[12] != addr_plus_count[12]) begin

with

if (((addr_reg & 13'hfff) + (op_word_count_reg & 13'hfff)) >> 12 != 0 || op_word_count_reg >> 12 != 0) begin

and let me know what happens.

[alexforencich]

I just committed those changes to the repo, try it out and see if that bug is fixed.

[luyong6]

Yes, It has been fixed. Thank you Alex!

[alexforencich]

Good to hear!