[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/cse: Add consistent HECI command id/group id naming

[Sridhar Siricilla (Code Review)]

Hello Rizwan Qureshi,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/38247

to review the following change.

Change subject: soc/intel/common/cse: Add consistent HECI command id/group id naming
......................................................................

soc/intel/common/cse: Add consistent HECI command id/group id naming

Below changes are done:
1. Consistent HECI command/group ID naming.
2. Rename macros to match with Intel ME BIOS Spec.
3. Add description for structure members.

TEST=Build and Boot hatch board.

Change-Id: Ia902095483d5badf778d0c1faa6bf8cc431f0e50
Signed-off-by: Sridhar Siricilla 
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 22 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38247/1

diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 74a0020..6891ae0 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -67,24 +67,27 @@
 #define MEI_HDR_CSE_ADDR_START	0
 #define MEI_HDR_CSE_ADDR	(((1 << 8) - 1) << MEI_HDR_CSE_ADDR_START)

-#define HECI_OP_MODE_SEC_OVERRIDE 5
+/* MKHI Command groups */
+#define MKHI_GROUP_ID_CBM	0x0
+#define MKHI_GROUP_ID_HMRFPO	0x5

/* Global Reset Command ID */
 #define MKHI_GLOBAL_RESET_REQ	0xb
-#define MKHI_GROUP_ID_CBM	0

/* RST Origin */
-#define GR_ORIGIN_BIOS_POST	2
-
-#define MKHI_HMRFPO_GROUP_ID	5
+#define GR_ORIGIN_BIOS_POST	0x2

/* HMRFPO Command Ids */
-#define MKHI_HMRFPO_ENABLE	1
-#define MKHI_HMRFPO_GET_STATUS	3
+#define MKHI_HMRFPO_ENABLE	0x1
+#define MKHI_HMRFPO_GET_STATUS	0x3

-#define ME_HFS_CWS_NORMAL	5
-#define ME_HFS_MODE_NORMAL	0
-#define ME_HFS_TEMP_DISABLE	3
+/* ME Current Working States */
+#define ME_HFS1_CWS_NORMAL	0x5
+
+/* ME Current Operation Modes */
+#define ME_HFS1_COM_NORMAL	0x0
+#define ME_HFS1_COM_SOFT_TEMP_DISABLE	0x3
+#define ME_HFS1_COM_SECOVER_MEI_MSG	0x5

static struct cse_device {
    uintptr_t sec_bar;
@@ -258,14 +261,14 @@
 }

/*
- * Checks if CSE is in SEC_OVERRIDE operation mode. This is the mode where
+ * Checks if CSE is in ME_HFS1_COM_SECOVER_MEI_MSG operation mode. This is the mode where
  * CSE will allow reflashing of CSE region.
  */
 static uint8_t check_cse_sec_override_mode(void)
 {
    union me_hfsts1 hfs1;
    hfs1.data = me_read_config32(PCI_ME_HFSTS1);
-	if (hfs1.fields.operation_mode == HECI_OP_MODE_SEC_OVERRIDE)
+	if (hfs1.fields.operation_mode == ME_HFS1_COM_SECOVER_MEI_MSG)
    	return 1;
    return 0;
 }
@@ -632,7 +635,7 @@
    /* HMRFPO Enable message */
    struct hmrfpo_enable_msg msg = {
    	.hdr = {
-			.group_id = MKHI_HMRFPO_GROUP_ID,
+			.group_id = MKHI_GROUP_ID_HMRFPO,
    		.command = MKHI_HMRFPO_ENABLE,
    	},
    	.nonce = {0},
@@ -641,7 +644,9 @@
    /* HMRFPO Enable response */
    struct hmrfpo_enable_resp {
    	struct mkhi_hdr hdr;
+		/* Base addr for factory data area, not relevant for client SKUs */
    	uint32_t fct_base;
+		/* Length of factory data area, not relevant for client SKUs */
    	uint32_t fct_limit;
    	uint8_t status;
    	uint8_t padding[3];
@@ -658,9 +663,9 @@
     * - Working state is normal and
     * - Operation mode is normal or temporary disable mode.
     */
-	if (hfs1.fields.working_state != ME_HFS_CWS_NORMAL ||
-		(hfs1.fields.operation_mode != ME_HFS_MODE_NORMAL &&
-		hfs1.fields.operation_mode != ME_HFS_TEMP_DISABLE)) {
+	if (hfs1.fields.working_state != ME_HFS1_CWS_NORMAL ||
+		(hfs1.fields.operation_mode != ME_HFS1_COM_NORMAL &&
+		hfs1.fields.operation_mode != ME_HFS1_COM_SOFT_TEMP_DISABLE)) {
    	printk(BIOS_ERR, "HECI: ME not in required Mode\n");
    	goto failed;
    }
@@ -697,7 +702,7 @@

struct hmrfpo_get_status_msg msg = {
    	.hdr = {
-			.group_id = MKHI_HMRFPO_GROUP_ID,
+			.group_id = MKHI_GROUP_ID_HMRFPO,
    		.command = MKHI_HMRFPO_GET_STATUS,
    	},
    };