Merge pull request #67 from pulp-platform/fix-axi_atop_filter

Fix axi_atop_filter and improve verification master/slave

CHANGELOG.md

@@ -8,16 +8,27 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## Unreleased
 
 ### Added
+- `axi_intf`: Add single-channel assertions to `AXI_BUS_DV`.
 
 ### Changed
 
 ### Fixed
-- `axi_top_filter`: The master interface of this module in one case depended on `aw_ready` before
-  applying `w_valid`, which is a violation of the AXI specification that can lead to deadlocks.
-  This issue has been fixed by removing that dependency.
 - `axi_lite_to_apb`: Fix the interface version (`axi_lite_to_apb_intf`) to match the changes from
   version `0.15.0`.
 - `axi_demux`: When `MaxTrans` was 1, the `IdCounterWidth` became 0.  This has been fixed.
+- `axi_atop_filter`:
+  - The master interface of this module in one case depended on `aw_ready` before applying
+    `w_valid`, which is a violation of the AXI specification that can lead to deadlocks.  This issue
+    has been fixed by removing that dependency.
+  - The slave interface of this module could illegally change the value of B and R beats between
+    valid and handshake.  This has been fixed.
+- `rand_axi_master` (in `axi_test`):
+  - Fix infinite wait in `send_ws` task.
+  - Decouple generation of AWs from sending them.  This allows to apply W beats before or
+    simultaneous with AW beats.
+- `rand_axi_slave` (in `axi_test`):
+  - Decouple receiving of Ws from receiving of AWs.  This allows to receive W beats independent of
+    AW beats.
 
 
 ## 0.15.0 - 2020-02-28

src/axi_atop_filter.sv

@@ -50,14 +50,19 @@ module axi_atop_filter #(
 );
 
   localparam int unsigned COUNTER_WIDTH = $clog2(AxiMaxWriteTxns+1);
-  typedef logic [COUNTER_WIDTH-1:0] cnt_t;
+  typedef struct packed {
+    logic                     underflow;
+    logic [COUNTER_WIDTH-1:0] cnt;
+  } cnt_t;
   cnt_t   w_cnt_d, w_cnt_q;
 
-  typedef enum logic [2:0] { W_FEEDTHROUGH, BLOCK_AW, ABSORB_W, INJECT_B, WAIT_R } w_state_t;
-  w_state_t   w_state_d, w_state_q;
+  typedef enum logic [2:0] {
+    W_FEEDTHROUGH, BLOCK_AW, ABSORB_W, HOLD_B, INJECT_B, WAIT_R
+  } w_state_e;
+  w_state_e   w_state_d, w_state_q;
 
-  typedef enum logic { R_FEEDTHROUGH, INJECT_R } r_state_t;
-  r_state_t   r_state_d, r_state_q;
+  typedef enum logic [1:0] { R_FEEDTHROUGH, INJECT_R, R_HOLD } r_state_e;
+  r_state_e   r_state_d, r_state_q;
 
   typedef logic [AxiIdWidth-1:0] id_t;
   id_t  id_d, id_q;
@@ -70,9 +75,17 @@ module axi_atop_filter #(
   } r_resp_cmd_t;
   r_resp_cmd_t  r_resp_cmd_push, r_resp_cmd_pop;
 
-  logic r_resp_cmd_push_valid,  r_resp_cmd_push_ready,
+  logic aw_without_complete_w_downstream,
+        complete_w_without_aw_downstream,
+        r_resp_cmd_push_valid,  r_resp_cmd_push_ready,
         r_resp_cmd_pop_valid,   r_resp_cmd_pop_ready;
 
+  // An AW without a complete W burst is in-flight downstream if the W counter is > 0 and not
+  // underflowed.
+  assign aw_without_complete_w_downstream = !w_cnt_q.underflow && (w_cnt_q.cnt > 0);
+  // A complete W burst without AW is in-flight downstream if the W counter is -1.
+  assign complete_w_without_aw_downstream = w_cnt_q.underflow && &(w_cnt_q.cnt);
+
   // Manage AW, W, and B channels.
   always_comb begin
     // Defaults:
@@ -95,14 +108,17 @@ module axi_atop_filter #(
     unique case (w_state_q)
       W_FEEDTHROUGH: begin
         // Feed AW channel through if the maximum number of outstanding bursts is not reached.
-        if (w_cnt_q < AxiMaxWriteTxns) begin
+        if (complete_w_without_aw_downstream || (w_cnt_q.cnt < AxiMaxWriteTxns)) begin
           mst_req_o.aw_valid  = slv_req_i.aw_valid;
           slv_resp_o.aw_ready = mst_resp_i.aw_ready;
         end
-        // Feed W channel through if at least one AW request is outstanding or a new non-ATOP AW is
-        // being applied.
-        if ((w_cnt_q > 0) ||
-            (slv_req_i.aw_valid && slv_req_i.aw.atop[5:4] == axi_pkg::ATOP_NONE)) begin
+        // Feed W channel through if ..
+        if (aw_without_complete_w_downstream // .. downstream is missing W bursts ..
+            // .. or a new non-ATOP AW is being applied and there is not already a complete W burst
+            // downstream (to prevent underflows of w_cnt).
+            || ((slv_req_i.aw_valid && slv_req_i.aw.atop[5:4] == axi_pkg::ATOP_NONE)
+                && !complete_w_without_aw_downstream)
+        ) begin
           mst_req_o.w_valid  = slv_req_i.w_valid;
           slv_resp_o.w_ready = mst_resp_i.w_ready;
         end
@@ -118,16 +134,22 @@ module axi_atop_filter #(
             // be emptied before going back to the `W_FEEDTHROUGH` state.
             r_resp_cmd_push_valid = 1'b1;
           end
-          // If there are outstanding W bursts, block the AW channel and let the W bursts complete.
-          if (w_cnt_q > 0) begin
+          // If downstream is missing W beats, block the AW channel and let the W bursts complete.
+          if (aw_without_complete_w_downstream) begin
             w_state_d = BLOCK_AW;
-          // If there are no outstanding W bursts, absorb the W beats for this atomic AW.
+          // If downstream is not missing W beats, absorb the W beats for this atomic AW.
           end else begin
             mst_req_o.w_valid  = 1'b0; // Do not let W beats pass to master port.
             slv_resp_o.w_ready = 1'b1; // Absorb W beats on slave port.
             if (slv_req_i.w_valid && slv_req_i.w.last) begin
               // If the W beat is valid and the last, proceed by injecting the B response.
-              w_state_d = INJECT_B;
+              // However, if there is a non-handshaked B on our response port, we must let that
+              // complete first.
+              if (slv_resp_o.b_valid && !slv_req_i.b_ready) begin
+                w_state_d = HOLD_B;
+              end else begin
+                w_state_d = INJECT_B;
+              end
             end else begin
               // Otherwise continue with absorbing W beats.
               w_state_d = ABSORB_W;
@@ -138,15 +160,19 @@ module axi_atop_filter #(
 
       BLOCK_AW: begin
         // Feed W channel through to let outstanding bursts complete.
-        if (w_cnt_q > 0) begin
+        if (aw_without_complete_w_downstream) begin
           mst_req_o.w_valid  = slv_req_i.w_valid;
           slv_resp_o.w_ready = mst_resp_i.w_ready;
         end else begin
           // If there are no more outstanding W bursts, start absorbing the next W burst.
           slv_resp_o.w_ready = 1'b1;
           if (slv_req_i.w_valid && slv_req_i.w.last) begin
             // If the W beat is valid and the last, proceed by injecting the B response.
-            w_state_d = INJECT_B;
+            if (slv_resp_o.b_valid && !slv_req_i.b_ready) begin
+              w_state_d = HOLD_B;
+            end else begin
+              w_state_d = INJECT_B;
+            end
           end else begin
             // Otherwise continue with absorbing W beats.
             w_state_d = ABSORB_W;
@@ -158,6 +184,17 @@ module axi_atop_filter #(
         // Absorb all W beats of the current burst.
         slv_resp_o.w_ready = 1'b1;
         if (slv_req_i.w_valid && slv_req_i.w.last) begin
+          if (slv_resp_o.b_valid && !slv_req_i.b_ready) begin
+            w_state_d = HOLD_B;
+          end else begin
+            w_state_d = INJECT_B;
+          end
+        end
+      end
+
+      HOLD_B: begin
+        // Proceed with injection of B response upon handshake.
+        if (slv_resp_o.b_valid && slv_req_i.b_ready) begin
           w_state_d = INJECT_B;
         end
       end
@@ -204,8 +241,6 @@ module axi_atop_filter #(
   end
   assign mst_req_o.w = slv_req_i.w;
 
-
-
   // Manage R channel.
   always_comb begin
     // Defaults:
@@ -222,7 +257,9 @@ module axi_atop_filter #(
 
     unique case (r_state_q)
       R_FEEDTHROUGH: begin
-        if (r_resp_cmd_pop_valid) begin
+        if (mst_resp_i.r_valid && !slv_req_i.r_ready) begin
+          r_state_d = R_HOLD;
+        end else if (r_resp_cmd_pop_valid) begin
           // Upon a command to inject an R response, immediately proceed with doing so because there
           // are no ordering requirements with other bursts that may be ongoing on the R channel at
           // this moment.
@@ -248,6 +285,12 @@ module axi_atop_filter #(
         end
       end
 
+      R_HOLD: begin
+        if (mst_resp_i.r_valid && slv_req_i.r_ready) begin
+          r_state_d = R_FEEDTHROUGH;
+        end
+      end
+
       default: r_state_d = R_FEEDTHROUGH;
     endcase
   end
@@ -260,10 +303,15 @@ module axi_atop_filter #(
   always_comb begin
     w_cnt_d = w_cnt_q;
     if (mst_req_o.aw_valid && mst_resp_i.aw_ready) begin
-      w_cnt_d += 1;
+      w_cnt_d.cnt += 1;
+    end
+    if (mst_req_o.w_valid && mst_resp_i.w_ready && mst_req_o.w.last) begin
+      w_cnt_d.cnt -= 1;
     end
-    if (mst_resp_i.b_valid && mst_req_o.b_ready) begin
-      w_cnt_d -= 1;
+    if (w_cnt_q.underflow && (w_cnt_d.cnt == '0)) begin
+      w_cnt_d.underflow = 1'b0;
+    end else if (w_cnt_q.cnt == '0 && &(w_cnt_d.cnt)) begin
+      w_cnt_d.underflow = 1'b1;
     end
   end
 
@@ -272,7 +320,7 @@ module axi_atop_filter #(
       id_q <= '0;
       r_beats_q <= '0;
       r_state_q <= R_FEEDTHROUGH;
-      w_cnt_q <= '0;
+      w_cnt_q <= '{default: '0};
       w_state_q <= W_FEEDTHROUGH;
     end else begin
       id_q <= id_d;

src/axi_intf.sv

@@ -22,7 +22,7 @@ interface AXI_BUS #(
   parameter AXI_ID_WIDTH   = -1,
   parameter AXI_USER_WIDTH = -1
 );
-  
+
   import axi_pkg::*;
 
   localparam AXI_STRB_WIDTH = AXI_DATA_WIDTH / 8;
@@ -188,6 +188,53 @@ interface AXI_BUS_DV #(
     output r_id, r_data, r_resp, r_last, r_user, r_valid, input r_ready
   );
 
+  // Single-Channel Assertions: Signals including valid must not change between valid and handshake.
+  // AW
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_id)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_addr)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_len)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_size)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_burst)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_lock)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_cache)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_prot)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_qos)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_region)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_atop)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> $stable(aw_user)));
+  assert property (@(posedge clk_i) (aw_valid && !aw_ready |=> aw_valid));
+  // W
+  assert property (@(posedge clk_i) ( w_valid && ! w_ready |=> $stable(w_data)));
+  assert property (@(posedge clk_i) ( w_valid && ! w_ready |=> $stable(w_strb)));
+  assert property (@(posedge clk_i) ( w_valid && ! w_ready |=> $stable(w_last)));
+  assert property (@(posedge clk_i) ( w_valid && ! w_ready |=> $stable(w_user)));
+  assert property (@(posedge clk_i) ( w_valid && ! w_ready |=> w_valid));
+  // B
+  assert property (@(posedge clk_i) ( b_valid && ! b_ready |=> $stable(b_id)));
+  assert property (@(posedge clk_i) ( b_valid && ! b_ready |=> $stable(b_resp)));
+  assert property (@(posedge clk_i) ( b_valid && ! b_ready |=> $stable(b_user)));
+  assert property (@(posedge clk_i) ( b_valid && ! b_ready |=> b_valid));
+  // AR
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_id)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_addr)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_len)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_size)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_burst)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_lock)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_cache)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_prot)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_qos)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_region)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> $stable(ar_user)));
+  assert property (@(posedge clk_i) (ar_valid && !ar_ready |=> ar_valid));
+  // R
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> $stable(r_id)));
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> $stable(r_data)));
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> $stable(r_resp)));
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> $stable(r_last)));
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> $stable(r_user)));
+  assert property (@(posedge clk_i) ( r_valid && ! r_ready |=> r_valid));
+
 endinterface
 
 /// An asynchronous AXI4 interface.

src/axi_test.sv

@@ -642,6 +642,7 @@ package axi_test;
     semaphore cnt_sem;
 
     ax_beat_t aw_queue[$],
+              w_queue[$],
               excl_queue[$];
 
     typedef struct packed {
@@ -1004,7 +1005,7 @@ package axi_test;
       end
     endtask
 
-    task send_aws(input int n_writes);
+    task create_aws(input int n_writes);
       automatic logic rand_success;
       repeat (n_writes) begin
         automatic bit excl = 1'b0;
@@ -1039,18 +1040,27 @@ package axi_test;
         end
         tot_w_flight_cnt++;
         aw_queue.push_back(aw_beat);
+        w_queue.push_back(aw_beat);
+      end
+    endtask
+
+    task send_aws(ref logic aw_done);
+      while (!(aw_done && aw_queue.size() == 0)) begin
+        automatic ax_beat_t aw_beat;
+        wait (aw_queue.size() > 0 || (aw_done && aw_queue.size() == 0));
+        aw_beat = aw_queue.pop_front();
         rand_wait(AX_MIN_WAIT_CYCLES, AX_MAX_WAIT_CYCLES);
         drv.send_aw(aw_beat);
       end
     endtask
 
     task send_ws(ref logic aw_done);
-      while (!(aw_done && aw_queue.size() == 0)) begin
+      while (!(aw_done && w_queue.size() == 0)) begin
         automatic ax_beat_t aw_beat;
         automatic addr_t addr;
         static logic rand_success;
-        wait (aw_queue.size() > 0);
-        aw_beat = aw_queue.pop_front();
+        wait (w_queue.size() > 0 || (aw_done && w_queue.size() == 0));
+        aw_beat = w_queue.pop_front();
         addr = aw_beat.ax_addr;
         for (int unsigned i = 0; i < aw_beat.ax_len + 1; i++) begin
           automatic w_beat_t w_beat = new;
@@ -1103,9 +1113,10 @@ package axi_test;
         end
         recv_rs(ar_done, aw_done);
         begin
-          send_aws(n_writes);
+          create_aws(n_writes);
           aw_done = 1'b1;
         end
+        send_aws(aw_done);
         send_ws(aw_done);
         recv_bs(aw_done);
       join
@@ -1143,9 +1154,9 @@ package axi_test;
     typedef axi_driver_t::w_beat_t w_beat_t;
 
     axi_driver_t          drv;
-    rand_ax_beat_queue_t  ar_queue,
-                          b_queue;
+    rand_ax_beat_queue_t  ar_queue;
     ax_beat_t             aw_queue[$];
+    int unsigned          b_wait_cnt;
 
     function new(
       virtual AXI_BUS_DV #(
@@ -1157,7 +1168,7 @@ package axi_test;
     );
       this.drv = new(axi);
       this.ar_queue = new;
-      this.b_queue = new;
+      this.b_wait_cnt = 0;
       this.reset();
     endfunction
 
@@ -1232,9 +1243,7 @@ package axi_test;
           if (w_beat.w_last)
             break;
         end
-        wait (aw_queue.size() > 0);
-        aw_beat = aw_queue.pop_front();
-        b_queue.push(aw_beat.ax_id, aw_beat);
+        b_wait_cnt++;
       end
     endtask
 
@@ -1243,15 +1252,16 @@ package axi_test;
         automatic ax_beat_t aw_beat;
         automatic b_beat_t b_beat = new;
         automatic logic rand_success;
-        wait (!b_queue.empty());
-        aw_beat = b_queue.pop();
+        wait (b_wait_cnt > 0 && (aw_queue.size() != 0));
+        aw_beat = aw_queue.pop_front();
         rand_success = std::randomize(b_beat); assert(rand_success);
         b_beat.b_id = aw_beat.ax_id;
         if (aw_beat.ax_lock) begin
           b_beat.b_resp[0]= $random();
         end
         rand_wait(RESP_MIN_WAIT_CYCLES, RESP_MAX_WAIT_CYCLES);
         drv.send_b(b_beat);
+        b_wait_cnt--;
       end
     endtask