Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45873 )
Change subject: lib, mb, soc: change mainboard_get_dram_part_num() prototype ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 44: = 0
Not required. dram_part_num_len is always set before using.
The compiler complains that "it could be uninitialized"
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/alderlake/rom... PS3, Line 92: part_name_overridden
Why is this bool added? You can still check !dram_part_num here.
The bool is added because if dram_part_num is not set above (i.e. name not overridden), then the first pass through this loop, it propertly sets dram_part_num. However, next time through this loop, dram_part_num will be set, so it will not execute this block.
Checking dram_part_num for null here would change initial intent of this code. This code was written to specifically use whatever part number was defined for that particular dimm (each dimm record has string storage for part name). In reality, I doubt we would ever see a mixed-dimm model where different slots may have different parts, but the original code was written to support that possibility, so I was retaining that.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/cannonlake/ro... PS3, Line 28: /* Default weak implementation, no need to override part number. */
return NULL;
Done
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... File src/soc/intel/elkhartlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 45: = 0
Same comment as alderlake.
Please see comment on alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/elkhartlake/r... PS3, Line 46: part_name_overridden
Same comment as alderlake. This flag is not required.
I disagree, please see explanation on alderlake.
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... File src/soc/intel/jasperlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/jasperlake/ro... PS3, Line 45: = 0
Same comment as alderlake.
please see my alderlake response
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45873/3/src/soc/intel/tigerlake/rom... PS3, Line 45: = 0;
Not required.
please see my alderlake response