Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37282 )
Change subject: soc/intel/common: Rename functions to have consistency naming ......................................................................
Patch Set 12:
(13 comments)
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 288: HECI_DELAY_READY
Just curious: How was this 15 second delay/wait time determined? I don't find any reference to it in […]
It's not available in BWG. Used the same timeout as the one used for wait_heci_ready.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 297:
Since you are adding debug prints, it might be helpful to have a print that says how much time it to […]
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 61: HECI
MKHI
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 104: me
While you are cleaning up names, may be update this to cse_.
This will be addressed in the separate patch later as there are multiple references to the function.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 111: is_cse_enabled
For consistency: cse_is_enabled()
This will be addressed in separate patch later as this function being referenced in many places.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 114: *
nit: space before *
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 121: wait_cse_sec_override_mode
For consistency: cse_wait_sec_override_mode()
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 128: send_heci_reset_req_message
I think this should be: cse_request_global_reset() to match what BWG says. […]
Addressed naming part in the patch : https://review.coreboot.org/c/coreboot/+/37584 Currently, the Host reset/CSE reset only options are not included in the BWG guide.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 134: send_hmrfpo_enable_msg
For consistency: cse_hmrfpo_enable()
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 141: send_hmrfpo_get_status_msg
For consistency: cse_hmrfpo_get_status()
Done
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 144: BIOS_HOST_ADDR
I think this should be HECI_HOST_ADDR for consistency.
It's a value of Host_Address field of Fixed Address MEI Header. So, macro rename is not required.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 147: /* Command GLOBAL_RESET_REQ Reset Types */ : #define GLOBAL_RESET 1 : #define HOST_RESET_ONLY 2 : #define CSE_RESET_ONLY 3
These should be placed closer to send_heci_reset_req_message(). […]
The comment is addressed in the patch : https://review.coreboot.org/c/coreboot/+/36786
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 152: *
nit: space after *
Addressed the change in in the https://review.coreboot.org/c/coreboot/+/38248/1