Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32293 )
Change subject: smbios: Add memory type 9 system slot support ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c@831 PS1, Line 831: const char *name, u8 type, u8 slot_char1,
please constify all arguments and use enums where possible. That would be: […]
Done
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c@849 PS1, Line 849: t->device_function_number = dev_func;
I don't see segment group number or peer useful at the moment.
Yes segment number can be 0xff for non-pci devices, and 0 for single segment pci spaces. As of now 0 maybe enough, but probably we shall at least let people have a chance to set.
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c@847 PS4, Line 847: t->slot_designation = smbios_add_string(t->eos, name);
Use a default string if name is NULL? Like SLOT-%d?
Done. Just "SLOT"? SLOT-%d will need slot_id? That's still open right?
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c@857 PS4, Line 857: t->data_bus_width = SlotDataBusWidthOther;
I need data_bus_width in https://review.coreboot.org/c/coreboot/+/32307/1. […]
Done
https://review.coreboot.org/#/c/32293/4/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/32293/4/src/include/smbios.h@624 PS4, Line 624: struct slot_peer_groups peer[1];
peer[0] if peer_group_count is always zero
Done