Change in coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT
Varshit B Pandya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27369 )
Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT ......................................................................
Patch Set 51:
(29 comments)
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@11 PS49, Line 11: Chromebooks
nit: Chromebooks,
Done
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@12 PS49, Line 12: reset
nit: reset,
Done
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@52 PS49, Line 52: single MCU
Humm that is true. You would need multiple entries for each MCU.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... File Documentation/soc/intel/ucode_update/microcode_update_model.md:
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 8: .
It would be good to mention here that MCU refers to the bundled package containing ucode and pcode.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 42: bot
boot
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 43:
block
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 67: .
and in case of RTC reset.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 74: lke
like
Done
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 94: reset
cold or warm?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Kconfig:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 1: config TOP_SWAP_BASED_VBOOT_UCODE_UPDATE
Can these configs be named with a specific prefix? INTEL_BASECODE_ or just INTEL_? Something like IN […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 6: select SOC_INTEL_COMMON_BLOCK_RTC
Help text would be very helpful for these configs.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 6: select
Don't you also need to auto select "CONFIG_SUPPORT_CPU_UCODE_IN_CBFS"
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 8: INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG
I don't understand what this name really means. […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 12:
Again, help text should be added for this.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 16: symbols
What is this used for?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 17: assert
What is this used for?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 27: spi_flash.h
What is this used for?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 28: string
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 50: int
It would be good to add a comment indicating the return values and what they mean.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 58: printk(BIOS_ERR, "Error locating staging area!\n");
Why is only this particular failure error message logged and not the other failures in this function […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 68: incoherent_rdev_init
Why not just return what incoherent_rdev_init() is returning? i.e. pointer to rdev or NULL.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 75:
extra blank line
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 82: Could
could
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 88: (size_t)
I don't think this is required.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 106: = NULL
This is set in L144. No need to initialize to NULL.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 107: NULL
This is set in L136. No need to initialize to NULL.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 120: printk(BIOS_INFO, "UCODE: Recovery enabled\n");
Is this print really necessary? I believe this already must be getting printed somewhere in the boot […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 121: ts_strap
ts_strap == TS_ENABLE […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 144: intel_microcode_find
This requires CONFIG_SUPPORT_CPU_UCODE_IN_CBFS to be auto-selected.
Done
-
Varshit B Pandya (Code Review)