Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18548 )
Change subject: nb/intel/i945: Program CxODT value for each channel ......................................................................
Patch Set 34:
(4 comments)
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... PS34, Line 2435: u8 populated_dimms = 0; As this is only used in two cases, in my opinion the explicit use of `sysinfo-dimm[i]` in each if condition is still justified.
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... PS34, Line 2437: populated_dimms : Remove the space before the colon.
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... PS34, Line 2450: one Although mathematicians know, that implicitly *at least* is assumed, make it clear, that at least one DIMM is there.
C00DT: Channel 0 has at least one DIMM.
or change the condition to
(populated_dimms & 3) == 1 != (populated_dimms & 3) == 2)
Maybe elaborate:
C0ODT: Channel 0 has %i DIMM(s).\n
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... PS34, Line 2457: populated_dimms = populated_dimms >> 2; I wouldn’t rewrite the variable here, and change it’s meaning. Just change the mask in the if condition.
-
Paul Menzel (Code Review)