Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22214 )
Change subject: nb/intel/sandybridge/raminit: Add ECC detection support ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/22214/9/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/22214/9/src/northbridge/intel/sandy... PS9, Line 407: ctrl.ecc_forced ? "yes" : "no");
I was wondering why we don't have to read SPDs on this path (see next line)... the only way to get here is to run through `if (!fastboot)` above, so we gathered/printed this information already. Code flow is a mess :-/ it seems somebody tried to save indentation levels.
The argument about code structure and necessity of (re-?)reading the SPD is in my opinion outside the scope of this change.
Sure. I just wanted to mention how I came to the conclusion.
Also, this seems informational, BIOS_INFO?
Almost all the other printks in this file are BIOS_DEBUG. I don't think this one is more significant than those.
Yep, a common issue: We have 9 log levels, but act as if there were only 3. IMHO, `ecc_forced = 1` would even deserve BIOS_NOTICE.
-
Nico Huber (Code Review)