Change in coreboot[master]: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

[WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY

Why does !CHROMEOS path unconditionally delay and why
is it only purism/librem_bdw that has a delay here by
default.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/broadwell/gma.c
1 file changed, 1 insertion(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48787/1

diff --git a/src/soc/intel/broadwell/gma.c b/src/soc/intel/broadwell/gma.c
index d42eebc..69cb153 100644
--- a/src/soc/intel/broadwell/gma.c
+++ b/src/soc/intel/broadwell/gma.c
@@ -512,12 +512,8 @@

/* Wait for any configured pre-graphics delay */
    if (!acpi_is_wakeup_s3()) {
-#if CONFIG(CHROMEOS)
-		if (display_init_required())
+		if (!CONFIG(CHROMEOS) || display_init_required())
    		mdelay(CONFIG_PRE_GRAPHICS_DELAY);
-#else
-		mdelay(CONFIG_PRE_GRAPHICS_DELAY);
-#endif
    }

/* Early init steps */

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 1:

I have no idea why this was added - I think dlaurie did the initial port for the Broadwell-based librem boards. I'm fairly certain the delay isn't needed on either of the librem_bdw boards, so I can test and push a patch to drop

[Matt DeVillier (Code Review)]

Attention is currently required from: Kyösti Mälkki.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 2:

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/2dfdeaf6_881acd43 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
this should probably be something like:
if (!acpi_is_wakeup_s3() && (CONFIG(VGA_ROM_RUN) ||
        (CONFIG(CHROMEOS) && display_init_required()))) {
    mdelay(CONFIG_PRE_GRAPHICS_DELAY);
}

as the delay isn't needed on S3 resume or with libgfxinit, only VBIOS

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Nico Huber, Matt DeVillier, Patrick Rudolph,

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

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

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

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

[WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY

Why does !CHROMEOS path unconditionally delay and why
is it only purism/librem_bdw that has a delay here by
default.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/broadwell/gma.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48787/4

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Matt DeVillier.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 4:

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/865a714a_00cbf227 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> this should probably be something like: […]

I did this for patchset #4 together with dropping CHROMEOS test, there is already VBOOT test in display_init_required. With the OPROM information sounds like the delay needs to move just before run_bios(), going to try it with next pstchset and see what you think.

Can you check if ChromeOS repos have any .configs with PRE_GRAPHICS_DELAY!=0?

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Matt DeVillier.
Hello build bot (Jenkins), Nico Huber, Matt DeVillier, Patrick Rudolph,

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

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

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

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

[WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY

Why does !CHROMEOS path unconditionally delay and why
is it only purism/librem_bdw that has a delay here by
default.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
---
M src/device/Kconfig
M src/device/pci_device.c
M src/northbridge/intel/haswell/Kconfig
M src/northbridge/intel/haswell/gma.c
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/broadwell/gma.c
6 files changed, 19 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48787/5

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Matt DeVillier, Angel Pons, Kyösti Mälkki.
Hello build bot (Jenkins), Nico Huber, Matt DeVillier, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

[WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY

Why does !CHROMEOS path unconditionally delay and why
is it only purism/librem_bdw that has a delay here by
default.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
---
M src/device/Kconfig
M src/device/pci_device.c
M src/northbridge/intel/haswell/Kconfig
M src/northbridge/intel/haswell/gma.c
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/broadwell/gma.c
6 files changed, 19 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48787/6

[Matt DeVillier (Code Review)]

Attention is currently required from: Duncan Laurie, Angel Pons, Kyösti Mälkki.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/4b9be92f_dd7a79f7 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> I did this for patchset #4 together with dropping CHROMEOS test, there is already VBOOT test in disp […]

I'm perfectly ok with dropping the entire hunk here. librem_bdw doesn't need it, and neither do any BDW CrOS boards built from upstream, even when using a VBIOS.

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Matt DeVillier, Duncan Laurie, Angel Pons.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6:

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/67d52290_0e903c78 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> I'm perfectly ok with dropping the entire hunk here. […]

The comments in Kconfig suggest that it was mainly problems with external monitors or TVs why the delay was added (and why it is a visible option). Probably case of DDC/EDID power sequencing when monitor is in one of the deeper suspend/standby modes.

If there is a problem with OPROM not retrying long enough, the default delay should probably be the experimental 50ms for platforms without panels.

[Matt DeVillier (Code Review)]

Attention is currently required from: Duncan Laurie, Angel Pons, Kyösti Mälkki.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6:

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/452a2467_fbdb45b2 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> The comments in Kconfig suggest that it was mainly problems with external monitors or TVs why the de […]

in that case we'd want it conditional on VGA_RUN_ROM, and not always true for the non-VBOOT case

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Matt DeVillier, Duncan Laurie, Angel Pons.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6:

(3 comments)

File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/807958d6_d62d1c2d 
PS6, Line 712: 	should_run = display_init_required();
VBOOT telling to skip run_bios() is detected here.

https://review.coreboot.org/c/coreboot/+/48787/comment/0a6f483e_6717bf92 
PS6, Line 747: 		return;
Delay only with VGA_ROM_RUN.

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/b287f999_be24ab4f 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> in that case we'd want it conditional on VGA_RUN_ROM, and not always true for the non-VBOOT case

Matt, I added comments in patchset #6? It adds the VGA_ROM_RUN conditionality.

[Matt DeVillier (Code Review)]

Attention is currently required from: Duncan Laurie, Angel Pons, Kyösti Mälkki.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6:

(1 comment)

File src/soc/intel/broadwell/gma.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/372ec361_ff4c3d25 
PS2, Line 516: if (!acpi_is_wakeup_s3()) {
             : 		if (!CONFIG(CHROMEOS) || display_init_required())
> Matt, I added comments in patchset #6? It adds the VGA_ROM_RUN conditionality.

sorry, missed that, LGTM

[Angel Pons (Code Review)]

Attention is currently required from: Duncan Laurie, Kyösti Mälkki.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: [WIP] soc/intel/broadwell: Skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 6: Code-Review+1

Change in coreboot[master]: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Duncan Laurie, Kyösti Mälkki.
Hello build bot (Jenkins), Nico Huber, Matt DeVillier, Duncan Laurie, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY
......................................................................

soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

It was commented that the need for the delay was mainly related
to external displays and only with VBIOS execution. Move the
delay such that it is done only when we actually need to execute
the VBIOS aka option rom.

A delay is currently only defined for librem/purism_bdw in
its Kconfig. As the description of the issue sounds like it
would equally happen on other platforms when VBIOS is involved,
promote the Kconfig visible opion to global scope.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
---
M src/device/Kconfig
M src/device/pci_device.c
M src/northbridge/intel/haswell/Kconfig
M src/northbridge/intel/haswell/gma.c
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/broadwell/gma.c
6 files changed, 19 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/48787/7

Change in coreboot[master]: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

[Angel Pons (Code Review)]

Attention is currently required from: Duncan Laurie, Kyösti Mälkki.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48787/comment/f9fdb322_e41d2ed2 
PS7, Line 17: opion
op*t*ion

Change in coreboot[master]: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Duncan Laurie.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY
......................................................................

Patch Set 8:

(3 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48787/comment/95143644_c25cb33c 
PS7, Line 17: opion
> op*t*ion

Done

File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/48787/comment/e116bb57_a0924a60 
PS6, Line 712: 	should_run = display_init_required();
> VBOOT telling to skip run_bios() is detected here.

Ack

https://review.coreboot.org/c/coreboot/+/48787/comment/98558fc3_d12df030 
PS6, Line 747: 		return;
> Delay only with VGA_ROM_RUN.

Ack

Change in coreboot[master]: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48787 )

Change subject: soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY
......................................................................

soc/intel/broadwell: Conditionally skip PRE_GRAPHICS_DELAY

It was commented that the need for the delay was mainly related
to external displays and only with VBIOS execution. Move the
delay such that it is done only when we actually need to execute
the VBIOS aka option rom.

A delay is currently only defined for librem/purism_bdw in
its Kconfig. As the description of the issue sounds like it
would equally happen on other platforms when VBIOS is involved,
promote the Kconfig visible option to global scope.

Change-Id: I4503158576f35057373f003586bbf76af4d59b3d
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48787
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/device/Kconfig
M src/device/pci_device.c
M src/northbridge/intel/haswell/Kconfig
M src/northbridge/intel/haswell/gma.c
M src/soc/intel/broadwell/Kconfig
M src/soc/intel/broadwell/gma.c
6 files changed, 19 insertions(+), 25 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/device/Kconfig b/src/device/Kconfig
index 0e5de45..421ad66 100644
--- a/src/device/Kconfig
+++ b/src/device/Kconfig
@@ -117,6 +117,16 @@

endchoice

+config PRE_GRAPHICS_DELAY
+	int "Graphics initialization delay in ms"
+	default 0
+	depends on VGA_ROM_RUN
+	help
+	  On some systems, coreboot boots so fast that connected monitors
+	  (mostly TVs) won't be able to wake up fast enough to talk to the
+	  VBIOS. On those systems we need to wait for a bit before executing
+	  the VBIOS.
+
 config ONBOARD_VGA_IS_PRIMARY
    bool "Use onboard VGA as primary video device"
    default n
diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 66f5447..cd98f07 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -732,6 +732,12 @@
    return 0;
 }

+static void oprom_pre_graphics_stall(void)
+{
+	if (CONFIG_PRE_GRAPHICS_DELAY)
+		mdelay(CONFIG_PRE_GRAPHICS_DELAY);
+}
+
 /** Default handler: only runs the relevant PCI BIOS. */
 void pci_dev_init(struct device *dev)
 {
@@ -760,6 +766,9 @@
    if (!should_run_oprom(dev, rom))
    	return;

+	/* Wait for any configured pre-graphics delay */
+	oprom_pre_graphics_stall();
+
    run_bios(dev, (unsigned long)ram);

gfx_set_init_done(1);
diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig
index 28b7551..dcc9162 100644
--- a/src/northbridge/intel/haswell/Kconfig
+++ b/src/northbridge/intel/haswell/Kconfig
@@ -90,15 +90,6 @@
      However, it prevents MRC from programming PEG AFE registers,
      which can make PEG devices unstable. When unsure, choose N.

-config PRE_GRAPHICS_DELAY
-	int "Graphics initialization delay in ms"
-	default 0
-	help
-	  On some systems, coreboot boots so fast that connected monitors
-	  (mostly TVs) won't be able to wake up fast enough to talk to the
-	  VBIOS. On those systems we need to wait for a bit before executing
-	  the VBIOS.
-
 # The UEFI System Agent binary needs to be at a fixed offset in the flash
 # and can therefore only reside in the COREBOOT fmap region
 config RO_REGION_ONLY
diff --git a/src/northbridge/intel/haswell/gma.c b/src/northbridge/intel/haswell/gma.c
index 3746fe3..e69cfec 100644
--- a/src/northbridge/intel/haswell/gma.c
+++ b/src/northbridge/intel/haswell/gma.c
@@ -444,7 +444,6 @@

if (!lightup_ok) {
    	printk(BIOS_SPEW, "FUI did not run; using VBIOS\n");
-		mdelay(CONFIG_PRE_GRAPHICS_DELAY);
    	pci_dev_init(dev);
    }

diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig
index 20254d5..4b9af49 100644
--- a/src/soc/intel/broadwell/Kconfig
+++ b/src/soc/intel/broadwell/Kconfig
@@ -141,15 +141,6 @@

endif # HAVE_MRC

-config PRE_GRAPHICS_DELAY
-	int "Graphics initialization delay in ms"
-	default 0
-	help
-	  On some systems, coreboot boots so fast that connected monitors
-	  (mostly TVs) won't be able to wake up fast enough to talk to the
-	  VBIOS. On those systems we need to wait for a bit before executing
-	  the VBIOS.
-
 config INTEL_PCH_UART_CONSOLE
    bool "Use Serial IO UART for console"
    default n
diff --git a/src/soc/intel/broadwell/gma.c b/src/soc/intel/broadwell/gma.c
index b6ce426..59379b5 100644
--- a/src/soc/intel/broadwell/gma.c
+++ b/src/soc/intel/broadwell/gma.c
@@ -513,12 +513,6 @@
    if (!CONFIG(NO_GFX_INIT))
    	pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);

-	/* Wait for any configured pre-graphics delay */
-	if (!acpi_is_wakeup_s3()) {
-		if (!CONFIG(CHROMEOS) || display_init_required())
-			mdelay(CONFIG_PRE_GRAPHICS_DELAY);
-	}
-
    /* Early init steps */
    if (is_broadwell) {
    	reg_script_run_on_dev(dev, broadwell_early_init_script);