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

AXI-Stream Formal Properties #3

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

AXI-Stream Formal Properties #3

wants to merge 38 commits into from

Conversation

rlee287
Copy link
Owner

@rlee287 rlee287 commented Dec 6, 2020

These are bare-bones assumes and asserts for verifying protocol compliance. Output signals such as transfer/byte counts may be added later.

Async reset handling is tracked in #4 and is handled correctly (outside of multiclock environments).

These do not fully consider rules about connecting mismatched interfaces

Perhaps we may need more property sets for a "passthrough" component that
asserts additional rules
(cherry picked from commit 9211c4e)
@rlee287 rlee287 marked this pull request as ready for review December 9, 2020 03:00
@rlee287 rlee287 changed the title WIP: AXI-Stream Formal Properties AXI-Stream Formal Properties Dec 14, 2020
@rlee287
Copy link
Owner Author

rlee287 commented Dec 14, 2020

Removed WIP designator for now: these properties do not correctly handle cores with an asynchronous reset, but this can be added later.

@rlee287 rlee287 marked this pull request as draft December 15, 2020 23:19
@rlee287
Copy link
Owner Author

rlee287 commented Dec 15, 2020

Back to draft again: the current reset handling is probably broken so I'll want to fix this and address #4 at the same time.

@rlee287 rlee287 linked an issue Dec 16, 2020 that may be closed by this pull request
@rlee287
Copy link
Owner Author

rlee287 commented Dec 16, 2020

#4 has been fixed but all this needs review before merging.

Copy link

@awygle awygle left a comment

Choose a reason for hiding this comment

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

This looks really good. Some global comments:

  • We don't have anything in here for interconnect, which we probably should at some point.
  • We should include "AXI4" and "Version 1.0 Revision A" somehow, for tracking purposes.
  • We should actually verify some IP with this, and correspondingly, we should have another state in our README state machine for "we wrote properties but haven't verified anything with them yet" (and then this PR should update the README to put AXI-Stream in that state).

@awygle
Copy link

awygle commented Dec 27, 2020

One more thing - per this convo with Strike we could use default values for optional signals in places where it makes sense (not sure where that might be tho). I did verify this works in Yosys.

@rlee287 rlee287 linked an issue Dec 27, 2020 that may be closed by this pull request
It's very unlikely that ARM will release a new version of this spec (given what AXI-Stream is), but add trackability anyway
@rlee287
Copy link
Owner Author

rlee287 commented Dec 27, 2020

@awygle Addressing your global comments:

  • "We don't have anything in here for interconnect, which we probably should at some point." Properties for that type of thing are tracked in Create AXI-Stream processing core properties #6, and example cores (such as an actual interconnect) are tracked in Write example AXI-Stream cores #5. The properties in this PR are for isolated interfaces, and I believe properties spanning multiple interfaces (e.g. interconnect properties) should be written separately.
  • "We should include "AXI4" and "Version 1.0 Revision A" somehow, for tracking purposes." Added in e19f039.
  • "We should actually verify some IP with this, and correspondingly, we should have another state in our README state machine for 'we wrote properties but haven't verified anything with them yet' (and then this PR should update the README to put AXI-Stream in that state)." README has been updated, and writing example cores is tracked in Write example AXI-Stream cores #5. I also have some simple cores locally that I've been using to test these properties, but they aren't in a state that's ready to be pushed to GitHub yet.

@rlee287 rlee287 marked this pull request as ready for review January 3, 2021 07:25
This doesn't make sense for interconnect-type cores that should not emit transfers before receiving them
(cherry picked from commit 8d7c4b6)
@rlee287 rlee287 mentioned this pull request Feb 12, 2021
1 task
@rlee287
Copy link
Owner Author

rlee287 commented Feb 13, 2021

Comments from Echo on the Discord (@dicta):

hey, just wanted to poke you with a couple thoughts regarding your AXI4-Stream properties PR:

  • There's some fun interactions with the tlast signal and tid/tdest: If tlast is being used by a master, tid/tdest can change after 1) a reset, or 2) after tlast has been asserted during a clock when data has been transferred, but otherwise must remain steady over the duration of a packet. This is section 2.5 of the standard. Slaves, on the other hand, are supposed to be able to deal with streams being interleaved so there's not a matching requirement there. I think the best way to reason about this is to create a flag indicating whether it's acceptable for a new packet to be started, and require that the master's tid/tdest be stable in all other cases.
  • I'm not sure if you're just looking at verifying individual modules or looking at a bus as a whole (including interconnects), but there's some important bus properties that probably should be included. The easy one is that (again for packetized interfaces using tkeep), the number of clocks at the master where tready && tvalid && tkeep must be equal to the number of clocks at the slave where tready && tvalid && tkeep: i.e. nothing in the middle of the bus is allowed to merge/split packets. The harder one to define properties for is that transfers must not arrive out-of-order. [I guess maybe time to go look at some AXI4/AXI4-Lite properties and see how that's done there...]

@rlee287
Copy link
Owner Author

rlee287 commented Feb 13, 2021

As stated on one of the code reviews earlier, I plan to focus on isolated interfaces first and then create a combined property set in a separate PR that includes stuff like the second bullet point. The first bullet point is something I'll need to recheck and to add to the property set if it's missing.

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

Successfully merging this pull request may close these issues.

Add support for asynchronous resets to AXI-Stream properties Create AXI-Stream Formal Property Set
2 participants