Skip to content

Move the bootrom to the ROM#447

Merged
engdoreis merged 13 commits intolowRISC:mainfrom
engdoreis:bootrom-verilator
Apr 28, 2026
Merged

Move the bootrom to the ROM#447
engdoreis merged 13 commits intolowRISC:mainfrom
engdoreis:bootrom-verilator

Conversation

@engdoreis
Copy link
Copy Markdown
Collaborator

@engdoreis engdoreis commented Apr 18, 2026

This PR:

  • Move the bootrom to the ROM
  • Move all tests to run on DRAM rather than SRAM.
  • Enable all verilator tests to use the bootrom.

@engdoreis engdoreis force-pushed the bootrom-verilator branch 13 times, most recently from 2d5ef23 to 5a4aeab Compare April 23, 2026 16:32
@engdoreis engdoreis changed the title [wip] Run tests on DRAM [wip] Move the bootrom to the ROM Apr 23, 2026
@engdoreis engdoreis changed the title [wip] Move the bootrom to the ROM Move the bootrom to the ROM Apr 23, 2026
@engdoreis engdoreis marked this pull request as ready for review April 23, 2026 17:13
@engdoreis engdoreis requested review from AlexJones0, marnovandermaas and ziuziakowska and removed request for ziuziakowska April 23, 2026 17:13
Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

This is a very cool PR. Just a few nits from my end and we need to make sure to test the DV run to make sure nothing breaks.

Comment thread sw/cmake/tests.cmake Outdated
Comment thread hw/top_chip/rtl/chip_mocha_genesys2.sv Outdated
Comment thread hw/top_chip/rtl/chip_mocha_genesys2.sv Outdated
Comment thread sw/cmake/tests.cmake Outdated
Comment thread sw/cmake/tests.cmake Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need to make sure to run and check this before merging.

Comment thread sw/cmake/tests.cmake
@engdoreis engdoreis force-pushed the bootrom-verilator branch 2 times, most recently from cf21f52 to fb93064 Compare April 24, 2026 10:39
Copy link
Copy Markdown
Contributor

@ziuziakowska ziuziakowska left a comment

Choose a reason for hiding this comment

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

These are really great changes, I just have some nits and comments.

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.

Something to think about: Maybe we could flip the condition to be NO_FPGA/SKIP_FPGA as that could stand out more, and it makes more sense with comments/TODOs explaining why that is there instead of why FPGA isn't there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is a good idea.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This PR is tool large already, so I opened an issue: #477

Comment thread hw/top_chip/dv/verilator/top_chip_verilator.sv
Comment thread sw/device/lib/boot/mocha_dram.ld Outdated
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.

Maybe we could have a lib/boot/linkerscript or lib/boot/ld subdirectory to contain these, as there are quite a few now.

Comment thread sw/device/bootrom/bootrom.c Outdated
Comment thread sw/device/bootrom/bootrom.c Outdated
Comment thread sw/device/bootrom/rom.ld Outdated
Comment thread sw/device/bootrom/CMakeLists.txt
Comment thread util/verilator_runner.sh
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.

Doesn't need to be done in this PR, but now we can remove this runner and have Verilator tests run the binary directly.

Copy link
Copy Markdown
Collaborator Author

@engdoreis engdoreis Apr 24, 2026

Choose a reason for hiding this comment

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

I like the idea, but rather than fixing the path to verilator, we should use cmake to find it. So I'll leave it to a next PR.

Comment thread sw/device/tests/CMakeLists.txt Outdated
Copy link
Copy Markdown

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

LGTM, with a few small nits & questions.

Comment thread sw/device/bootrom/rom.ld Outdated
Comment thread sw/device/bootrom/CMakeLists.txt Outdated
Comment thread hw/top_chip/rtl/chip_mocha_genesys2.sv Outdated
Comment thread hw/vendor/cva6_cheri/core/include/cv64a6_imafdchzcheri_sv39_config_pkg.sv Outdated
Comment thread sw/cmake/tests.cmake Outdated
Comment thread sw/device/tests/CMakeLists.txt Outdated
@engdoreis engdoreis marked this pull request as draft April 24, 2026 16:00
@martin-velay
Copy link
Copy Markdown
Contributor

@thommythomaso, can you take a look at the RTL change of this commit: "[hw] Fix error sram error in dv"

@engdoreis engdoreis force-pushed the bootrom-verilator branch 2 times, most recently from 31960e0 to ff6bb31 Compare April 27, 2026 13:50
The bootrom expect pin 8 to be pulled down to enter bootstrap mode, some
making all pins up by default would skip bootstrap in verilator.

Signed-off-by: Douglas Reis <doreis@lowrisc.org>
Macros share the scope with the caller, which can lead to undefined
behaviour in complex macros.
To start from a clean state, otherwise a dram will never boot if sram is
valid.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This patch can now be removed due to the DRAM fix PR.

Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Let's make sure that the top-level DV passes before merging but after that I'm happy for this to be merged.

@engdoreis engdoreis marked this pull request as ready for review April 28, 2026 15:31
Copy link
Copy Markdown

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work on this.

I'm assuming the SRAM HW change is correct in that we cannot have sram_we when we are trying to read data, but I don't know enough about mocha's SRAM to validate that myself.

engdoreis and others added 4 commits April 28, 2026 17:36
Signed-off-by: Douglas Reis <doreis@lowrisc.org>
The axi_to_detailed_mem module requires a valid read request to be
accompanied by every write. This is problematic since at the start of
time the content of SRAM is understandably undefined. Instead of feeding
don't cares through the module alongside an rvalid signal. Now this
feeds a recognisable hex sequence through the module so that it still
works but it is also somewhat detectable if this read propagates to the
output.
build_cmd = ["cmake", "--build", build_dir, "-v", "--target"]
install_cmd = ["cmake", "--install", build_dir, "--prefix", out_dir, "--component"]
for img in args.sw_images:
for img in args.sw_images.split():
Copy link
Copy Markdown

@AlexJones0 AlexJones0 Apr 28, 2026

Choose a reason for hiding this comment

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

Nit: if it can be done quickly, we should look into what part of DVSim / the HJSON flows is requiring us to do this and fix it there instead. This isn't a problem at all since we're running on Linux but is a bit of a code smell generally if it can be avoided.

@engdoreis engdoreis merged commit f17b7f6 into lowRISC:main Apr 28, 2026
5 checks passed
@engdoreis engdoreis deleted the bootrom-verilator branch April 28, 2026 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants