[rv_dm,dv] Make the scoreboard TL handling a bit clearer#29920
[rv_dm,dv] Make the scoreboard TL handling a bit clearer#29920rswarbrick wants to merge 1 commit intolowRISC:masterfrom
Conversation
rswarbrick
requested review from
hcallahan-lowrisc
and removed request for
a team
rswarbrick
force-pushed
the
rv-dm-invalid-regs
branch
from
0a5cfc3 to
575599b
Compare
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
Some comments from my side. I feel like this PR does more than the commit message says since it essentially rewrites all the logic, maybe it is worth explaining that rewrite.
| do_read_check = 0; | ||
| end else if (csr inside {blk.dataaddr}) begin | ||
| end else if (csr inside {blk.flags}) begin | ||
| // hw can update this value. So check manually. |
There was a problem hiding this comment.
Nit: start comment with capital.
There was a problem hiding this comment.
I agree. I hadn't actually touched the line, but (since I touched the one next door) it probably makes sense to fix.
Done!
|
|
||
| // If reg_mirrored has been cleared, this is not a register that we model. Nothing more to do | ||
| // here. | ||
| if (!reg_mirrored) return; |
There was a problem hiding this comment.
Can't we put this inside the else statement above?
There was a problem hiding this comment.
Let's do that. I suspect I was thinking about the csr == null case when I wrote this, but that has completely handled already, so there's no need.
I suspect that I'd restructured this a bit when writing the PR. Thanks for spotting that things are now a bit silly.
rswarbrick
force-pushed
the
rv-dm-invalid-regs
branch
from
575599b to
666efd8
Compare
The reason for making the change is that the scoreboard was failing an
assertion that tried to check every access was either to a memory or
to a known CSR.
This isn't true, because the debug memory is represented as an array
of CSRs and out-of-bound accesses in the debug memory map are ignored
in the design. Indeed, the layout of the debug memory in the PULP
project is documented as an "implementation detail of the Debug
Module".
Changes in this commit:
- Split the process_tl_access task into separate parts for the
register and debug memory interfaces.
- Do the case split on register directly ("if (csr ==
blk.some_register)" instead of doing a shell globbing match on the
CSR name.
- Actually check the result from calling csr.predict
- Allow access to unknown registers in the debug memory, but check
they respond with no error and with zero read data.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
rswarbrick
force-pushed
the
rv-dm-invalid-regs
branch
from
666efd8 to
7d3a2a8
Compare
Good point. I've expanded out the commit message (and will update the PR description in a sec). |
2 participants
The reason for making the change is that the scoreboard was failing an assertion that tried to check every access was either to a memory or to a known CSR.
This isn't true, because the debug memory is represented as an array of CSRs and out-of-bound accesses in the debug memory map are ignored in the design. Indeed, the layout of the debug memory in the PULP project is documented as an "implementation detail of the Debug Module".
Changes in this commit:
Split the process_tl_access task into separate parts for the register and debug memory interfaces.
Do the case split on register directly ("if (csr == blk.some_register)" instead of doing a shell globbing match on the CSR name.
Actually check the result from calling csr.predict
Allow access to unknown registers in the debug memory, but check they respond with no error and with zero read data.