[coreboot-gerrit] Change in coreboot[master]: cr50: verify if the new image needs to be turned on and act ...

[Vadim Bendebury (Code Review)]

Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/18946 )

Change subject: cr50: verify if the new image needs to be turned on and act on it
......................................................................


Patch Set 1:

(4 comments)

https://review.coreboot.org/#/c/18946/1/src/vendorcode/google/chromeos/cr50_turn_on.c
File src/vendorcode/google/chromeos/cr50_turn_on.c:

PS1, Line 28: if (ret != VB2_SUCCESS) {
            : 		printk(BIOS_ERR, "%s: tlcl_lib_init() failed: %x\n",
            : 		       __func__, ret);
            : 		return;
            : 	}
> Do we want to trigger reboot or recovery mode when there is failure?
I don't think so - this check is more for debugging case than for real execution environment.

In any case I would rather run the older TPM version and expect the reboot to happen on next restart than to stun user with an error or rolling reboot.


PS1, Line 38: printk(BIOS_ERR, "%s Attempt to turn update on failed: %x\n",
            : 		       __func__, ret);
> I am guessing we do not want any reboot or recovery mode trigger here since
right.


PS1, Line 44: while(1)
            : 				;
> Can we have a timed loop here? That way, if the reset fails for any reason,
this is a good point, but this code is mostly a placeholder at this time. Aaron had some activities in mind, like adding an entry to elog, quiescing the hardware, etc.

we could put a timeout to all this and maybe even send a straight reset request to the H1 after the timeout expires, this all is open for debate.


PS1, Line 49: BS_PAYLOAD_LOAD
> Why are we doing this so late? Since it might require a reboot, wouldn't it
well, we definitely want this to happen in ramstage, and we want to do this only if it is not an s3 resume.

Spending extra couple of hundred ms on a reboot when this restart is required not a big deal IMO for being able to do it cleanly, on a callback in a separate file, etc.


-- 
To view, visit https://review.coreboot.org/18946
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I45fd6058c03f32ff8edccd56ca2aa5359d9b21b1
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes