axis_adapter incorrect works with parameter S_KEEP_ENABLE = 0 and when input port s_axis_tkeep = 1'b0

[pkuzmin]

Hi, In the axis_adapter module, if the S_KEEP_ENABLE parameter is disabled, the tkeep input signal should be taken as 1'b1. This was the case in the previous version of the module. In the current version, if the s_axis_tkeep port is set to 1'b0, then this value participates in the logic of the module.

In order to see it in tests, you need to change the test. For example

diff --git a/tb/axis_adapter/Makefile b/tb/axis_adapter/Makefile
index 3e06415..730b77b 100644
--- a/tb/axis_adapter/Makefile
+++ b/tb/axis_adapter/Makefile
@@ -35,7 +35,7 @@ VERILOG_SOURCES += ../../rtl/$(DUT).v
 export PARAM_S_DATA_WIDTH := 8
 export PARAM_S_KEEP_ENABLE := $(shell expr $(PARAM_S_DATA_WIDTH) \> 8 )
 export PARAM_S_KEEP_WIDTH := $(shell expr \( $(PARAM_S_DATA_WIDTH) + 7 \) / 8 )
-export PARAM_M_DATA_WIDTH := 8
+export PARAM_M_DATA_WIDTH := 64
 export PARAM_M_KEEP_ENABLE := $(shell expr $(PARAM_M_DATA_WIDTH) \> 8 )
 export PARAM_M_KEEP_WIDTH := $(shell expr \( $(PARAM_M_DATA_WIDTH) + 7 \) / 8 )
 export PARAM_ID_ENABLE := 1
diff --git a/tb/axis_adapter/test_axis_adapter.py b/tb/axis_adapter/test_axis_adapter.py
index 5997d61..e49f7d1 100644
--- a/tb/axis_adapter/test_axis_adapter.py
+++ b/tb/axis_adapter/test_axis_adapter.py
@@ -88,6 +88,7 @@ async def run_test(dut, payload_lengths=None, payload_data=None, idle_inserter=N

     for test_data in [payload_data(x) for x in payload_lengths()]:
         test_frame = AxiStreamFrame(test_data)
+        if tb.source.byte_lanes == 1: test_frame.tkeep = [0]*len(test_data)
         test_frame.tid = cur_id
         test_frame.tdest = cur_id

Besides, the test script tb/test_axis_adapter_8_64.py with the current version of the module hangs in an infinite loop.

BR, Pavel

[pkuzmin]

I propose this patch for the axis_adapter module

diff --git a/rtl/axis_adapter.v b/rtl/axis_adapter.v
index 0dedfac..c77e930 100644
--- a/rtl/axis_adapter.v
+++ b/rtl/axis_adapter.v
@@ -115,6 +115,8 @@ initial begin
     end
 end

+wire [S_KEEP_WIDTH-1:0] s_axis_tkeep_int = S_KEEP_ENABLE ? s_axis_tkeep : {S_KEEP_WIDTH{1'b1}};
+
 generate

 if (M_BYTE_LANES == S_BYTE_LANES) begin : bypass
@@ -123,7 +125,7 @@ if (M_BYTE_LANES == S_BYTE_LANES) begin : bypass
     assign s_axis_tready = m_axis_tready;

     assign m_axis_tdata  = s_axis_tdata;
-    assign m_axis_tkeep  = M_KEEP_ENABLE ? s_axis_tkeep : {M_KEEP_WIDTH{1'b1}};
+    assign m_axis_tkeep  = M_KEEP_ENABLE ? s_axis_tkeep_int : {M_KEEP_WIDTH{1'b1}};
     assign m_axis_tvalid = s_axis_tvalid;
     assign m_axis_tlast  = s_axis_tlast;
     assign m_axis_tid    = ID_ENABLE   ? s_axis_tid   : {ID_WIDTH{1'b0}};
@@ -175,10 +177,10 @@ end else if (M_BYTE_LANES > S_BYTE_LANES) begin : upsize

             if (seg_reg == 0) begin
                 m_axis_tdata_reg[seg_reg*SEG_DATA_WIDTH +: SEG_DATA_WIDTH] <= s_axis_tvalid_reg ? s_axis_tdata_reg : s_axis_tdata;
-                m_axis_tkeep_reg <= s_axis_tvalid_reg ? s_axis_tkeep_reg : s_axis_tkeep;
+                m_axis_tkeep_reg <= s_axis_tvalid_reg ? s_axis_tkeep_reg : s_axis_tkeep_int;
             end else begin
                 m_axis_tdata_reg[seg_reg*SEG_DATA_WIDTH +: SEG_DATA_WIDTH] <= s_axis_tdata;
-                m_axis_tkeep_reg[seg_reg*SEG_KEEP_WIDTH +: SEG_KEEP_WIDTH] <= s_axis_tkeep;
+                m_axis_tkeep_reg[seg_reg*SEG_KEEP_WIDTH +: SEG_KEEP_WIDTH] <= s_axis_tkeep_int;
             end
             m_axis_tlast_reg <= s_axis_tvalid_reg ? s_axis_tlast_reg : s_axis_tlast;
             m_axis_tid_reg <= s_axis_tvalid_reg ? s_axis_tid_reg : s_axis_tid;
@@ -207,7 +209,7 @@ end else if (M_BYTE_LANES > S_BYTE_LANES) begin : upsize
         end else if (s_axis_tvalid && s_axis_tready) begin
             // store input data in skid buffer
             s_axis_tdata_reg <= s_axis_tdata;
-            s_axis_tkeep_reg <= s_axis_tkeep;
+            s_axis_tkeep_reg <= s_axis_tkeep_int;
             s_axis_tvalid_reg <= 1'b1;
             s_axis_tlast_reg <= s_axis_tlast;
             s_axis_tid_reg <= s_axis_tid;
@@ -264,7 +266,7 @@ end else begin : downsize
             // output register empty

             m_axis_tdata_reg <= s_axis_tvalid_reg ? s_axis_tdata_reg : s_axis_tdata;
-            m_axis_tkeep_reg <= s_axis_tvalid_reg ? s_axis_tkeep_reg : s_axis_tkeep;
+            m_axis_tkeep_reg <= s_axis_tvalid_reg ? s_axis_tkeep_reg : s_axis_tkeep_int;
             m_axis_tlast_reg <= 1'b0;
             m_axis_tid_reg <= s_axis_tvalid_reg ? s_axis_tid_reg : s_axis_tid;
             m_axis_tdest_reg <= s_axis_tvalid_reg ? s_axis_tdest_reg : s_axis_tdest;
@@ -284,7 +286,7 @@ end else begin : downsize
             end else if (s_axis_tvalid && s_axis_tready) begin
                 // buffer is empty; store from input
                 s_axis_tdata_reg <= s_axis_tdata >> SEG_DATA_WIDTH;
-                s_axis_tkeep_reg <= s_axis_tkeep >> SEG_KEEP_WIDTH;
+                s_axis_tkeep_reg <= s_axis_tkeep_int >> SEG_KEEP_WIDTH;
                 s_axis_tlast_reg <= s_axis_tlast;
                 s_axis_tid_reg <= s_axis_tid;
                 s_axis_tdest_reg <= s_axis_tdest;
@@ -292,7 +294,7 @@ end else begin : downsize

                 m_axis_tvalid_reg <= 1'b1;

-                if ((s_axis_tkeep >> SEG_KEEP_WIDTH) == 0) begin
+                if ((s_axis_tkeep_int >> SEG_KEEP_WIDTH) == 0) begin
                     s_axis_tvalid_reg <= 1'b0;
                     m_axis_tlast_reg <= s_axis_tlast;
                 end else begin
@@ -302,7 +304,7 @@ end else begin : downsize
         end else if (s_axis_tvalid && s_axis_tready) begin
             // store input data
             s_axis_tdata_reg <= s_axis_tdata;
-            s_axis_tkeep_reg <= s_axis_tkeep;
+            s_axis_tkeep_reg <= s_axis_tkeep_int;
             s_axis_tvalid_reg <= 1'b1;
             s_axis_tlast_reg <= s_axis_tlast;
             s_axis_tid_reg <= s_axis_tid;

BR, Pavel

[keegandent]

Took me a while to realize this was the same issue I was having. When I lock s_axis_tkeep = 1 suddenly my code using verilog-uart hooked into an 8-32-8 series of adapters for loopback is working instead of only propagating every fourth byte starting with the first sent.

[alexforencich]

Nice catch; I think I have a habit of tying tkeep to 1 even when not used so I didn't trip over this problem myself. I think adding the _int signal is a pretty general solution, I'll go ahead and get that added. Also, I think I will modify the tests to set tkeep to 0 when the signal is disabled to make sure that everything is handled correctly, but basing it on the parameter value instead of the signal width.

[keegandent]

@alexforencich I'm not sure if I should open a new issue about this, but are BYTE_SIZEs that aren't 8 even legal in AXI4-Stream? It seems to me like they aren't according to the spec.

As a consequence of this code

// force keep width to 1 when disabled
localparam S_BYTE_LANES = S_KEEP_ENABLE ? S_KEEP_WIDTH : 1;
localparam M_BYTE_LANES = M_KEEP_ENABLE ? M_KEEP_WIDTH : 1;

// bus byte sizes (must be identical)
localparam S_BYTE_SIZE = S_DATA_WIDTH / S_BYTE_LANES;
localparam M_BYTE_SIZE = M_DATA_WIDTH / M_BYTE_LANES;

// bus width assertions
initial begin
    if (S_BYTE_SIZE * S_BYTE_LANES != S_DATA_WIDTH) begin
        $error("Error: input data width not evenly divisible (instance %m)");
        $finish;
    end

    if (M_BYTE_SIZE * M_BYTE_LANES != M_DATA_WIDTH) begin
        $error("Error: output data width not evenly divisible (instance %m)");
        $finish;
    end

    if (S_BYTE_SIZE != M_BYTE_SIZE) begin
        $error("Error: byte size mismatch (instance %m)");
        $finish;
    end
end

When I set up an 8-16-8 Stream chain, it drops every other byte (and every fourth byte for 8-32-8, and so on) even if I set all KEEP_ENABLEs to off. It turns out some EDA tools (looking at you Quartus) don't throw errors for $error. I've changed the above code to the following in my working copy to avoid dealing with tkeep signals everywhere.

localparam S_BYTE_LANES = (S_DATA_WIDTH + 7) / 8;
localparam M_BYTE_LANES = (M_DATA_WIDTH + 7) / 8;

// bus width assertions
initial begin
    if (8 * S_BYTE_LANES != S_DATA_WIDTH) begin
        $error("Error: input data width not evenly divisible (instance %m)");
        $finish;
    end

    if (8 * M_BYTE_LANES != M_DATA_WIDTH) begin
        $error("Error: output data width not evenly divisible (instance %m)");
        $finish;
    end
end

Maybe it makes sense to have the KEEP_WIDTHs be derived localparams from the DATA_WIDTHs as that is how the spec table shows it.