HAOUAS Elyes 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 co […]
Ack
https://review.coreboot.org/c/coreboot/+/18548/34/src/northbridge/intel/i945... PS34, Line 2437: populated_dimms :
Remove the space before the colon.
Ack
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 on […]
here we are not looking for *at least* but *only one* http://nowni.tistory.com/attachment/4900606d18872AU.pdf - Case Single module per channel -> 150 ohms (can be 75ohms if DDR2-400/533MHz, or 50ohms for 667/800MHz ?) - Case 2 memory modules per channel --> 75 ohms if DDR2-400/533MHz 50 ohms if DDR2-667/800MHz
(populated_dimms & 3) == 1 != (populated_dimms & 3) == 0 != (populated_dimms & 3) == 2) ...
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. […]
Ack