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 26:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 90: cse_bp_info.bp_entries[BP2].bp_status);
Shouldn't we check `total_number_of_bp` and only print valid BPs? A loop […]
Done
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 93: uint8_t
When using 1 for success and 0 for failure, bool and true/false would […]
Done
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 112: size_t resp_size = sizeof(struct get_boot_partition_info_resp);
Nit, `sizeof(info_resp)` would be easier to read, imho.
Done
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 125: sizeof(struct cse_boot_partition_info));
Nit, `sizeof(*cse_bp_info)` would make it clearer that it's the correct size.
Done
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 269:
Shouldn't we check `total_number_of_bp` too?
Done
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 155: * Otherwise, it contains only 2 partitions(BP1+BP3)
Is `BP3` a typo here? How would it work w/o BP2?
Typically, when redundancy is enabled BP1 will be duplicated, that is represented as BP2. So, CSE's Image layout is represented as BP1+BP2+BP3.
When no redundancy is enabled, BP1+BP3 will represent CSE Image layout.
-
Sridhar Siricilla (Code Review)