Move the bootrom to the ROM#447
Conversation
2d5ef23 to
5a4aeab
Compare
5a4aeab to
4ea2a85
Compare
marnovandermaas
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We need to make sure to run and check this before merging.
cf21f52 to
fb93064
Compare
ziuziakowska
left a comment
There was a problem hiding this comment.
These are really great changes, I just have some nits and comments.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this is a good idea.
There was a problem hiding this comment.
This PR is tool large already, so I opened an issue: #477
There was a problem hiding this comment.
Maybe we could have a lib/boot/linkerscript or lib/boot/ld subdirectory to contain these, as there are quite a few now.
There was a problem hiding this comment.
Doesn't need to be done in this PR, but now we can remove this runner and have Verilator tests run the binary directly.
There was a problem hiding this comment.
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.
AlexJones0
left a comment
There was a problem hiding this comment.
LGTM, with a few small nits & questions.
fb93064 to
3c3895e
Compare
3c3895e to
5da738b
Compare
|
@thommythomaso, can you take a look at the RTL change of this commit: "[hw] Fix error sram error in dv" |
31960e0 to
ff6bb31
Compare
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.
There was a problem hiding this comment.
This patch can now be removed due to the DRAM fix PR.
marnovandermaas
left a comment
There was a problem hiding this comment.
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.
ff6bb31 to
93ecd0a
Compare
93ecd0a to
1bf9ba4
Compare
AlexJones0
left a comment
There was a problem hiding this comment.
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.
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.
1bf9ba4 to
992a877
Compare
| 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(): |
There was a problem hiding this comment.
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.
This PR: