Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Chrome EC SOCs: Integrate Chrome EC check
Behave the same as newer platforms. Skylake has the same code.
This also adds brackets around &(gnvs->chromeos), so the it’s clear, what pointer is meant.
Change-Id: I43ff551c717f598c7a65547ec49157853681314f Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/soc/amd/picasso/acpi.c M src/soc/amd/stoneyridge/acpi.c M src/soc/intel/apollolake/acpi.c 3 files changed, 21 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/42258/1
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index fe5879c..7068ac0 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -254,8 +254,13 @@
if (CONFIG(CHROMEOS)) { /* Initialize Verified Boot data */ - chromeos_init_chromeos_acpi(&gnvs->chromeos); - gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; + 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; + } }
/* Set unknown wake source */ diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c index ea67aa3..54b5521 100644 --- a/src/soc/amd/stoneyridge/acpi.c +++ b/src/soc/amd/stoneyridge/acpi.c @@ -229,8 +229,13 @@
if (CONFIG(CHROMEOS)) { /* Initialize Verified Boot data */ - chromeos_init_chromeos_acpi(&gnvs->chromeos); - gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; + 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; + } }
/* Set unknown wake source */ diff --git a/src/soc/intel/apollolake/acpi.c b/src/soc/intel/apollolake/acpi.c index 595e6a2..f9962e9 100644 --- a/src/soc/intel/apollolake/acpi.c +++ b/src/soc/intel/apollolake/acpi.c @@ -83,8 +83,13 @@
if (CONFIG(CHROMEOS)) { /* Initialize Verified Boot data */ - chromeos_init_chromeos_acpi(&gnvs->chromeos); - gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO; + 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; + } }
/* Set unknown wake source */
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42258
to look at the new patch set (#2).
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Chrome EC SOCs: Integrate Chrome EC check
Behave the same as newer platforms. Skylake has the same code.
This also adds brackets around &(gnvs->chromeos), so the it’s clear, what pointer is meant.
Change-Id: I43ff551c717f598c7a65547ec49157853681314f Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/soc/amd/picasso/acpi.c M src/soc/amd/stoneyridge/acpi.c M src/soc/intel/apollolake/acpi.c 3 files changed, 24 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/42258/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c@... PS2, Line 259: 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; : } I think it would be better to move this to src/vendorcode/google/chromeos/gnvs.c to live within the function `chromeos_init_chromeos_acpi()`. It doesn't really need to be repeated for each caller.
Also, the else {} block looks incorrect. I think there should not be any else{} block since there is no CHROMEEC being used and so we don't really set vbt2 to ACTIVE_ECFW_RO or ACTIVE_ECFW_RW. I understand ACTIVE_ECFW_RO is defined as 0, but setting it explicitly to ACTIVE_ECFW_RO seems confusing.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c@... PS2, Line 259: 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; : }
I think it would be better to move this to src/vendorcode/google/chromeos/gnvs. […]
Thank you for the suggestions.
The code is discussed in Ia6601bfd6e7a11eca6bbc32f6757c165ba5a4cff. Could you please comment there?
[1]: https://review.coreboot.org/c/coreboot/+/42201
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/42258/2/src/soc/amd/picasso/acpi.c@... PS2, Line 259: 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; : }
Thank you for the suggestions. […]
I will have a suggestion on this as I work thru common GNVS setup framework. The entire APM_CNT_GNVS_UPDATE stuff can be thrown out.. will be like +50 -500 commit.
Attention is currently required from: Paul Menzel. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2: abandon?
Paul Menzel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42258 )
Change subject: Chrome EC SOCs: Integrate Chrome EC check ......................................................................
Abandoned
Superseded by Kyösti’s GNVS work. Thank you!
-
Furquan Shaikh (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)