Change in coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT
Arthur Heymans 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 67: Code-Review+1
(7 comments)
A few minor remarks but looks good to me.
https://review.coreboot.org/c/coreboot/+/27369/67//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/27369/67//COMMIT_MSG@27 PS67, Line 27: register nit: PCR register
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Kconfig:
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 6: select maybe depends on is better? That way you are a bit more aware in the soc code which dependency you pull in?
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 111: if (rdev_writeat(staging_rdev, cbfs_microcode, 0, : get_microcode_size(cbfs_microcode)) < 0) { : printk(BIOS_ERR, "ucode: Failed to write microcode to staging area\n"); : return UCODE_FWU_STAGING_WRITE_ERROR; : } If the MCU is bigger than the staging area this will fail but it might be a good idea to be more verbose, by for instance adding an additional check after the intel_microcode_find() call?
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 162: tatic int use the return type enum ucode_fwu_failure_reason (Add success to the enum?)?
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 211: stagin staging
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 260: BOOT_STATE_INIT_ENTRY(BS_PRE_DEVICE, BS_ON_ENTRY, ucode_fw_sync, NULL); Can this be done as part of coreboot_init_cpus() and somehow avoid calling intel_microcode_find() twice?
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/include/intelbasecode/ucode_update.h:
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 6: /* : * This is a weak implementation to reboot after ucode update is finished. : * This can be overridden by mainboard/SoC specific implementation. : */ : void ucode_reboot_platform(void); : : /* : * This is a weak implementation for the ucode update module to know if receovery mdoe is : * enabled. This returns non-zero if recovery mode enabled and can be overridden by : * mainboard/SoC specific implementation. : */ : int ucode_update_rec_mode_enabled(void); What kind of override do you plan to implement? Maybe keep it non weak for now and change that when needed?
-
Arthur Heymans (Code Review)