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 79:
(12 comments)
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/cannonlake/m... PS79, Line 163: #if CONFIG(SOC_INTEL_CSE_CUSTOM_SKU) I think this can be moved to custom_bp.c
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/cannonlake/m... PS79, Line 164: BS_PRE_DEVICE This seems to have changed since last time?
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/cannonlake/m... PS79, Line 164: BS_DEV_INIT This is not correct. It is supposed to be either BS_ON_EXIT or BS_ON_ENTRY.
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 21: SKU Custom nit: Custom SKU
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
PS79: Name this file custom_sku.c?
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 89: enum This will have to fixed width since it is being received from the CSE.
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 127: struct const struct
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 128: struct const struct
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 133: struct const
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 135: struct const
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 273: /* Control never reaches here */ Add a die("") here?
https://review.coreboot.org/c/coreboot/+/35402/79/src/soc/intel/common/block... PS79, Line 307: if (vboot_recovery_mode_enabled()) { : printk(BIOS_DEBUG, "cse_bp: Skip switching to RW in the recovery path\n"); : return; : } I think this can be checked even before custom sku is checked above?
-
Furquan Shaikh (Code Review)