Change in coreboot[master]: mb/google/dedede: Read DRAM part number from CBI

[Marco Chen (Code Review)]

Hello Marco Chen,

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

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

to review the following change.

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

mb/google/dedede: Read DRAM part number from CBI

The index of MEM_STRAPS will be migrated from per DRAM part number to
per DRAM characteristic therefore one index mapped to a single SPD
binary can represent to multiple DRAM part numbers as long as their
characteristic is the same for DRAM controller to support. In this case,
the real DRAM part number would be provisioned in the CBI instead of SPD
in the factory flow. As a result, we need to extract DRAM part number
from CBI.

BUG=b:152019429
BRANCH=None
TEST=1. provision dram_part_num field of CBI
     2. check DRAM part number is correct in SMBIOS for memory device

Change-Id: I40780a35e04efb279591e9db179cb86b5e907c0d
Signed-off-by: Marco Chen 
---
M src/mainboard/google/dedede/romstage.c
1 file changed, 31 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/40836/1

diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c
index 2efaaf1..a5ded5f 100644
--- a/src/mainboard/google/dedede/romstage.c
+++ b/src/mainboard/google/dedede/romstage.c
@@ -6,9 +6,13 @@
  */

#include <baseboard/variants.h>
+#include <console/console.h>
+#include <ec/google/chromeec/ec.h>
 #include <gpio.h>
+#include <memory_info.h>
 #include <soc/meminit.h>
 #include <soc/romstage.h>
+#include <string.h>
 #include <variant/gpio.h>

void mainboard_memory_init_params(FSPM_UPD *memupd)
@@ -22,3 +26,30 @@

memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated);
 }
+
+bool mainboard_get_dram_part_num(const char **part_num, size_t *len)
+{
+	static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
+	static enum {
+		PART_NUM_NOT_READ,
+		PART_NUM_AVAILABLE,
+		PART_NUM_NOT_IN_CBI,
+	} part_num_state = PART_NUM_NOT_READ;
+
+	if (part_num_state == PART_NUM_NOT_READ) {
+		if (google_chromeec_cbi_get_dram_part_num(&part_num_store[0],
+						sizeof(part_num_store)) < 0) {
+			printk(BIOS_ERR, "No DRAM part number in CBI!\n");
+			part_num_state = PART_NUM_NOT_IN_CBI;
+		} else {
+			part_num_state = PART_NUM_AVAILABLE;
+		}
+	}
+
+	if (part_num_state == PART_NUM_NOT_IN_CBI)
+		return false;
+
+	*part_num = &part_num_store[0];
+	*len = strlen(part_num_store) + 1;
+	return true;
+}

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 1: Code-Review+2

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 37: part_num_state = PART_NUM_NOT_READ;
I am not sure if we need to maintain state information.

From what I understand, this function is called only once during the romstage. It is not retried after that.

Thinking about the hypothetical/future case of mixing different DRAM part numbers, we might need to call this function multiple times. Even in that scenario, it does not make sense to store the part number information. We have to reach out to EC to get the part number on individual channels and nodes.

Thoughts??

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 53: strlen(part_num_store)
Nit: Can we store sizeof(part_num_store) instead? This way it is consistent with the non-override implementation in romstage.c

That way we don't have to explicitly account for '\0' termination character.

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 37: part_num_state = PART_NUM_NOT_READ;
> I am not sure if we need to maintain state information. […]

* Yes, agree with you. The function originally was introduced when romstage calls this function inside the channel loop and assume we need a single DRAM part only. Therefore it makes sense to maintain state for not calling multiple times to EC for the same info.
* Regarding supporting different DRAM Part Numbers in a DUT, I would suggest to defer to another CL stack if we really need to introduce it since currently we call it once in romstage of Jasperlake only. As a result, we focus on single DRAM part first.

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 53: strlen(part_num_store)
> Nit: Can we store sizeof(part_num_store) instead? This way it is consistent with the non-override im […]

copy that.

[Marco Chen (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,

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

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

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

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

mb/google/dedede: Read DRAM part number from CBI

The index of MEM_STRAPS will be migrated from per DRAM part number to
per DRAM characteristic therefore one index mapped to a single SPD
binary can represent to multiple DRAM part numbers as long as their
characteristic is the same for DRAM controller to support. In this case,
the real DRAM part number would be provisioned in the CBI instead of SPD
in the factory flow. As a result, we need to extract DRAM part number
from CBI.

BUG=b:152019429
BRANCH=None
TEST=1. provision dram_part_num field of CBI
     2. check DRAM part number is correct in SMBIOS for memory device

Change-Id: I40780a35e04efb279591e9db179cb86b5e907c0d
Signed-off-by: Marco Chen 
---
M src/mainboard/google/dedede/romstage.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/40836/2

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 37: part_num_state = PART_NUM_NOT_READ;
> Yes, agree with you. […]

Done

https://review.coreboot.org/c/coreboot/+/40836/1/src/mainboard/google/dedede... 
PS1, Line 53: strlen(part_num_store)
> copy that.

Done

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
PS2, Line 42: strlen(part_num_store)
Did you mean to use sizeof(part_num_store) instead of strlen?

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
PS2, Line 42: strlen(part_num_store)
> Did you mean to use sizeof(part_num_store) instead of strlen?

Just think why do we need to return a fixed size which normally is bigger then size of part number instead of a real length?

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
PS2, Line 42: strlen(part_num_store)
> Just think why do we need to return a fixed size which normally is bigger then size of part number i […]

And refer to [1], it is processed by strncpy and MIN(sizeof(dimm->module_part_number), module_part_number_size). If we do sizeof here then no optimization copy there but a duplicated info?

[1] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/hea...

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
PS2, Line 42: strlen(part_num_store)
> And refer to [1], it is processed by strncpy and MIN(sizeof(dimm->module_part_number), module_part_n […]

I was wondering whether google_chromeec_cbi_get_dram_part_num puts in a NUL terminated string. It seems it is doing so.

Also sizeof is a compile-time expression whereas strlen needs to determine the size at run-time.

Anyways I am fine either ways.

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/romstage.c:

https://review.coreboot.org/c/coreboot/+/40836/2/src/mainboard/google/dedede... 
PS2, Line 42: strlen(part_num_store)
> I was wondering whether google_chromeec_cbi_get_dram_part_num puts in a NUL terminated string. […]

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40836 )

Change subject: mb/google/dedede: Read DRAM part number from CBI
......................................................................

mb/google/dedede: Read DRAM part number from CBI

The index of MEM_STRAPS will be migrated from per DRAM part number to
per DRAM characteristic therefore one index mapped to a single SPD
binary can represent to multiple DRAM part numbers as long as their
characteristic is the same for DRAM controller to support. In this case,
the real DRAM part number would be provisioned in the CBI instead of SPD
in the factory flow. As a result, we need to extract DRAM part number
from CBI.

BUG=b:152019429
BRANCH=None
TEST=1. provision dram_part_num field of CBI
     2. check DRAM part number is correct in SMBIOS for memory device

Change-Id: I40780a35e04efb279591e9db179cb86b5e907c0d
Signed-off-by: Marco Chen 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40836
Tested-by: build bot (Jenkins) 
Reviewed-by: Karthik Ramasubramanian 
---
M src/mainboard/google/dedede/romstage.c
1 file changed, 20 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/mainboard/google/dedede/romstage.c b/src/mainboard/google/dedede/romstage.c
index 7a700f4..3155f70 100644
--- a/src/mainboard/google/dedede/romstage.c
+++ b/src/mainboard/google/dedede/romstage.c
@@ -6,9 +6,13 @@
  */

#include <baseboard/variants.h>
+#include <console/console.h>
+#include <ec/google/chromeec/ec.h>
 #include <gpio.h>
+#include <memory_info.h>
 #include <soc/meminit.h>
 #include <soc/romstage.h>
+#include <string.h>
 #include <variant/gpio.h>

void mainboard_memory_init_params(FSPM_UPD *memupd)
@@ -22,3 +26,19 @@

memcfg_init(&memupd->FspmConfig, board_cfg, &spd_info, half_populated);
 }
+
+bool mainboard_get_dram_part_num(const char **part_num, size_t *len)
+{
+	static char part_num_store[DIMM_INFO_PART_NUMBER_SIZE];
+
+	if (google_chromeec_cbi_get_dram_part_num(&part_num_store[0],
+					sizeof(part_num_store)) < 0) {
+		printk(BIOS_ERR, "No DRAM part number in CBI!\n");
+		return false;
+	}
+
+
+	*part_num = &part_num_store[0];
+	*len = strlen(part_num_store);
+	return true;
+}