[adc_ctrl,dv] Get things working with Xcelium too #28630
Conversation
This file contains hidden or 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
This won't really matter, but the unnecessary "packed" shows that I didn't really understand the difference between packed and unpacked structs in SystemVerilog when I wrote the code in 2022. Or indeed yesterday morning... Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
requested review from
eshapira and
hcallahan-lowrisc
and removed request for
a team
|
For some proof that this really does work with Xcelium, here's a snapshot of my laptop before I got annoyed and killed it: The three errors looked like this: |
For example, consider flash_mp_region_cfg_t. Structs of this type are
randomised in several places (see flash_ctrl_error_mp_vseq, for
example). This isn't going to do what we need if the struct is packed.
For example (without further constraints), each of those seven mubi4_t
elements is going to be have an invalid encoding with probability
14/16.
This might be worked around. Indeed, I see that
flash_ctrl_error_mp_vseq has explicit constraints for the regions,
that will force the fields to take named enum values. But it's
probably more sensible to default to more guessable behaviour.
As well as flash_ctrl_error_mp_vseq, this commit makes the same change
to:
- flash_bank_mp_info_page_cfg_t (another 7 mubi values, with the
same analysis).
- flash_op_t (contains several enums and gets confusingly randomised
in several places, which is why I noticed this in the first place)
- rd_cache_t (contains an enum variable)
It turns out that removing "packed" isn't quite enough. The biggest
reason is that there were quite a few "solve before" lines that aren't
actually allowed if you have something more interesting than a bit
vector.
Changes needed:
- Make the fields of flash_op_t themselves be rand. This is needed
because calling my_class.randomise(some_class_variable) doesn't
work if some_class_variable is an unpacked struct whose fields
aren't explicitly marked rand.
- This is also needed in flash_mp_region_cfg_t and
flash_bank_mp_info_page_cfg_t. For some reason, *that* isn't
visible at compile time, but you end up with a runtime error where
something is randomised with X values (not allowed by the spec!)
and the root cause is that we're not actually randomising e.g. a
region config struct, so it gets its initial value (of X).
- Rephrase lots of rules of the form "solve xyz before
flash_op" to things like "solve xyz before flash_op.addr". This is
because the SystemVerilog spec doesn't allow anything except an
integer in as a thing to be solved before/after. I think the DV
engineer just has to pretend to be a compiler. Sigh...
- Remove some ordering constraints of the form "solve en_mp_regions
before mp_regions". This is definitely not allowed (because
mp_regions is an array of unpacked structs, so not a bit vector).
I'm not completely convinced we care but, if we do, my proposal
would be to randomise the items of the vector explicitly as we get
to them ("late randomisation"). The code ends up simpler, I think.
- Fix flat-out bogus syntax in flash_ctrl_invalid_op_vseq.sv.
Antonio and I have fixed the same error in quite a few places in
the past. In this case, it came from commit 9b01461 in 2022.
Signed-off-by: Rupert Swarbrick <[email protected]>
The layout of this structure doesn't really matter and it gets randomised (see e.g. adc_ctrl_env_cfg::filter_cfg), which will mean that min_v and max_v are picked uniformly from [2^31, 2^31-1]. I really don't believe that's intended. Signed-off-by: Rupert Swarbrick <[email protected]>
This is inspired by trying to solve randomisation errors. It turns out that randomising values that aren't in a *static* structure causes some exciting problems with "solve before" constraints. In hindsight, I think this change isn't quite needed: I could have bodged something into the existing code. But I think it's probably an improvement anyway (and I've got it working!). One change is to make some of the class variable names a bit clearer. For example, the "cond" field in adc_chn*_filter_ctl_* controls whether the filter matches inside the range or outside of it. I actually guessed wrong at first, but I think this makes a strong argument for making the code more explicit on the DV side. That is now reflected with a variable called "match_outside" (instead of my original guess of "match_inside", which is exactly backwards!) Indeed, I've just checked again and looked at `theory_op_operation.md`. That document *does* mention a field called "cond" and that it controls whether the filter matches inside or outside of the range. It *doesn't* say which... This clearly needs tidying up, but I think that sort of work can be folded into the next proper release. Signed-off-by: Rupert Swarbrick <[email protected]>
This is equivalent, but doesn't require the reader to decompile it in quite the same way. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
adc-ctrl-dv
branch
from
93b566d to
ab5452c
Compare
|
Hmm. The failing lint check is a bug in the Verible frontend, tracked here: chipsalliance/verible#2431. How annoying! I'll squint and try to produce a variable that pulses at the relevant time, so that the assertion only needs one clock. It's very irritating to have to write worse code in order to satisfy a linter. Grr... |
There is no functional change, but dropping the ASSERT macro makes things a little easier to read. The hardware reset assertions should now be a little more efficient. I don't think we really need to bother asserting that the signals don't magically start changing somewhere in the middle of a reset(!), so I've restructured them slightly to check the first clock after the start of a reset. The first syntax I used for this (which is equivalent to the previous version, but without noise from the macro) uses two "clocks" and is of the form @(negedge rst_aon_ni) 1 |=> @(posedge clk_aon_i) XYZ Here, the |=> waits for the event on the right hand side, so it means "On the first clock edge after reset is asserted, XYZ". Very nice! Unfortunately, this isn't actually supported by Verible's SystemVerilog front-end. Grr! I was very annoyed at first, but realised that there's actually an even cleaner way to do it. Because this is a bound-in interface, we can easily define a helper signal. This commit defines a "start_of_reset" flag, which set when reset is asserted and then cleared on the first clock. Now, the properties can be very simple, looking like: start_of_reset -> XYZ (checked on the positive edge of clk_aon_i because of the default clocking statement). That's even shorter! Yippee! When I initially wrote this, I worried about the scheduling because we clear start_of_reset on the clock edge and look at the value at the same time. Fortunately, this is ok: the value tested in the expression is its value from the preponed region (see IEEE 1800, 16.5.1), so we'll get the value that hasn't yet been cleared. Signed-off-by: Rupert Swarbrick <[email protected]>
At the start of this sequence it tries to set the filters to always match. It does so by making the range [0, 1023] (the whole input space) but then sets the cond flag to 1. That means that they will match values that are not in [0, 1023]... Invert the meaning. Rather oddly, the test seems to have worked equally well either way, so maybe this was completely pointless! But at least the comments and code are now consistent. Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change, but it allows the code to avoid a bit of repetition and avoids the reader (me!) getting confused about whether [0] refers to the channel or filter number. Signed-off-by: Rupert Swarbrick <[email protected]>
The DV_ASSERT_CTRL macro expands into an $assertoff(...) call with the hierarchy argument to choose where the assertions get turned off (or on). $assertoff() and $asserton() don't work with this style of path in all cases. The LRM only requires them to work with local or absolute paths: it doesn't allow up-references. VCS *does* allow them (which is why the code was written like this), but Xcelium doesn't. There are nice ways to do this cleanly, but they are rather more effort than just shoving the absolute path of the dut into the file here. Something to clean up when we come back to this module, I think! Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
adc-ctrl-dv
branch
from
ab5452c to
210ac95
Compare
|
Take n+1, rephrasing the assertion as something Verible will parse. I was originally rather grumpy about this, but it's actually cleaner than what I had before! (Thanks, Verible!) The only change is to the "[adc_ctrl,dv] Expand comments a bit and tidy up adc_ctrl_fsm_sva_if" commit and the "Compare" line above shows the diff nicely. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 was inspired by debugging some bogus randomisation (my fault) when I was writing #28581. I tried running the
adc_ctrltest with Xcelium and it failed miserably! It turns out that this is just because the author had implemented it with VCS. This PR builds on #28581 and then getsadc_ctrlDV working with Xcelium too.As a quick sanity check, I now get every ADC test passing when running with
--fixed-seed 1 --tool xcelium.Note that much of this PR comes from the linked one. Only the last 5 commits are unique to this PR.