[coreboot-gerrit] [S] Change in coreboot[main]: nb/intel/sandybridge: Add CFR objects for existing options

[Angel Pons (Code Review)]

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 1:

(5 comments)

File src/northbridge/intel/sandybridge/cfr.h:

https://review.coreboot.org/c/coreboot/+/87388/comment/7e8b5555_224a1cd2?usp... :
PS1, Line 7: #include <drivers/option/cfr_frontend.h>
nit: shouldn't the include be within the cpp guard? (ifndef/define/endif)

https://review.coreboot.org/c/coreboot/+/87388/comment/96ef7952_bf7c49d0?usp... :
PS1, Line 12: /* Values must match nb/sandybridge/Kconfig */
I don't remember seeing IGD UMA size in Kconfig...

https://review.coreboot.org/c/coreboot/+/87388/comment/a269acc3_3aa897bc?usp... :
PS1, Line 17: 1282
What's up with this number?

https://review.coreboot.org/c/coreboot/+/87388/comment/f90b9703_bff3a5ef?usp... :
PS1, Line 20: /* Power state after power loss */
Ahem, that's copy-pasta

https://review.coreboot.org/c/coreboot/+/87388/comment/b0e424a6_5f7a6878?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		},
There's a lot more possible values than that, I think it can go up to 1 GiB as per some CMOS layouts (I remember cross-checking that against the datasheet)