Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AXI4 Interface and Functional Modeling #159

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

kimmeljo
Copy link
Contributor

Description & Motivation

AXI4 is a common protocol and thus adding a ROHD implementation for its interface and some of its behavior. The structure is very similar to what was done for APB4.

Related Issue(s)

N/A

Testing

Unit tests were created very similar to the unit tests for APB4.

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

Fully backwards compatible.

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

Documentation is forthcoming.

@mkorbel1 mkorbel1 self-requested a review January 29, 2025 00:19
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Looks great so far! Added some initial comments

lib/src/interfaces/axi4.dart Outdated Show resolved Hide resolved
lib/src/interfaces/axi4.dart Show resolved Hide resolved
lib/src/interfaces/axi4.dart Outdated Show resolved Hide resolved
lib/src/interfaces/interfaces.dart Show resolved Hide resolved
final Axi4SystemInterface sIntf;

/// AXI4 Read Interface.
final Axi4ReadInterface rIntf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there always a pair of 1 write and 1 read interface in AXI? Are there cases where someone might have variable numbers of both/either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually a good question. I definitely know of instances where the block has a vectorized number of AXI interfaces. I can't think of an instance where the number of read instances doesn't match the number of write instances but maybe the spec further clarifies this?

Say we just support a vectorized number. Would we just instantiate that many drivers and sequencers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you could perhaps group things (e.g. a "write agent" has a driver and sequencer bundled together). I think a configurable number makes a lot of sense independently for reads and writes unless the spec specifically calls out that they must be the same number.

this.dropDelayCycles = 30,
}) : super(name, parent) {
sequencer = Sequencer<Axi4RequestPacket>('sequencer', this);

Copy link
Contributor

Choose a reason for hiding this comment

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

should the main agent come with a monitor included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess yeah it probably should, right? I was just copying what APB did here (it also doesn't have a monitor).

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the bad example haha, i think it makes sense probably to have a monitor included

/// A driver for the [Axi4ReadInterface] and [Axi4WriteInterface] interfaces.
///
/// Driving from the perspective of the Main agent.
class Axi4MainDriver extends PendingClockedDriver<Axi4RequestPacket> {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this driver be split into two, one for read and one for write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could? Doesn't seem like the logic is too complicated though for a single driver to handle both reads and writes. Maybe there's another benefit though?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have independent numbers of write and read interfaces, then you'd want them split apart

/// A model for the subordinate side of
/// an [Axi4ReadInterface] and [Axi4WriteInterface].
class Axi4SubordinateAgent extends Agent {
/// The system interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

should this subordinate be split into an agent, monitor, and two subordinate drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change would follow from our decision to split out the read and write drivers presumably.

Copy link
Contributor

Choose a reason for hiding this comment

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

right

: LogicValue.ofInt(Axi4RespField.okay.value, wIntf.bResp!.width));
wIntf.bUser?.put(0); // don't support user field for now

// TODO: how to deal with delays??
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to follow up on any remaining TODOs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two (semi) big ones that I want to discuss regarding delays between request/response and handling of the ready signal (i.e., backpressure).

import 'package:rohd_vf/rohd_vf.dart';

/// A monitor for [Axi4ReadInterface]s and [Axi4WriteInterface]s.
class Axi4Monitor extends Monitor<Axi4RequestPacket> {
Copy link
Contributor

Choose a reason for hiding this comment

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

split monitor into a read and write monitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could? Doesn't seem like the logic is too complicated though for a single monitor to handle both reads and writes. Maybe there's another benefit though?

Copy link
Contributor

Choose a reason for hiding this comment

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

same consideration as above -- if you have different numbers of read and write interfaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants