Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32723
Change subject: vboot: remove OPROM-related code ......................................................................
vboot: remove OPROM-related code
As of CL:1605641, vboot2 code should be used for setting and checking display init state. Remove all vboot1 OPROM-related code, and use the vboot2 display init code which has already been added in previous commits.
coreboot should not be reading vboot NVRAM flags directly. Remove the function vboot_wants_oprom(), and instead rely on display_init_required(), which uses the VBOOT_WD_FLAG_DISPLAY_INIT value stored in vboot_working_data.flags, initialized during verstage. Note that this means in the case of CONFIG_VBOOT=y, the return value of display_init_required() can only be trusted after verstage has been executed. This should not be a problem assuming that all display initialization occurs in ramstage.
BUG=b:124141368, b:124192753, chromium:948529 TEST=Build locally TEST=make clean && make test-abuild BRANCH=none
Change-Id: Ic8f9dc5a3c7f1546a8fed82bde02be4d04568f8d Signed-off-by: Joel Kitching kitching@google.com Cq-Depend: chromium:1605641, chromium:1605525 --- M src/device/pci_device.c M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_handoff.c M src/soc/intel/broadwell/igd.c 5 files changed, 1 insertion(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/32723/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index b5453c7..31c35dc 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -780,9 +780,6 @@ */ should_run = display_init_required();
- if (!should_run && CONFIG(VBOOT)) - should_run = vboot_wants_oprom(); - if (!should_run) printk(BIOS_DEBUG, "Not running VGA Option ROM\n"); return should_run; diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index 78502f7..0c4f33b 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -140,13 +140,6 @@ return vbnv_data(RECOVERY_OFFSET); }
-/* Read the BOOT_DISPLAY_REQUEST flag from VBNV. */ -int vboot_wants_oprom(void) -{ - vbnv_setup(); - return (vbnv_data(BOOT_OFFSET) & BOOT_DISPLAY_REQUEST) ? 1 : 0; -} - /* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { diff --git a/src/security/vboot/vbnv.h b/src/security/vboot/vbnv.h index c8e689f..a2f0b4c 100644 --- a/src/security/vboot/vbnv.h +++ b/src/security/vboot/vbnv.h @@ -25,7 +25,6 @@ void regen_vbnv_crc(uint8_t *vbnv_copy); int get_recovery_mode_from_vbnv(void); void set_recovery_mode_into_vbnv(int recovery_reason); -int vboot_wants_oprom(void);
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void); diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index fccbdfc..8a6b3d6 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -68,13 +68,6 @@ vb_sd->flags |= VBSD_BOOT_DEV_SWITCH_ON; vb_sd->flags |= VBSD_LF_DEV_SWITCH_ON; } - /* TODO(chromium:948529): Remove these two flags after downstream - vboot code longer reads them. */ - if (vboot_wants_oprom() || vb2_sd->recovery_reason || - vb2_sd->flags & VB2_SD_FLAG_DEV_MODE_ENABLED) - vb_sd->flags |= VBSD_OPROM_LOADED; - if (CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) - vb_sd->flags |= VBSD_OPROM_MATTERS;
/* In vboot1, VBSD_FWB_TRIED is * set only if B is booted as explicitly requested. Therefore, if B is diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index 319549d..b9b4281 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -513,7 +513,7 @@ /* Wait for any configured pre-graphics delay */ if (!acpi_is_wakeup_s3()) { #if CONFIG(CHROMEOS) - if (display_init_required() || vboot_wants_oprom()) + if (display_init_required()) mdelay(CONFIG_PRE_GRAPHICS_DELAY); #else mdelay(CONFIG_PRE_GRAPHICS_DELAY);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32723 )
Change subject: vboot: remove OPROM-related code ......................................................................
Patch Set 1: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32723 )
Change subject: vboot: remove OPROM-related code ......................................................................
Patch Set 1: Code-Review+2
Hello Randall Spangler, Patrick Rudolph, Aaron Durbin, Julius Werner, Paul Menzel, build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Simon Glass,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32723
to look at the new patch set (#2).
Change subject: vboot: remove OPROM-related code ......................................................................
vboot: remove OPROM-related code
As of CL:1605641, vboot2 code should be used for setting and checking display init state. Remove all vboot1 OPROM-related code, and use the vboot2 display init code which has already been added in previous commits.
coreboot should not be reading vboot NVRAM flags directly. Remove the function vboot_wants_oprom(), and instead rely on display_init_required(), which uses the VBOOT_WD_FLAG_DISPLAY_INIT value stored in vboot_working_data.flags, initialized during verstage. Note that this means in the case of CONFIG_VBOOT=y, the return value of display_init_required() can only be trusted after verstage has been executed. This should not be a problem assuming that all display initialization occurs in ramstage.
BUG=b:124141368, b:124192753, chromium:948529 TEST=Build locally TEST=make clean && make test-abuild BRANCH=none
Change-Id: Ic8f9dc5a3c7f1546a8fed82bde02be4d04568f8d Signed-off-by: Joel Kitching kitching@google.com Cq-Depend: chromium:1605641, chromium:1605525 --- M src/device/pci_device.c M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_handoff.c M src/soc/intel/broadwell/igd.c 5 files changed, 1 insertion(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/32723/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32723 )
Change subject: vboot: remove OPROM-related code ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32723 )
Change subject: vboot: remove OPROM-related code ......................................................................
vboot: remove OPROM-related code
As of CL:1605641, vboot2 code should be used for setting and checking display init state. Remove all vboot1 OPROM-related code, and use the vboot2 display init code which has already been added in previous commits.
coreboot should not be reading vboot NVRAM flags directly. Remove the function vboot_wants_oprom(), and instead rely on display_init_required(), which uses the VBOOT_WD_FLAG_DISPLAY_INIT value stored in vboot_working_data.flags, initialized during verstage. Note that this means in the case of CONFIG_VBOOT=y, the return value of display_init_required() can only be trusted after verstage has been executed. This should not be a problem assuming that all display initialization occurs in ramstage.
BUG=b:124141368, b:124192753, chromium:948529 TEST=Build locally TEST=make clean && make test-abuild BRANCH=none
Change-Id: Ic8f9dc5a3c7f1546a8fed82bde02be4d04568f8d Signed-off-by: Joel Kitching kitching@google.com Cq-Depend: chromium:1605641, chromium:1605525 Reviewed-on: https://review.coreboot.org/c/coreboot/+/32723 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/pci_device.c M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_handoff.c M src/soc/intel/broadwell/igd.c 5 files changed, 1 insertion(+), 19 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index b5453c7..31c35dc 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -780,9 +780,6 @@ */ should_run = display_init_required();
- if (!should_run && CONFIG(VBOOT)) - should_run = vboot_wants_oprom(); - if (!should_run) printk(BIOS_DEBUG, "Not running VGA Option ROM\n"); return should_run; diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index 78502f7..0c4f33b 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -140,13 +140,6 @@ return vbnv_data(RECOVERY_OFFSET); }
-/* Read the BOOT_DISPLAY_REQUEST flag from VBNV. */ -int vboot_wants_oprom(void) -{ - vbnv_setup(); - return (vbnv_data(BOOT_OFFSET) & BOOT_DISPLAY_REQUEST) ? 1 : 0; -} - /* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { diff --git a/src/security/vboot/vbnv.h b/src/security/vboot/vbnv.h index c8e689f..a2f0b4c 100644 --- a/src/security/vboot/vbnv.h +++ b/src/security/vboot/vbnv.h @@ -25,7 +25,6 @@ void regen_vbnv_crc(uint8_t *vbnv_copy); int get_recovery_mode_from_vbnv(void); void set_recovery_mode_into_vbnv(int recovery_reason); -int vboot_wants_oprom(void);
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void); diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index fccbdfc..8a6b3d6 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -68,13 +68,6 @@ vb_sd->flags |= VBSD_BOOT_DEV_SWITCH_ON; vb_sd->flags |= VBSD_LF_DEV_SWITCH_ON; } - /* TODO(chromium:948529): Remove these two flags after downstream - vboot code longer reads them. */ - if (vboot_wants_oprom() || vb2_sd->recovery_reason || - vb2_sd->flags & VB2_SD_FLAG_DEV_MODE_ENABLED) - vb_sd->flags |= VBSD_OPROM_LOADED; - if (CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) - vb_sd->flags |= VBSD_OPROM_MATTERS;
/* In vboot1, VBSD_FWB_TRIED is * set only if B is booted as explicitly requested. Therefore, if B is diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index 319549d..b9b4281 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -513,7 +513,7 @@ /* Wait for any configured pre-graphics delay */ if (!acpi_is_wakeup_s3()) { #if CONFIG(CHROMEOS) - if (display_init_required() || vboot_wants_oprom()) + if (display_init_required()) mdelay(CONFIG_PRE_GRAPHICS_DELAY); #else mdelay(CONFIG_PRE_GRAPHICS_DELAY);
-
Furquan Shaikh (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)