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 69:
(15 comments)
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@52 PS49, Line 52: single MCU
Thanks Arthur.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 1: ramstage
Yes, that is the plan. […]
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 33: __weak
It is recommended to do a cold reset after the ucode update. […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 39: ucode_update_rec_mode_enabled
My intention was to make this update procedure generic and not tightly bound to to vboot. […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 98: boot_device_wp_region
Yes, do we need to check? we can apply the spi ctrlr protection anyway. […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 99: return -1;
Why not just return whatever boot_device_wp_region() is returning?
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 119: ucode_update_rec_mode_enabled
same reason as at line 139
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 122: configure_rtc_buc_top_swap(TS_DISABLE);
Instead a print here would be helpful indicating why we are doing a reset and what action is being t […]
Flow has changed in the latest patch.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 130: init_staging_irdev(&staging_irdev)
staging_rdev = init_staging_irdev(...) and check staging_rdev for NULL.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 136: rdev_mmap_full
I believe this is making the assumption that boot media is memory mapped and hence the memory leak i […]
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 177: TODO
TODO for? Is there a bug?
Removed in latest patch
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 181: Finished writing
Control can reach here in two cases: […]
Flow updated with the latest patch.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 199: /* TODO: Write Elog */
Wanted to add a ELOG entry for failure case. This can also be done on the caller side.
Done
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 200: return
I put the error handling in the caller side like https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/27369/56/src/soc/intel/common/basec... File src/soc/intel/common/basecode/include/intelbasecode/ucode_update.h:
https://review.coreboot.org/c/coreboot/+/27369/56/src/soc/intel/common/basec... PS56, Line 7: Caller should implement this function
Then you generally don't want a weak function but just fail the build?
Done
-
Sugnan Prabhu S (Code Review)