Skip to content

Main PR#7

Open
mbabaie wants to merge 67 commits intodevelopfrom
dcache_orb
Open

Main PR#7
mbabaie wants to merge 67 commits intodevelopfrom
dcache_orb

Conversation

@mbabaie
Copy link
Collaborator

@mbabaie mbabaie commented Oct 15, 2021

No description provided.

aakahlow and others added 30 commits February 23, 2021 23:03
Change-Id: Iacad811fb48cf65be8cfadab7fdb421ad0c98a6b
Change-Id: I3b782f6d72ef64a96d7a2d3133b745dc99d7a025
Change-Id: If38a31c4ba0af68d7b90a8cb2ee99a364b66d561
Change-Id: Ifbfbf4aff79cbab758a35ec2ba07c19fb0b99d43
Change-Id: I3d335f8c64a605c24c0136c06087e41779e5ea30
Change-Id: Ic50fab42b25fcd98d32fab5d849e896882f82d02
Change-Id: If79e442f3c562ab0455f1c35146a032524d4ec9b
Signed-off-by: Jason Lowe-Power <jason@lowepower.com>
Change-Id: Ie7f78f53c5a20b42ff73c1459aaff551a9de1705
Change-Id: Iea35abd1d85d22731b6859940517642420411452
Change-Id: I724a84be596e84327488f485ded727f69e60ced2
Change-Id: I7812f21e19552aab60e003d57ae1b76fc1bf6769
Change-Id: I56d5178272a10f1d811de721c4c19962f4fd2abb
Change-Id: I4bea6ea7b61ad7ca404016e6f584c7a6be61b1f0
Change-Id: I888dcda7b7dbbaee6e39dce66245a2913b48817d
Change-Id: Ib1cc88d8daab2a896b848d41ff622a0f4ea99d1f
Change-Id: Id7a9742a140d9bd8de7f01326ce0181129d902de
Change-Id: I172c4c4b0b6c1de681de5dcbc2feef571b418689
Change-Id: Id6a8fb12f05ca7ac3a618a80e7bc51dc6f0b00c0
Change-Id: I0322ce93032f1e1d7ac323350036497f9f34b831
Change-Id: Iaa6f0c0a123080b6cc893e0317e7abc7c111b30d
Change-Id: I68228d5aa01bda1f22c57794a092190cce62a457
Change-Id: I1109c0e2db161e5fd69a278b355c1777f3f11a22
Change-Id: I812c6f545206b30eb7890eba0f6fa2282c4caec2
Change-Id: I87674ed82968c125ef1ee49d7b8be5c4ba3f71c2
Change-Id: I0e62bee5f2324cf5a66dc3a713b2d16e087b9e6a
Change-Id: Ic54bbe8affd720d0a80e73fe9272a375d4d6882e
mbabaie and others added 8 commits October 15, 2021 00:48
Change-Id: Ibca7d9c95ccea82959cba988502759fc7ff18db4
Change-Id: I23e6670d2838857d001f300355783c3193d52256
Change-Id: I0035689d9e0efb57bf290de3bb534251c45f09be
Change-Id: I620038aaaa3252297cbe4465976cc1c63e2540c2
Change-Id: I7af51e2c46da527d4f9b0a6a75787522025c8d2d
Copy link
Member

@powerjg powerjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to review the main part of this change since most of the file is copy-pasted. I used a local diff tool to make it a bit easier, and so I have a few comments on the main files. However, most of my comments are going to have to wait until they are unified.

I was able to get through everything but the dcache_ctrl.cc

It's really important to do the unification that I have been pushing for. If you need help figuring out how to do this, I can definitely lend a hand.

simple.py Outdated
in_addr_map=False)
system.mem_ctrl.nvm = NVM_2400_1x64(range=AddrRange('8GB'))

system.mem_ctrl.dram.tREFI = "200"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should put it on the to do list to figure out this parameter

simple.py Outdated
system.mem_ctrl.orb_max_size = "512"
system.mem_ctrl.crb_max_size = "32"

system.mem_ranges = [AddrRange('4GB')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 4GB when the NVM has 8GB?

Packet(const RequestPtr &_req, MemCmd _cmd)
: cmd(_cmd), id((PacketId)_req.get()), req(_req),
data(nullptr), addr(0), _isSecure(false), size(0),
data(nullptr),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

* Incremented when a refresh event is started as well
* Used to determine when a low-power state can be entered
*/
uint32_t outstandingEvents;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should seriously consider changing this back to a uint8 and adding an assert that it's <20 or something. I don't think we're exactly using the interface code in a way that's expected, and it worries me some that we're not going to get reasonable results.

*/
void chooseRead(dccPacketQueue& queue);

void processReadPkt(dccPacket* pkt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new function. Why is it needed? What does it do? This needs documentation.

}

Tick
writeRespQueueFront()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs documentation

uint64_t deviceCapacity = deviceSize / (1024 * 1024) * devicesPerRank *
ranksPerChannel;

dramDeviceCapacity = deviceCapacity;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is different from the original. What's going on here?

// banks state is closed but haven't transitioned pwrState to IDLE
// or have outstanding ACT,RD/WR,Auto-PRE sequence scheduled
// should have outstanding precharge or read response event
//assert(prechargeEvent.scheduled() ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it ok to drop this assert? It looks like this should never be hit because whenever you open a page you should always close it later.

}

void
NVMDCInterface::processReadPkt(dccPacket* pkt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the chooseReadFunction above but with a specific packet? Some documentation would be helpful.

If it's just the inner part of the above function, then you should remove that inner part and call this function there.

Comment on lines +2345 to +2348
bool read_rdy = pkt->isRead() && (ctrl->inReadBusState(false)) &&
(pkt->readyTime <= curTick()) && (numReadDataReady > 0);
bool write_rdy = !pkt->isRead() && !ctrl->inReadBusState(false) &&
!writeRespQueueFull();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original the inReadBusState condition is flipped (it's true, not false). Can you explain this change?

mbabaie and others added 21 commits October 20, 2021 15:21
Change-Id: I9ce0296b0861bd0dff21f5fad75f9009490150dd
Change-Id: Ia90cb99eb6c944e56b3848d2371eaa1aad5e5a01
Change-Id: I88b1ca25c27e8721ba8325756f5f930a7cad4ec2
Change-Id: Ib2d556830cdabd44268bd805d22c8009638c0580
Change-Id: I79e823b6d76a5e87a903bae20481ee13af4fd877
Change-Id: I77398c0f700563d269c2aeabe58f3bafe6ea7ff9
Change-Id: Ic037fb09c1c56a78869108caaadbabfd7d38ec48
Change-Id: I20ac210b464025eea4946c4f3548ce39783092f5
Change-Id: I6abafa642119727e480f8034fbf514c54693a356
Change-Id: I8693b3a696b92fc3a2577e3c19d9675c4812259c
Change-Id: I9e482921f1955a6b5b05ddf4c98d462ebda343d5
Change-Id: I2b8447a99ef9459e1f15c35b31d7bec892a37244
Change-Id: I4b95d58526bc9cd03d9bebc725244e6e46e5a314
Change-Id: I510ad554b99797d84e99c31d6e33da394c07cbcd
Change-Id: Idc97a96895069db76f041591a776cf173f778a47
Change-Id: Ibc413241753d671465b62ed36f66aa2ee2da8ede
Change-Id: I64b39747f3e5f23046c15c03c4d5e6ddb705b3a1
Change-Id: I446c31c261810440e6cb3d883ed4ccceb3fff28d
Change-Id: I0422f1e6a924c37895e67c109884deee98a05a40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants