[M] Change in coreboot[main]: nb/sandybridge: Implement automatic DRAM voltage setting

[Riku Viitanen (Code Review)]

Attention is currently required from: Angel Pons.

Riku Viitanen has posted comments on this change by Riku Viitanen. ( https://review.coreboot.org/c/coreboot/+/85793?usp=email )

Change subject: nb/sandybridge: Implement automatic DRAM voltage setting
......................................................................

Patch Set 7:

(4 comments)

File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/85793/comment/e1f46220_903ca757?usp... :
PS5, Line 196: #if CONFIG(MAINBOARD_HAS_ADJUSTABLE_DRAM_VOLTAGE)
> Is it necessary to guard this with preprocessor?

Done

https://review.coreboot.org/c/coreboot/+/85793/comment/ac600da2_baeea55e?usp... :
PS5, Line 197: VOLTAGE_MIN
> What about DDR3L (1. […]

Paranoid me thinks it could cause unstability in the IMC. Also all DDR3(L) DIMMs are supposed to work at 1.5V.

Hmm, I was able to find online some datasheets of DDR3L modules that have a 1.35V XMP profile (though they also had the same timings in SPD??).
 As the code currently stands, we would reject an XMP profile in that case and use SPD instead. Maybe I should change the comparison:
dimm->voltage != ctrl->voltage
to
dimm->voltage > ctrl->voltage
in the loop below. Since the first (only voltage checking) round eliminates the possibility of overvolting already.

https://review.coreboot.org/c/coreboot/+/85793/comment/e3eff057_84055801?usp... :
PS5, Line 197: #define VOLTAGE_MIN 1500
             : #define VOLTAGE_MAX 1650
             : #define VOLTAGE_DEFAULT 1500
> I would suggest adding the units as a suffix to the macro names, e.g. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/85793/comment/7535c40e_da1de785?usp... :
PS5, Line 211: spd_xmp_decode_ddr3(&dimm, spd[spd_slot],
             : 				                       DDR3_XMP_PROFILE_1);
             : 				if (dimm.dram_type != SPD_MEMORY_TYPE_SDRAM_DDR3) {
             : 					/* Non-XMP DIMM. Use default voltage since
             : 					 * every DIMM is supposed to work with it */
             : 					printram("Non-XMP DIMM detected. Defaulting to %imV.\n",
             : 					         VOLTAGE_DEFAULT);
             : 					return VOLTAGE_DEFAULT;
             : 
             : 				} else if (dimm.voltage > VOLTAGE_MAX) {
             : 					printram("XMP requested voltage too high!\n");
             : 					if (voltage != 0)
             : 						voltage = MIN(voltage, VOLTAGE_MAX);
             : 					else
             : 						voltage = VOLTAGE_MAX;
             : 
             : 				} else if (dimm.voltage < VOLTAGE_MIN) {
             : 					printram("XMP requested voltage too low!?\n");
             : 					return VOLTAGE_MIN;
             : 
             : 				} else {
             : 					voltage = MAX(voltage, dimm.voltage);
             : 				}
> This decodes the XMP SPD a second time, is it necessary?

At first I tried to integrate voltage selection into the loop in dram_find_spds_ddr3, but that became messy very quickly.

Consider what happens if we have mixed modules, where the first one we read supports faster XMP timings at 1.65V, but some module that's scanned after it doesn't tolerate voltages that high. My code reverts to 1.5V in that case. The XMP DIMM would then be running at speeds to fast for the actual voltage it gets.

Maybe there's a method to only parse the voltage quickly.