Axi4 passive vip

[csabakiss-semify]

1. Developed AXI4 VIP supporting passive mode only
2. VIP integrated to the top level environment
3. Added scoreboard to the AXI XBAR

Tested with top level test cases.

Will close issue #168

[martin-velay]

I have added some comments, but I am still not done, I'll do another batch later. There are still some coding style issues

[martin-velay]

Is this 64 bits width linked with top_pkg::AxiDataWidth? It's better to point to a pkg value, this should be applied wherever required in this PR

[csabakiss-semify]

This is ID width to handle when the interconnect extends the ID width to be able to back-route the transactions to the managers. However, good spot, the scoreboard cuts the unnecessary bits before anything is pushed to this queue. Replaced by parameter.

[rswarbrick]

Sorry for the comments about ancient items: I'm working through the history of the PR

I think I understand this now: the 64 bit value is a masked ID, as produced by get_masked_id, which is chopped down from a 64 bit value by taking the AND with AxiIdWidth.

I don't understand why we need a 64 bit index though. Couldn't this array be indexed by bit [top_pkg::AxiIdWidth-1:0] for this dimension? My proposal:

• Add a typedef at the start of the class (for now, at least. Maybe it could be promoted elsewhere in future) to be something like typedef bit [top_pkg::AxiIdWidth-1:0] masked_id_t.
• Change get_masked_id_t to return that type.
• Change the expected_queue to something like protected axi4_vip_item expected_queue[string][masked_id_t][$].
• Do the same for actual_queue.
• Work through the rest of the scoreboard and make sure nothing is getting extra bits.

[rswarbrick]

Here are some initial notes. I've got half way through axi4_vip_monitor.svh from the first commit.

[csabakiss-semify]

@rswarbrick Thank you very much for the detailed review. I believe, all comments are addressed now.

[rswarbrick]

Thanks for addressing lots of comments! To make sure nothing gets lost, the remaining items from me are:

...
(Update some time later: all items that I raised above are resolved)
...

I'll start reviewing the scoreboard commit now.

[rswarbrick]

This seems way more complicated than it needs to be. I suggest getting rid of the loop and writing out the cases explicitly: it will be shorter and do exactly the same thing.

[rswarbrick]

This comment is still open.

[rswarbrick]

This still applies. At the moment, the code Is the equivalent of this:

for (int i = 0; i < 3; i++) {
  if (i == 0) print("zero");
  else if (i == 1) print("one");
  else if (i == 2) print("two");
}

I claim that this is simpler:

print("zero");
print("one");
print("two");

[marnovandermaas]

It would be good to have some documentation on what this VIP does. I think it checks the routing and unmapped memory accesses. Can we do more checks? For example wrong last indicator or too much data sent?

Thanks for this work!

[marnovandermaas]

Can we use the Max defines here for the values?

[csabakiss-semify]

It does not really matter, but I can add it.

[marnovandermaas]

What needs to go here? Do we need a driver initially if we are just monitoring?

[csabakiss-semify]

UVM VIPs have driver, monitor and sequencer. In case of UVM_PASSIVE mode, the driver and the sequncer are not instantiated. This is just a placeholder for UVM_ACTIVE mode.

[rswarbrick]

You're right, but it seems a bit mad to have a placeholder that doesn't do anything. My strong suggestion: delete this fail and make axi4_vip_manager_agent fail with an error if m_cfg.m_manager_active_passive == UVM_ACTIVE.

[marnovandermaas]

Is this a function we actually need to use often?

[csabakiss-semify]

This is just a helper function which eliminates some castings. Actually it is used in the VIP monitor and the scoreboard as well.

[rswarbrick]

I wasn't a big fan of this at first, but then realised that we don't need to bother checking whether the cast on the next line succeeds (which something outside of this class would need to check). This is rather neat!

It needs documentation by the declaration though.

[marnovandermaas]

In which case should we raise an error?

[rswarbrick]

Yes!

@csabakiss-semify: This is still pending.

[marnovandermaas]

This is a useful check.

[marnovandermaas]

Does this just check that any request made by the manager actually gets to the subordinate?

[csabakiss-semify]

The scoreboard checks that transactions are routed according to the address map and compares the attributes of the incoming and outgoing transactions (including the data).

[marnovandermaas]

Can we avoid removing the alignment here?

[marnovandermaas]

This file was added to the core file in the previous commit.

[marnovandermaas]

There's a lot of repetition in this file can we make a function at least to connect up the subordinates.

[marnovandermaas]

Lots of unnecessary reformatting happening in this file.

[martin-velay]

Still some nits to fix, and also you haven't replied to this comment:
https://github.com/lowRISC/mocha/pull/391/changes#r3264215001

[rswarbrick]

Since we don't have any code that needs these variables, it's probably better just to drop them.

[rswarbrick]

This is a bit awkward because the last of the arguments lines up with the body of the function. I'd suggest a slight realignment.

But it's also worth adding some error checking and documentation to the behaviour while we're at it. I propose the following for the declaration:

  // Set the configuration with a single function call
  //
  // Most arguments translate directly to class variables, but widths have a special treatment. The
  // value zero is taken to mean the maximum supported width. For example, passing id_width = 0 is
  // the same as passing id_width = `AXI4_MAX_ID_WIDTH.
  //
  // The arguments all have default values that behave the same as the initial values for the class
  // constructor.
  extern virtual function void
    set_config(string                  inst_id = "AXI4",
               bit                     has_manager = 0,
               uvm_active_passive_enum manager_active_passive = UVM_PASSIVE,
               bit                     has_subordinate = 0,
               uvm_active_passive_enum subordinate_active_passive = UVM_PASSIVE,
               bit                     has_coverage = 0,
               bit                     has_checker = 0,
               int unsigned            id_width = 0,
               int unsigned            addr_width = 0,
               int unsigned            data_width = 0,
               int unsigned            user_width = 0,
               int unsigned            region_width = 0,
               int unsigned            qos_width = 0);

(note this also has a more helpful default inst_id) and then the following definition:

function void
  axi4_vip_cfg::set_config(string                  inst_id = "AXI4",
                           bit                     has_manager = 0,
                           uvm_active_passive_enum manager_active_passive = UVM_PASSIVE,
                           bit                     has_subordinate = 0,
                           uvm_active_passive_enum subordinate_active_passive = UVM_PASSIVE,
                           bit                     has_coverage = 0,
                           bit                     has_checker = 0,
                           int unsigned            id_width = 0,
                           int unsigned            addr_width = 0,
                           int unsigned            data_width = 0,
                           int unsigned            user_width = 0,
                           int unsigned            region_width = 0,
                           int unsigned            qos_width = 0);
  m_inst_id                    = inst_id;
  m_has_manager                = has_manager;
  m_manager_active_passive     = manager_active_passive;
  m_has_subordinate            = has_subordinate;
  m_subordinate_active_passive = subordinate_active_passive;
  m_has_coverage               = has_coverage;
  m_has_checker                = has_checker;

  m_id_width     = translate_width("id_width",     `AXI4_MAX_ID_WIDTH,     id_width);
  m_addr_width   = translate_width("addr_width",   `AXI4_MAX_ADDR_WIDTH,   addr_width);
  m_data_width   = translate_width("data_width",   `AXI4_MAX_DATA_WIDTH,   data_width);
  m_user_width   = translate_width("user_width",   `AXI4_MAX_USER_WIDTH,   user_width);
  m_region_width = translate_width("region_width", `AXI4_MAX_REGION_WIDTH, region_width);
  m_qos_width    = translate_width("qos_width",    `AXI4_MAX_QOS_WIDTH,    qos_width);
endfunction : set_config

using this helper function:

  // Translate a width as provided to set_config.
  //
  // If provided is greater than max_val, this raises an error (involving the name of the field,
  // from field_name). If provided is zero, this returns max_val.
  extern local function int unsigned translate_width(string       field_name,
                                                     int unsigned provided,
                                                     int unsigned max_val);

...

function int unsigned axi4_vip_cfg::translate_width(string       field_name,
                                                    int unsigned provided,
                                                    int unsigned max_val);
  if (provided == 0) return max_val;
  if (provided > max_val) begin
    `uvm_error(m_inst_id,
               $sformatf({"Width for %0s cannot be set to %0d. This is greater than %0d ",
                          "(the maximum supported width for this field)."},
                         field_name, provided, max_val))
    return max_val;
  end
  return provided;
endfunction

[rswarbrick]

You can probably drop this line: it doesn't convey any information.

[rswarbrick]

Again, this comment doesn't convey any information. Probably better to drop it.

[rswarbrick]

You're right, but it seems a bit mad to have a placeholder that doesn't do anything. My strong suggestion: delete this fail and make axi4_vip_manager_agent fail with an error if m_cfg.m_manager_active_passive == UVM_ACTIVE.

[rswarbrick]

I think these parameters should probably be localparams. Genuinely parameterised interfaces are a right pain with UVM (because they don't interact nicely with the virtual interface handles).

I strongly recommend getting rid of the parameters here.

[rswarbrick]

Output arguments (through pointers here) are always confusing. I'm speaking as a recovering C++ programmer :-)

Fortunately, we can pretend we're writing in a functional language. I suggest changing the merge_tx function to merge more explicitly. Something like this, maybe:

  // Return a version of req with AW information from aw_item.
  //
  // The returned item with have dir equal to AXI_WRITE (and so should aw_item).
  extern protected function axi4_vip_item merge_aw(axi4_vip_item req, axi4_vip_item aw_item);

...

function axi4_vip_item axi4_vip_monitor::merge_aw(axi4_vip_item req, axi4_vip_item aw_item);
  axi4_vip_item write_item = req.item_clone();

  if (aw_item.dir != AXI_WRITE) begin
    `uvm_fatal("MON_AW", "Cannot take AW information from non-write aw_item.")
  end

  write_item.dir      = AXI_WRITE;
  write_item.awid     = aw_item.awid;
  write_item.awaddr   = aw_item.awaddr;
  write_item.awlen    = aw_item.awlen;
  write_item.awsize   = aw_item.awsize;
  write_item.awburst  = aw_item.awburst;
  write_item.awlock   = aw_item.awlock;
  write_item.awcache  = aw_item.awcache;
  write_item.awprot   = aw_item.awprot;
  write_item.awqos    = aw_item.awqos;
  write_item.awregion = aw_item.awregion;
  write_item.awuser   = aw_item.awuser;

  return write_item;
endfunction : merge_aw

and then the call-site (this line) can look more like this:

      if (w_pending_q.size() > 0) begin
        axi4_vip_item w_tr = w_pending_q.pop_front();
        write_q_by_id[tr.awid].push_back(merge_aw(w_tr, tr));
      end else begin
        aw_pending_q.push_back(tr);
      end

(which is much nicer!)

[rswarbrick]

With my proposal above, I think the code here becomes just:

        if (aw_pending_q.size() > 0) begin
          axi4_vip_item aw_tr = aw_pending_q.pop_front();
          write_q_by_id[tr.awid].push_back(merge_aw(w_burst, aw_tr));
        end else begin
          w_pending_q.push_back(w_burst);
        end

[rswarbrick]

Shouldn't this modification happen before we clone w_burst when merging it with AW data?

[rswarbrick]

This could be simpler with some typedefs too. For the AW channel, I suggested this. Updating the typedefs to include data, strb and user gives:

  typedef bit [      `AXI4_MAX_ID_WIDTH-1:0] axi4_id_t;
  typedef bit [    `AXI4_MAX_ADDR_WIDTH-1:0] axi4_addr_t;
  typedef bit [    `AXI4_MAX_USER_WIDTH-1:0] axi4_user_t;
  typedef bit [  `AXI4_MAX_REGION_WIDTH-1:0] axi4_region_t;
  typedef bit [     `AXI4_MAX_QOS_WIDTH-1:0] axi4_qos_t;
  typedef bit [    `AXI4_MAX_DATA_WIDTH-1:0] axi4_data_t;
  typedef bit [(`AXI4_MAX_DATA_WIDTH/8)-1:0] axi4_strb_t;
  typedef bit [    `AXI4_MAX_USER_WIDTH-1:0] axi4_user_t;

Then I think the code here could be just:

  axi4_vip_item w_burst;
  axi4_data_t   data_mask = (axi4_data_t'(1) << m_cfg.m_data_width) - 1;
  axi4_strb_t   strb_mask = (axi4_strb_t'(1) << m_cfg.m_strb_width) - 1;
  axi4_user_t   user_mask = (axi4_user_t'(1) << m_cfg.m_user_width) - 1;

A bit shorter!