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 41:
(31 comments)
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@11 PS40, Line 11: chrome
Chromium OS?
Done
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@18 PS40, Line 18: the coreboot
Remove the article.
Done
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@19 PS40, Line 19: partition(either BP1(RO) or BP2)
Please add a space after *partition* before (.
Done
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 103: force_cse_init
What is the use of this variable? […]
cse_init_bp_info() call is must in every API to intialize boot partition info. It helps initializing the boot_partition_info when any boot partition info API is called.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 105: cse_bp_init
Can we just not use the "total_number_of_bp" field in cse_boot_partition_info to check if the partit […]
Agreed. Implemented the same
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 166: if (!cse_init_bp_info(true)) { : printk(BIOS_ERR, "Failed to update boot partition info\n"); : return false; : }
just call the function to update the partition info, check comment above.
Please see my first comment
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 192: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 201: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 210: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 221: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 232: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 304: cse_init_bp_info
check comment on cse_init_bp_info()
Please see my first comment.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 23: */
Please add a space before.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 47: uint8_t
Why not `unsigned int`?
The structure definition is align with the response buffer of get_boot_partition_info. Hence, data types of the struct members have been defined accordingly
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 64: bool
Use CB_SUCCESS and friends?
bool type seems to be ok here.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 89: printk(BIOS_ERR, "Get partition info resp failed:%d\n", info_resp.hdr.result);
Please add a space after the colon.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 105:
Remove the blank line.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 108: fail
failed
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 135: Command(BP%d)
Please add a space before (.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 146: printk(BIOS_ERR, "Set Boot Partion Info Response Failed:%d\n",
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 305: No
Number
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 308: =0x%x
Please add a space.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 172: Enabled
lowercase
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 174: * So, CSE Image contains 3 partitions when redundancy is enabled
Period/dot at the end?
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 180:
Remove blank line.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 183: * and it's a valid bootable partition.
Fits on one line?
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 195: * not a bootable partition on it's own.
One line?
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 202:
Remove blank line.
Done
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 262: };
Might this be available in other parts of coreboot already?
I think No!
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 274: partition(BP1
Space.
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)
Thanks. Alas, I still don't get it. Below it says BP3 is loaded by BP2. […]
I agree with your inputs, and in line with comments mentioned for enum definitions of Boot partition ids, I have modified comment for CSE_MAX_BOOT_PARTITIONS macro.
-
Sridhar Siricilla (Code Review)