Change in coreboot[master]: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor
Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor
This unifies the file with `src/soc/intel/cannonlake/acpi.c`.
Change-Id: Ia6601bfd6e7a11eca6bbc32f6757c165ba5a4cff Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/soc/intel/skylake/acpi.c 1 file changed, 15 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/42201/1
diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index bdfacd3..5d48184 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -1,3 +1,4 @@ + /* SPDX-License-Identifier: GPL-2.0-only */
#include <acpi/acpi.h> @@ -170,15 +171,20 @@ gnvs->cbmc = (u32)cbmem_find(CBMEM_ID_CONSOLE); #endif
-#if CONFIG(CHROMEOS) - /* Initialize Verified Boot data */ - chromeos_init_chromeos_acpi(&(gnvs->chromeos)); -#if CONFIG(EC_GOOGLE_CHROMEEC) - gnvs->chromeos.vbt2 = google_ec_running_ro() ? - ACTIVE_ECFW_RO : ACTIVE_ECFW_RW; -#endif - gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; -#endif + /* Update the mem console pointer. */ + if (CONFIG(CONSOLE_CBMEM)) + gnvs->cbmc = (uintptr_t)cbmem_find(CBMEM_ID_CONSOLE); + + if (CONFIG(CHROMEOS)) { + /* Initialize Verified Boot data */ + chromeos_init_chromeos_acpi(&(gnvs->chromeos)); + if (CONFIG(EC_GOOGLE_CHROMEEC)) { + gnvs->chromeos.vbt2 = google_ec_running_ro() ? + ACTIVE_ECFW_RO : ACTIVE_ECFW_RW; + } else { + gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; + } + }
/* Enable DPTF based on mainboard configuration */ gnvs->dpte = config->dptf_enable;
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... PS1, Line 1: new line ?
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... PS1, Line 184: else { maybe not needed
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42201
to look at the new patch set (#3).
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor
This also reduces differences with `src/soc/intel/cannonlake/acpi.c`.
Change-Id: Ia6601bfd6e7a11eca6bbc32f6757c165ba5a4cff Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/soc/intel/skylake/acpi.c 1 file changed, 13 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/42201/3
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... PS3, Line 1: please, why this new line?
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... PS3, Line 179: else { please why this new 'else' ? it is not needed
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... PS3, Line 179: else {
please why this new 'else' ? […]
The code as-is looks a bit non-sensical (first setting vbt2 to ACTIVE_ECFW_RO/RW depending on running_ro(), then always setting it to _RO), so if anything this is a bug fix. Cannon-, Ice-, Jasper-, Tiger-Lake also have the semantics of the new version, while Broadwell also has the old one here (and probably should be fixed as well).
The code seems to be originally form Broadwell, and introduced by Duncan so looping in him in case I (and all the editors of the newer intel chipset code) missed something.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... PS1, Line 1:
new line ?
Good catch. Will fix.
https://review.coreboot.org/c/coreboot/+/42201/1/src/soc/intel/skylake/acpi.... PS1, Line 184: else {
maybe not needed
Good question, this indeed changes the behavior from the original code. I’ll add a comment.
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... PS3, Line 1:
please, why this new line?
Will fix.
Hello build bot (Jenkins), Duncan Laurie, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42201
to look at the new patch set (#4).
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor
This also reduces differences with `src/soc/intel/cannonlake/acpi.c`.
Change-Id: Ia6601bfd6e7a11eca6bbc32f6757c165ba5a4cff Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/soc/intel/skylake/acpi.c 1 file changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/42201/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/42201/3/src/soc/intel/skylake/acpi.... PS3, Line 179: else {
The code as-is looks a bit non-sensical (first setting vbt2 to ACTIVE_ECFW_RO/RW depending on runnin […]
Would it make sense to factor this out, or is the different GNVS a problem?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 5:
CB:42389 is duplicate but maybe more accurate commit message.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 5: Code-Review-1
I would +2 this if there wasn't CB:42199 :P see my comment there
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Patch Set 5: -Code-Review
Patch Set 5: Code-Review-1
I would +2 this if there wasn't CB:42199 :P see my comment there
This change has been superseded by CB:42389 and should be abandoned.
Paul Menzel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42201 )
Change subject: soc/intel/skylake: Check Kconfig symbols in C instead of preprocessor ......................................................................
Abandoned
Superseded by CB:42389
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Kyösti Mälkki (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)