Axi4 passive vip by csabakiss-semify · Pull Request #391 · lowRISC/mocha

csabakiss-semify · 2026-03-30T17:44:20Z

Developed AXI4 VIP supporting passive mode only
VIP integrated to the top level environment
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 · 2026-03-31T12:28:04Z

@@ -0,0 +1,306 @@
+  // This AXI4 VIP shall be always UVM_PASSIVE on top level


The copyright header is missing

martin-velay · 2026-03-31T12:34:54Z

+  typedef enum bit[2:0] {
+    mst0 = 0,
+    slv0 = 1,
+    slv1 = 2,
+    slv2 = 3,
+    slv3 = 4
+  } axi_if_t;


Would it be possible to change the field names into something more explicit like that maybe:

Suggested change

typedef enum bit[2:0] {

mst0 = 0,

slv0 = 1,

slv1 = 2,

slv2 = 3,

slv3 = 4

} axi_if_t;

typedef enum bit[2:0] {

mst0_cva6 = 0,

slv0_sram = 1,

slv1_mailbox = 2,

slv2_tl_xbar = 3,

slv3_dram = 4

} axi_if_t;

top_pkg field names are used.

martin-velay · 2026-03-31T12:39:11Z

+  // AXI VIP configuration
+  axi_cfg = new[NUM_OF_AXI_IFS];
+  env.cfg.m_axi_cfg = new[NUM_OF_AXI_IFS];
+  foreach(axi_cfg[i]) begin


Nit: you need a space here and same below for the case. Can you please check else where in your code that you follow this rule.
https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#space-around-keywords

martin-velay · 2026-03-31T12:49:56Z

+
+  foreach (actual_q[s]) foreach (actual_q[s][i]) if (actual_q[s][i].size() > 0)
+    `uvm_error("SCB_DRAIN", $sformatf("Slave trans on %s (ID %h) never reached Master", s, i))
+endfunction : check_phase


You need to configure your editor to make sure you insert an empty line at the end of each file (it's also part of the coding rules somewhere)

martin-velay · 2026-03-31T12:52:34Z

+    string     slave_name;
+    bit [63:0] start_addr;
+    bit [63:0] end_addr;
+  } local_addr_range_t;


Can you move this typedef into the env_pkg

martin-velay · 2026-03-31T12:58:58Z

+  int unsigned       master_id_width  = 4; 
+
+  // Analysis Implementation Ports
+  `uvm_analysis_imp_decl(_mst0)


In the names if we can make it clearer which block is connected, as commented in the env_pkg, that would be better for debug purposes

Updated using the top_pkg names

martin-velay · 2026-03-31T13:00:56Z

+
+  // Queues to handle out-of-order monitor arrivals
+  // [SlaveName][MaskedID]
+  axi4_vip_item expect_q[string][bit[63:0]][$]; // Master arrived first


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

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.

martin-velay · 2026-03-31T13:03:51Z

+  mem_map.push_back('{"slv0", 64'h1000_0000, 64'h1001_FFFF});
+  mem_map.push_back('{"slv1", 64'h2001_0000, 64'h2001_FFFF});
+  mem_map.push_back('{"slv2", 64'h4000_0000, 64'h4FFF_FFFF});
+  mem_map.push_back('{"slv3", 64'h8000_0000, 64'hBF7F_FFFF});


Here, the same applies, could you point at the pkg values please axi_addr_start_t

martin-velay · 2026-03-31T13:07:01Z

+
+function string top_chip_dv_axi_scoreboard::decode_addr(bit [63:0] addr);
+  foreach (mem_map[i]) begin
+    if (addr >= mem_map[i].start_addr && addr <= mem_map[i].end_addr)


You need to add the begin...end for each if whenever it doesn't fit on a single line as a rule. But as we talk about DV here, I'd prefer to add the begin...end and always jump a line this will facilitate breakpoint debug

martin-velay · 2026-03-31T13:54:39Z

+
+function void top_chip_dv_axi_scoreboard::check_phase(uvm_phase phase);
+  super.check_phase(phase);
+  foreach (expect_q[s]) foreach (expect_q[s][i]) if (expect_q[s][i].size() > 0)


This should be break down into multi lines and I think one foreach is useless:

Suggested change

foreach (expect_q[s]) foreach (expect_q[s][i]) if (expect_q[s][i].size() > 0)

foreach (expect_q[s][i]) begin

if (expect_q[s][i].size() > 0) begin

`uvm_error("SCB_DRAIN", $sformatf("Master req for %s (ID %h) never reached Slave", s, i))

end

thommythomaso · 2026-04-01T06:53:00Z

+// maximum supported bus widths
+`define AXI4_MAX_ID_WIDTH     16
+`define AXI4_MAX_ADDR_WIDTH   64
+`define AXI4_MAX_DATA_WIDTH   512


Is there a reason why DATA_WIDTH 1024 is not supported?

Updated to 1024.

thommythomaso · 2026-04-01T07:31:38Z

+      if (current_burst == null) current_burst = axi4_vip_item::type_id::create("w_burst");
+
+      current_burst.dir = AXI_WRITE;
+      current_burst.wdata.push_back(vif.monitor_cb.wdata & ((64'h1 << m_cfg.m_data_width) - 1));


Shouldn't this be 512'h1 (or 1024'h1) for all rdata and wdata types?

Updated with the given define.

thommythomaso · 2026-04-01T07:45:09Z

+
+  // Local Address Map Struct
+  typedef struct {
+    string     slave_name;


Nit: the new nomenclature of AXI uses manager and subordinate.

Yes, we talked about this. Is it better if the naming matches the AXI standard or the current RTL. I know, the new standard uses manager and subordinate, but the RTL uses master and slave.

Good point. I'd say that we should follow the latest nomenclature with manager/subordinate. So it might be that the RTL needs to be changed? But from our concern in this VIP, as its purpose goes beyond this particular DUT eventually, I'd suggest to use the new wording. Do you think there will be some confusing piece of code where the 2 kinds will live together?

Updated names.

thommythomaso · 2026-04-01T07:54:10Z

+  begin
+    bit [63:0] addr = (tr.dir == AXI_WRITE) ? tr.awaddr : tr.araddr;
+    bit [63:0] id   = (tr.dir == AXI_WRITE) ? tr.awid   : tr.arid;
+    // Note: resp logic simplified here for example; normally bit-sliced per beat


Can you please formulate this comment more precise?

This is a legacy comment from a previous version. Thanks for spotting it. Removed.

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

…top UVM environment Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

martin-velay · 2026-04-07T10:13:41Z

+  bit                     m_has_checker                = 1;
+
+  // actual bus widths (<= max defines)
+  int unsigned m_id_width      =  16;


Why don't you point to the values you have defined in the pkg? And I assume the m_data_width should be 1024?

It does not really matter, because the configuration object is set in the test base. However, it does not hurt. Updating.

rswarbrick

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

rswarbrick · 2026-04-07T11:04:21Z

+targets:
+  default:
+    filesets:
+      - files_dv


Obviously a minor nit, but I'd suggest sticking a \n at the end.

And the same for the other files missing newlines.

rswarbrick · 2026-04-07T11:05:24Z

+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+
+`ifndef AXI4_VIP_CFG_SVH


Why have we got include guards here? They make me raise my eyebrows: fusesoc is supposed to avoid us needing this...

If you have a process to avoid double definition, I can remove the guards. However, they are pretty useful for larger projects.

rswarbrick · 2026-04-07T11:06:50Z

+  uvm_active_passive_enum m_subordinate_active_passive = UVM_PASSIVE;
+
+  // Future placeholders
+  bit                     m_has_coverage               = 1;


I'm a bit surprised if a future placeholder that tracks if we have coverage/checker starts equal to 1. Shouldn't they be zero for now?

rswarbrick · 2026-04-07T11:08:47Z

+  int unsigned region_width =   8,
+  int unsigned qos_width    =   8
+);
+  m_inst_id               = inst_id;


Minor nit, but I'd probably suggest lining this up with the next line.

rswarbrick · 2026-04-07T11:13:40Z

+  clocking manager_cb @(posedge aclk);
+
+    // write address
+    output awvalid, awid, awaddr, awlen, awsize, awburst;


How does this work in passive mode? Don't you want a wire and an output-enable flag?

The monitor uses the monitor_cb. manager_cb and subordinta_cb will be used in the drivers in the future. Here you can see them for completeness.

rswarbrick · 2026-04-07T11:20:41Z

+  super.build_phase(phase);
+
+  if (!uvm_config_db#(axi4_vip_cfg)::get(this,"","m_cfg",m_cfg)) begin
+    `uvm_fatal("NOCFG", {"Configuration item  must be set for: ", get_full_name(), "m_cfg"})


Nit: Double space.

rswarbrick · 2026-04-07T11:21:05Z

+function void axi4_vip_manager_agent::build_phase(uvm_phase phase);
+  super.build_phase(phase);
+
+  if (!uvm_config_db#(axi4_vip_cfg)::get(this,"","m_cfg",m_cfg)) begin


Nit: I think it's more conventional to have spaces after commas in argument lists.

rswarbrick · 2026-04-07T12:18:34Z

+      begin b_proc  = process::self(); collect_b_channel();  end
+      begin ar_proc = process::self(); collect_ar_channel(); end
+      begin r_proc  = process::self(); collect_r_channel();  end
+    join_none


Couldn't you structure this without the join_none? Something like this would work, I think:

fork begin aw_proc = process::self(); collect_aw_channel(); end ... begin r_proc = process::self(); collect_r_channel(); end begin wait(!vif.aresetn); stop_processes(); end join cleanup_queues();

I think the 2 implementations are equivalent from functional point of view. Would you like me to change it?

rswarbrick · 2026-04-07T12:19:48Z

+endtask : run_phase
+
+function void axi4_vip_monitor::stop_processes();
+  process p_list[$] = {aw_proc, w_proc, b_proc, ar_proc, r_proc};


This is kind of ok, but I don't think we really care about naming the elements of the list. Can't you just make the class variable the list itself, and make the items in the run_phase method be more like proc_list.push_back(process::self()) ?

That will also simplify the code in the loop below (because we'll never push null into the list)

rswarbrick · 2026-04-07T12:22:23Z

+endfunction : cleanup_queues
+
+task axi4_vip_monitor::collect_aw_channel();
+  bit [`AXI4_MAX_ID_WIDTH-1:0]     id_one     = 1'b1;


Couldn't we build the masks here instead of needing a bunch of "one" variables? I'm imagining things like this:

bit [`AXI4_MAX_ID_WIDTH-1:0] id_mask = {m_cfg.m_id_width{1'b1}};

That said, I'm a little unsure about the masks in general. It seems strange to me for the monitor to be responsible for figuring out the bit width of the consumer or the bus itself.

The implementation could be updated. The masking itself is useful when an interconnect adds extra ID bits to be able to route back the response to the right master. In this case, the VIP shall be configured properly, otherwise the scoreboard cannot recognize if the transactions are matching on the upstream and downstream side. This is just a general solution, currently not used.

martin-velay self-requested a review March 31, 2026 07:15

martin-velay assigned csabakiss-semify Mar 31, 2026

martin-velay requested a review from marnovandermaas March 31, 2026 07:16

martin-velay mentioned this pull request Mar 31, 2026

Axi4 passive vip and scoreboard #356

Closed

martin-velay reviewed Mar 31, 2026

View reviewed changes

thommythomaso reviewed Apr 1, 2026

View reviewed changes

csabakiss-semify added 3 commits April 2, 2026 15:39

[axi,dv] Initial AXI4 VIP development with passive mode support

a689fe1

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

[top,script] Added AXI4 VIP files to the top chip simulation build

317515e

Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

[top,dv] Integrated AXI4 passive VIP and added AXI scoreboard to the …

78b7a22

…top UVM environment Signed-off-by: Csaba Kiss <csaba.kiss@semify-eda.com>

csabakiss-semify force-pushed the axi4_passive_vip branch from f698608 to 78b7a22 Compare April 2, 2026 14:40

martin-velay reviewed Apr 7, 2026

View reviewed changes

rswarbrick reviewed Apr 7, 2026

View reviewed changes

		@@ -0,0 +1,306 @@
		// This AXI4 VIP shall be always UVM_PASSIVE on top level

-  foreach (expect_q[s]) foreach (expect_q[s][i]) if (expect_q[s][i].size() > 0)
+  foreach (expect_q[s][i]) begin
+    if (expect_q[s][i].size() > 0) begin
+      `uvm_error("SCB_DRAIN", $sformatf("Master req for %s (ID %h) never reached Slave", s, i))
+    end

Conversation

csabakiss-semify commented Mar 30, 2026 • edited by martin-velay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-velay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thommythomaso Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-velay Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rswarbrick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabakiss-semify commented Mar 30, 2026 •

edited by martin-velay

Loading

thommythomaso Apr 1, 2026 •

edited

Loading

martin-velay Apr 2, 2026 •

edited

Loading