Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 )
Change subject: soc/intel/common/block/cse: Add boot partition related APIs ......................................................................
Patch Set 78:
(10 comments)
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 4: #include <boot_device.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 7: #include <fmap.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 10: #include <commonlib/region.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 13: /* Converts bp index to boot partition number */
Comment is no longer correct.
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 129: COW
CWS?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 129: RW partition
Why is this tied to partition type? Is it just the Operating Mode? i.e. […]
When CSE is in RO, then COM is Soft Temp Disable by default. This will simplify CSE boot flows. No change to COM of CSE when it is in RW.
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 130: COW
CWS?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 131: COW
CWS?
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 310: goto failed;
Don't trigger recovery mode when system is already in recovery.
Done
https://review.coreboot.org/c/coreboot/+/35402/77/src/soc/intel/common/block... PS77, Line 322: cse_set_next_boot_partition(RO);
CSE FW update will never be triggered as part of recovery. There are two types of recovery: […]
In recovery, coreboot skips CSE FW update and switching to RW. There is no change in this flow.
I'm talking about post recovery case. After user recovers the system, then CSE FW update may require as latest CSE FW will be part of CB FW RW A/B. In this scenario, if CSE is already in RO, coreboot does not trigger a GLOBAL RESET rst type to place CSE in RO. Just coreboot go ahead and update CSE RW partition.
Also, it doesn't make any sense sending SET_BP(RO) if system enters recovery mode through manual recovery. Conclusion, no need to send set_bp* command in recovery mode.
-
Sridhar Siricilla (Code Review)