Misc Additions to Local FPGA Branch #669
Conversation
abejgonzalez
changed the title
There was a problem hiding this comment.
Seem pretty good. I think we don't need to consume the weird style habits in the makefile, and instead can make the names more similar to the rest of chipyard.
Also obviously testing would be good, but I assume that can happen after the merge into the orig PR.
Not blocking merge since this is just a PR to a PR
| @@ -0,0 +1,3 @@ | |||
| * | |||
There was a problem hiding this comment.
I think we should make the builds in here look more standard so we can change this ignore all to something like generated-src,output, etc.
There was a problem hiding this comment.
Reasonable goal.
| ######################################################################################### | ||
| all_vsrcs := \ | ||
| $(sim_vsrcs) \ | ||
| $(base_dir)/generators/sifive-blocks/vsrc/SRLatch.v \ |
There was a problem hiding this comment.
Why these included via the standard firrtl method?
There was a problem hiding this comment.
These are just black boxes and do not have a resource associated with them. So we would have to make a PR for these to be included through the "normal automatic" way.
There was a problem hiding this comment.
For now, I just blindly included them, later I think a PR should be opened in both places to make this automatic.
fpga/Makefile
Outdated
| $(build_dir)/$(long_name).rom.v | ||
|
|
||
| ######################################################################################### | ||
| # build rom for the fpga |
There was a problem hiding this comment.
Sure. Just copied the names.
fpga/Makefile
Outdated
| export LONG_NAME=$(long_name) | ||
| export ROMCONF=$(build_dir)/$(long_name).rom.conf | ||
|
|
||
| romgen := $(build_dir)/$(long_name).rom.v |
There was a problem hiding this comment.
I don't like this name. It is the generated rom not a rom generator. I think rom_v rom_file etc are better names.
There was a problem hiding this comment.
Sure. Just copied the names.
fpga/bootrom/xip/Makefile
Outdated
| @@ -35,11 +40,11 @@ hex: $(hex) | |||
|
|
|||
There was a problem hiding this comment.
I can't comment on the above lines since they aren't in the diff for this PR, but I have one question.
Is the hardcoded target_address coming from an RTL config fragment? If so we should think about how to do this without repeating ourselves.
There was a problem hiding this comment.
Yea. Im not sure. I need to look into this more... they end up using a MaskROM over a BootROM in the code which is a bit confusing to me. Why not use a BootROM? I need to further investigate.
There was a problem hiding this comment.
I believe MaskROM implies a specific kind of ROM that is reprogrammable with just a single VLSI mask update. I think there is an overload used here sometimes for an FPGA ROM that can be updated without the rest of the bit stream being changed.
Besides that definition, I think a MaskROM can be a BootROM (a ROM that contains code to boot, and is where the core resets to), but not all BootROMs would be MaskROMs and not all MaskROMs would be BootROMs.
There was a problem hiding this comment.
So it looks like the MaskROM is used to load in a ROM after elaboration using readmemh. This differs from the BootROM which bakes the .img directly into the module in Chisel.
There was a problem hiding this comment.
Should be good to just swap MaskROM for the BootROM
|
|
||
| .PHONY: romgen | ||
| romgen: $(romgen) | ||
|
|
||
| .PHONY: clean | ||
| clean:: | ||
| rm -rf $(hex) $(elf) |
| @@ -51,6 +51,7 @@ class E300ArtyDevKitPlatformIO(implicit val p: Parameters) extends Bundle { | |||
| //------------------------------------------------------------------------- | |||
|
|
|||
| class E300ArtyDevKitPlatform(implicit val p: Parameters) extends Module { | |||
| //val sys = Module(LazyModule(new E300ArtyDevKitSystem).module) This can be DigitalTop? | |||
There was a problem hiding this comment.
Is this just remnants from some other testing?
There was a problem hiding this comment.
Oh. Yea. This should be deleted.
| val maskROMs = p(MaskROMLocated(location)).map { MaskROM.attach(_, this, CBUS) } | ||
|
|
||
| val maskROMResetVectorSourceNode = BundleBridgeSource[UInt]() | ||
| tileResetVectorNexusNode := maskROMResetVectorSourceNode |
There was a problem hiding this comment.
It seems weird to support both bootROM and maskROM above but then not do some connection here.
Do we require a maskROM but optionally allow a bootROM that never can be the reset location?
There was a problem hiding this comment.
Yea. This was a hack to try and do what the original code wanted. I need to look into this.
| GENERATOR_PACKAGE := chipyard | ||
| TB := none # unused | ||
| TOP := E300ArtyDevKitPlatform | ||
|
|
There was a problem hiding this comment.
Should the goal be to eventually eliminate most of these overrides? I believe eventually everything can just live as part of the core chipyard generator.
There was a problem hiding this comment.
I think thats related to the other checkbox in my review of the original PR. I believe the answer is yes.
There was a problem hiding this comment.
Yes. The goal is to get rid of these. I just hard-coded them so that this particular example works all the time. Ideally, this will change with more configuration exposed.
|
Updated. Now uses the CY BootRom, addressed some comments, and now uses |
abejgonzalez
changed the title
Related issue: #666
Type of change: new feature
Impact: rtl change + other
Release Notes
This builds off of #666 to have it use the Chipyard make system and try to make the compile work with the newest RC changes. Note, this doesn't check for RTL functionality... still needs to be checked on an Arty board.