Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 19:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 79: MKHI_HMRFPO_GROUP_ID
Can you please make the names more consistent. […]
Addressed as part of patch- https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 652: uint32_t fct_base; : uint32_t fct_limit; : uint8_t status;
What are these fields?
fct_base/fct_limit are not applicable to client platforms. These fields refers to factory data area. Status field is a value returned by CSE for HMRFPO_ENABLE command. Status Value 0 indicates enabling HMRFPO success, and non-zero value indicates error.
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 683: }
Don't you care about the status?
status check is implemented in the patch: https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 117: Send HMRFPO_ENABLE command.
This should have a better explanation of what this command really means.
We have added more information as part of patch:https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 118: failure
Does failure mean failure to send message or failure to enable?
It indicates failure to send command and to enable hmrfpo mode. I updated the comment to reflect the same as part of patch: https://review.coreboot.org/c/coreboot/+/35546