Skip to content

[rom_ctrl,dv] Connect the block-level rom_ctrl env to chip tb - #30851

Draft
rswarbrick wants to merge 6 commits into
lowRISC:masterfrom
rswarbrick:rom-ctrl-vertical-reuse
Draft

[rom_ctrl,dv] Connect the block-level rom_ctrl env to chip tb#30851
rswarbrick wants to merge 6 commits into
lowRISC:masterfrom
rswarbrick:rom-ctrl-vertical-reuse

Conversation

@rswarbrick

@rswarbrick rswarbrick commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

This PR is in draft because it has the following dependencies:

These were actually written as part of getting this PR to work properly, but I think it probably makes sense to review them separately first.

After they have been merged and this PR is rebased, just two commits will remain. The first (defining "set_is_active") is general and pretty trivial. The final commit includes the environment in the top-level testbench and isn't actually all that big: +159/-9.

[rom_ctrl,dv] Connect the block-level rom_ctrl env to chip tb

Getting this to work took quite a lot of other changes but this (finally) builds and runs a test successfully.

@rswarbrick rswarbrick added Component:DV DV issue: testbench, test case, etc. IP:rom_ctrl labels Jul 24, 2026
@rswarbrick

Copy link
Copy Markdown
Contributor Author

I'm about to force-push with a lint fix (which had already made it into the relevant dependency PR), but want to show this screenshot first, proving that this actually does something!

image

(This is with the originally pushed version of the PR)

@vogelpi vogelpi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @rswarbrick , this looks good to me!

I've also reviewed and approved the PR this one here relies on (where you simplify the backdoor utils).

One thing I don't yet understand is: where do you tell the KMAC HWIP to abort the hashing when you skip the ROM verification in the middle? Or does the mechanism work by not even forward anything to the KMAC HWIP in case the plusarg is set? I guess this would also work and would be even simpler.

@rswarbrick
rswarbrick force-pushed the rom-ctrl-vertical-reuse branch from 9a5f9fd to 94c9d90 Compare August 14, 2026 14:55
@rswarbrick

rswarbrick commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

The communication with KMAC works by rom_ctrl sending a stream of words and asserting a "last" flag on the last one. The trick is to force signals so that rom_ctrl skips over the middle. Instead of sending 0,1,2,3,...98,99,100, it can now send "0,1,99,100". This way, the design code in rom_ctrl handles the handshake with the KMAC request. As far as KMAC is concerned, we're just asking for the hash of a very small image.

@rswarbrick
rswarbrick marked this pull request as ready for review August 14, 2026 14:58
@rswarbrick
rswarbrick requested a review from a team as a code owner August 14, 2026 14:58
@rswarbrick
rswarbrick requested review from KinzaQamar and removed request for a team August 14, 2026 14:58
This doesn't do anything interesting unless you want to configure
activity of an environment which itself contains agents.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick
rswarbrick force-pushed the rom-ctrl-vertical-reuse branch from 94c9d90 to d28e3bb Compare August 14, 2026 15:18
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Force-push fixes the chip-level build. I'd changed a bound interface in #30985 to require a parameter. A good idea! But I'd forgotten to update this PR to match.

@rswarbrick

Copy link
Copy Markdown
Contributor Author

Hmm. I'm not quite sure why the chip-level tests are failing with this PR. Switching back to draft while I debug things properly.

@rswarbrick
rswarbrick marked this pull request as draft August 14, 2026 15:47
@vogelpi

vogelpi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

The communication with KMAC works by rom_ctrl sending a stream of words and asserting a "last" flag on the last one. The trick is to force signals so that rom_ctrl skips over the middle. Instead of sending 0,1,2,3,...98,99,100, it can now send "0,1,99,100". This way, the design code in rom_ctrl handles the handshake with the KMAC request. As far as KMAC is concerned, we're just asking for the hash of a very small image.

Thanks for explaining @rswarbrick , this makes a lot of sense!

@rswarbrick
rswarbrick force-pushed the rom-ctrl-vertical-reuse branch from d28e3bb to 207ad5d Compare August 14, 2026 17:27
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Hmm. I'm not quite sure why the chip-level tests are failing with this PR. Switching back to draft while I debug things properly.

Gah. The problem is that I hadn't adjusted this PR to match the more sensible behaviour in #31006. Now fixed, I think.

@rswarbrick
rswarbrick marked this pull request as ready for review August 14, 2026 17:28
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Force-push fixes various bits of untidiness (in part inspired by code
reviews with Claude). But this will have to be a draft again until next
week because it is actually failing chip_csr_hw_reset and
chip_csr_rw.

I think the problem is that the bound-in scoreboard is incorrectly
treating some CSR accesses as unmapped and then being surprised when
the TL response doesn't contain an error. This probably just means
there's a missing get_root_map somewhere.

Changes since the last version:

  • Don't create an extra rom_ctrl_mem_bkdr_util! I'd obviously
    written this over an extended period of time, and had passed the
    block-level environment the backdoor utility that was already being
    created (good). But I'd also created another one and passed that as
    well (with a different type). Oops.

  • Cope slightly more reasonably with DISABLE_ROM_INTEGRITY_CHECK by
    passing a flag from the testbench to say it is defined and not
    expecting fsm_vif to be provided if so.

  • Slightly simplify how information gets passed from the tb to the
    environment. This removes a bit of repetition of reg block names,
    and also avoids passing a special clk_rst_if for the default RAL
    (which is not what dv_base_env expects).

@rswarbrick
rswarbrick force-pushed the rom-ctrl-vertical-reuse branch from 207ad5d to 6ef7caf Compare August 14, 2026 18:33
@rswarbrick
rswarbrick marked this pull request as draft August 14, 2026 18:33
The is_tl_access_mapped_addr function is designed to tell the
scoreboard whether an address is actually mapped to something in the
block.

This works by asking a uvm_reg_map to look up the address. *This* only
works with the root map, because UVM doesn't pass the addresses to
child maps when setting everything up.

It matters for a chip-level simulation with a block-level scoreboard
sitting inside it. In this case, "block" might be the block's
uvm_reg_block, and block.get_default_map() will return that block's
local map, rather than the chip-level one.

One example of this going wrong the is is_tl_access_mapped_addr
function. Without using the root map, the scoreboard will conclude
that the address is unmapped, and will expect a TileLink error in the
response.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
If get_reg_by_addr fails to find a register, it returns a null result.
The `downcast macro will perfectly happily cast it to a null handle
for an arbitrary type.

Unpack things more carefully and also give a more helpful message if
either of the following casts fails.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
The cip_base_scoreboard::check_alert_triggered task worked by counting
cycles an cfg.clk_rst_vif. This is ok... as long as the alert
interface is running that fast.

When trying verticular reuse, I've just found that rom_ctrl gets run
with a slower alert interface than the main clock for rom_ctrl. This
was then causing timeouts in checks like "If an alert should have been
triggered, wait up to 10 cycles": the ten cycles were waiting on the
fast clock and the actual alert trigger was running on the slower one.

The "alert_due_to_ping" mechanism still looks a bit strange in this
task, but that's a matter for a follow-up commit.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
In case this is seeing vertical reuse, we need to look up registers
through the root map.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
Getting this to work took quite a lot of other changes but
this (finally) builds and runs a test successfully.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick
rswarbrick force-pushed the rom-ctrl-vertical-reuse branch from 6ef7caf to 3a4231c Compare August 16, 2026 20:51
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Well, that was tricky! I think the force-push that I have just done should fix things. Changes:

  • Wire some more register lookups through the root map in cip_base_scoreboard and rom_ctrl_scoreboard.
  • Fix the possibility for a null-pointer dereference (that was caused before I fixed the previous item). This isn't technically needed in this PR: I'll drop it when the commit is in another one.
  • Correct the timing when waiting for an alert if the alert interface is running on a slower clock than the main clock for the block. (Apparently, we had never tested that situation at block-level).

I'm going to leave this PR in draft and split out the register lookup changes and timing calculation fix into two separate PRs that can land first. This has been rather difficult!

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

Labels

Component:DV DV issue: testbench, test case, etc. IP:rom_ctrl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants