Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots (WIP) ......................................................................
mainboard/hatch: Fix puff display on cold boots (WIP)
BUG=b:XXX BRANCH=none TEST=none
Change-Id: I19d40056e58f1737f87fd07d62b07a723a63d610 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 39 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/38475/1
diff --git a/src/mainboard/google/hatch/variants/puff/mainboard.c b/src/mainboard/google/hatch/variants/puff/mainboard.c index 9c2b5fb..cc1dd39 100644 --- a/src/mainboard/google/hatch/variants/puff/mainboard.c +++ b/src/mainboard/google/hatch/variants/puff/mainboard.c @@ -15,9 +15,48 @@
#include <baseboard/variants.h> #include <chip.h> +#include <gpio.h> #include <device/device.h> #include <ec/google/chromeec/ec.h>
+#define GPIO_HDMI_HPD GPP_E13 +#define GPIO_DP_HPD GPP_E14 + +/* TODO: This can be moved to common directory */ +static void wait_for_hpd(gpio_t gpio, long timeout) +{ + struct stopwatch sw; + + printk(BIOS_INFO, "Waiting for HPD\n"); + gpio_input(gpio); + + stopwatch_init_msecs_expire(&sw, timeout); + while (!gpio_get(gpio)) { + if (stopwatch_expired(&sw)) { + printk(BIOS_WARNING, + "HPD not ready after %ldms. Abort.\n", timeout); + return; + } + mdelay(200); + } + printk(BIOS_INFO, "HPD ready after %lu ms\n", + stopwatch_duration_msecs(&sw)); +} + +void variant_ramstage_init(void) +{ + static const long display_timeout_ms = 3000; + + /* This is reconfigured back to whatever FSP-S expects by + gpio_configure_pads. */ + gpio_input(GPIO_HDMI_HPD); + if (display_init_required() && !gpio_get(GPIO_HDMI_HPD)) { + /* This has to be done before FSP-S runs. */ + if (google_chromeec_wait_for_displayport(display_timeout_ms)) + wait_for_hpd(GPIO_DP_HPD, display_timeout_ms); + } +} + /* * For type-C chargers, set PL2 to 90% of max power to account for * cable loss and FET Rdson loss in the path from the source.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
Patch Set 2:
This change is ready for review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG@9 PS2, Line 9: beind behind
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG@29 PS2, Line 29: AP does not wait (and firmware screen is displayed on HDMI monitor). What monitors did you test with?
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 42: mdelay(200); 200 ms seems too long for fast boot experience, doesn’t it? 10 ms or 20 ms?
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 45: stopwatch_duration_msecs(&sw)); Should fit on one line.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
Patch Set 2:
(4 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 33: gpio_input(gpio); This would be unnecessary with the suggestion below.
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 42: mdelay(200);
200 ms seems too long for fast boot experience, doesn’t it? 10 ms or 20 ms?
From testing, this tends to complete after 800-1200ms.
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 54: gpio_input(GPIO_HDMI_HPD); gpio_input(GPIO_DP_HPD);
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 55: ) && !gpio_get(GPIO_DP_HPD)
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
Patch Set 2:
(7 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG@9 PS2, Line 9: beind
behind
Ack
https://review.coreboot.org/c/coreboot/+/38475/2//COMMIT_MSG@29 PS2, Line 29: AP does not wait (and firmware screen is displayed on HDMI monitor).
What monitors did you test with?
Ack
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 33: gpio_input(gpio);
This would be unnecessary with the suggestion below.
Done
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 42: mdelay(200);
200 ms seems too long for fast boot experience, doesn’t it? 10 ms or 20 ms?
Are you sure you understand the code?
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 45: stopwatch_duration_msecs(&sw));
Should fit on one line.
Ack
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 54: gpio_input(GPIO_HDMI_HPD);
gpio_input(GPIO_DP_HPD);
Done
https://review.coreboot.org/c/coreboot/+/38475/2/src/mainboard/google/hatch/... PS2, Line 55: )
&& !gpio_get(GPIO_DP_HPD)
Done
Hello build bot (Jenkins), Daniel Kurtz,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38475
to look at the new patch set (#3).
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
mainboard/hatch: Fix puff display on cold boots
Similar to that of b:72387533 however our DP&HDMI are beind a MST.
Some Type-C monitors do not immediately assert HPD. If we continue to boot without HPD asserted, Depthcharge fails to show pictures on a monitor even if HPD is asserted later.
Also, if an HDMI monitor is connected, no wait is needed. If only an HDMI monitor is connected, currently the API always loops until the stopwatch expires.
This patch will make the AP skip DisplayPort wait loop if it detects an HDMI monitor. And if an HDMI monitor is not detected, the AP will wait for DisplayPort mode (like before) but also its HPD signal.
This patch also extends the wait loop time-out to 3 seconds.
BUG=b:147992492 BRANCH=none TEST=Verify firmware screen is displayed even when a type-c monitor does not immediately assert HPD. Verify if HDMI monitor is connected, AP does not wait (and firmware screen is displayed on HDMI monitor).
Change-Id: I19d40056e58f1737f87fd07d62b07a723a63d610 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/38475/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff display on cold boots ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 56: && !gpio_get(GPIO_DP_HPD )) { space prohibited before that close parenthesis ')'
Hello build bot (Jenkins), Daniel Kurtz,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38475
to look at the new patch set (#4).
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
mainboard/hatch: Fix puff DP output on cold boots
Wait for HPD DP unless HDMI is plugged.
Some Type-C monitors do not immediately assert HPD. If we continue to boot without HPD asserted, Depthcharge fails to show pictures on a monitor even if HPD is asserted later.
Similar to that of b:72387533 however our DP&HDMI are beind a MST. See commit d182b63347c744c on how this was done for mainboard/fizz.
BUG=b:147992492 BRANCH=none TEST=Verify firmware screen is displayed even when a type-c monitor does not immediately assert HPD. Verify if HDMI monitor is connected, AP does not wait (and firmware screen is displayed on HDMI monitor).
Change-Id: I19d40056e58f1737f87fd07d62b07a723a63d610 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/38475/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 56: && !gpio_get(GPIO_DP_HPD )) {
space prohibited before that close parenthesis ')'
Done
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 4:
Adding in Daisuke who authored the referenced patch.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 27: /* TODO: This can be moved to common directory */ Let's do the refactor first, then extend for puff. It will make it easier to spot subtle differences in behavior as pointed out below.
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 56: && !gpio_get(GPIO_DP_HPD ) This subtly changes behavior w/rt the original fizz implementation by checking HPD before timing out the regular DP ready check. Are we sure we want to do that?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 27: /* TODO: This can be moved to common directory */
Let's do the refactor first, then extend for puff. […]
I don't think we want to do that here as we have a fundamentally different display topology than that of Fizz. The link in the commit msg is for background on where this code was inspired from however differs in practical terms.
https://review.coreboot.org/c/coreboot/+/38475/3/src/mainboard/google/hatch/... PS3, Line 56: && !gpio_get(GPIO_DP_HPD )
This subtly changes behavior w/rt the original fizz implementation by checking HPD before timing out […]
Yes.
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38475/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/4/src/mainboard/google/hatch/... PS4, Line 28: wait_for_hpd nit: This is generic except messages. It's nice to be renamed (e.g. wait_for_gpio) and placed in a common file (gpio.c). Messages can be printed by the callers, differentiated by the return value.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38475/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38475/4/src/mainboard/google/hatch/... PS4, Line 28: wait_for_hpd
nit: This is generic except messages. It's nice to be renamed (e.g. […]
I'll consolidate the code paths in a follow up that cleans up both boards with a generic function. Essentially fixing the TODO as a separate item to avoid convolving changes to a unrelated board with a specific puff issue being fixed.
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
mainboard/hatch: Fix puff DP output on cold boots
Wait for HPD DP unless HDMI is plugged.
Some Type-C monitors do not immediately assert HPD. If we continue to boot without HPD asserted, Depthcharge fails to show pictures on a monitor even if HPD is asserted later.
Similar to that of b:72387533 however our DP&HDMI are beind a MST. See commit d182b63347c744c on how this was done for mainboard/fizz.
BUG=b:147992492 BRANCH=none TEST=Verify firmware screen is displayed even when a type-c monitor does not immediately assert HPD. Verify if HDMI monitor is connected, AP does not wait (and firmware screen is displayed on HDMI monitor).
Change-Id: I19d40056e58f1737f87fd07d62b07a723a63d610 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38475 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Daisuke Nojiri dnojiri@chromium.org --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 42 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Daisuke Nojiri: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/puff/mainboard.c b/src/mainboard/google/hatch/variants/puff/mainboard.c index 9c2b5fb..7354ce9 100644 --- a/src/mainboard/google/hatch/variants/puff/mainboard.c +++ b/src/mainboard/google/hatch/variants/puff/mainboard.c @@ -15,8 +15,50 @@
#include <baseboard/variants.h> #include <chip.h> +#include <delay.h> #include <device/device.h> #include <ec/google/chromeec/ec.h> +#include <gpio.h> +#include <timer.h> + +#define GPIO_HDMI_HPD GPP_E13 +#define GPIO_DP_HPD GPP_E14 + +/* TODO: This can be moved to common directory */ +static void wait_for_hpd(gpio_t gpio, long timeout) +{ + struct stopwatch sw; + + printk(BIOS_INFO, "Waiting for HPD\n"); + stopwatch_init_msecs_expire(&sw, timeout); + while (!gpio_get(gpio)) { + if (stopwatch_expired(&sw)) { + printk(BIOS_WARNING, + "HPD not ready after %ldms. Abort.\n", timeout); + return; + } + mdelay(200); + } + printk(BIOS_INFO, "HPD ready after %lu ms\n", + stopwatch_duration_msecs(&sw)); +} + +void variant_ramstage_init(void) +{ + static const long display_timeout_ms = 3000; + + /* This is reconfigured back to whatever FSP-S expects by + gpio_configure_pads. */ + gpio_input(GPIO_HDMI_HPD); + gpio_input(GPIO_DP_HPD); + if (display_init_required() + && !gpio_get(GPIO_HDMI_HPD) + && !gpio_get(GPIO_DP_HPD)) { + /* This has to be done before FSP-S runs. */ + if (google_chromeec_wait_for_displayport(display_timeout_ms)) + wait_for_hpd(GPIO_DP_HPD, display_timeout_ms); + } +}
/* * For type-C chargers, set PL2 to 90% of max power to account for
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/484 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/483 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/482
Please note: This test is under development and might not be accurate at all!
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38475 )
Change subject: mainboard/hatch: Fix puff DP output on cold boots ......................................................................
Patch Set 5:
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38740
-
9elements QA (Code Review) -
build bot (Jenkins) (Code Review)
-
Daisuke Nojiri (Code Review)
-
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)
-
Paul Menzel (Code Review)
-
Sam McNally (Code Review)