Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37283 )
Change subject: soc/intel/common/block/cse: Modify handling of HMRFPO_ENABLE command ......................................................................
Patch Set 33: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/37283/33/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/37283/33/src/soc/intel/common/block... PS33, Line 653: static int cse_check_hmrfpo_enable_prerequisites(void) Maybe return a bool? Also, the function name can be worded as a question:
static bool cse_is_hmrfpo_enable_allowed(void)
https://review.coreboot.org/c/coreboot/+/37283/33/src/soc/intel/common/block... PS33, Line 661: cse_is_hfs1_cws_normal() Since this needs to be true for both cases, how about factoring it out?
if (!cse_is_hfs1_cws_normal()) return false;
if (cse_is_hfs1_com_normal()) return true;
if (cse_is_hfs3_fw_sku_custom() && cse_is_hfs1_com_soft_temp_disable()) return true;
https://review.coreboot.org/c/coreboot/+/37283/33/src/soc/intel/common/block... PS33, Line 704: /* : * This command can be run only if: : * - ME's current working state is Normal and current operation mode is Normal : * - (or) current working state is Normal and current operating mode is : * Soft Temp Disable while CSE Firmware SKU is Custom. : */ Isn't this comment the same as in cse_check_hmrfpo_enable_prerequisites() ?