----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1927/#review4487 -----------------------------------------------------------
src/mem/SimpleDRAM.py <http://reviews.gem5.org/r/1927/#comment4205> I don't see how the bus width and burst size (in bits/bytes) is of any relevance here any longer. Now it is all about the burst length (in beats), is it not? src/mem/SimpleDRAM.py <http://reviews.gem5.org/r/1927/#comment4206> I would change the comment to say, 8 beats across a ... src/mem/SimpleDRAM.py <http://reviews.gem5.org/r/1927/#comment4207> Once again, the size in bytes is not as relevant here, it is the BL that matters. src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4208> ...such that we know when the whole packet is served. src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4209> Perhaps name the variable pktCount here as well? src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4210> Same as above src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4212> Could you add a comment that this is called multiple times on the same packet if a system packet is larger than the burst of the memory and that the addr is used for the offset within that packet. src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4211> unsigned int src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4213> I still don't understand why this is not const src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4214> And this one :-) src/mem/simple_dram.hh <http://reviews.gem5.org/r/1927/#comment4215> readBursts? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4216> I would move this to the initialisation list. Space around '/' src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4217> \n at the end of the DPRINTF string. src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4218> Move to initialisation list for these two. src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4219> \n at the end src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4220> Is the packet ptr still needed here? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4221> Would this not be easy enough to fix here? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4224> pktCount > 1, then new BurstHelper here? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4222> Why involve the absolute address here, and not just divide getSize and burstSize? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4223> wrtie src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4225> Once again I'm not sure of the while loop here. Why not a for look with pktCount? src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4226> Alignment here? Worth dealing with that as well I would imagine. src/mem/simple_dram.cc <http://reviews.gem5.org/r/1927/#comment4227> dram_pkt->burstHelper still points to the burstHelper. - Andreas Hansson On June 29, 2013, 10:44 p.m., Amin Farmahini wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1927/ > ----------------------------------------------------------- > > (Updated June 29, 2013, 10:44 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > This patch gets rid of bytesPerCacheLine parameter and makes the DRAM > configuration separate from cache line size. > Instead of bytesPerCacheLine, I define a parameter for DRAM called > burst_length. The burst_length parameter shows the length of a DRAM device > burst in bits. > Also, I replace lines_per_rowbuffer with device_rowbuffer_size to improve > code portablity. > > Updates: > - a burst length in beats for each memory type. > - an interface width for each memory type. > - the memory controller model is extended to reason about "system" packets vs > "dram" packets and assemble the responses properly. It means that system > packets larger than a full burst are split into multiple dram packets. > > > Diffs > ----- > > src/mem/SimpleDRAM.py UNKNOWN > src/mem/simple_dram.hh UNKNOWN > src/mem/simple_dram.cc UNKNOWN > > Diff: http://reviews.gem5.org/r/1927/diff/ > > > Testing > ------- > > None > > > Thanks, > > Amin Farmahini > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
