Attention is currently required from: Eric Lai, Felix Held, Fred Reitberger, Jason Glenesk, Martin L Roth, Matt DeVillier, Paul Menzel, Raul Rangel.
Konrad Adamczyk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76107?usp=email )
Change subject: vendorcode/amd/fsp/common: Refactor dmi_info.h ......................................................................
Patch Set 2:
(5 comments)
Patchset:
PS2:
I think we should list what's actually supported by coreboot, not the theoretical maximums supported […]
Please let me know if I'm missing something here, but I'm not sure if we can do it this-straight-forward. There's a parsing code of this structure in [src/soc/amd/common/fsp/dmi.c](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...) which heavily relies on compatibility between AGESA dmi_table layout and the coreboot one.
Since the dmi.c iterates over the channel/dimms, decreasing values in there without doing same thing in AGESA could lead to missing some memory module entries to be reported by SMBIOS.
File src/vendorcode/amd/fsp/common/dmi_info.h:
https://review.coreboot.org/c/coreboot/+/76107/comment/f6f17287_ecb0c64e : PS2, Line 31: * This code was copied from src/vendorcode/amd/pi/00670F00/AGESA.h
Why remove the comment, and not extend it?
I've removed it since it's no longer valid (as this is not a copy of the AGESA.h). I see your point to extend it, will fix it in v2.
https://review.coreboot.org/c/coreboot/+/76107/comment/0a24894d_30e1c6d5 : PS2, Line 144: } DMI_T17_MEMORY_TYPE;
This is the same like https://source.chromium. […]
That's valid point (in fact, more enums/structs defined in this file might be reused here).
However, as the DMI_T17_MEMORY_TYPE is part of TYPE17_DMI_INFO structure and the [dmi.c](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...) relies heavily on FSP/AGESA structure compatibility, I'd rather keep TYPE17_DMI_INFO untouched (and not rely on external files to define it).
File src/vendorcode/amd/fsp/glinda/soc_dmi_info.h:
https://review.coreboot.org/c/coreboot/+/76107/comment/c6385edb_0ae30eb7 : PS2, Line 6:
Please add `/* TODO: Update for Glinda */`
Done
File src/vendorcode/amd/fsp/phoenix/soc_dmi_info.h:
https://review.coreboot.org/c/coreboot/+/76107/comment/c0cc53bf_e3549366 : PS2, Line 10: #define MAX_SOCKETS_SUPPORTED 4 ///< Max number of sockets in system : #define MAX_CHANNELS_PER_SOCKET 12 ///< Max Channels per sockets : #define MAX_DIMMS_PER_CHANNEL 2 ///< Max DIMMs on a memory channel (independent of platform)
MB Design guide for FP7/FP7R2: #56920 Rev1.06 […]
I'm not sure how AGESA will report it for FP7/FP7R2, but as for FP8 (at least on Myst proto0) AGESA reports it as 4 separate channels.
Those defines are used in parsing code of [dmi.c](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...).
However due to the fact that AGESA uses T17 struct of with 2 MAX_DIMMS_PER_CHANNEL, the scenario for `dmi.c` to report all four installed modules is to use 1/4/2 (at least) configuration.
-
Konrad Adamczyk (Code Review)