Hello Rizwan Qureshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35231
to review the following change.
Change subject: src/soc/intel/(apollolake,cnl): Modify code to point common function ......................................................................
src/soc/intel/(apollolake,cnl): Modify code to point common function
Removed function print_me_version from cnl/skl, and dump_me_version(apollolake), and made changes to call print_me_firmware_version() which is defined in the cse lib.
Change-Id: I46431426e88b086b326197ba1f9487da1e12074b Signed-off-by: sridhar sridhar.siricilla@intel.com Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/apollolake/cse.c M src/soc/intel/cannonlake/me.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 4 files changed, 3 insertions(+), 221 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/35231/1
diff --git a/src/soc/intel/apollolake/cse.c b/src/soc/intel/apollolake/cse.c index 32e9222..386662e 100644 --- a/src/soc/intel/apollolake/cse.c +++ b/src/soc/intel/apollolake/cse.c @@ -36,8 +36,6 @@ #define READ_FILE_FLAG_EMULATED (1 << 2) #define READ_FILE_FLAG_HW (1 << 3)
-#define MKHI_GROUP_ID_GEN 0xff -#define GET_FW_VERSION 0x02
#define MCA_MAX_FILE_PATH_SIZE 64
@@ -182,63 +180,6 @@ printk(BIOS_CRIT, "failed to save FPF state\n"); }
-static void dump_cse_version(void *unused) -{ - int res; - size_t reply_size; - - struct fw_version_cmd { - union mkhi_header mkhi_hdr; - } __packed msg; - - struct version { - uint16_t minor; - uint16_t major; - uint16_t build; - uint16_t hotfix; - } __packed; - - struct fw_version_response { - union mkhi_header mkhi_hdr; - struct version code; - struct version nftp; - struct version fitc; - } __packed rsp; - - /* - * Print ME version only if UART debugging is enabled. Else, it takes - * ~0.6 second to talk to ME and get this information. - */ - if (!CONFIG(CONSOLE_SERIAL)) - return; - - msg.mkhi_hdr.fields.group_id = MKHI_GROUP_ID_GEN; - msg.mkhi_hdr.fields.command = GET_FW_VERSION; - - res = heci_send(&msg, sizeof(msg), BIOS_HOST_ADDR, HECI_MKHI_ADDR); - - if (!res) { - printk(BIOS_ERR, "Failed to send HECI message.\n"); - return; - } - - reply_size = sizeof(rsp); - res = heci_receive(&rsp, &reply_size); - - if (!res) { - printk(BIOS_ERR, "Failed to receive HECI reply.\n"); - return; - } - - if (rsp.mkhi_hdr.fields.result != 0) { - printk(BIOS_ERR, "Failed to get ME version.\n"); - return; - } - - printk(BIOS_DEBUG, "ME: Version: %d.%d.%d.%d\n", rsp.code.major, - rsp.code.minor, rsp.code.hotfix, rsp.code.build); -} - static void dump_cse_state(void) { uint32_t fwsts1; @@ -292,4 +233,4 @@ }
BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_ENTRY, fpf_blown, NULL); -BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, dump_cse_version, NULL); +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, print_me_firmware_version, NULL); diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index dab909b..931f4cd 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -24,13 +24,6 @@ #include <stdint.h> #include <stdlib.h>
-/* Miscellaneous constants */ -enum { - MKHI_GEN_GROUP_ID = 0xFF, - MKHI_GET_FW_VERSION = 0x02, - ME_OPMODE_NORMAL = 0x00, - ME_WSTATE_NORMAL = 0x05, -};
/* Host Firmware Status Register 1 */ union hfsts1 { @@ -145,80 +138,7 @@ } __packed fields; };
-/* - * From reading the documentation, this should work for both WHL and CML - * platforms. Also, calling this function from dump_me_status() does not - * work, as the ME does not respond and the command times out. - */ -static void print_me_version(void *unused) -{ - struct mkhi_hdr { - uint8_t group_id; - uint8_t command :7; - uint8_t is_resp :1; - uint8_t rsvd; - uint8_t result; - } __packed; - - struct version { - uint16_t minor; - uint16_t major; - uint16_t build; - uint16_t hotfix; - } __packed; - - struct fw_ver_resp { - struct mkhi_hdr hdr; - struct version code; - struct version rec; - struct version fitc; - } __packed; - - union hfsts1 hfsts1; - const struct mkhi_hdr fw_ver_msg = { - .group_id = MKHI_GEN_GROUP_ID, - .command = MKHI_GET_FW_VERSION, - }; - struct fw_ver_resp resp; - size_t resp_size = sizeof(resp); - - /* Ignore if UART debugging is disabled */ - if (!CONFIG(CONSOLE_SERIAL)) - return; - - hfsts1.raw = me_read_config32(PCI_ME_HFSTS1); - - /* - * Prerequisites: - * 1) HFSTS1 Current Working State is Normal - * 2) HFSTS1 Current Operation Mode is Normal - * 3) It's after DRAM INIT DONE message (taken care of by calling it - * during ramstage - */ - if ((hfsts1.fields.working_state != ME_WSTATE_NORMAL) || - (hfsts1.fields.operation_mode != ME_OPMODE_NORMAL)) - goto fail; - - heci_reset(); - - if (!heci_send(&fw_ver_msg, sizeof(fw_ver_msg), BIOS_HOST_ADDR, - HECI_MKHI_ADDR)) - goto fail; - - if (!heci_receive(&resp, &resp_size)) - goto fail; - - if (resp.hdr.result) - goto fail; - - printk(BIOS_DEBUG, "ME: Version: %d.%d.%d.%d\n", resp.code.major, - resp.code.minor, resp.code.hotfix, resp.code.build); - return; - -fail: - printk(BIOS_DEBUG, "ME: Version: Unavailable\n"); -} -BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_version, NULL); +BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_firmware_version, NULL);
void dump_me_status(void *unused) { diff --git a/src/soc/intel/skylake/include/soc/me.h b/src/soc/intel/skylake/include/soc/me.h index bec54d8..3c0da58 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -195,12 +195,6 @@ } __packed fields; };
-#define MKHI_GEN_GROUP_ID 0xff - -#define MKHI_GET_FW_VERSION 0x02 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07
void intel_me_status(void); int send_global_reset(void); diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index fde4b13..57ea95f 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -198,84 +198,11 @@ "M0 kernel load", };
-static void print_me_version(void *unused) -{ - struct mkhi_hdr { - uint8_t group_id; - uint8_t command:7; - uint8_t is_resp:1; - uint8_t rsvd; - uint8_t result; - } __packed; - - struct version { - uint16_t minor; - uint16_t major; - uint16_t build; - uint16_t hotfix; - } __packed; - - struct fw_ver_resp { - struct mkhi_hdr hdr; - struct version code; - struct version rec; - struct version fitc; - } __packed; - - const struct mkhi_hdr fw_ver_msg = { - .group_id = MKHI_GEN_GROUP_ID, - .command = MKHI_GET_FW_VERSION, - }; - - struct fw_ver_resp resp; - size_t resp_size = sizeof(resp); - union me_hfs hfs; - - /* - * Print ME version only if UART debugging is enabled. Else, it takes ~1 - * second to talk to ME and get this information. - */ - if (!CONFIG(CONSOLE_SERIAL)) - return; - - hfs.data = me_read_config32(PCI_ME_HFSTS1); - /* - * This command can be run only if: - * - Working state is normal and - * - Operation mode is normal. - */ - if ((hfs.fields.working_state != ME_HFS_CWS_NORMAL) || - (hfs.fields.operation_mode != ME_HFS_MODE_NORMAL)) - goto failed; - - /* - * It is important to do a heci_reset to ensure BIOS and ME are in sync - * before reading firmware version. - */ - heci_reset(); - - if (!heci_send(&fw_ver_msg, sizeof(fw_ver_msg), BIOS_HOST_ADD, - HECI_MKHI_ADD)) - goto failed; - - if (!heci_receive(&resp, &resp_size)) - goto failed; - - if (resp.hdr.result) - goto failed; - - printk(BIOS_DEBUG, "ME: Version : %d.%d.%d.%d\n", resp.code.major, - resp.code.minor, resp.code.hotfix, resp.code.build); - return; - -failed: - printk(BIOS_DEBUG, "ME: Version : Unavailable\n"); -} /* * This can't be put in intel_me_status because by the time control * reaches there, ME doesn't respond to GET_FW_VERSION command. */ -BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_version, NULL); +BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_firmware_version, NULL);
void intel_me_status(void) {