Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35403 )
Change subject: soc/intel/common/basecode: Implement CSE update flow ......................................................................
Patch Set 82:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... File Documentation/soc/intel/cse_fw_update/cse_fw_update.md:
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 11: chrome ChromiumOS
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 13: -
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 13: to to the
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 15: -
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 26: API based mechanism used An API-based mechanism is used to decide from which partition the CSE will boot.
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 29: Below HECI APIs support is implemented in CSE suggestion: "The HECI APIs below will be supported in this CSE FW:"
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 31: The command allows to set CSE in special mode which prevents : CSE to do any writes to CSE region. Also, Host gets temporary write : access to CSE RW region (RO being GPR0 protected). suggestion: "This command requests the CSE enter a mode in which writes to the CSE region in the IFWI from the CSE are disabled. It also grants temporary write access to the RW subregion region from the host (RO is still protected by GPR0)."
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 35: The command retrieves the boot partition info from CSE : like version, start/end offsets of a partition within CSE region, and boot : partition status. Also, it provides current boot partition which is : currently being used by CSE to boot, next boot partition which will be used : by CSE to boot on next reset, and number of boot partitions available in : the CSE region. suggestion: "This command retrieves the following information about the CSE's boot partition:
- Version - Beginning and ending offsets - Partition status (more explanation here) - Which partition the CSE most recently booted from - The next partition the CSE is set to boot from - Number of boot partitions available."
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 42: The command allows firmware to set CSE's next boot : partition to boot from. It can be RO or RW partition. suggestion: "This command allows the firmware to request the CSE to boot from either its RO or RW partition at its next reset"
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 45: coreboot uses the command to request CSE to reset data : partition, and to restore the data back to manufacturing defaults. suggestion: "This command requests the CSE to reset its data partition back to manufacturing defaults."
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 48: FW Layout, RW/RO Partitions: *Bold this?*
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 50: CSE The CSE RO...
https://review.coreboot.org/c/coreboot/+/35403/82/Documentation/soc/intel/cs... PS82, Line 58: Boot partition selection: *Bold this?*
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 28: 0x000055aa The old IBM PC MBR magic signature? 😄
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 200: const struct cse_bp_info *bp_info) one tab here
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 260: since CSE current operation mode get change "since the CSE's current operating mode is changed"
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 360: It sets CSE's next boot Set the CSE's next boot
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 388: It calculates CSE RW region's start offset and size Calculate the CSE RE region's start address and size.
https://review.coreboot.org/c/coreboot/+/35403/82/src/soc/intel/common/block... PS82, Line 390: size_t cse_region_start_off) one too many tabs