Change in ...coreboot[master]: mainboard/sarien: Save the Cr50 recovery switch state in VBNV

[Keith Short (Code Review)]

Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31801

Change subject: mainboard/sarien: Save the Cr50 recovery switch state in VBNV
......................................................................

mainboard/sarien: Save the Cr50 recovery switch state in VBNV

Update sarien so the Cr50 recovery switch is only read during verstage
and save the recovery switch state to VBNV.

BUG=b:123360379
BRANCH=none
TEST=build coreboot on sarien and arcada. Test normal boot and recovery
boot on arcada - confirm that that tpm transaction errors are gone.

Change-Id: Iadf0cec651b3a26ceebadfeb637e189805c328bf
Signed-off-by: Keith Short 
---
M src/mainboard/google/sarien/chromeos.c
1 file changed, 16 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/31801/1

diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c
index 1e363fd..308b682 100644
--- a/src/mainboard/google/sarien/chromeos.c
+++ b/src/mainboard/google/sarien/chromeos.c
@@ -20,18 +20,12 @@
 #include <variant/gpio.h>
 #include <vendorcode/google/chromeos/chromeos.h>
 #include <security/tpm/tss.h>
+#include <security/vboot/vbnv.h>
 #include <device/device.h>
 #include <intelblocks/pmclib.h>
 #include <soc/pmc.h>
 #include <soc/pci_devs.h>

-enum rec_mode_state {
-	REC_MODE_UNINITIALIZED,
-	REC_MODE_NOT_REQUESTED,
-	REC_MODE_REQUESTED,
-};
-static enum rec_mode_state saved_rec_mode;
-
 void fill_lb_gpios(struct lb_gpios *gpios)
 {
    struct lb_gpio chromeos_gpios[] = {
@@ -84,30 +78,33 @@

int get_recovery_mode_switch(void)
 {
-	enum rec_mode_state state = saved_rec_mode;
+	int rec_switch;
    uint8_t recovery_button_state = 0;

-	/* Check the global variable first. */
-	if (state == REC_MODE_NOT_REQUESTED)
-		return 0;
-	else if (state == REC_MODE_REQUESTED)
-		return 1;
+	/*
+	 * Only verstage performs a real check of the Cr50 recovery switch.
+	 * The recovery switch state is cleared on the first access by the AP
+	 * so there's no point in querying the Cr50 at later stages.  All other
+	 * stages use the state saved in VBNV.
+	 */
+	if (!ENV_VERSTAGE &&
+		!get_recovery_switch_from_vbnv(&rec_switch))
+		return rec_switch;

-	state = REC_MODE_NOT_REQUESTED;
+	rec_switch = 0;

/* Read state from the GPIO controlled by servo. */
    if (cros_get_gpio_value(CROS_GPIO_REC))
-		state = REC_MODE_REQUESTED;
+		rec_switch = 1;
    /* Read one-time recovery request from cr50. */
    else if (tlcl_cr50_get_recovery_button(&recovery_button_state)
    	 == TPM_SUCCESS)
-		state = recovery_button_state ?
-			REC_MODE_REQUESTED : REC_MODE_NOT_REQUESTED;
+		rec_switch = !!recovery_button_state;

/* Store the state in case this is called again in verstage. */
-	saved_rec_mode = state;
+	set_recovery_switch_into_vbnv(rec_switch);

-	return state == REC_MODE_REQUESTED;
+	return rec_switch;
 }

int get_lid_switch(void)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31801 )

Change subject: mainboard/sarien: Save the Cr50 recovery switch state in VBNV
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31801/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31801/1//COMMIT_MSG@15 
PS1, Line 15: that tpm
the TPM

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31801 )

Change subject: mainboard/sarien: Save the Cr50 recovery switch state in VBNV
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31801/1/src/mainboard/google/sarien/chromeos... 
File src/mainboard/google/sarien/chromeos.c:

https://review.coreboot.org/#/c/31801/1/src/mainboard/google/sarien/chromeos... 
PS1, Line 90: !ENV_VERSTAGE
I am a bit confused by this check. If this is VERSTAGE and you end up reading the recovery switch second time, then it still queries cr50. Wouldn't that return incorrect value since it is read one-time?

[Keith Short (Code Review)]

Keith Short has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31801 )

Change subject: mainboard/sarien: Save the Cr50 recovery switch state in VBNV
......................................................................

Abandoned

Replaced by change https://review.coreboot.org/c/coreboot/+/32030