AXI4 VIP development and integration

[tchilikov-semify]

This PR does a few things:

1. Pulls @rswarbrick's AXI driver from his Caliptra repository as a local copy and expands on its functionality with multi-beat support
2. Merges it with Csaba's AXI monitor from Axi4 passive vip #391, creating one unified agent for Active (manager) and Passive (monitor) modes
3. Applies various bug and compilation fixes to the agent
4. Integrates the agent in the top TB as a monitor on each port of the crossbar
5. creates an environment level scoreboard

This was tested with the top level CVA6-based test cases.

Will close issue #168

[rswarbrick]

Thank you very much for doing this (and getting my completely untested code working...)

Do you think you could cherry-pick the various fixes in "axi agent compilation fixes" to the commits that made the mistakes? Since we don't use squash merge in Mocha, this is a way to avoid putting Rupert's silly mistakes into the history :-)

[tchilikov-semify]

Thank you very much for doing this (and getting my completely untested code working...)

Do you think you could cherry-pick the various fixes in "axi agent compilation fixes" to the commits that made the mistakes? Since we don't use squash merge in Mocha, this is a way to avoid putting Rupert's silly mistakes into the history :-)

Hey Rupert,

Sure, I can merge the patches into your commits. This would also encompass the handshaking bugfix commit, for a total of two commits :)

[tchilikov-semify]

Hey @rswarbrick,
Thanks a lot for the in-depth review! I'll go through all of the comments and address them.

In the meanwhile, I've folded the old compilation bugfixes commit into the respective commits that you made, so the commit history should be clear of bugs now.

[rswarbrick]

Here are some notes about the "fix axi handshaking" and "multi-beat transaction support" commits.

These both look really good: I like the code, and it's just some nitty notes about comments.

[rswarbrick]

Two more minor notes about "add multi-beat transaction support"

[rswarbrick]

A minor note about error messages in "add clk_rst_if to axi interfaces". (I really like this change though!)

[rswarbrick]

Some notes about the (excellent!) change that adds axi_widths_pkg. They're all about alignment: sorry - very boring.

[rswarbrick]

One specific question: Is there a good reason to use dv_base_agent for the IP? It's designed to a very specific OpenTitan shape, as chosen by the DV lead at the time.

It's also actively against my design for this agent. If someone more important than me mandates that we use the class, I guess we have to. If not, I'd prefer we didn't.

[rswarbrick]

Is this design following a pattern for which you have a reference? I'm slightly surprised that you don't set things in the config db from the testbench, rather than the interface that gets instantiated.

[tchilikov-semify]

Yes! The intent of this file is perhaps quite unclear from the perspective of this PR... Its purpose is to take the shape of the AXI peripherals used in mocha. In our agent, the AXI bus is represented by 5 interfaces, which map to the channels. In mocha, the AXI devices have ports which are represented by 2 interfaces, basically inputs and outputs (req and resp). This interface bridges the two.

[rswarbrick]

Ah, I see. I wonder whether it would make sense to split this file out of the axi_vip directory. Although it's notionally parametised on the type of e.g. req_t, it isn't really!

Lines like this aren't really parametric in the type!

    assign axi_req.aw.id     = $bits(axi_req.aw.id)'(aw_if.awid);

(This is an example of the standard fact from type theory that there is only one way to define a function A -> A for all types, A)

[rswarbrick]

I'm repeating myself here, but I think this should be set by calling a setter on the agent, which calls a setter on each of the drivers.

Note that you wrote earlier that the agent config just contained a list of virtual interfaces. I wasn't convinced. I'm still not :-)

[tchilikov-semify]

Yes perhaps an agent wide config permeated into the configs... You were right in this regard. 😉

For my understanding, are you proposing that the agent reads this config and then calls the setters of the drivers, rather than passing the config directly to the drivers? This value is a genuine agent-wide knob, so I think it should live in the config. Is this what you had in mind?

[rswarbrick]

I think I was rather confused when I was writing that suggestion... I think there's a nice way to structure things though! (details below). Does it sound reasonable to you?

[tchilikov-semify]

One specific question: Is there a good reason to use dv_base_agent for the IP? It's designed to a very specific OpenTitan shape, as chosen by the DV lead at the time.

It's also actively against my design for this agent. If someone more important than me mandates that we use the class, I guess we have to. If not, I'd prefer we didn't.

Honestly - its not strictly needed. This was a suggestion from an internal review that Martin did with Claude, which he told me to implement if it seems fitting. I thought it was fitting, since I saw that other mocha TBs use the dv base classes, but honestly I don't have a strong opinion on this (and honestly, I think the less dependencies an independent agent has, the better).

Shall I remove it?

[tchilikov-semify]

Hey @rswarbrick - thanks for the in depth review again! It seems this PR is starting to come together 😄 I think I've addressed most of your comments. There's still 1 or 2 open questions, but I've re-triggered another review. Thanks!

[rswarbrick]

@tchilikov-semify, thanks for the prompt! I'll go through the commits in detail again in a minute, but I've been thinking a bit about how configuration should probably work, and I think that (at last!) I've finally worked out a sensible structure.

Thinking about configuration for the agent, I think there are two different sorts of configuration:

• Structure: This is things like a bus width. The interface doesn't need things like this to be parameters exactly (this is the whole point of the "max footprint stuff"), but they are static and completely governed by the dut RTL. As such, I think the right thing is for this to be customised by the testbench as it instantiates the interface.
• Behaviour: For example, I might want to configure how quickly the agent responds to requests, or maybe how frequently it sends pings, or maybe the behaviour you're configuring with drive_x_when_idle. This sort of thing should probably be configured by the test or environment, "downstream" of tb.sv.

Focusing on the experience of an environment that wants to use the agent, I think the right thing is to create something like what you've currently got in the config file: essentially, it's a list of virtual interfaces, together with the behaviour configuration parameters.

For the internals of the agent, I don't think that the various components (monitors and drivers) should see that config object. The danger is that everything ends up "very global" (a recurring problem in OpenTitan) and the thing that's called an "agent config" ends up being a way to store state and communicate between the different components. In the limit, you end up with "everything is a global variable". Yuck! Instead of doing that, I think the agent should responsible for copying the configuration information down to the components it creates (in the agent's build_phase).

That should separate concerns nicely:

• A driver can see the way it has been configured (with the relevant config values stored as class variables inside the driver).
• It can also see information about its internal state.
• But it can't see stuff it doesn't need. For example, a monitor shouldn't need to care about drive_x_when_idle.
• What's more, a test or environment can't really see components' internal state: it can just see the agent config object.
• If there is internal state that we want to expose, we can pull it explicitly through the agent (not the agent config! Sequences shouldn't see this sort of thing!) with a my_agent.get_monitor_xyz_state() function.

This is not how the historical dv_base_agent class worked. But I think we can do much better than that! And I also think that we've got most of the right structure in this PR now.

[rswarbrick]

I've just read carefully through the read burst virtual sequence and have left lots of suggestions (sorry!)

They will apply to the write side of things as well.

[rswarbrick]

This might be my fault (sorry), but we've got a double "it" here. How about "To use the sequence:" on the previous line?

[rswarbrick]

The indentation is a bit wonky here: I think the lines after the - line are indented by an extra space each time.

[rswarbrick]

Nit: I'd suggest a more obviously arbitrary address here. How about 'h1234 ?

[rswarbrick]

I'd avoid "left" here: saying that something is "left in a queue" makes it sound a bit like it's there and nowhere else. Thinking about it, I don't think I'd talk about m_read_beats at all: isn't that just an implementation detail?

[rswarbrick]

Grammar nit: This would be more standard as "... it only holds ..."

[rswarbrick]

It's not a big deal, but I don't think there's any need for these accept_one_beat tasks to run in parallel with each other. Couldn't this be:

      fork begin
        for (int unsigned i = 0; i < n_beats; i++) accept_one_beat(i);
      end join_none

?

[rswarbrick]

This isn't quite correct: it might be accepting a beat that has no relation to this sequence's request! I suppose a counter might be slightly useful to someone debugging in the future, but I think the comment needs changing.

[rswarbrick]

I'd expand this to something like // We have seen a reset, to make sure it doesn't look like an instruction to reset.

[rswarbrick]

Nit: Extra space before the "=" signs on these two lines.

[rswarbrick]

Thinking about this, I think it might make more sense to define an extra sequence for the fixed example. It's extremely simple! I just did a bit of typing, and I think it ends up like this, and avoids a load of do-nothing variables.

// Copyright lowRISC contributors
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
// SPDX-License-Identifier: Apache-2.0

// A fixed sequence that sends a single fixed item.
//
// This item is neither created nor randomised: a virtual sequence that wants to use this sequence
// should provide the item before starting the sequence.
//
// When the sequence completes, the rsp field will contain a status item that shows whether the
// sequence ran to completion (rather than being interrupted by a reset).

class axi_mgr_fixed_txn_request_seq extends uvm_sequence #(axi_txn_request_item, axi_status_item);
  `uvm_object_utils(axi_mgr_fixed_txn_request_seq)

  // This item is sent verbatim; no randomisation. Provide the item before starting the sequence.
  axi_txn_request_item m_req;

  extern function new(string name="");
  extern task pre_start();
  extern task body();
endclass

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

task axi_mgr_fixed_txn_request_seq::pre_start();
  super.pre_start();
  if (m_req == null) `uvm_fatal(get_full_name(), "Cannot run fixed sequence with no item.")
endtask

task axi_mgr_fixed_txn_request_seq::body();
  axi_txn_request_item item;
  uvm_sequence_item base_status_item;

  if (m_req != null) begin
    // Send the caller-supplied item verbatim: it is already built, so no randomisation.
    item = m_req;
    start_item(item);
    finish_item(item);
  end

  // Get a response, which will always be sent by the driver (and is available already: there's no
  // pipelining and finish_item just completed).
  get_base_response(base_status_item);
  if (!$cast(rsp, base_status_item)) begin
    `uvm_fatal(get_full_name(), "Status response is not an axi_status_item")
  end
endtask

[rswarbrick]

If we really don't want to repeat ourselves, we should probably define a base version of this sequence as a virtual class and give it a pure virtual "give me an item to run" function.

I'm not really convinced that's worth the hassle though: I think we repeat about 5 lines.

[rswarbrick]

One specific question: Is there a good reason to use dv_base_agent for the IP? It's designed to a very specific OpenTitan shape, as chosen by the DV lead at the time.
It's also actively against my design for this agent. If someone more important than me mandates that we use the class, I guess we have to. If not, I'd prefer we didn't.

Honestly - its not strictly needed. This was a suggestion from an internal review that Martin did with Claude, which he told me to implement if it seems fitting. I thought it was fitting, since I saw that other mocha TBs use the dv base classes, but honestly I don't have a strong opinion on this (and honestly, I think the less dependencies an independent agent has, the better).

Shall I remove it?

Yeah, if you're happy to :-)

I think that Claude recommends writing code that matches other code nearby. A very good general principle! But we're trying to do better...