Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37282 )
Change subject: soc/intel/common: Rename set_host_ready() to cse_set_host_ready() ......................................................................
Patch Set 4:
(13 comments)
Some of the comments I provided might impact other SoC code. Please feel free to submit those changes as separate CLs.
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 BWG.
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 took for cse to be ready in override mode here.
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
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_.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 111: is_cse_enabled For consistency: cse_is_enabled()
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 114: * nit: space before *
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()
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. But I don't see Host reset or CSE reset only options in it. Can you please point me to the right doc?
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()
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()
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.
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(). Actually, I think having an enum here might be helpful and then the param to send_heci_reset_req_message() can be of the enum type.
https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... PS4, Line 152: * nit: space after *