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

[Angel Pons (Code Review)]

Attention is currently required from: Riku Viitanen.

Angel Pons 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 5:

(5 comments)

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

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

https://review.coreboot.org/c/coreboot/+/85793/comment/011e1714_b092ceb2?usp... :
PS5, Line 197: VOLTAGE_MIN 
What about DDR3L (1.35 V)?

https://review.coreboot.org/c/coreboot/+/85793/comment/46aa5189_bbb9c67a?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. `VOLTAGE_MIN_MV`

https://review.coreboot.org/c/coreboot/+/85793/comment/07eb6fbb_a932f5c0?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?

File src/northbridge/intel/sandybridge/sandybridge.h:

https://review.coreboot.org/c/coreboot/+/85793/comment/a065c212_0e834d17?usp... :
PS5, Line 66: optional 
Not exactly optional, it has to be implemented if `MAINBOARD_HAS_ADJUSTABLE_DRAM_VOLTAGE` is enabled, and is never called otherwise.