Change in coreboot[master]: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Edward O'Callaghan (Code Review)

29 Apr 2020 29 Apr '20

1:08 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue

Since https://review.coreboot.org/c/coreboot/+/40558/13 got merged Puff will not be able to boot up and stop as below log:

``` ... SPD: banks 16, ranks 2, rows 16, columns 10, density 8192 Mb SPD: device width 8 bits, bus width 64 bits SPD: module size is 16384 MB (per channel) ASSERTION ERROR: file 'src/soc/intel/cannonlake/cnl_memcfg_init.c', line 47 ```

BUG=b:155220125 BRANCH=none TEST=none

Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/romstage_spd_smbus.c 1 file changed, 15 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/40820/1

diff --git a/src/mainboard/google/hatch/romstage_spd_smbus.c b/src/mainboard/google/hatch/romstage_spd_smbus.c index 4aa37fa..bac5d58 100644 --- a/src/mainboard/google/hatch/romstage_spd_smbus.c +++ b/src/mainboard/google/hatch/romstage_spd_smbus.c @@ -18,15 +18,24 @@

/* Access memory info through SMBUS. */ get_spd_smbus(&blk); - memcfg.spd[0].read_type = READ_SPD_MEMPTR; - memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; - memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; + + if (blk.spd_array[0] == NULL) { + memcfg.spd[0].read_type = NOT_EXISTING; + } else { + memcfg.spd[0].read_type = READ_SPD_MEMPTR; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; + }

memcfg.spd[1].read_type = NOT_EXISTING;

- memcfg.spd[2].read_type = READ_SPD_MEMPTR; - memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; - memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; + if (blk.spd_array[1] == NULL) { + memcfg.spd[2].read_type = NOT_EXISTING; + } else { + memcfg.spd[2].read_type = READ_SPD_MEMPTR; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; + }

memcfg.spd[3].read_type = NOT_EXISTING; dump_spd_info(&blk);

Attachments:

attachment.htm (text/html — 4.8 KB)

Show replies by date

build bot (Jenkins) (Code Review)

29 Apr 29 Apr

1:08 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40820 )

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... PS1, Line 27: memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; line over 96 characters

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... PS1, Line 37: memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; line over 96 characters

Edward O'Callaghan (Code Review)

1:43 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... PS1, Line 27: memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0];

...

line over 96 characters

Ack

https://review.coreboot.org/c/coreboot/+/40820/1/src/mainboard/google/hatch/... PS1, Line 37: memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1];

...

line over 96 characters

Ack

Edward O'Callaghan (Code Review)

1:48 a.m.

Hello build bot (Jenkins), Jamie Chen,

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

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

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue

Since `commit 0ee9b14c09c` the SPD array is set to NULL if no DIMM is present. This causes failure due to an unconditional use of `blk.spd_array[i]`, : i={0,1}.

This validates the spd_array is non-NULL before use otherwise it sets the DIMM as not present.

Puff fails boot with the following log:

BUG=b:155220125 BRANCH=none TEST=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/40820/2

Jamie Chen (Code Review)

1:50 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 04:50:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Sam McNally (Code Review)

1:53 a.m.

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40820 )

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 04:53:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

EricR Lai (Code Review)

1:56 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40820 )

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40820/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:

https://review.coreboot.org/c/coreboot/+/40820/2/src/mainboard/google/hatch/... PS2, Line 15: struct spd_block blk = { this would be good to follow Furquan's patch for TGL. https://review.coreboot.org/c/coreboot/+/39866/15/src/soc/intel/tigerlake/in..., define 4 address and you do with loop for the following things.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 04:56:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

1:59 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 04:59:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Daniel Kurtz (Code Review)

2:27 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Daniel Kurtz djkurtz@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 05:27:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

2:41 a.m.

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

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40820/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:

https://review.coreboot.org/c/coreboot/+/40820/2/src/mainboard/google/hatch/... PS2, Line 15: struct spd_block blk = {

...

this would be good to follow Furquan's patch for TGL. https://review.coreboot. […]

Will consider as a follow up, lets un-break boot for now.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Daniel Kurtz djkurtz@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Comment-Date: Wed, 29 Apr 2020 05:41:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

2:41 a.m.

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40820 )

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue

Since `commit 0ee9b14c09c` the SPD array is set to NULL if no DIMM is present. This causes failure due to an unconditional use of `blk.spd_array[i]`, : i={0,1}.

This validates the spd_array is non-NULL before use otherwise it sets the DIMM as not present.

Puff fails boot with the following log:

BUG=b:155220125 BRANCH=none TEST=none

Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40820 Reviewed-by: Jamie Chen jamie.chen@intel.com Reviewed-by: Sam McNally sammc@google.com Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Daniel Kurtz djkurtz@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/romstage_spd_smbus.c 1 file changed, 15 insertions(+), 6 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Daniel Kurtz: Looks good to me, approved Jamie Chen: Looks good to me, but someone else must approve Sam McNally: Looks good to me, approved

memcfg.spd[1].read_type = NOT_EXISTING;

memcfg.spd[3].read_type = NOT_EXISTING; dump_spd_info(&blk);

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Daniel Kurtz djkurtz@google.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-MessageType: merged

9elements QA (Code Review)

1 May 1 May

8:28 a.m.

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40820 )

Change subject: mb/google/hatch/romstage_spd_smbus.c: Fix missing DIMM issue ......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2846 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2845 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2844 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2843

Please note: This test is under development and might not be accurate at all!

-- To view, visit https://review.coreboot.org/c/coreboot/+/40820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f47c849344951d53fa8c67e779b7c46d632d124 Gerrit-Change-Number: 40820 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Daniel Kurtz djkurtz@google.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Comment-Date: Fri, 01 May 2020 11:28:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1815

days inactive

1817

days old

coreboot-gerrit@coreboot.org

11 comments

8 participants

tags (0)

participants (8)

9elements QA (Code Review)
build bot (Jenkins) (Code Review)
Daniel Kurtz (Code Review)
Edward O'Callaghan (Code Review)
EricR Lai (Code Review)
Furquan Shaikh (Code Review)
Jamie Chen (Code Review)
Sam McNally (Code Review)