Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/16/src/lib/spd_bin.c@202 PS16, Line 202: spd_get_name(spd, spd_name, ARRAY_SIZE(spd_name), type); : : printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name); Sorry, I am a little late to the party here. I am thinking if we really need to do memcpy of the part name into a local buffer here? Instead can we organize the code such that `spd_get_name()` returns a pointer to a character buffer which is the start of the part name and length indicating how long the name is.
Something like: spd_get_name(spd, type, &spd_name, &spd_name_len); printk(BIOS_INFO, "SPD: module part number is "%.*s\n", spd_name_len, spd_name);
And the spd_get_name() function can return a pointer within the SPD buffer or whatever the mainboard provides w.r.t. part name:
static void spd_get_name(const uint8_t spd[], int type, const char **spd_name, size_t *spd_name_len) { if (mainboard_get_dram_part_num(&spd_name, &spd_name_len)) return;
switch (type) { } }
Also, I think we should move the declaration of `mainboard_get_dram_part_number()` to src/include/spd_bin.h and provide a weak declaration here in spd_bin.c. This function is going to be required for all Intel and AMD platforms going forward at least for Chrome OS devices. I think rather than duplicating the weak function in multiple places and adding it to every SoC header file, we can just have it in one place here in spd_bin.c/h.
Additionally, we can provide the strong implementation of `mainboard_get_dram_part_num()` in src/vendorcode/google/chromeos/ so that it can be used by mainboards without having to duplicate that code by selecting a Kconfig.
I know this is much more than what you started with, but I think there is some clean up that we can benefit from. Also, the part about `mainboard_get_dram_part_number()` should really be done as separate CLs.