Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 4:
(8 comments)
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.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, 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.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, 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 ^^
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2965: * 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.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2974: (ctrl->tRCD << 16) | It's a single sub-sequence command, is the delay applied at all with only one iteration?
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2975: (ctrl->tFAW >> 2) + 1 DIV_ROUND_UP() might be more accurate.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, 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.)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, 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.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, 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 :-/