Patch set 16:Code-Review +1
16 comments:
File src/northbridge/intel/sandybridge/raminit.c:
Patch Set #16, Line 341: printk(BIOS_DEBUG, "ECC supported: %s ECC forced: %s\n",
Maybe don't add it here on the previous patchset?
Maybe initialize this to ctrl->ecc_supported instead?
Patch Set #16, Line 105: supports_ecc
maybe rename to "can_use_ecc" ? Or use ctrl->ecc_supported directly?
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!");
Patch Set #16, Line 216: (CONFIG(RAMINIT_ENABLE_ECC) && supports_ecc))
fits in one line
Patch Set #16, Line 220: ctrl->ecc_enabled ? "enabled" : "disabled");
same
File src/northbridge/intel/sandybridge/raminit_common.h:
Patch Set #16, 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! */
File src/northbridge/intel/sandybridge/raminit_common.c:
This was moved to mchbar_regs.h, so adding it here seems spurious
empty line
int channel, slotrank, row;
int rowsize;
declare all in one line?
spaces
Patch Set #16, Line 3185: 0x0001f006
please replace with JEDEC macros
(MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD) << 10)
| 1 | (ctrl->tRCD << 16)
maybe precalculate this?
Patch Set #16, Line 3190: (slotrank << 24)
This could also be precalculated (to make things fit in one line)
capital E
empty line
To view, visit change 22215. To unsubscribe, or for help writing mail filters, visit settings.