Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions hw/dv/vip/axi4_vip/axi4_vip.core
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
CAPI=2:
# Copyright lowRISC contributors (COSMIC project).
# Licensed under the Apache License, Version 2.0, see LICENSE for details.
# SPDX-License-Identifier: Apache-2.0

name : lowrisc:dv:axi4_vip:0.1

filesets:
files_dv:
files:
- axi4_vip_if.sv
- axi4_vip_pkg.sv
- axi4_vip_defines.svh: {is_include_file: true}
- axi4_vip_types.svh: {is_include_file: true}
- axi4_vip_cfg.svh: {is_include_file: true}
- axi4_vip_item.svh: {is_include_file: true}
- axi4_vip_driver.svh: {is_include_file: true}
- axi4_vip_sequencer.svh: {is_include_file: true}
- axi4_vip_monitor.svh: {is_include_file: true}
- axi4_vip_manager_agent.svh: {is_include_file: true}
- axi4_vip_subordinate_agent.svh: {is_include_file: true}
- axi4_vip_env.svh: {is_include_file: true}
file_type: systemVerilogSource

targets:
default:
filesets:
- files_dv
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

And the same for the other files missing newlines.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs fixing.

104 changes: 104 additions & 0 deletions hw/dv/vip/axi4_vip/axi4_vip_cfg.svh
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Copyright lowRISC contributors (COSMIC project).
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
// SPDX-License-Identifier: Apache-2.0

`ifndef AXI4_VIP_CFG_SVH
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, most of the files in question are included by axi4_vip_pkg.sv. Even without any package manager, something has gone VERY wrong if you end up needing include guards on e.g. axi4_vip_cfg.svh.

But you also shouldn't need include guards on axi4_vip_pkg.sv. As you say, things can go wrong if you don't have a package manager (so you end up having to hack together something with include guards). The solution is to use one. This project uses fusesoc, which should solve the problem nicely.

As such, I strongly believe the correct path is to avoid littering the code with extra cruft.

`define AXI4_VIP_CFG_SVH

class axi4_vip_cfg extends uvm_object;

// Currently this is a passive VIP (monitor only)
string m_inst_id = "AXI4";
bit m_has_manager = 0;
uvm_active_passive_enum m_manager_active_passive = UVM_PASSIVE;
bit m_has_subordinate = 0;
uvm_active_passive_enum m_subordinate_active_passive = UVM_PASSIVE;

// Future placeholders
bit m_has_coverage = 1;
Comment thread
rswarbrick marked this conversation as resolved.
bit m_has_checker = 1;

// actual bus widths (<= max defines)
int unsigned m_id_width = 16;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int unsigned m_addr_width = 64;
int unsigned m_data_width = 512;
int unsigned m_user_width = 32;
int unsigned m_region_width = 8;
int unsigned m_qos_width = 8;

`uvm_object_utils_begin(axi4_vip_cfg)
`uvm_field_string( m_inst_id, UVM_DEFAULT | UVM_STRING)
`uvm_field_int ( m_has_manager, UVM_DEFAULT)
`uvm_field_int ( m_has_subordinate, UVM_DEFAULT)
`uvm_field_int ( m_has_coverage, UVM_DEFAULT)
`uvm_field_int ( m_has_checker, UVM_DEFAULT)
`uvm_field_enum (uvm_active_passive_enum, m_manager_active_passive, UVM_DEFAULT)
`uvm_field_enum (uvm_active_passive_enum, m_subordinate_active_passive, UVM_DEFAULT)
`uvm_field_int ( m_id_width, UVM_DEFAULT)
`uvm_field_int ( m_addr_width, UVM_DEFAULT)
`uvm_field_int ( m_data_width, UVM_DEFAULT)
`uvm_field_int ( m_user_width, UVM_DEFAULT)
`uvm_field_int ( m_region_width, UVM_DEFAULT)
`uvm_field_int ( m_qos_width, UVM_DEFAULT)
`uvm_object_utils_end

// External Method Declarations
extern function new(string name="axi4_vip_cfg");

extern virtual function void set_config(
string inst_id = "",
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 = 16,
int unsigned addr_width = 64,
int unsigned data_width = 512,
int unsigned user_width = 32,
int unsigned region_width = 8,
int unsigned qos_width = 8
);

endclass : axi4_vip_cfg

//------------------------------------------------------------------------------
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment convey any information? If not, I'd suggest dropping it (and similarly in the other files with the same three lines)

Copy link
Copy Markdown

@rswarbrick rswarbrick Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this review comment still applies.

// External Method Implementations
//------------------------------------------------------------------------------

function axi4_vip_cfg::new(string name="axi4_vip_cfg");
super.new(name);
endfunction : new

function void axi4_vip_cfg::set_config(
string inst_id = "",
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 = 16,
int unsigned addr_width = 64,
int unsigned data_width = 512,
int unsigned user_width = 32,
int unsigned region_width = 8,
int unsigned qos_width = 8
);
m_inst_id = inst_id;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this is worth doing.

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 = id_width;
m_addr_width = addr_width;
m_data_width = data_width;
m_user_width = user_width;
m_region_width = region_width;
m_qos_width = qos_width;
endfunction : set_config

`endif // AXI4_VIP_CFG_SVH
16 changes: 16 additions & 0 deletions hw/dv/vip/axi4_vip/axi4_vip_defines.svh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright lowRISC contributors (COSMIC project).
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
// SPDX-License-Identifier: Apache-2.0

`ifndef AXI4_VIP_DEFINES_SVH
`define AXI4_VIP_DEFINES_SVH

// maximum supported bus widths
`define AXI4_MAX_ID_WIDTH 16
`define AXI4_MAX_ADDR_WIDTH 64
`define AXI4_MAX_DATA_WIDTH 1024
`define AXI4_MAX_USER_WIDTH 32
`define AXI4_MAX_REGION_WIDTH 8
`define AXI4_MAX_QOS_WIDTH 8

`endif // AXI4_VIP_DEFINES_SVH
49 changes: 49 additions & 0 deletions hw/dv/vip/axi4_vip/axi4_vip_driver.svh
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright lowRISC contributors (COSMIC project).
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
// SPDX-License-Identifier: Apache-2.0

`ifndef AXI4_VIP_DRIVER_SVH
`define AXI4_VIP_DRIVER_SVH

class axi4_vip_driver extends uvm_driver #(axi4_vip_item);

`uvm_component_utils(axi4_vip_driver)

axi4_vip_cfg m_cfg;
virtual axi4_vip_if vif;

// External Method Declarations
extern function new(string name, uvm_component parent);
extern function void build_phase(uvm_phase phase);
extern task run_phase(uvm_phase phase);

endclass : axi4_vip_driver

//------------------------------------------------------------------------------
// External Method Implementations
//------------------------------------------------------------------------------

function axi4_vip_driver::new(string name, uvm_component parent);
super.new(name, parent);
endfunction : new

function void axi4_vip_driver::build_phase(uvm_phase phase);
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"})
end

if (! uvm_config_db #(virtual interface axi4_vip_if)::get(this, get_full_name(),"vif", vif)) begin
`uvm_fatal("NOVIF",{"virtual interface must be set for: ",get_full_name(),".vif"})
end
endfunction : build_phase

task axi4_vip_driver::run_phase(uvm_phase phase);
forever begin
// TODO: Placeholder
seq_item_port.get_next_item(req);
seq_item_port.item_done();
end
endtask : run_phase

`endif // AXI4_VIP_DRIVER_SVH
47 changes: 47 additions & 0 deletions hw/dv/vip/axi4_vip/axi4_vip_env.svh
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright lowRISC contributors (COSMIC project).
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
// SPDX-License-Identifier: Apache-2.0

`ifndef AXI4_VIP_ENV_SVH
`define AXI4_VIP_ENV_SVH

class axi4_vip_env extends uvm_env;

`uvm_component_utils(axi4_vip_env)

axi4_vip_cfg m_cfg;

axi4_vip_manager_agent m_manager;
axi4_vip_subordinate_agent m_subordinate;

// External Method Declarations
extern function new(string name, uvm_component parent);
extern function void build_phase(uvm_phase phase);

endclass : axi4_vip_env

//------------------------------------------------------------------------------
// External Method Implementations
//------------------------------------------------------------------------------

function axi4_vip_env::new(string name, uvm_component parent);
super.new(name, parent);
endfunction : new

function void axi4_vip_env::build_phase(uvm_phase phase);
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"})
end

if (m_cfg.m_has_manager == 1) begin
m_manager = axi4_vip_manager_agent::type_id::create("m_manager", this);
end

if (m_cfg.m_has_subordinate == 1) begin
m_subordinate = axi4_vip_subordinate_agent::type_id::create("m_subordinate", this);
end
endfunction : build_phase

`endif // AXI4_VIP_ENV_SVH
Loading