Change in coreboot[master]: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41667 )

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

AGESA f14/f15tn/f16kb: Clean up buildOpts.c files

Until now, the buildOpts.c files were primarily made out of copy-pasted
AGESA options, commented-out definitions and several useless comments;
that is, the materialization of technical debt in GCC-parsable form...

Until now.

It is assumed that the boards in the tree still boot. So, by comparing
their settings, we can extract saner defaults to place into AGESA. Many
of the settings were common across all boards of the same family, so we
promote those values to default settings. In some cases flipping a flag
was required, so the macros to alter that option had to be adapted as
well. Since those AGESA versions are expected to never receive updates,
it should not be a problem to change their files to suit our needs.

As a result, all but two buildOpts.c files now have less than 100 lines.
AGESA f14 boards need less than 50 lines, and f15tn/f16kb just require
about 60 or 70 lines in those files. Hopefully, this will make porting
more mainboards using AGESA f14/f15tn/f16kb a substantially easier task.

TEST=Use abuild --timeless to check that all AGESA f14/f15tn/f16kb
mainboards result in identical coreboot binaries.

Change-Id: Ife1ca5177d85441b9a7b24d64d7fcbabde6e0409
Signed-off-by: Angel Pons 
---
M src/mainboard/amd/inagua/buildOpts.c
M src/mainboard/amd/olivehill/buildOpts.c
M src/mainboard/amd/parmer/buildOpts.c
M src/mainboard/amd/persimmon/buildOpts.c
M src/mainboard/amd/south_station/buildOpts.c
M src/mainboard/amd/thatcher/buildOpts.c
M src/mainboard/amd/union_station/buildOpts.c
M src/mainboard/asrock/e350m1/buildOpts.c
M src/mainboard/asrock/imb-a180/buildOpts.c
M src/mainboard/asus/am1i-a/buildOpts.c
M src/mainboard/asus/f2a85-m/buildOpts.c
M src/mainboard/bap/ode_e20XX/buildOpts.c
M src/mainboard/biostar/a68n_5200/buildOpts.c
M src/mainboard/biostar/am1ml/buildOpts.c
M src/mainboard/elmex/pcm205400/buildOpts.c
M src/mainboard/gizmosphere/gizmo/buildOpts.c
M src/mainboard/gizmosphere/gizmo2/buildOpts.c
M src/mainboard/hp/abm/buildOpts.c
M src/mainboard/hp/pavilion_m6_1035dx/buildOpts.c
M src/mainboard/jetway/nf81-t56n-lf/buildOpts.c
M src/mainboard/lenovo/g505s/buildOpts.c
M src/mainboard/lippert/frontrunner-af/buildOpts.c
M src/mainboard/lippert/toucan-af/buildOpts.c
M src/mainboard/msi/ms7721/buildOpts.c
M src/mainboard/pcengines/apu1/buildOpts.c
M src/vendorcode/amd/agesa/f14/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Proc/CPU/cpuLateInit.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionGnbInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f16kb/Proc/CPU/cpuLateInit.h
33 files changed, 786 insertions(+), 5,000 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/41667/1

[Angel Pons (Code Review)]

Hello Mike Banon, build bot (Jenkins), Michał Żygowski, Mike Banon, Alexander Couzens, Patrick Rudolph, Piotr Król,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41667

to look at the new patch set (#3).

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

AGESA f14/f15tn/f16kb: Clean up buildOpts.c files

Until now, the buildOpts.c files were primarily made out of copy-pasted
AGESA options, commented-out definitions and several useless comments;
that is, the materialization of technical debt in GCC-parsable form...

Until now.

It is assumed that the boards in the tree still boot. So, by comparing
their settings, we can extract saner defaults to place into AGESA. Many
of the settings were common across all boards of the same family, so we
promote those values to default settings. In some cases flipping a flag
was required, so the macros to alter that option had to be adapted as
well. Since those AGESA versions are expected to never receive updates,
it should not be a problem to change their files to suit our needs.

As a result, all but two buildOpts.c files now have less than 100 lines.
AGESA f14 boards need less than 50 lines, and f15tn/f16kb just require
about 60 or 70 lines in those files. Hopefully, this will make porting
more mainboards using AGESA f14/f15tn/f16kb a substantially easier task.

TEST=Use abuild --timeless to check that all AGESA f14/f15tn/f16kb
mainboards result in identical coreboot binaries.

Change-Id: Ife1ca5177d85441b9a7b24d64d7fcbabde6e0409
Signed-off-by: Angel Pons 
---
M src/mainboard/amd/inagua/buildOpts.c
M src/mainboard/amd/olivehill/buildOpts.c
M src/mainboard/amd/parmer/buildOpts.c
M src/mainboard/amd/persimmon/buildOpts.c
M src/mainboard/amd/south_station/buildOpts.c
M src/mainboard/amd/thatcher/buildOpts.c
M src/mainboard/amd/union_station/buildOpts.c
M src/mainboard/asrock/e350m1/buildOpts.c
M src/mainboard/asrock/imb-a180/buildOpts.c
M src/mainboard/asus/am1i-a/buildOpts.c
M src/mainboard/asus/f2a85-m/buildOpts.c
M src/mainboard/bap/ode_e20XX/buildOpts.c
M src/mainboard/biostar/a68n_5200/buildOpts.c
M src/mainboard/biostar/am1ml/buildOpts.c
M src/mainboard/elmex/pcm205400/buildOpts.c
M src/mainboard/gizmosphere/gizmo/buildOpts.c
M src/mainboard/gizmosphere/gizmo2/buildOpts.c
M src/mainboard/hp/abm/buildOpts.c
M src/mainboard/hp/pavilion_m6_1035dx/buildOpts.c
M src/mainboard/jetway/nf81-t56n-lf/buildOpts.c
M src/mainboard/lenovo/g505s/buildOpts.c
M src/mainboard/lippert/frontrunner-af/buildOpts.c
M src/mainboard/lippert/toucan-af/buildOpts.c
M src/mainboard/msi/ms7721/buildOpts.c
M src/mainboard/pcengines/apu1/buildOpts.c
M src/vendorcode/amd/agesa/f14/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Proc/CPU/cpuLateInit.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionGnbInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f16kb/Proc/CPU/cpuLateInit.h
33 files changed, 786 insertions(+), 5,000 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/41667/3

[Mike Banon (Code Review)]

Mike Banon 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 5:

This one is a bit big, need some time to go through it...

[Angel Pons (Code Review)]

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 5:
> Patch Set 5:
>
> This one is a bit big, need some time to go through it...

I know it's large, but it should be reproducible for all boards. That means, resulting binaries built with BUILD_TIMELESS=1 do not change after applying this change.

[Angel Pons (Code Review)]

Angel Pons has removed Nico Huber from this change.  ( https://review.coreboot.org/c/coreboot/+/41667 )

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

Removed reviewer Nico Huber.

[Mike Banon (Code Review)]

Mike Banon 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: Code-Review+1

(2 comments)

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

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 ?

[Mike Banon (Code Review)]

Mike Banon 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:

(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

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?

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

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 ?

[Angel Pons (Code Review)]

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.

[Mike Banon (Code Review)]

Mike Banon 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: Code-Review+2

(5 comments)

Thank you for reply. After these "agesa cobwebs" all get merged including this one, I'll try to bring these changes to A88XM-E.

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
> AGESA f14 only has one socket, so this would be the equivalent of disabling it by default. […]

Done

https://review.coreboot.org/c/coreboot/+/41667/6/src/vendorcode/amd/agesa/f1... 
PS6, Line 239: OPTION_DMI
> IIRC, it's about DMI tables. […]

Done

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
> No idea, but all boards seem to use the latter, so I made it the default value.

Done

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
> IIRC all f16kb boards were choosing this value. […]

Done

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
> f16kb boards seem to use "P0" instead of "C0" on older families. […]

Done

[Angel Pons (Code Review)]

Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/41667 )

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

Removed Verified+1 by build bot (Jenkins)

[Angel Pons (Code Review)]

Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/41667 )

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

Removed Verified+1 by build bot (Jenkins)

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41667 )

Change subject: AGESA f14/f15tn/f16kb: Clean up buildOpts.c files
......................................................................

AGESA f14/f15tn/f16kb: Clean up buildOpts.c files

Until now, the buildOpts.c files were primarily made out of copy-pasted
AGESA options, commented-out definitions and several useless comments;
that is, the materialization of technical debt in GCC-parsable form...

Until now.

It is assumed that the boards in the tree still boot. So, by comparing
their settings, we can extract saner defaults to place into AGESA. Many
of the settings were common across all boards of the same family, so we
promote those values to default settings. In some cases flipping a flag
was required, so the macros to alter that option had to be adapted as
well. Since those AGESA versions are expected to never receive updates,
it should not be a problem to change their files to suit our needs.

As a result, all but two buildOpts.c files now have less than 100 lines.
AGESA f14 boards need less than 50 lines, and f15tn/f16kb just require
about 60 or 70 lines in those files. Hopefully, this will make porting
more mainboards using AGESA f14/f15tn/f16kb a substantially easier task.

TEST=Use abuild --timeless to check that all AGESA f14/f15tn/f16kb
mainboards result in identical coreboot binaries.

Change-Id: Ife1ca5177d85441b9a7b24d64d7fcbabde6e0409
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41667
Tested-by: build bot (Jenkins) 
Reviewed-by: Mike Banon 
---
M src/mainboard/amd/inagua/buildOpts.c
M src/mainboard/amd/olivehill/buildOpts.c
M src/mainboard/amd/parmer/buildOpts.c
M src/mainboard/amd/persimmon/buildOpts.c
M src/mainboard/amd/south_station/buildOpts.c
M src/mainboard/amd/thatcher/buildOpts.c
M src/mainboard/amd/union_station/buildOpts.c
M src/mainboard/asrock/e350m1/buildOpts.c
M src/mainboard/asrock/imb-a180/buildOpts.c
M src/mainboard/asus/am1i-a/buildOpts.c
M src/mainboard/asus/f2a85-m/buildOpts.c
M src/mainboard/bap/ode_e20XX/buildOpts.c
M src/mainboard/biostar/a68n_5200/buildOpts.c
M src/mainboard/biostar/am1ml/buildOpts.c
M src/mainboard/elmex/pcm205400/buildOpts.c
M src/mainboard/gizmosphere/gizmo/buildOpts.c
M src/mainboard/gizmosphere/gizmo2/buildOpts.c
M src/mainboard/hp/abm/buildOpts.c
M src/mainboard/hp/pavilion_m6_1035dx/buildOpts.c
M src/mainboard/jetway/nf81-t56n-lf/buildOpts.c
M src/mainboard/lenovo/g505s/buildOpts.c
M src/mainboard/lippert/frontrunner-af/buildOpts.c
M src/mainboard/lippert/toucan-af/buildOpts.c
M src/mainboard/msi/ms7721/buildOpts.c
M src/mainboard/pcengines/apu1/buildOpts.c
M src/vendorcode/amd/agesa/f14/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f15tn/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f15tn/Proc/CPU/cpuLateInit.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionFchInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/OptionGnbInstall.h
M src/vendorcode/amd/agesa/f16kb/Config/PlatformInstall.h
M src/vendorcode/amd/agesa/f16kb/Proc/CPU/cpuLateInit.h
33 files changed, 786 insertions(+), 5,000 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Mike Banon: Looks good to me, approved

[9elements QA (Code Review)]

9elements QA 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 9:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4467
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4466
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4465
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4464

Please note: This test is under development and might not be accurate at all!