mem: Fix address interleaving bug in DRAM controller

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10286:08c7971d5f0a
---------------------------
mem: Fix address interleaving bug in DRAM controller

This patch fixes a bug in the DRAM controller address decoding. In
cases where the DRAM burst size was smaller than the interleaving
stripe size (e.g. LPDDR3 x32 with a 64 byte cache line) one address
bit effectively got used as a channel bit when it should have been a
low-order column bit.

This patch adds a notion of "columns per stripe", and more clearly
deals with the low-order column bits and high-order column bits. The
patch also relaxes the granularity check such that it is possible to
use interleaving granularities other than the cache line size.

The patch also adds a missing M5_CLASS_VAR_USED to the tCK member as
it is only used in the debug build for now.

Testing Done

While I don't understand why there are column bits at the lower end of the address,
I suggest we change the name of the addressing policy to reflect that column 
address bits have two subsets.

Andreas Hansson Aug. 17, 2014, 1:28 a.m. (Aug. 17, 2014, 1:28 a.m.)

Obviously I need to work on my explanation. Welcome back btw :-)

In essence, if the system uses a cache line size of 64 bytes, this is the smallest possible granularity we will interleave things on in the crossbar (as we cannot go any smaller without cracking a cache line in two). Now, if the DRAM in question has a burst size of 32 bytes, then a specific controller is left with a  cache line being split across two columns, ranks, banks or rows. The most sensible choice here is two columns as it is guaranteed to be a row hit. Does that make more sense?

I agree with you that it would be nice to capture that there might be column bits as the LSBs in the name, but it gets pretty tricky as this only happens if interleaving granularity > DRAM burst size. I could add Coh (Co high) and Col (Co low) or similar, but I remain unconvinced as to whether that makes people more or less confused. What do you think?

Nilay Vaish Aug. 20, 2014, 10:36 a.m. (Aug. 20, 2014, 10:36 a.m.)

I think I need some explanation on what address interleaving
trying to achieve before carry this discussion further.  I am asking
this because the dram controller makes use of it.

Andreas Hansson Aug. 20, 2014, 10:42 a.m. (Aug. 20, 2014, 10:42 a.m.)

I'm not sure I understand your question. The interleaving of channels? It is for performance, and a trade-off between locality (more sequential things to the same channel), and more parallelism (interleave on a fine granularity).

Could you elaborate on what it is you are wondering?

src/mem/dram_ctrl.cc (Diff revision 1)

at least as large as, instead of larger.

src/mem/dram_ctrl.cc (Diff revision 1)

at most as large as instead of smaller.

Description:

~		Changeset 10285:218ddd25c88c
	~	Changeset 10286:08c7971d5f0a

		mem: Fix address interleaving bug in DRAM controller

		This patch fixes a bug in the DRAM controller address decoding. In
		cases where the DRAM burst size was smaller than the interleaving
		stripe size (e.g. LPDDR3 x32 with a 64 byte cache line) one address
		bit effectively got used as a channel bit when it should have been a
		low-order column bit.

		This patch adds a notion of "columns per stripe", and more clearly
		deals with the low-order column bits and high-order column bits. The
		patch also relaxes the granularity check such that it is possible to
		use interleaving granularities other than the cache line size.

		The patch also adds a missing M5_CLASS_VAR_USED to the tCK member as
		it is only used in the debug build for now.

Diff:

Revision 2 (+40 -15)

Show changes

	src/mem/dram_ctrl.hh
	src/mem/dram_ctrl.cc

Not important, but I'd suggest changing the description to:
"In cases where the DRAM burst size (e.g. 32 bytes in a rank with a single LPDDR3 x32) was smaller than the channel interleaving size (e.g. systems with a 64-byte cache line) ..."
I'd also use the "channel interleaving/striping" term instead of just simply "interleaving/striping" if that is what you mean.

Andreas Hansson Aug. 22, 2014, 12:45 a.m. (Aug. 22, 2014, 12:45 a.m.)
```
Thanks Amin. Sorted.
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 10285:218ddd25c88c

Changeset 10286:08c7971d5f0a

Diff:

Status: Closed (submitted)