-
Notifications
You must be signed in to change notification settings - Fork 34
DTI Messages and HW #275
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
base: main
Are you sure you want to change the base?
DTI Messages and HW #275
Conversation
…efore maxNumBeats.
| @@ -0,0 +1,83 @@ | |||
| import 'package:rohd/rohd.dart'; | |||
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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.
|
Checks fail on long lines and default arguments. |
|
@desmonddak I copied |
| }, actions: []), | ||
| ], | ||
| ); | ||
| sendIdle <= sendState.currentState.eq(AxiStreamBeatState.idle.index); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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
There is one minor backward incompatible change to the AXI-S interface definition. There was a bug where the
TUSERsignal was not taking the correct width if present.Documentation
Documentation will be added as part of this change set under doc/components/amba/dti.md.