[dv,sram] add axi_sram verification plan and tag architecture spec#667
[dv,sram] add axi_sram verification plan and tag architecture spec#667tchilikov-semify wants to merge 7 commits into
Conversation
martin-velay
left a comment
There was a problem hiding this comment.
I have some comments, and also I'd like to know if you feel it was a bad experience to create a vPlan based on DVPlan? I think you are among th 1st to do the full exercise (except myself) and I need to get feedback 😃
| It needs to support AXI4 protocol including:<!-- mcykq8 --> | ||
| - Bursts, where the last signal must be indicated correctly.<!-- o02amt --> | ||
| - Response must have the same AXI4 ID as the request<!-- 4t4cew --> | ||
| - Atomic support is *excluded*.<!-- bsi4rc --> |
There was a problem hiding this comment.
This is not covered, how the block behaves when an atomic/exclusive access arrives? What do you reckon, we should update the spec or add a vPlan new item?
There was a problem hiding this comment.
Are atomics planned to be supported? Personally, I think if they are planned in the future, we should add tests to the vplan as V2/V3 milestones, and skip the implementation for now. I was under the impression that there's no support planned.
There was a problem hiding this comment.
Yes, it not planned from what I understood too, so you reckon it means that we shouldn't try to see if the design breaks in that case? I'd probably agree with this statement, so I think we probably need to exclude this line. WDYT?
There was a problem hiding this comment.
I think thats a valid observation - the sram shouldn't break if an atomic comes through, and I think its worthwhile adding some scenarios to verify this.
A follow up question from my side is: what should the expected behavior be in this case? Should some block outside the SRAM enforce no atomics to SRAM, or should the SRAM accept the request and treat it as a regular read/write, or should the SRAM ignore it all together?
The current behavior is that the SRAM treats atomic requests as normal requests, from my understanding at least.
| Description: Reads of only part of a 64-bit value from a valid cap region return data but with the tag cleared | ||
| Node_Type: metric | ||
| Priority: 2 | ||
| Comment: Need to clarify here if the tag is only returned as a clear, or if it permeates as cleared in the tag memory |
There was a problem hiding this comment.
As unclear, we need to request a spec clarification to the designer.
@marnovandermaas, could you clarify the doubt on this spec line?
| @@ -0,0 +1,379 @@ | |||
| { | |||
There was a problem hiding this comment.
Missing scenarios?
- Reset during a burst:
rst_sanityonly checks a clean reset exit. Add a testpoint forrst_niasserted mid-burst (transaction dropped cleanly, no lingering state, clean recovery). - Backpressure / ready-throttling: No stress on
aw_ready/w_ready/ar_readydeassertion under load. Add a testpoint (and constrained-random throttling) to confirm data is not corrupted on resumption. - Richer concurrency:
concurrent_data_tagcovers only one data+cap pairing. Consider multiple outstanding transactions, read/write to the same region in flight, and write-after-write ordering.
There was a problem hiding this comment.
These are all good scenarios to test.
I'll update the vplan accordingly.
| - Response must have the same AXI4 ID as the request<!-- 4t4cew --> | ||
| - Atomic support is *excluded*.<!-- bsi4rc --> | ||
| - The data width is 64 bits.<!-- jeluga --> | ||
| - The address range and size of the SRAM are defined in the [memory map](#memory-map). Accesses outside this range must return an error, including if only part of the burst is outside the memory range.<!-- u0s8nt --> |
There was a problem hiding this comment.
@marnovandermaas, I think a clarification is required here.
This sentence should maybe be reworded to state that out-of-range error is an SoC/interconnect guarantee (xbar decode -> DECERR), not an axi_sram block behavior, or confirm the block is meant to range-check (a design change).
| Node_Type: metric | ||
| Priority: 1 | ||
| } | ||
| sram_geometry: |
There was a problem hiding this comment.
Add a comment to tell from where this comes:
Comment: SramMemSize 128 KiB, last word 0x1FFF8 and last cap slot 0x1FFF0 are derived from SRAMLength in top_pkg (0x2_0000) and the memmap.svg image, which is untaggable. TODO: point Reference at a real spec tag once the memory map base/size is added as tagged text in arch.md.
@marnovandermaas: Would it be possible to have the memory map in Markdown in this file directly instead of the current svg file?
There was a problem hiding this comment.
updated the comment - It would be good to have the mem map as markdown, so we can tag each line.
Co-authored-by: Martin Velay <165653139+martin-velay@users.noreply.github.com>
Co-authored-by: Martin Velay <165653139+martin-velay@users.noreply.github.com>
Co-authored-by: Martin Velay <165653139+martin-velay@users.noreply.github.com>
59c77a5 to
f588fc4
Compare
|
Thanks for the feedback, @martin-velay 😃 Regarding DVPlan: I found that the flow and working with the tool was actually quite nice and polished (so far). One pain point I have though, is the spec annotation capabilities. A random generated tag means that each time the spec is updated and changed, all of the tags would be re-generated. This would make updating the vplans a bit of a nightmare, since all of the tags would have to be re-mapped. I think a solution to this could be a per-line / per-sentence hash. This way, the hash only changes if the contents of the line change. Of course, this sounds simple but it would introduce edge cases (white spaces, punctuation, etc). I have yet to re-generate or re-map anything, but I anticipate that this would be tedious as it currently stands. Other than this, it was quite straight-forward! I plan to map the actual SV testcases with a comment to the tags as well. |
No description provided.