Remove GenerateSimFiles and use make instead #879
Conversation
| firrtl.FileUtils.makeDirectory("./bootrom/") | ||
| writeResource("/testchipip/bootrom/bootrom.rv64.img", "./bootrom/") | ||
| writeResource("/testchipip/bootrom/bootrom.rv32.img", "./bootrom/") | ||
| writeResource("/bootrom/bootrom.img", "./bootrom/") |
There was a problem hiding this comment.
What was this /bootrom/bootrom.img in the old code? I didn't completely understand where it was used in Chipyard before but I knew it wouldn't interfere with what I was doing in FireSim. I'm only asking because I don't see where this is being maintained in the new makefile version.
There was a problem hiding this comment.
Good catch. This is Rocket Chips bootrom file: https://github.com/ucb-bar/chipyard/blob/dev/generators/utilities/src/main/resources/bootrom. If you want to use the config fragment in RC (https://github.com/chipsalliance/rocket-chip/blob/b888d0d0eb9975344292d695072e7a9b438cefbb/src/main/scala/subsystem/Configs.scala#L40) you need to have the RC bootrom reside at the top-level in CHIPYARD/bootrom/bootrom.img. Ideally RC does what CY does and uses TargetDirKey... but for now, this should suffice.
There was a problem hiding this comment.
I should also note... that 99% of the time people will not use the RC bootrom since the CY one does the same thing for the most part so I'm inclined to ignore this bootrom and keep the makefiles clean.
There was a problem hiding this comment.
Ignoring it seems fine to me if they do the same thing. I didn't realize that's where it was coming from or I would have tried to address that in the PR where I added the TargetDirKey.
There was a problem hiding this comment.
I don't think they quite do the exact same thing anymore. There are some features we support, which the RC bootrom does not, and vice versa.
We should always use the CY bootrom though.
There was a problem hiding this comment.
In the past we had a SUB_PROJECT=rocketchip which setup things to explicitly use RC collateral and not any CY collateral. However, we have drifted from that over time. So I think we can just ignore the RC bootrom.
| @@ -21,3 +21,6 @@ SIM_LDFLAGS = \ | |||
| -lfesvr \ | |||
| -ldramsim \ | |||
| $(EXTRA_SIM_LDFLAGS) | |||
|
|
|||
| SIM_FILE_REQS += \ | |||
| $(ROCKETCHIP_RSRCS_DIR)/vsrc/EICG_wrapper.v | |||
There was a problem hiding this comment.
Note: If chipsalliance/rocket-chip#2810 goes through then this is unnecessary.
| SIM_FILE_REQS += \ | ||
| $(ROCKETCHIP_RSRCS_DIR)/vsrc/EICG_wrapper.v | ||
|
|
||
| # copy files but ignore *.h files in *.f (match vcs) |
There was a problem hiding this comment.
Are you trying to match vcs here or are you assuming that all FPGA synthesis tools will support +incdir like VCS does and you can do the same thing as you're doing for VCS? If you're really trying to match VCS, why?
There was a problem hiding this comment.
This matches what GenerateSimFiles did in the past (filter out headers). I guess another justification for doing this is that #871 is using VCS to do FPGA-level simulation.
| .settings(libraryDependencies ++= rocketLibDeps.value) | ||
| .settings(commonSettings) | ||
|
|
||
| lazy val utilities = (project in file("generators/utilities")) | ||
| .sourceDependency(testchipip, testchipipLib) |
There was a problem hiding this comment.
Nice. Everything I see this I have to remind myself why we need this. Glad to see it getting removed
Related issue: #872
Type of change: other enhancement
Impact: other
Release Notes
By using
makewe can track the files created byGenerateSimFilesand properly move them / update them on changes. This additionally comes with a bonus of removing 1 SBT call.@timsnyder