-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rambus design #24
base: policy_stack
Are you sure you want to change the base?
Rambus design #24
Conversation
Change-Id: I4966076551bbcb234b3aad30c3cbfae30477007e
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.
Some quick thoughts :)
@@ -73,7 +73,7 @@ class MemCtrl(QoSMemCtrl): | |||
# threshold in percentage for when to start writes if the read | |||
# queue is empty | |||
write_low_thresh_perc = Param.Percent(50, "Threshold to start writes") | |||
|
|||
oldest_write_age_threshold = Param.Unsigned(5000000, "The age of oldest write request in the write queue in ticks") |
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 should be a Param.Latency
and should be given in seconds (or ns). The current code will break if the tick rate changes.
@@ -426,10 +426,116 @@ DRAMInterface::doBurstAccess(MemPacket* mem_pkt, Tick next_burst_at, | |||
DPRINTF(DRAM, "Schedule RD/WR burst at tick %d\n", cmd_at); | |||
|
|||
// update the packet ready time | |||
if (mem_pkt->isRead()) { | |||
mem_pkt->readyTime = cmd_at + tRL + tBURST; | |||
if(mem_pkt->isTagCheck) { |
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.
Should have a getter instead of making member variables public.
@@ -48,6 +48,7 @@ | |||
#include "debug/DRAM.hh" | |||
#include "debug/DRAMPower.hh" | |||
#include "debug/DRAMState.hh" | |||
#include "debug/MemCtrl.hh" |
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.
General comment on this file... rather than modifying gem5's DRAM interface, it probably would be better to create a new subclass of interface.
This is OK for the paper, but if this is going to ever be pushed into gem5, we will need to make this change.
@@ -70,6 +70,8 @@ MemCtrl::MemCtrl(const MemCtrlParams &p) : | |||
writeBufferSize(dram->writeBufferSize), | |||
writeHighThreshold(writeBufferSize * p.write_high_thresh_perc / 100.0), | |||
writeLowThreshold(writeBufferSize * p.write_low_thresh_perc / 100.0), | |||
oldestWriteAgeThreshold(p.oldest_write_age_threshold), |
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.
Same as above, like the HBM ctrl, this should probably be a new subclass.
DPRINTF(MemCtrl, "recvAtomic: %s 0x%x\n", | ||
DPRINTF(MemCtrl, "recvAtomic: %s %d\n", |
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 change from hex to decimal? IMO, hex addresses are much easier to read :)
@@ -158,7 +158,7 @@ class PolicyManager : public AbstractMemory | |||
bool validLine = false; | |||
// constant to indicate that the cache line is dirty | |||
bool dirtyLine = false; | |||
Addr farMemAddr; | |||
Addr farMemAddr = -1; |
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'm not sure this is going to do what you want. -1
is a signed int not a signed long long. I don't know if the compiler is going to put 0xFFFFFFFF
or 0xFFFFFFFFFFFFFFFF
here. You should use the following.
Addr farMemAddr = -1; | |
Addr farMemAddr = MaxAddr; |
system.mem_ctrl.tRP = '14.16ns' | ||
system.mem_ctrl.tRCD_RD = '14.16ns' | ||
system.mem_ctrl.tRL = '14.16ns' | ||
system.mem_ctrl.tRP = '14ns' | ||
system.mem_ctrl.tRCD_RD = '12ns' | ||
system.mem_ctrl.tRL = '18ns' | ||
system.mem_ctrl.loc_mem_policy = 'Rambus' | ||
|
||
system.loc_mem_ctrl = MemCtrl() | ||
system.loc_mem_ctrl.dram = DDR4_2400_16x4_Alloy(range=AddrRange('1GiB'),in_addr_map=False, null=True) | ||
system.loc_mem_ctrl.dram = HBM_2000_4H_1x64_Rambus(range=AddrRange('1GiB'), in_addr_map=False, null=True) | ||
|
||
system.loc_mem_ctrl.dram.device_rowbuffer_size = "512B" | ||
system.loc_mem_ctrl.dram.banks_per_rank = 32 | ||
system.loc_mem_ctrl.dram.bank_groups_per_rank = 8 | ||
system.loc_mem_ctrl.dram.page_policy = 'close' | ||
|
||
system.loc_mem_ctrl.dram.burst_length = 8 | ||
system.loc_mem_ctrl.dram.tCCD_L = "4ns" | ||
system.loc_mem_ctrl.dram.tBURST = "4ns" |
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.
It would be much better to use some object oriented design and not duplicate the code from ruby_system.py
here. What happens if you forget to make a change in both places?
system.mem_ctrl.loc_mem_policy = 'Rambus' | ||
|
||
system.loc_mem_ctrl = MemCtrl() | ||
system.loc_mem_ctrl.dram = DDR4_2400_16x4_Alloy(range=AddrRange('1GiB'),in_addr_map=False, null=True) | ||
system.loc_mem_ctrl.dram = DDR4_2400_16x4(range=AddrRange('1GiB'),in_addr_map=False, null=True) |
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.
These should probably be parameters, not hard coded.
No description provided.