Arthur Heymans 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 22:
(3 comments)
Please justify this by checking what the vendor firmware does.
https://review.coreboot.org/#/c/18548/22/src/northbridge/intel/i945/raminit.... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/#/c/18548/22/src/northbridge/intel/i945/raminit.... 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.
https://review.coreboot.org/#/c/18548/22/src/northbridge/intel/i945/raminit.... PS22, Line 2463: (dimm_mask & 3) != 3 you have to look up what this means, while the previous statement was very clear.
https://review.coreboot.org/#/c/18548/22/src/northbridge/intel/i945/raminit.... PS22, Line 2473: MCHBAR32(C1ODT) There is no documentation on this one and only very limited documentation on C0ODT. Did you observe with vendor firmware if this can differ from the setting in C0ODT?