Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58513 )
Change subject: soc/intel/common: Check if CSE is function disable ......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/58513/comment/b203c0b5_41f68fd6 PS3, Line 251: /* : * If ME is already hidden then reading ME HFS1 register would be wrong and will : * receive junk, hence, return `true` as CSE is already disable. : */ : if (!is_cse_enabled()) : return true;
I think the second option makes sense too, or else the entire CSE API would have to change to have 3 […]
I had little different opinion about leaving things on caller side because in API world we should try to make the caller knows as little as possible about the real operations and expectations. Hence, i would advocate to make sure we handle things in API itself about taking care of any corner case we have and pass the right return value.
ideally i should have handle me_read_config32 function to return -1 if device is not there in that way, we are not reading wrong data. If not that, then atleast cse_is_hfs1_com_normal, cse_is_hfs1_com_secover_mei_msg, cse_is_hfs1_com_soft_temp_disable and cse_is_hfs3_fw_sku_lite function should check if CSE is visible else don't request to read the ME status register.