Furquan Shaikh 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 80: Code-Review+2
(10 comments)
Just some minor nits to update comments. Rest looks okay.
https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG@32 PS80, Line 32: coreboot ensure CSE to boot from RO(BP1) if system is in Recovery mode. This is no longer true.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 21: SKU Custom Still pending.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 23: nit: No space before /*
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 29: It's a valid bootable partition. nit: Does this fit on the previous line?
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 44: present based on layout, but it is not valid. nit: I think part of this comment can go to previous line.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 50: present per initial image layout. nit: This can go on previous line.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 58: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ nit: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 331: do_global_reset Just making a note: This is not the right way to handle the failure case. It is just being done until it is replaced with proper triggering of recovery in follow-up CLs.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 231: triggers recovery mode if CSE fails to jump to RW Just a note: This is not yet implemented in this CL. It will be done in follow-up CLs.
https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/block... PS80, Line 232: RW partition nit: whatever is the currently selected partition.