[S] Change in coreboot[main]: nb/intel/sandybridge: Add CFR objects for existing options
Attention is currently required from: Intel coreboot Reviewers, Matt DeVillier.
Angel Pons has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/coreboot/+/87388?usp=email )
Change subject: nb/intel/sandybridge: Add CFR objects for existing options ......................................................................
Patch Set 3: Code-Review+2
(2 comments)
File src/northbridge/intel/sandybridge/cfr.h:
https://review.coreboot.org/c/coreboot/+/87388/comment/3b012624_e0d3aaf6?usp... : PS1, Line 12: /* Values must match nb/sandybridge/Kconfig */
it's at the very bottom: […]
Ah, added in CB:84225
https://review.coreboot.org/c/coreboot/+/87388/comment/8b2ea5b9_6ac50a30?usp... : PS1, Line 26: .values = (const struct sm_enum_value[]) { : { " 32MB", IGD_UMA_SIZE_32MB }, : { " 64MB", IGD_UMA_SIZE_64MB }, : { " 96MB", IGD_UMA_SIZE_96MB }, : { "128MB", IGD_UMA_SIZE_1282MB }, : SM_ENUM_VALUE_END },
these are the values currently offered via Kconfig
I see, for some reason CB:84225 only added those. For some reason, some boards only have up to 224 MiB, whereas others have up to 1 GiB (e.g. https://github.com/coreboot/coreboot/blob/0307f52cd9a907fd15d70311abee3f79eb...).
In any case, the option encoding is the same as the bitfield in the GGC register but offset by one (option values are 1 less than GGC values): https://github.com/coreboot/coreboot/blob/0307f52cd9a907fd15d70311abee3f79eb...
I would prefer to have all possible values available for the sake of completeness, but I also know that there's little sense in statically reserving huge amounts of RAM for the iGPU when DVMT exists (which allows the OS' iGPU driver to dynamically request as much memory as it needs). CMOS layouts should also be made uniform if possible...
TL;DR not a big deal, and there's other things that should be updated at the same time anyway.
-
Angel Pons (Code Review)