Change in coreboot[master]: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

[Johnny Lin (Code Review)]

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

There are 4 slots in YV3, Location In Chassis should be 1~4.

Tested=on OCP Delta Lake, dmidecode -t 2 verified the string is correct.

Change-Id: I3b65ecc6f6421d85d1cb890c522be4787362a01b
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ipmi.c
M src/mainboard/ocp/deltalake/ipmi.h
M src/mainboard/ocp/deltalake/ramstage.c
3 files changed, 48 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42277/1

diff --git a/src/mainboard/ocp/deltalake/ipmi.c b/src/mainboard/ocp/deltalake/ipmi.c
index fe620fa..dd81bfd 100644
--- a/src/mainboard/ocp/deltalake/ipmi.c
+++ b/src/mainboard/ocp/deltalake/ipmi.c
@@ -45,3 +45,28 @@

return CB_SUCCESS;
 }
+
+enum cb_err ipmi_get_slot_id(uint8_t *slot_id)
+{
+	int ret;
+	struct ipmi_config_rsp {
+		struct ipmi_rsp resp;
+		uint8_t board_sku_id;
+		uint8_t board_rev_id;
+		uint8_t slot_id;
+		uint8_t slot_config_id;
+	} __packed;
+	struct ipmi_config_rsp rsp;
+
+	ret = ipmi_kcs_message(BMC_KCS_BASE, IPMI_NETFN_OEM, 0x0, IPMI_OEM_GET_BOARD_ID,
+			NULL, 0, (unsigned char *) &rsp, sizeof(rsp));
+
+	if (ret < sizeof(struct ipmi_rsp) || rsp.resp.completion_code) {
+		printk(BIOS_ERR, "IPMI: %s command failed (ret=%d resp=0x%x)\n",
+			__func__, ret, rsp.resp.completion_code);
+		return CB_ERR;
+	}
+	*slot_id = rsp.slot_id;
+
+	return CB_SUCCESS;
+}
diff --git a/src/mainboard/ocp/deltalake/ipmi.h b/src/mainboard/ocp/deltalake/ipmi.h
index 310ff27..b167473 100644
--- a/src/mainboard/ocp/deltalake/ipmi.h
+++ b/src/mainboard/ocp/deltalake/ipmi.h
@@ -8,6 +8,7 @@
 #define IPMI_NETFN_OEM			0x30
 #define   IPMI_OEM_SET_PPIN		0x77
 #define   IPMI_OEM_GET_PCIE_CONFIG	0xf4
+#define   IPMI_OEM_GET_BOARD_ID     0x37

#define PCIE_CONFIG_UNKNOWN	0x0
 #define PCIE_CONFIG_A		0x1
@@ -24,4 +25,5 @@

enum cb_err ipmi_set_ppin(struct ppin_req *req);
 enum cb_err ipmi_get_pcie_config(uint8_t *config);
+enum cb_err ipmi_get_slot_id(uint8_t *slot_id);
 #endif
diff --git a/src/mainboard/ocp/deltalake/ramstage.c b/src/mainboard/ocp/deltalake/ramstage.c
index ae11296..f08eb9b 100644
--- a/src/mainboard/ocp/deltalake/ramstage.c
+++ b/src/mainboard/ocp/deltalake/ramstage.c
@@ -4,10 +4,31 @@
 #include <drivers/ipmi/ipmi_ops.h>
 #include <drivers/ocp/dmi/ocp_dmi.h>
 #include <soc/ramstage.h>
+#include <stdio.h>

#include "ipmi.h"

+#define SLOT_ID_LEN 2
+
 extern struct fru_info_str fru_strings;
+static char slot_id_str[SLOT_ID_LEN];
+
+/* Override SMBIOS 2 Location In Chassis from BMC */
+const char *smbios_mainboard_location_in_chassis(void)
+{
+	uint8_t slot_id = 0;
+
+	if (ipmi_get_slot_id(&slot_id) != CB_SUCCESS)
+		return "";
+
+	/* Sanity check, slot_id can only be 1~4 since there are 4 slots in YV3 */
+	if (!slot_id || slot_id > 4) {
+		printk(BIOS_ERR, "slot_id %d is not between 1~4\n", slot_id);
+		return "";
+	}
+	snprintf(slot_id_str, SLOT_ID_LEN, "%d", slot_id);
+	return slot_id_str;
+}

static void dl_oem_smbios_strings(struct device *dev, struct smbios_type11 *t)
 {

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Jingle Hsu, Morgan Jang,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

There are 4 slots in YV3, Location In Chassis should be 1~4.

Tested=on OCP Delta Lake, dmidecode -t 2 verified the string is correct.

Change-Id: I3b65ecc6f6421d85d1cb890c522be4787362a01b
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ipmi.c
M src/mainboard/ocp/deltalake/ipmi.h
M src/mainboard/ocp/deltalake/ramstage.c
3 files changed, 48 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42277/3

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
Could you align this properly?

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Christian Walter, Jingle Hsu, Morgan Jang,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#8).

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

There are 4 slots in YV3, Location In Chassis should be 1~4.

Tested=on OCP Delta Lake, dmidecode -t 2 verified the string is correct.

Change-Id: I3b65ecc6f6421d85d1cb890c522be4787362a01b
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ipmi.c
M src/mainboard/ocp/deltalake/ipmi.h
M src/mainboard/ocp/deltalake/ramstage.c
3 files changed, 56 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42277/8

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
> Could you align this properly?

Done

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
> Done

nope

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
> nope

I was referencing the alignment style in
https://review.coreboot.org/c/coreboot/+/39425/22/src/soc/intel/xeon_sp/incl...
Or should I put only one tab before the value?
Thanks.

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
> I was referencing the alignment style in […]

See comment below

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 11:   
I am referencing to this here which does not align with the other options

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 11:
> I am referencing to this here which does not align with the other options

The commands that belong to the same Net function are aligned together, similar style in
https://review.coreboot.org/c/coreboot/+/33255/18/src/drivers/ipmi/ipmi_kcs....
LN 9~11 all belong to OEM NetFn at LN 8.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ramstage.c:

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
PS8, Line 22: 		return "";
If the result is not CB_SUCCESS, either the BMC communication is not working or there is bug in BMC, let's print an ERROR message.

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
PS8, Line 24: 	/* Sanity check, slot_id can only be 1~4 since there are 4 slots in YV3 */
Let's set up an enum in ipmi.h for slot1 to slot4, that would be better than hard coding here.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 11:      
Use tabs for alignment of the values?

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Christian Walter, Jingle Hsu, Morgan Jang,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#9).

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

There are 4 slots in YV3, Location In Chassis should be 1~4.

Tested=on OCP Delta Lake, dmidecode -t 2 verified the string is correct.

Change-Id: I3b65ecc6f6421d85d1cb890c522be4787362a01b
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ipmi.c
M src/mainboard/ocp/deltalake/ipmi.h
M src/mainboard/ocp/deltalake/ramstage.c
3 files changed, 59 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42277/9

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 9:

(5 comments)

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ipmi.h:

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 8: #define IPMI_NETFN_OEM			0x30
> See comment below

Ack

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 11:
> The commands that belong to the same Net function are aligned together, similar style in […]

Ack

https://review.coreboot.org/c/coreboot/+/42277/6/src/mainboard/ocp/deltalake... 
PS6, Line 11:
> Use tabs for alignment of the values?

It's already aligned by tabs in patchset 8.

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/ramstage.c:

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
PS8, Line 22: 		return "";
> If the result is not CB_SUCCESS, either the BMC communication is not working or there is bug in BMC, […]

Done

https://review.coreboot.org/c/coreboot/+/42277/8/src/mainboard/ocp/deltalake... 
PS8, Line 24: 	/* Sanity check, slot_id can only be 1~4 since there are 4 slots in YV3 */
> Let's set up an enum in ipmi.h for slot1 to slot4, that would be better than hard coding here.

Done

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 9: Code-Review+1

LGTM

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

Patch Set 9: Code-Review+2

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42277 )

Change subject: mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC
......................................................................

mb/ocp/deltalake: Update SMBIOS type 2 Location In Chassis from BMC

There are 4 slots in YV3, Location In Chassis should be 1~4.

Tested=on OCP Delta Lake, dmidecode -t 2 verified the string is correct.

Change-Id: I3b65ecc6f6421d85d1cb890c522be4787362a01b
Signed-off-by: Johnny Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42277
Tested-by: build bot (Jenkins) 
Reviewed-by: Jonathan Zhang 
Reviewed-by: Christian Walter 
---
M src/mainboard/ocp/deltalake/ipmi.c
M src/mainboard/ocp/deltalake/ipmi.h
M src/mainboard/ocp/deltalake/ramstage.c
3 files changed, 59 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Jonathan Zhang: Looks good to me, but someone else must approve
  Christian Walter: Looks good to me, approved

diff --git a/src/mainboard/ocp/deltalake/ipmi.c b/src/mainboard/ocp/deltalake/ipmi.c
index b8a4c53..f60abf2 100644
--- a/src/mainboard/ocp/deltalake/ipmi.c
+++ b/src/mainboard/ocp/deltalake/ipmi.c
@@ -46,3 +46,28 @@

return CB_SUCCESS;
 }
+
+enum cb_err ipmi_get_slot_id(uint8_t *slot_id)
+{
+	int ret;
+	struct ipmi_config_rsp {
+		struct ipmi_rsp resp;
+		uint8_t board_sku_id;
+		uint8_t board_rev_id;
+		uint8_t slot_id;
+		uint8_t slot_config_id;
+	} __packed;
+	struct ipmi_config_rsp rsp;
+
+	ret = ipmi_kcs_message(CONFIG_BMC_KCS_BASE, IPMI_NETFN_OEM, 0x0, IPMI_OEM_GET_BOARD_ID,
+			NULL, 0, (unsigned char *) &rsp, sizeof(rsp));
+
+	if (ret < sizeof(struct ipmi_rsp) || rsp.resp.completion_code) {
+		printk(BIOS_ERR, "IPMI: %s command failed (ret=%d resp=0x%x)\n",
+			__func__, ret, rsp.resp.completion_code);
+		return CB_ERR;
+	}
+	*slot_id = rsp.slot_id;
+
+	return CB_SUCCESS;
+}
diff --git a/src/mainboard/ocp/deltalake/ipmi.h b/src/mainboard/ocp/deltalake/ipmi.h
index 310ff27..039c576 100644
--- a/src/mainboard/ocp/deltalake/ipmi.h
+++ b/src/mainboard/ocp/deltalake/ipmi.h
@@ -5,15 +5,18 @@

#include <stdint.h>

-#define IPMI_NETFN_OEM			0x30
-#define   IPMI_OEM_SET_PPIN		0x77
-#define   IPMI_OEM_GET_PCIE_CONFIG	0xf4
+#define IPMI_NETFN_OEM				0x30
+#define   IPMI_OEM_SET_PPIN			0x77
+#define   IPMI_OEM_GET_PCIE_CONFIG		0xf4
+#define   IPMI_OEM_GET_BOARD_ID			0x37

-#define PCIE_CONFIG_UNKNOWN	0x0
-#define PCIE_CONFIG_A		0x1
-#define PCIE_CONFIG_B		0x2
-#define PCIE_CONFIG_C		0x3
-#define PCIE_CONFIG_D		0x4
+enum config_type {
+	PCIE_CONFIG_UNKNOWN = 0x0,
+	PCIE_CONFIG_A = 0x1,
+	PCIE_CONFIG_B = 0x2,
+	PCIE_CONFIG_C = 0x3,
+	PCIE_CONFIG_D = 0x4,
+};

struct ppin_req {
    uint32_t cpu0_lo;
@@ -24,4 +27,5 @@

enum cb_err ipmi_set_ppin(struct ppin_req *req);
 enum cb_err ipmi_get_pcie_config(uint8_t *config);
+enum cb_err ipmi_get_slot_id(uint8_t *slot_id);
 #endif
diff --git a/src/mainboard/ocp/deltalake/ramstage.c b/src/mainboard/ocp/deltalake/ramstage.c
index a8f92ad..9380304 100644
--- a/src/mainboard/ocp/deltalake/ramstage.c
+++ b/src/mainboard/ocp/deltalake/ramstage.c
@@ -4,10 +4,32 @@
 #include <drivers/ipmi/ipmi_ops.h>
 #include <drivers/ocp/dmi/ocp_dmi.h>
 #include <soc/ramstage.h>
+#include <stdio.h>

#include "ipmi.h"

+#define SLOT_ID_LEN 2
+
 extern struct fru_info_str fru_strings;
+static char slot_id_str[SLOT_ID_LEN];
+
+/* Override SMBIOS 2 Location In Chassis from BMC */
+const char *smbios_mainboard_location_in_chassis(void)
+{
+	uint8_t slot_id = 0;
+
+	if (ipmi_get_slot_id(&slot_id) != CB_SUCCESS) {
+		printk(BIOS_ERR, "IPMI get slot_id failed\n");
+		return "";
+	}
+	/* Sanity check, slot_id can only be 1~4 since there are 4 slots in YV3 */
+	if (slot_id < PCIE_CONFIG_A || slot_id > PCIE_CONFIG_D) {
+		printk(BIOS_ERR, "slot_id %d is not between 1~4\n", slot_id);
+		return "";
+	}
+	snprintf(slot_id_str, SLOT_ID_LEN, "%d", slot_id);
+	return slot_id_str;
+}

static void dl_oem_smbios_strings(struct device *dev, struct smbios_type11 *t)
 {