David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 3: Code-Review-1
(4 comments)
Mostly looks good - The only major thing to change is to I think we need to set the array type to MEMORY_ARRAY_ECC_SINGLE_BIT
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 34: //in GB Coding style in coreboot generally follows Linux, so we use /* */ for comments.
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 78: //The ECC setting can`t be confirmed in FSP, so hardcode it. comment style
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 79: MEMORY_ARRAY_ECC_NONE If we're hardcoding it, should it be hardcoded to MEMORY_ARRAY_ECC_SINGLE_BIT?
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 83: (u32)(CONFIG_DIMM_MAX * MAX_DIMM_SIZE) << 20; Suggestion: Since MAX_DIMM_SIZE is only used here, it might be more obvious to define it as: #define MAX_DIMM_SIZE_GB (32UL << 20)
That way you can avoid the casting here, and this becomes: maximum_capacity = CONFIG_DIMM_MAX * MAX_DIMM_SIZE
which is more obvious