Tried to mark things as resolved that aren't introduced
by this change. I'll try to also have a closer look at
the timings when I find the time.
8 comments:
File src/northbridge/intel/sandybridge/raminit_common.c:
Patch Set #4, Line 2962: slotrank & 0xC
AFAICS, `slotrank` is set to a number from 0 to 3 by FOR_ALL_POPULATED_RANKS.
So this would be constant 0.
Patch Set #4, Line 2964: * Writes 128 * 8 bytes in a loop
So this is almost true for a single iteration of the innermost loop.
The subsequence loop in the IOSAV writes 129 * 8 (burst length) *
8 (bus width) bytes. I would never have guessed that it refers to
that when I read the comment. It should go to line 2981, IMO.
Maybe the comment here should start with something about the 4(!)
nested loops that actually happen here (banks, rows, cmd sequence,
cmd sub sequence). Even 5 if one includes the write burst ^^
* Bank bits are located at bit 13:15 in physical memory map
* Rowbits start at bit 20 in physical memory map
What purpose do these comments serve? Even if that is the
current memory map, we don't use it here.
Patch Set #4, Line 2974: (ctrl->tRCD << 16) |
It's a single sub-sequence command, is the delay applied at all with only one iteration?
Patch Set #4, Line 2975: (ctrl->tFAW >> 2) + 1
DIV_ROUND_UP() might be more accurate.
Patch Set #4, Line 2978: 0x60000
(row_bits - 10) << 16
I guess it doesn't matter for a single iteration as long is it's high enough (6 is max.)
Patch Set #4, Line 2985: row |
For this command, we set the column address. To clear the whole row,
we just iterate over all columns. It doesn't matter where we start
because the IOSAV wraps around for us, but `row` here looks too odd.
Patch Set #4, Line 2994: (bank << 20)
The 0x400 (bit 10) make it a PREA (precharge all banks), so the bank address is not needed.
I wonder, though, why we do a PREA :-/
To view, visit change 40721. To unsubscribe, or for help writing mail filters, visit settings.