Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43351 )
Change subject: soc/amd/picasso: supply SMBIOS type 17 ......................................................................
Patch Set 5:
(19 comments)
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG@7 PS3, Line 7: smbios
SMBIOS
Done
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG@9 PS3, Line 9: dimm
DRAM. I doubt that google/zork has DIMM slots.
Done
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG@11 PS3, Line 11: smbios
SMBIOS
Done
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG@11 PS3, Line 11: from cbmem to build smbios type 17 tables .
Please remove the space before the dot/period.
Done
https://review.coreboot.org/c/coreboot/+/43351/3//COMMIT_MSG@14 PS3, Line 14: TEST=dmidecode -t 17
Is boot time affected by this?
I don't expect a significant impact, but it has not been measured.
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:
PS3:
Name this file `smbios. […]
I considered smbios.c, memory.c and dmi.c. I went with dmi.c because there are other smbios.c files in coreboot with different purposes. I'm open to changing it if there's a consensus.
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1 PS3, Line 1: /* : * This file is part of the coreboot project. : * : * SPDX-License-Identifier: GPL-2.0-or-later : */
/* SPDX-License-Identifier: GPL-2. […]
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 16: #if CONFIG(CHROMEOS)
nit: I don't think we need to guard this.
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@3... PS3, Line 33: smbios_memory_size_to_mib(dmi17->MemorySize, dmi17->ExtSize);
Should fit in 96 characters?
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 139: int
size_t
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 148: printk(BIOS_ERR, "Failed to add memory info to CBMEM.\n");
… DMI tables will be incomplete.
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 158: printk(BIOS_ERR, "AMD_FSP_DMI_HOB not found\n");
… DMI table 17 won’t be populated.
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 163: #if CONFIG(CHROMEOS)
Use; […]
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 173: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 173: int channel = 0
I'm not sure if we have any rule about for-loop variable declaration. […]
I see both. The style guide does not specify.
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 174: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 176: DIMMS
DIMMs
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@1... PS3, Line 192: - 1
Move to the previous line?
Done
https://review.coreboot.org/c/coreboot/+/43351/3/src/soc/amd/picasso/dmi.c@2... PS3, Line 202: // so check it just in time for writing tables.
- Should fit in one line. […]
Done