esynr3z / corsair

Control and Status Register map generator for HDL projects
https://corsair.readthedocs.io
MIT License
95 stars 33 forks source link

Verilog AXI read queue for first word fall through fifo. #7

Closed stridge-cruxml closed 1 year ago

stridge-cruxml commented 2 years ago

The current implementation of verilog axi interface does not function correctly for a FWFT fifo.

If csr_data_fifo_rvalid_ff is held high by a fifo (such as with FWFT), there is a cycle mismatch which results in rdata register read a cycle early.

corsair_bug

Snippet of regmap json file:

       {
            "name": "DATA",
            "description": "Data register",
            "bitfields": [
                {
                    "name": "FIFO",
                    "description": "Write to push value to TX FIFO, read to get data from RX FIFO",
                    "reset": 0,
                    "width": 32,
                    "lsb": 0,
                    "access": "rw",
                    "hardware": "q",
                    "enums": []
                }
            ]
        },

Generated code

reg [63:0] rdata_ff;
always @(posedge clk) begin
    if (rst) begin
        rdata_ff <= 64'h0;
    end else if (ren) begin
        case (raddr)
            40'h0: rdata_ff <= csr_id_rdata;
            40'h8: rdata_ff <= csr_data_rdata;
            40'h10: rdata_ff <= csr_stat_rdata;
            40'h18: rdata_ff <= csr_ctrl_rdata;
            default: rdata_ff <= 64'h0;
        endcase
    end else begin
        rdata_ff <= 64'h0;
    end
end
assign rdata = rdata_ff;

//------------------------------------------------------------------------------
// Read data valid
//------------------------------------------------------------------------------
reg rvalid_ff;
always @(posedge clk) begin
    if (rst) begin
        rvalid_ff <= 1'b0;
    end else if (ren && rvalid) begin
        rvalid_ff <= 1'b0;
    end else if (ren) begin
        rvalid_ff <= 1'b1;
    end
end

reg rvalid_drv;
always @(*) begin
    if (csr_data_ren)
        rvalid_drv = csr_data_fifo_rvalid_ff;
    else
        rvalid_drv = rvalid_ff;
end

assign rvalid = rvalid_drv;

It should be made clear if fifo with FWFT is supported or not. Perhaps a read latency field could be added to the reg map? In cases such as FWFT it would be 0. In HW it doesn't make sense to generate something to support both cases even if the resources are pretty negligible.

Xtyll commented 2 years ago

Hello. Thanks for your research regarding the bug. Actually, the problem is not in the AXILite interface, but with the registers block. I will add the test to the registers testbench and will try to fix the behavior

Xtyll commented 2 years ago

@stridge-cruxml I have done some fixes. Opened PR-19 Check it please if you will have a chance.

FYI now interface to FWFT FIFO still uses extra registers just for the compatibility. Refactoring will done in the future.