Skip to content

Conversation

@kimmeljo
Copy link
Contributor

Description & Motivation

This adds LogicStructures for AMBA compliant DTI_TBU messages. This also adds a generic DTI controller module that is capable of sending and receiving DTI messages in a compliant fashion over a pair of AXI-S interfaces.

As part of the controller module, we also add AXI-S compliant link layer controller modules for TX and RX on the physical interface.

Related Issue(s)

N/A

Testing

Unit testing add for all HW modules described above.

Backwards-compatibility

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

There is one minor backward incompatible change to the AXI-S interface definition. There was a bug where the TUSER signal was not taking the correct width if present.

Documentation

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

Documentation will be added as part of this change set under doc/components/amba/dti.md.

@@ -0,0 +1,83 @@
import 'package:rohd/rohd.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Need Copyright headers. Copilot does a decent job if you say 'put our usual Copyright header on files without'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment below about ready_valid_interface.dart. I think it's better for that to come from a different PR but I needed it for my work. Anything else that's missing a copyright header I will add.

@desmonddak
Copy link
Contributor

Checks fail on long lines and default arguments.

@kimmeljo
Copy link
Contributor Author

@desmonddak I copied ready_valid_interface.dart from bubbly_einstein because I wanted to use it for this implementation. But it really doesn't fit in this PR. Do you already have a PR with ready_valid_interface.dart? Maybe I'll just wait for you to merge that one as a dependency for this. Feels cleaner to me...

}, actions: []),
],
);
sendIdle <= sendState.currentState.eq(AxiStreamBeatState.idle.index);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: rather than querying currentState, move driving of related signals for different states into the actions

import 'package:rohd_hcl/rohd_hcl.dart';

/// A generic module to send messages over AXI-S.
class AxiStreamInterfaceTx extends Module {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why have the word Interface in the class name? why not just AxiSTx or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particularly strong reasoning here. If you think it's better without, I have no problem changing!

final countEnable = stream.valid & (stream.ready ?? Const(1));
final acceptNextSend =
sendIdle | (countEnable & (stream.last ?? Const(1)));
final beatCounter = Counter.simple(
Copy link
Contributor

Choose a reason for hiding this comment

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

without having read the spec in detail, is there an opportunity to use the Serializer (etc.) components to reduce complexity/increase reuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes it sure does look like Serializer (and Deserializer for rx) would work nicely here - let me try refactoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out there is a limitation that would prevent me from using these standard components. I filed an issue here:
#277

I might just try to crank the issue out myself and then apply it.

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.

3 participants