Jonathan Kollasch 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.
It does appear that this message would be printed twice should the err path be taken, and can therefore be removed.
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.
There is a printk in #22215 (the one that indicates ECC is being enabled) that should probably be advanced from BIOS_DEBUG to BIOS_INFO however.
-
Jonathan Kollasch (Code Review)