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

[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.

[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]

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 file and make axi4_vip_manager_agent fail with an error if m_cfg.m_manager_active_passive == UVM_ACTIVE.

[csabakiss-semify]

I can remove it, but there was a discussion about this VIP making it active. Shall I remove the driver at all?

[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]

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're right, but it seems a bit mad to have a placeholder that doesn't do anything. My strong suggestion: delete this file and make axi4_vip_manager_agent fail with an error if m_cfg.m_manager_active_passive == UVM_ACTIVE.

[rswarbrick]

I've just gone through this PR in detail (yet) again, tracking down all the questions that had been asked. The text below gives various items that I think still need dealing with before it's worth going through a proper review of the code again.

I've gone through and resolved items from reviews that have been dealt with. There are a few that I wasn't sure I could tell whether they could be resolved (@marnovandermaas):

• You asked a question about the scoreboard on May 8th. Does the response resolve things for you?
• You asked a question about VIP connections on May 8th. I think the code in question has been made into macro calls, which look reasonably sane to me. Does this match what you were talking about?

Things that I believe are still pending, with links to when they were originally raised:

• The code in this PR doesn't obey the 100 character line limit. Raised on April 27th.
• The passive agent is still defining a driver. Originally raised on May 8th.
• Thanks for taking the code change I suggested for axi4_vip_cfg.svh on May 25th. You didn't pick up the documentation comment for the new function though. I even bothered writing one! Could you take it or add your own?

[csabakiss-semify]

I've just gone through this PR in detail (yet) again, tracking down all the questions that had been asked. The text below gives various items that I think still need dealing with before it's worth going through a proper review of the code again.

I've gone through and resolved items from reviews that have been dealt with. There are a few that I wasn't sure I could tell whether they could be resolved (@marnovandermaas):

• You asked a question about the scoreboard on May 8th. Does the response resolve things for you?
• You asked a question about VIP connections on May 8th. I think the code in question has been made into macro calls, which look reasonably sane to me. Does this match what you were talking about?

Things that I believe are still pending, with links to when they were originally raised:

• The code in this PR doesn't obey the 100 character line limit. Raised on April 27th.
• The passive agent is still defining a driver. Originally raised on May 8th.
• Thanks for taking the code change I suggested for axi4_vip_cfg.svh on May 25th. You didn't pick up the documentation comment for the new function though. I even bothered writing one! Could you take it or add your own?

I believe the code is updated. The only remaining question is the driver. As I replied to the original comment, it is added for future placeholder.

[csabakiss-semify]

I mainly used this command for compilation/build debug and then run other tests using the same format:
dvsim hw/top_chip/dv/top_chip_sim_cfg.hjson -i uart_smoke -t xcelium