Change in coreboot[master]: mainboard/google: Rework Hatch so that SPD in CBFS is optional

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Edward O'Callaghan (Code Review)

23 Oct 2019 23 Oct '19

4:34 a.m.

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

mainboard/google: Rework Hatch so that SPD in CBFS is optional

All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus.

BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a

Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Kconfig.name M src/mainboard/google/hatch/Makefile.inc R src/mainboard/google/hatch/romstage_spd_cbfs.c 4 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/36250/1

diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 004cc28..278d009 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -58,6 +58,10 @@ int default 512

+config ROMSTAGE_SPD_CBFS + bool + default n + config DRIVER_TPM_SPI_BUS default 0x1

diff --git a/src/mainboard/google/hatch/Kconfig.name b/src/mainboard/google/hatch/Kconfig.name index fbc4c15..a2fea9f 100644 --- a/src/mainboard/google/hatch/Kconfig.name +++ b/src/mainboard/google/hatch/Kconfig.name @@ -4,29 +4,35 @@ bool "-> Akemi" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_16384 + select ROMSTAGE_SPD_CBFS

config BOARD_GOOGLE_DRATINI bool "-> Dratini" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_16384 + select ROMSTAGE_SPD_CBFS

config BOARD_GOOGLE_HATCH bool "-> Hatch" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_32768 + select ROMSTAGE_SPD_CBFS

config BOARD_GOOGLE_KOHAKU bool "-> Kohaku" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_16384 + select ROMSTAGE_SPD_CBFS

config BOARD_GOOGLE_KINDRED bool "-> Kindred" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_16384 select SOC_INTEL_COMMON_MMC_OVERRIDE + select ROMSTAGE_SPD_CBFS

config BOARD_GOOGLE_HELIOS bool "-> Helios" select BOARD_GOOGLE_BASEBOARD_HATCH select BOARD_ROMSIZE_KB_16384 + select ROMSTAGE_SPD_CBFS diff --git a/src/mainboard/google/hatch/Makefile.inc b/src/mainboard/google/hatch/Makefile.inc index 01a1eb8..3ed82e7 100644 --- a/src/mainboard/google/hatch/Makefile.inc +++ b/src/mainboard/google/hatch/Makefile.inc @@ -20,7 +20,7 @@ ramstage-$(CONFIG_CHROMEOS) += chromeos.c ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC) += ec.c

-romstage-y += romstage.c +romstage-$(CONFIG_ROMSTAGE_SPD_CBFS) += romstage_spd_cbfs.c romstage-$(CONFIG_CHROMEOS) += chromeos.c

verstage-$(CONFIG_CHROMEOS) += chromeos.c diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage_spd_cbfs.c similarity index 100% rename from src/mainboard/google/hatch/romstage.c rename to src/mainboard/google/hatch/romstage_spd_cbfs.c

Attachments:

attachment.htm (text/html — 5.9 KB)

Show replies by date

Kangheui Won (Code Review)

29 Oct 29 Oct

5:22 a.m.

Kangheui Won has uploaded a new patch set (#2) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

mainboard/google: Rework Hatch so that SPD in CBFS is optional

BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a Also confirmed that it boots to kernel on hatch, kohaku and kindred.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/36250/2

Furquan Shaikh (Code Review)

1:56 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 2:

There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068

Edward O'Callaghan (Code Review)

2:05 p.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 2:

...

Patch Set 2:

There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068

From inspection I don't think that is actually workable in its current form. variant boards are assumed to define GPIO straps that wouldn't exist for a smbus romstage type board. Frankly there is too much love of pre-processor going on and it makes a tangled mess for no apparent value. The weak symbols only make it worse by making runtime fragile as well as compile time.

I would strongly suggest we make a split between romstages that are appropriate for each path instead of trying to conflate them and hack around with ifdefs. The former solution is simple and robust.

Furquan Shaikh (Code Review)

2:10 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 61: ROMSTAGE_SPD_CBFS Wouldn't it be easier to add a Kconfig ROMSTAGE_SPD_SMBUS instead of ROMSTAGE_SPD_CBFS. Then this can be set only by boards that obtain SPD using SMBus and you also don't need to compile out the romstage.c file.

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 23: CONFIG_ROMSTAGE_SPD_CBFS I think including this file can be helpful even without SPD being in CBFS. See comments on file.

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_cbfs.c:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 44: mainboard_memory_init_params This function will have to be re-organized a little bit.

1. Call variant_memory_params()

2. Get single channel info using GPIO_MEM_CH_SEL

3. If ROMSTAGE_SPD_SMBUS is set, call memory_init_spd_smbus() which will set read_type to READ_SPD_SMBUS and set the addresses as required.

4. If #3 is not true, call memory_init_spd_cbfs() which will set read_type to READ_SPD_CBFS and get the mem_sku using variant_memory_sku().

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 81: This function can just return early if ROMSTAGE_SPD_SMBUS is set.

Furquan Shaikh (Code Review)

2:18 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 2:

...

Patch Set 2:

...
Patch Set 2:

There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068

From inspection I don't think that is actually workable in its current form. variant boards are assumed to define GPIO straps that wouldn't exist for a smbus romstage type board. Frankly there is too much love of pre-processor going on and it makes a tangled mess for no apparent value. The weak symbols only make it worse by making runtime fragile as well as compile time.

I would strongly suggest we make a split between romstages that are appropriate for each path instead of trying to conflate them and hack around with ifdefs. The former solution is simple and robust.

In order to handle the GPIO straps problem, variant_memory_sku() just needs to move into variants/baseboard/ which provides default GPIO straps. Also, check my latest comment about dealing with read_type differently. In that case, variant_memory_sku() will not even get compiled in and will get dropped by the linker.

You can use runtime "if CONFIG(...)" without having to use #ifdefs. Splitting romstage file is just going to end up duplicating most of the code in that file. Any ways, if that seems cleaner, feel free to go ahead. Please ensure that you add Shelley and Tim on the CLs you are pushing.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Tue, 29 Oct 2019 14:18:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

30 Oct 30 Oct

12:35 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

mainboard/google: Rework Hatch so that SPD in CBFS is optional

BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a

Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc R src/mainboard/google/hatch/romstage_spd_cbfs.c 3 files changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/36250/3

Daniel Kurtz (Code Review)

2:18 a.m.

Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 3:

...

Patch Set 2:

...
Patch Set 2:

...
Patch Set 2:

There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068

From inspection I don't think that is actually workable in its current form. variant boards are assumed to define GPIO straps that wouldn't exist for a smbus romstage type board. Frankly there is too much love of pre-processor going on and it makes a tangled mess for no apparent value. The weak symbols only make it worse by making runtime fragile as well as compile time.

I would strongly suggest we make a split between romstages that are appropriate for each path instead of trying to conflate them and hack around with ifdefs. The former solution is simple and robust.

In order to handle the GPIO straps problem, variant_memory_sku() just needs to move into variants/baseboard/ which provides default GPIO straps. Also, check my latest comment about dealing with read_type differently. In that case, variant_memory_sku() will not even get compiled in and will get dropped by the linker.

You can use runtime "if CONFIG(...)" without having to use #ifdefs. Splitting romstage file is just going to end up duplicating most of the code in that file. Any ways, if that seems cleaner, feel free to go ahead. Please ensure that you add Shelley and Tim on the CLs you are pushing.

I agree with furquan here that we should avoid the code duplication inherent in forking romstage.c. Comparing this CL with CL:36449 shows there is much overlap between the two.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Wed, 30 Oct 2019 02:18:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

2:38 a.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 61: ROMSTAGE_SPD_CBFS

...

Wouldn't it be easier to add a Kconfig ROMSTAGE_SPD_SMBUS instead of ROMSTAGE_SPD_CBFS. […]

+1 to Kconfig

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Wed, 30 Oct 2019 02:38:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

2:59 a.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 3:

...

Patch Set 3:

(1 comment)

This was resolved from patch 2, if you see its now the positive case instead of the anti-positive. That is to say, it defaults to having SPD in CBFS so this patch is a nop for variants Kconfigs as per required.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Wed, 30 Oct 2019 02:59:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

3:02 a.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 2:

...
Patch Set 2:

...
Patch Set 2:

There is also this change to make SPDs optional in CBFS: https://review.coreboot.org/c/coreboot/+/36068

From inspection I don't think that is actually workable in its current form. variant boards are assumed to define GPIO straps that wouldn't exist for a smbus romstage type board. Frankly there is too much love of pre-processor going on and it makes a tangled mess for no apparent value. The weak symbols only make it worse by making runtime fragile as well as compile time.

I would strongly suggest we make a split between romstages that are appropriate for each path instead of trying to conflate them and hack around with ifdefs. The former solution is simple and robust.

In order to handle the GPIO straps problem, variant_memory_sku() just needs to move into variants/baseboard/ which provides default GPIO straps. Also, check my latest comment about dealing with read_type differently. In that case, variant_memory_sku() will not even get compiled in and will get dropped by the linker.

You can use runtime "if CONFIG(...)" without having to use #ifdefs. Splitting romstage file is just going to end up duplicating most of the code in that file. Any ways, if that seems cleaner, feel free to go ahead. Please ensure that you add Shelley and Tim on the CLs you are pushing.

I agree with furquan here that we should avoid the code duplication inherent in forking romstage.c. Comparing this CL with CL:36449 shows there is much overlap between the two.

Are you sure there is overlap? The functions are actually very different.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Wed, 30 Oct 2019 03:02:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

5:56 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

mainboard/google: Rework Hatch so that SPD in CBFS is optional

BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a

Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc R src/mainboard/google/hatch/romstage_spd_cbfs.c 3 files changed, 6 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/36250/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

4 Nov 4 Nov

4:52 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 4: Code-Review+2

Change looks okay to me.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Mon, 04 Nov 2019 04:52:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Aaron Durbin (Code Review)

4:02 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 4: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Mon, 04 Nov 2019 16:02:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

5 Nov 5 Nov

3:17 p.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 61: ROMSTAGE_SPD_CBFS

...

+1 to Kconfig

If this patch is ready, please mark all comments as resolved so it can be merged.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Tue, 05 Nov 2019 15:17:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

6 Nov 6 Nov

12:19 a.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36250 )

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

Patch Set 5:

(4 comments)

...

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 61: ROMSTAGE_SPD_CBFS

...

If this patch is ready, please mark all comments as resolved so it can be merged.

Done

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 23: CONFIG_ROMSTAGE_SPD_CBFS

...

I think including this file can be helpful even without SPD being in CBFS. See comments on file.

Ack

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_cbfs.c:

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 44: mainboard_memory_init_params

...

This function will have to be re-organized a little bit. […]

Ack

https://review.coreboot.org/c/coreboot/+/36250/2/src/mainboard/google/hatch/... PS2, Line 81:

...

This function can just return early if ROMSTAGE_SPD_SMBUS is set.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Wed, 06 Nov 2019 00:19:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

10:15 a.m.

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

Change subject: mainboard/google: Rework Hatch so that SPD in CBFS is optional ......................................................................

mainboard/google: Rework Hatch so that SPD in CBFS is optional

BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a

Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Signed-off-by: Edward O'Callaghan quasisec@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36250 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc R src/mainboard/google/hatch/romstage_spd_cbfs.c 3 files changed, 6 insertions(+), 2 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved

diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 004cc28..219be22 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -58,6 +58,10 @@ int default 512

+config ROMSTAGE_SPD_CBFS + bool + default y + config DRIVER_TPM_SPI_BUS default 0x1

diff --git a/src/mainboard/google/hatch/Makefile.inc b/src/mainboard/google/hatch/Makefile.inc index 01a1eb8..a226bd6 100644 --- a/src/mainboard/google/hatch/Makefile.inc +++ b/src/mainboard/google/hatch/Makefile.inc @@ -20,7 +20,7 @@ ramstage-$(CONFIG_CHROMEOS) += chromeos.c ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC) += ec.c

-romstage-y += romstage.c +romstage-$(CONFIG_ROMSTAGE_SPD_CBFS) += romstage_spd_cbfs.c romstage-$(CONFIG_CHROMEOS) += chromeos.c

verstage-$(CONFIG_CHROMEOS) += chromeos.c @@ -33,4 +33,4 @@ subdirs-y += variants/$(VARIANT_DIR) CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/$(VARIANT_DIR)/include

-subdirs-y += spd +subdirs-$(CONFIG_ROMSTAGE_SPD_CBFS) += spd diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage_spd_cbfs.c similarity index 100% rename from src/mainboard/google/hatch/romstage.c rename to src/mainboard/google/hatch/romstage_spd_cbfs.c

-- To view, visit https://review.coreboot.org/c/coreboot/+/36250 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie1637d08cdd85bc8d7c3b6f2d6f386d0e0c6589b Gerrit-Change-Number: 36250 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Daniel Kurtz djkurtz@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Shelley Chen shchen@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: merged

1790

days inactive

1804

days old

coreboot-gerrit@coreboot.org

16 comments

8 participants

tags (0)

participants (8)

Aaron Durbin (Code Review)
Daniel Kurtz (Code Review)
Edward O'Callaghan (Code Review)
Furquan Shaikh (Code Review)
Kangheui Won (Code Review)
Martin Roth (Code Review)
Patrick Georgi (Code Review)
Tim Wawrzynczak (Code Review)