Change in coreboot[master]: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display

[Johnny Lin (Code Review)]

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

Change subject: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display
......................................................................

mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display

Overwrite FSP version to coreboot table LB_TAG_EXTRA_VERSION.

Tested=On OCP Delta Lake LinuxBoot, u-root shell cbmem can see
"LB_TAG_EXTRA_VERSION": "FSPv2.1-0.0.1.120"

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ramstage.c
1 file changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/1

diff --git a/src/mainboard/ocp/deltalake/ramstage.c b/src/mainboard/ocp/deltalake/ramstage.c
index 0acef91..d0baca4 100644
--- a/src/mainboard/ocp/deltalake/ramstage.c
+++ b/src/mainboard/ocp/deltalake/ramstage.c
@@ -1,13 +1,18 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* This file is part of the coreboot project. */

+#include <boot/coreboot_tables.h>
 #include <console/console.h>
 #include <drivers/ipmi/ipmi_ops.h>
 #include <drivers/ocp/dmi/ocp_dmi.h>
+#include <fsp/util.h>
 #include <soc/ramstage.h>
+#include <string.h>

#include "ipmi.h"

+#define FSP_VER_LEN 30
+
 extern struct fru_info_str fru_strings;

static void dl_oem_smbios_strings(struct device *dev, struct smbios_type11 *t)
@@ -67,6 +72,41 @@
    	printk(BIOS_ERR, "ipmi_set_ppin failed\n");
 }

+static void get_fsp_version(char *buf)
+{
+	/* FSP header has been read and validated at this stage */
+	struct fsp_header *hdr = &fsps_hdr;
+	union {
+		uint32_t val;
+		struct {
+			uint8_t bld_num;
+			uint8_t revision;
+			uint8_t minor;
+			uint8_t major;
+		} rev;
+	} revision;
+
+	revision.val = hdr->fsp_revision;
+	snprintf(buf, FSP_VER_LEN, "FSPv%u.%u-%u.%u.%u.%u", (hdr->spec_version >> 4),
+		hdr->spec_version & 0xf, revision.rev.major,
+		revision.rev.minor, revision.rev.revision, revision.rev.bld_num);
+}
+
+/* Overwrite FSP version to coreboot table LB_TAG_EXTRA_VERSION */
+void lb_board(struct lb_header *header)
+{
+	struct lb_string *rec;
+	size_t len;
+	char fsp_version[FSP_VER_LEN] = {0};
+
+	get_fsp_version(fsp_version);
+	rec = (struct lb_string *)lb_new_record(header);
+	rec->tag = LB_TAG_EXTRA_VERSION;
+	len = strlen(fsp_version);
+	rec->size = ALIGN_UP(sizeof(*rec) + len + 1, 8);
+	memcpy(rec->string, fsp_version, len+1);
+}
+
 struct chip_operations mainboard_ops = {
    .enable_dev = mainboard_enable,
    .final = mainboard_final,

[Johnny Lin (Code Review)]

Hello Jonathan Zhang, Jingle Hsu, Morgan Jang, Patrick Rudolph,

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

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

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

Change subject: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display
......................................................................

mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display

Add FSP version to coreboot table LB_TAG_EXTRA_VERSION.

Tested=On OCP Delta Lake LinuxBoot, u-root shell cbmem can see
"LB_TAG_EXTRA_VERSION": "FSPv2.1-0.0.1.120"

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ramstage.c
1 file changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/2

[Johnny Lin (Code Review)]

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

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

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

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

Change subject: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display
......................................................................

mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display

Add FSP version to coreboot table LB_TAG_EXTRA_VERSION.

Tested=On OCP Delta Lake LinuxBoot, u-root shell cbmem can see
"LB_TAG_EXTRA_VERSION": "FSPv2.1-0.0.1.120"

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/mainboard/ocp/deltalake/ramstage.c
1 file changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/3

[Paul Menzel (Code Review)]

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

Change subject: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display
......................................................................

Patch Set 4:

It’s a useful feature. Can this be added to common code?

[Jonathan Zhang (Code Review)]

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

Change subject: mb/ocp/deltalake: Add FSP version to coreboot table for cbmem to display
......................................................................

Patch Set 4:
> Patch Set 4:
>
> It’s a useful feature. Can this be added to common code?

Makes sense. We should update drivers/intel/fsp2_0, we either piggy back on DISPLAY_FSP_HEADER, or make a new Kconfig item.

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

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

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

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

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

Add option FSP_ADD_VERSION_TO_CBMEM for coreboot table to add
FSP version to LB_TAG_EXTRA_VERSION, default is n.

Tested=On OCP Delta Lake LinuxBoot, u-root shell cbmem can see
"LB_TAG_EXTRA_VERSION": "FSPv2.1-0.0.1.120"

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 31 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/5

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

Patch Set 5:
> Patch Set 4:
> > Patch Set 4:
> >
> > It’s a useful feature. Can this be added to common code?
>
> Makes sense. We should update drivers/intel/fsp2_0, we either piggy back on DISPLAY_FSP_HEADER, or make a new Kconfig item.

Add to fsp2_0 and lib/coreboot_table.

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Jingle Hsu, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

Add option FSP_ADD_VERSION_TO_CBMEM for coreboot table to add
FSP version to LB_TAG_FSP_VERSION, default is n.

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 31 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/6

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

Patch Set 8:

This change is ready for review.

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Jingle Hsu, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

Add option FSP_ADD_VERSION_TO_CBMEM for coreboot table to add
FSP version to LB_TAG_FSP_VERSION, default is n.

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 31 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/10

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Jingle Hsu, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

Add option FSP_ADD_VERSION_TO_CBMEM for coreboot table to add
FSP version to LB_TAG_FSP_VERSION, default is n.

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 31 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/11

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Paul Menzel (Code Review)]

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

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

Patch Set 11:

(2 comments)

I do not see, why this should be configurable by a Kconfig option. I’d always add the FSP version to the coreboot tables.

https://review.coreboot.org/c/coreboot/+/41760/11/src/drivers/intel/fsp2_0/K... 
File src/drivers/intel/fsp2_0/Kconfig:

PS11: 
The Kconfig addition should probably go into the next commit?

https://review.coreboot.org/c/coreboot/+/41760/11/src/drivers/intel/fsp2_0/K... 
PS11, Line 213: cbmem
What does CBMEM have to do with coreboot tables?

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41760 )

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

Patch Set 11:

It is generally easier to design such APIs from the callers
perspective. All the #if in the follow-up could be avoided
that way. For instance

size_t lb_get_chipset_blob_version(char *buf);

This could be paired with a Kconfig (also declared by the
caller) which would be selected by the blob driver and decide
if the API can be used. Alternatively, one could add a weak
implementation (but that would increase complexity for the
caller and I'm generally not fond of those).

The coreboot-tables code treats its table space as infinite
anyway, so this would also not need any constant for the
buffer size.

Change in coreboot[master]: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41760 )

Change subject: intel/fsp2_0: Add fsp_get_version() to copy FSP version to a char buffer
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41760/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41760/11//COMMIT_MSG@11 
PS11, Line 11: 
can u please add a test and sample string what u read using this API.

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Jonathan Zhang, Jingle Hsu, Subrata Banik, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table

Intel FSP version would be added to coreboot table LB_TAG_FSP_VERSION which
is defined in [1].

Tested=On OCP Delta Lake, with an updated LinuxBoot payload cbmem utility
can see "LB_TAG_FSP_VERSION": "2.1-0.0.1.120"

[1] https://review.coreboot.org/c/coreboot/+/41809

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
2 files changed, 40 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/12

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41760 )

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

Patch Set 12: Code-Review+2

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

Patch Set 12:

(3 comments)
> Patch Set 11:
>
> (2 comments)
>
> I do not see, why this should be configurable by a Kconfig option. I’d always add the FSP version to the coreboot tables.

Removed the new Kconfig option, it applies when PLATFORM_USES_FSP2_0 is selected.
Move functions here so coreboot_table.c in CB:41809 can avoid using #if preprocessing.

https://review.coreboot.org/c/coreboot/+/41760/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41760/11//COMMIT_MSG@11 
PS11, Line 11:
> can u please add a test and sample string what u read using this API.

Done

https://review.coreboot.org/c/coreboot/+/41760/11/src/drivers/intel/fsp2_0/K... 
File src/drivers/intel/fsp2_0/Kconfig:

PS11:
> The Kconfig addition should probably go into the next commit?

Done

https://review.coreboot.org/c/coreboot/+/41760/11/src/drivers/intel/fsp2_0/K... 
PS11, Line 213: cbmem
> What does CBMEM have to do with coreboot tables?

I meant the cbmem utility. I removed this option and this feature applies when PLATFORM_USES_FSP2_0 is selected.

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Paul Menzel (Code Review)]

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

Patch Set 13: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@9 
PS13, Line 9: would be added
is added

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@10 
PS13, Line 10: is defined in [1].
The reference is not needed, as the commits are in the same branch and Gerrit (and once committed git) show the dependency.

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@12 
PS13, Line 12: LinuxBoot payload cbmem utility
Do you mean u-root? Do they have there own cbmem utility, which has an option to print the coreboot tables?

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@15 
PS13, Line 15: [1] https://review.coreboot.org/c/coreboot/+/41809
As written above, this could be removed.

(For the future, I guess the Change-Id and commit message summary, should also be mentioned. Otherwise, once committed, (only) the URL makes it difficult to work with native git commands.)

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Jonathan Zhang, Paul Menzel, Jingle Hsu, Subrata Banik, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table

Intel FSP version would be added to coreboot table LB_TAG_FSP_VERSION which
is defined in [1].

Tested=On OCP Delta Lake, with an updated LinuxBoot payload cbmem utility
can see "LB_TAG_FSP_VERSION": "2.1-0.0.1.120"

[1] https://review.coreboot.org/c/coreboot/+/41809

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
2 files changed, 40 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/14

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Jonathan Zhang, Paul Menzel, Jingle Hsu, Subrata Banik, Morgan Jang, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table

Intel FSP version is added to coreboot table LB_TAG_FSP_VERSION.

Tested=On OCP Delta Lake, with an updated LinuxBoot payload cbmem utility
can see "LB_TAG_FSP_VERSION": "2.1-0.0.1.120"

Change-Id: I579fa87028b9ed0734b2fac206daf43ecae311be
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
2 files changed, 40 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41760/15

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

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

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

Patch Set 15:

(4 comments)

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@9 
PS13, Line 9: would be added
> is added

Done

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@10 
PS13, Line 10: is defined in [1].
> The reference is not needed, as the commits are in the same branch and Gerrit (and once committed gi […]

Done

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@12 
PS13, Line 12: LinuxBoot payload cbmem utility
> Do you mean u-root? Do they have there own cbmem utility, which has an option to print the coreboot  […]

Yes, u-root has their own cbmem utility that by default can print them.

https://review.coreboot.org/c/coreboot/+/41760/13//COMMIT_MSG@15 
PS13, Line 15: [1] https://review.coreboot.org/c/coreboot/+/41809
> As written above, this could be removed. […]

Done

Change in coreboot[master]: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot ...

[Johnny Lin (Code Review)]

Johnny Lin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41760 )

Change subject: intel/fsp2_0: Add functions for adding Intel FSP version to coreboot table
......................................................................

Abandoned

Merged into CB:41809 because they depend on each other