Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
drivers/fsp1_1/raminit: fix use of mrc_hob
Commit 509f469 [drivers/fsp1_1/raminit.c: Always check FSP HOBs] inadvertently made use of the mrc_hob conditional on CONFIG_DISPLAY_HOBS, when there is no relation between the two, leading to MRC cache data not being used / RAM training being redone on the 3rd boot (and thereafter) following a flash. On some devices, this causes MRC cache corruption and a bricked device.
Fix this by removing the condition on CONFIG_DISPLAY_HOBS.
Test: boot google/{cyan,edgar}, observe third boot and onward do not brick device, properly use mrc_hob via cbmem console and timestamps.
Change-Id: I01f6d1d6dfd10297b30de638301c5e0b6545da9c Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/drivers/intel/fsp1_1/raminit.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34685/1
diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index e71c9a2..21f4ab9 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -259,7 +259,7 @@
/* Locate the memory configuration data to speed up the next reboot */ mrc_hob = get_next_guid_hob(&mrc_guid, hob_list_ptr); - if ((mrc_hob == NULL) && CONFIG(DISPLAY_HOBS)) + if (mrc_hob == NULL) printk(BIOS_DEBUG, "Memory Configuration Data Hob not present\n"); else if (!vboot_recovery_mode_enabled()) {
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34685
to look at the new patch set (#2).
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
drivers/fsp1_1/raminit: fix use of mrc_hob
Commit 509f469 [drivers/fsp1_1/raminit.c: Always check FSP HOBs] inadvertently made use of the mrc_hob conditional on CONFIG_DISPLAY_HOBS, when there is no relation between the two, leading to MRC cache data being corrupted. On some devices this caused RAM training to be redone, on others it resulted in a bricked device.
Fix this by removing the condition on CONFIG_DISPLAY_HOBS.
Test: boot google/{cyan,edgar}, observe third boot and onward do not brick device, properly use mrc_hob via cbmem console and timestamps.
Change-Id: I01f6d1d6dfd10297b30de638301c5e0b6545da9c Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/drivers/intel/fsp1_1/raminit.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34685/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 2: Code-Review+1
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
drivers/fsp1_1/raminit: fix use of mrc_hob
Commit 509f469 [drivers/fsp1_1/raminit.c: Always check FSP HOBs] inadvertently made use of the mrc_hob conditional on CONFIG_DISPLAY_HOBS, when there is no relation between the two, leading to MRC cache data being corrupted. On some devices this caused RAM training to be redone, on others it resulted in a bricked device.
Fix this by removing the condition on CONFIG_DISPLAY_HOBS.
Test: boot google/{cyan,edgar}, observe third boot and onward do not brick device, properly use mrc_hob via cbmem console and timestamps.
Change-Id: I01f6d1d6dfd10297b30de638301c5e0b6545da9c Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34685 Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/raminit.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c index e71c9a2..21f4ab9 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -259,7 +259,7 @@
/* Locate the memory configuration data to speed up the next reboot */ mrc_hob = get_next_guid_hob(&mrc_guid, hob_list_ptr); - if ((mrc_hob == NULL) && CONFIG(DISPLAY_HOBS)) + if (mrc_hob == NULL) printk(BIOS_DEBUG, "Memory Configuration Data Hob not present\n"); else if (!vboot_recovery_mode_enabled()) {
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/raminit.c:
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... PS3, Line 262: if (mrc_hob == NULL) Shouldn't the check for CONFIG(DISPLAY_HOBS) be added within if statement? if (mrc_hob == NULL) if (CONFIG(DISPLAY_HOBSS) printk( ..... ) else if
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/raminit.c:
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... PS3, Line 262: if (mrc_hob == NULL)
Shouldn't the check for CONFIG(DISPLAY_HOBS) be added within if statement? […]
no, this is about the use of the mrc_hob, not the display of its contents
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34685 )
Change subject: drivers/fsp1_1/raminit: fix use of mrc_hob ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/raminit.c:
https://review.coreboot.org/c/coreboot/+/34685/3/src/drivers/intel/fsp1_1/ra... PS3, Line 262: if (mrc_hob == NULL)
no, this is about the use of the mrc_hob, not the display of its contents
IMO this print is related to Hob information (hob present or not) I can live with your patch
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Martin Roth (Code Review)
-
Matt DeVillier (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)