Samples dropped in axis_read_data.v when ready goes low

[njpillitteri]

I modified the testbench to demonstrate at line 209 as follows

`ifdef TB_VERBOSE $display("test read"); endif

    ready <= 1'b1;
    repeat(5) @(posedge clk);

    axi_rdata   <= {32'd8, 32'd7, 32'd6, 32'd5, 32'd4, 32'd3, 32'd2, 32'd1};
    axi_rvalid  <= 1'b1;
    @(posedge clk);
    axi_rdata   <= {32'd9, 32'd8, 32'd7, 32'd6, 32'd5, 32'd4, 32'd3, 32'd2};
    while ( ~axi_rready) @(posedge clk);
    axi_rvalid  <= 1'b1;
    @(posedge clk);
    axi_rdata   <= 'b0;
    axi_rvalid  <= 1'b0;
    **repeat(5) @(posedge clk);
    ready <= 1'b0;
    repeat(5) @(posedge clk);
    ready <= 1'b1;**

    repeat(15) @(posedge clk);

ifdef TB_VERBOSE $display("END"); endif`

If you look at the marker, valid deasserts, but by the AXIS protocol it needs to be held until ready completes the transfer. In this case the data value 0x3 would be missed by the downstream slave.

I'm speculating, but maybe there's a similar error in axis_write_data (even though I've never seen that interface to the HP port deassert tready)

[bmartini]

This is not really a bug but a design chose. The 'stream' bus uses the ready flag as non-blocking back-pressure and not as a stall signal like the axi bus does. You can see in the axis_write_data module that the stream bus feeds into a fifo using only the valid flag (which would over flow if it was stalled on active) and the ready flag is generated using the half full point in the fifo so it can take a few data words even after the ready flag has been deasserted. This chose was done as I've been using the module in high frequency applications where I didn't want to have long stalling data pipelines.

My feeling is that I don't want to change the interface from the current 'relaxed' back-pressure to a 'stalling' bank-pressure interface. However, I'm open to discussion and can be convinced if there is a good argument made. I could also improve the documentation to make it clearer or I can see about making a conversion module that could be used to convert the 'relaxed' to 'stall' interface as a compromise.

Let me know what you think.

[njpillitteri]

Ah, I didn't realize it was by design.

But I guess I don't understand why it cant be both AXI stream and not stop your pipeline? Xilinx has an AXI Stream FIFO with a half full flag.

As a user I would prefer if it conformed to AXI Stream as opposed to a slightly modified version of it. The way I found this was by just hooking it directly up to an AXI Stream Xilinx FIR and seeing it drop samples.

Also, initial results look good now with the FIR and my simple change (which doesn't affect the pipeline). But I want your review to see if it would negatively affect something else.

[bmartini]

Taking the example of the 'read' path in the AXI Stream. If we feed the output from the AXI Stream into a long, non-stalling pipeline you need to ensure the first use of the valid is filtered by the ready with a binary AND op to ensure the valid does not propagate down the pipeline when the ready is deasserted:

assign using_valid = valid & ready;

This is not much of a problem and is easy to do, however, the problems start with the write path. In the 'relaxed' mode when the ready is deasserted the pipeline will continue to drain and there needs to be something that can catch/use the data even when the ready is now, hence the 'half full' buffer in the write path. If you have the 'stalling' AXI Stream you need to ether put a fifo/buffer between it and the pipeline or you need to use the ready flag as a stall for the pipeline up to the source of the data to make sure you don't lose data. I didn't want to use the stall on a long pipeline and the axis_write_data module already had a buffer so I just used that.

I can see the someone who is trying to use the AXIS module with Xilinx IP might get confused so maybe its a good enough reason to make it an option.

[njpillitteri]

I'd appreciate it. I moved some resampling code that used upfirdn in the PS to zynq-axis with the Xilinx FIR and got a 10x speedup.

I should also mention I've used zynq-axis a lot by directly connecting it to AXIS components in the past, but these were all "streaming cores" (i.e. would never deassert tready).

[njpillitteri]

Okay I think I better appreciate the problem, and specifically how it ties back to the the AXI4 interface (and I imagine burst read/writes).

I think I can make it AXIS compliant by having two Xilinx AXIS FIFOs (and they can be small in depth) on the output of the src and the input to the dst. However, instead of using the s_axis_treadys out of the FIFOs for backpressure, I'll cause backpressure (deassert tready) when the FIFO are almost full (the IP supports occupancy counts). That way no writes will be dropped on the src, and the dst will still have it's adequate FIFO depth.