Nico Huber 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 36:
(2 comments)
The thing is: the FIT has to be part of immutable FW too(if we do not do that we lose the whole purpose of recovery mode). So we do not have flexibility to update RO FIT in the field to extend additional uCode patches. In this specific design when we select RO using top swap we are absolutely assured that "NO" immutable code executes in the boot path. This whole logic although may sound a bit complex, addresses the requirements of Chrome recovery requirements as well as IA uCode loading requirements (through FIT before CPU reset). Hope to see simplified implementations in future if either side some requirements can be eased up.
I do understand the process in this change. Let me elaborate my theory more accurately. You can have a RO FIT with entries to both RO MCU and RW MCU (at least that is what I understood earlier). Booting with the process of this change and top-swap disabled should have the same effect as booting with such a two-entry FIT and erased RW MCU. Right?
https://review.coreboot.org/#/c/27369/36//COMMIT_MSG Commit Message:
PS36: Just for the future (not saying you should change it here as your paragraphs look nice), line length for commit messages is usually 72 chars, and somebody set the limit to 75 for coreboot.
https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... PS30, Line 33: define locate_staging_rw(rdev) \ : fmap_locate_area_as_rdev_rw(CONFIG_INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG, \ : rdev) : : #define locate_staging_ro(rdev) \ : fmap_locate_area_as_rdev(CONFIG_INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG, \ : rdev)
I put these defines just to reduce the line width where I actually call the functions.
If it's only about the line length, how about giving these long CONFIG names a shorter name in C? for instance:
static const char *const ts_fit_reg = CONFIG_INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG;
Then you wouldn't need macros or functions.