-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate Hyperbus peripheral #47
base: devel
Are you sure you want to change the base?
Conversation
19cbd63
to
d1090d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
e8e14e2
to
a91fe71
Compare
5f569cd
to
d1090d3
Compare
d1090d3
to
abdb469
Compare
abdb469
to
0c20226
Compare
0c20226
to
840a1dd
Compare
@Lore0599 @arpansur @Scheremo @adimauro-iis |
I think both are blocking but trivial to resolve. You can extend the register memory map range (0x3000_0000 - 0x4000_0000) contiguously with the config registers and map the hyperbus memory "somewhere reasonable" (why not 0x5000_0000?) |
@@ -8,4 +8,4 @@ | |||
COMMON_TARGS ?= | |||
COMMON_TARGS += -t snitch_cluster -t cv32a6_convolve -t cva6 -t rtl | |||
|
|||
SIM_TARGS = -t test -t sim | |||
SIM_TARGS = $(COMMON_TARGS) -t test -t sim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you add here $(COMMON_TARGS)? Isn't the variable already used in sim.mk
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also answers the other comment
Just a small modification to make (in my opinion) the flow clearer. At some point we'll have many targets in the bender.mk
, e.g. SIM_TARGS
, ASIC_TARGS
, FPGA_XILING_TARGS
, FPGA_whatever_TARGS
, etc...
So I explicitly add $(COMMON_TARGS)
(which is common to all of them) within all those other TARGS variables.
Previously, it was left to whoever wrote the independent target .mk
(e.g., sim.mk
, asic.mk
, ...) to use both variables, as in: bender script vsim $(COMMON_TARGS) $(SIM_TARGS) [...]
.
But ultimately it's the same argument for bender
so we might as well use this clearer form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, thanks
$(CHIM_SIM_DIR)/vsim/compile.tcl: chs-hw-init snitch-hw-init | ||
@bender script vsim $(COMMON_TARGS) $(SIM_TARGS) --vlog-arg="$(VLOG_ARGS)"> $@ | ||
@bender script vsim $(SIM_TARGS) --vlog-arg="$(CHIM_VLOG_ARGS)" > $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove $(COMMON_TARGS)?
.UserPreload (HypUserPreload), | ||
.mem_file_name(HypUserPreloadMemFiles[i]), | ||
.TimingModel ("S27KS0641DPBHI020") | ||
) dut ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Chimera instance inside fixture_chimera_soc.sv
is already called DUT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True but the hierarchy is different so no problem arises. Should I change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly fine to me, some comments sprinkled throughout. Is there some consideration behind having two PHYs? I understand two endpoints (HyperRAM and HyperFlash), but that should only necessitate another CS pin.
- hw/chimera_top_wrapper.sv | ||
|
||
- target: any(simulation, test) | ||
files: | ||
- target/sim/models/s27ks0641/s27ks0641.v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this included in Cheshire?
|
||
module chimera_top_wrapper | ||
import cheshire_pkg::*; | ||
import chimera_pkg::*; | ||
import chimera_reg_pkg::*; | ||
#( | ||
parameter int unsigned SelectedCfg = 0 | ||
parameter int unsigned SelectedCfg = 0, | ||
parameter int unsigned HypNumPhys = 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this correspond to 2 PHYs? In this case I think a smarter default is 1 PHY.
localparam byte_bt HyperbusIdx = MemIslandIdx + 1; | ||
localparam doub_bt HyperbusRegionStart = 64'h5000_0000; | ||
//TODO(smazzola): Correct size of HyperRAM? | ||
localparam doub_bt HyperbusRegionEnd = HyperbusRegionStart + 64'h0800_0000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think after our discussion last week, we can extend the region size of 64'h1000_0000
.RstChipBase (ChsCfg.LlcOutRegionStart), | ||
.RstChipSpace (HypNumPhys * HypNumChips * 'h800_0000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is correct, can you comment?
.UserPreload (HypUserPreload), | ||
.mem_file_name(HypUserPreloadMemFiles[i]), | ||
.TimingModel ("S27KS0641DPBHI020") | ||
) dut ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for now.
This PR:
integrates the Hyperbus peripheral in the Chimera top RTL
integrates the HyperRAM VIP in the Chimera testbench
adds an addressability test for the Hyperbus and adds it to the CI
Config registers are still to be added add config registers to hyperbus #43
What address range should we use for the HyperRAM? https://github.com/pulp-platform/chimera/blob/smazzola/hyperbus/hw/chimera_pkg.sv#L128-L130