[pattgen,dv] Start to tidy up cnt_rollover #26319
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rswarbrick
requested review from
matutem and
hcallahan-lowrisc
and removed request for
a team
No functional change, but this adds some documentation comments and also sets list_of_alerts a bit earlier (in the constructor instead of initialize()). Signed-off-by: Rupert Swarbrick <[email protected]>
The variable is only used in the stress sequence, so it probably makes more sense to define it there, instead of the base vseq. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
This is just getting rid of a class variable. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
Rather than a function that operates in place on a reference, just use a task: there's only ever one thing being modified. Also, change the naming to "left rotate", which matches how things look in MSB-first SystemVerilog. Signed-off-by: Rupert Swarbrick <[email protected]>
We now have a setup_pattgen_channel task, which takes the channel index as an argument. Signed-off-by: Rupert Swarbrick <[email protected]>
It's dramatically easier to use this with the channel index instead of the enum that we were using. Write the simpler version and add a documentation comment. Signed-off-by: Rupert Swarbrick <[email protected]>
This should be a little less tied in with the two channel setup and avoid a bit of repetition. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
The call sites already had the "is an error injected?" logic. Use that. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
This was an enum representing the four possibilities for a 2-bit bitmask in a rather confusing way. Get rid of it. Signed-off-by: Rupert Swarbrick <[email protected]>
This is only ever set to 50% and is used to weight the random choice of a bit (and it seems unlikely we'll ever care more about one polarity). Get rid of the unused customizability. Signed-off-by: Rupert Swarbrick <[email protected]>
The text was a bit hard to understand. Fix that, and also give a "pattgen_" prefix to the name of the test, matching the style of other tests. Signed-off-by: Rupert Swarbrick <[email protected]>
Unfortunately, it doesn't actually do what it's supposed to do! Add a big TODO notice linked to an issue that tracks the problem. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
pattgen-dv-tidyup-1
branch
from
21b741b to
5e9ad0c
Compare
matutem
reviewed
Feb 20, 2025
Comment on lines
+11
to
+13
| // Unfortunately, the clk_cnt_q signal in pattgen_chan isn't actually exposed to other blocks. If | ||
| // we want to work with large values, we'll probably need to force its value to "just below | ||
| // prediv_q") |
There was a problem hiding this comment.
I tried to understand this by looking at #26317 with no luck. Am I thick in the head or is the spec for pattgen rather confusing?
Also, it seems with small constraints you are able to test the behavior is correct. So is the problem that you cannot test the behavior for larger values of the config registers, and that is the reason you would need to set the counters to values close to rollover?
There was a problem hiding this comment.
It's rather confusing (and I'm enthusiastically hacking away to try to tidy lots of this up). I think the original testplan authors wanted to make sure we "sent entire patterns" with maximal values of prediv and/or length. But those counters are enormous! So we either need to abandon doing so or force an internal signal. But the existing code seemed rather silly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This looks enormous, but it actually builds on top of #26318. Once that is merged, this PR only contains the last two commits.
Sadly, I don't think the test actually does what it's supposed to do. I've filed #26317 to track this and linked that to a TODO note in the second commit.