Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18548 )
Change subject: nb/intel/i945: Programm CxODT value for each channel ......................................................................
Patch Set 26:
(8 comments)
https://review.coreboot.org/c/coreboot/+/18548/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/18548/17//COMMIT_MSG@7 PS17, Line 7: proper
I mean "own"
Done
https://review.coreboot.org/c/coreboot/+/18548/26//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/18548/26//COMMIT_MSG@10 PS26, Line 10:
no, […]
Done
https://review.coreboot.org/c/coreboot/+/18548/14/src/northbridge/intel/i945... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/18548/14/src/northbridge/intel/i945... PS14, Line 2458: have
Channel 0 […]
Done
https://review.coreboot.org/c/coreboot/+/18548/14/src/northbridge/intel/i945... PS14, Line 2466: Channel
Channel 0 […]
Done
https://review.coreboot.org/c/coreboot/+/18548/17/src/northbridge/intel/i945... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/18548/17/src/northbridge/intel/i945... PS17, Line 2465: ve
has
Done
https://review.coreboot.org/c/coreboot/+/18548/22/src/northbridge/intel/i945... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/18548/22/src/northbridge/intel/i945... PS22, Line 2452: : for (i = 0; i < (2 * DIMM_SOCKETS); i++) { : if (sysinfo->dimm[i] != SYSINFO_DIMM_NOT_POPULATED) : dimm_mask |= (1 << i); : }
I'm not fond of such arithmetics.
Done
https://review.coreboot.org/c/coreboot/+/18548/22/src/northbridge/intel/i945... PS22, Line 2463: (dimm_mask & 3) != 3
It can be written even clearer by pulling the outer ! in: […]
Elyes, please consider Nico's proposal, it really is much clearer.
https://review.coreboot.org/c/coreboot/+/18548/22/src/northbridge/intel/i945... PS22, Line 2473: MCHBAR32(C1ODT)
Right, ODT is also enabled with a single DIMM. […]
So what's the actionable outcome here: is the code okay to go in as is, or do you think there needs to be more work? (I generally agree with Nico's assessment and would put it in, but giving you guys a chance to chime in with how that breaks things)