Change in coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT
Sugnan Prabhu S 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 68:
(7 comments)
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
Done
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 p […]
Done
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 ver […]
Done
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?)?
Done
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/basec... PS67, Line 211: stagin
staging
Done
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() tw […]
This will be moved as part of intel_fw_update where coreboot triggers CSE FW Update and UCODE Update.
https://review.coreboot.org/c/coreboot/+/46819/6/src/soc/intel/common/baseco...
This is required to consolidate number of resets required as part of FW Update.
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 […]
Done
-
Sugnan Prabhu S (Code Review)