Morgan Jang 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 7:
(5 comments)
Patch Set 6:
I'd prefer a common driver for type16 entries. In the last few weeks everybody seem to duplicate those table creation code in mainboard directory. In the end you only need the maximum slot count, DIMM size per slot, soldered memory size and ECC capability. Those can easily be set in the devicetree.
The memory related configurations are not defined in device so far, should I wait for the configurations ready and then write a common driver for SMBIOS type16 entries?
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... PS1, Line 175: t->memory_error_correction = MEMORY_ARRAY_ECC_NONE; //The ECC setting can`t be confirmed in FSP, so hardcode it.
line over 96 characters
Done
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.
Done
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
Done
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?
We checked the ECC setting with the tool, and confirmed it`s ECC enabled, so hardcode it 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: […]
Done