Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41667 )
Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files ......................................................................
Patch Set 6:
(5 comments)
Patch Set 6:
(3 comments)
Overall this change looks quite good - and, considering that the built binaries do not change as you said, I just need more understanding about some options before giving it a +2
I honestly don't know what over half of these options mean. I just compared the values and promoted the most commonly used value as default.
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f14/Config/PlatformInstall.h:
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... PS6, Line 210: OPTION_GFX_RECOVERY
What this OPTION_GFX_RECOVERY means, and why it has been removed?
AGESA f14 only has one socket, so this would be the equivalent of disabling it by default. Since all boards disabled it, I flipped the default to disabled. Note that the BLDOPT macro that originally disabled this option is now inverted, so as to allow enabling this option.
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... PS6, Line 239: OPTION_DMI
More info about OPTION_DMI is also needed for review, to understand its' change from TRUE to FALSE
IIRC, it's about DMI tables. All boards disabled DMI, so I disabled it by default and flipped the BLDOPT macros to allow enabling it.
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f16kb/Config/OptionGnbInstall.h:
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... PS6, Line 163: PcieTrainingDistributed
Which algorithm is better - PcieTrainingStandard or PcieTrainingDistributed ?
No idea, but all boards seem to use the latter, so I made it the default value.
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f16kb/Config/PlatformInstall.h:
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... PS6, Line 860: CStateModeDisabled
Is this a better default? In my experience, both CStateModeC6 and CStateModeDisabled are working
IIRC all f16kb boards were choosing this value. To be reproducible, I've taken this value as the default, and it's what the boards would have been tested with.
If CStateModeC6 is working as intended, we can flip it later for all boards from here, in a separate commit. Does it sound reasonable?
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f16kb/Proc/CPU/cpuLateInit.h:
https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... PS6, Line 232: SCOPE_NAME_P
What does it mean - these changes from SCOPE_NAME_C to SCOPE_NAME_P ?
f16kb boards seem to use "P0" instead of "C0" on older families. I factored it out for reproducibility. If I had to take a guess, P0 is a performance state, which older platforms may lack.