Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22215 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Add ECC support ......................................................................
Patch Set 16: Code-Review+1
(16 comments)
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 341: printk(BIOS_DEBUG, "ECC supported: %s ECC forced: %s\n", Maybe don't add it here on the previous patchset?
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 105: true Maybe initialize this to ctrl->ecc_supported instead?
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 105: supports_ecc maybe rename to "can_use_ecc" ? Or use ctrl->ecc_supported directly?
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 210: I think we can assume that no CPU will ever force ECC without having ECC capabilities, especially if using `ctrl->ecc_supported` as the initial value for the `supports_ecc` flag:
if (ctrl->ecc_forced || CONFIG(RAMINIT_ENABLE_ECC)) ctrl->ecc_enabled = supports_ecc;
if (ctrl->ecc_forced && !ctrl->ecc_enabled) die("ECC mode forced but non-ECC DIMM installed!");
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 216: (CONFIG(RAMINIT_ENABLE_ECC) && supports_ecc)) fits in one line
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 220: ctrl->ecc_enabled ? "enabled" : "disabled"); same
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/raminit_common.h:
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 137: bool ecc_enabled; I guess this was missed when rebasing, but I put FOUR warnings about bumping MRC_CACHE_VERSION when the saved data changes! 😄
/* WARNING: Do not forget to increase MRC_CACHE_VERSION when this struct is changed! */
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 21: /* This was moved to mchbar_regs.h, so adding it here seems spurious
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3174: empty line
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3175: int channel, slotrank, row; : int rowsize; declare all in one line?
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3179: >> spaces
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3185: 0x0001f006 please replace with JEDEC macros
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3187: (MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD) << 10) : | 1 | (ctrl->tRCD << 16) maybe precalculate this?
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3190: (slotrank << 24) This could also be precalculated (to make things fit in one line)
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3206: e capital E
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3212: empty line