Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
soc/intel/apollolake: Fix FSP/GOP display init
Configuring the panel power and backlight for an internal display when using FSP/GOP display init causes FSP to turn the backlight off. Fix this by skipping panel init when FSP/GOP display init is used.
Test: build/boot google/ampton with FSP/GOP display init selected, verify backlight enabled and payload display visible
Change-Id: Ifdc3591b714205e864cd1d0db0897d43bd6616a2 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/apollolake/graphics.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/48858/1
diff --git a/src/soc/intel/apollolake/graphics.c b/src/soc/intel/apollolake/graphics.c index 24d4772..24a0620 100644 --- a/src/soc/intel/apollolake/graphics.c +++ b/src/soc/intel/apollolake/graphics.c @@ -62,6 +62,9 @@ if (!conf) return;
+ if (CONFIG(RUN_FSP_GOP)) + return; + mmio_res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!mmio_res || !mmio_res->base) return;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... File src/soc/intel/apollolake/graphics.c:
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... PS1, Line 65: if (CONFIG(RUN_FSP_GOP)) : return; I wonder if this should be done as part of `gma_init` in common/block/graphics/graphics.c as it is applicable to all SoCs and we will keep running into the same problem for platforms selecting RUN_FSP_GOP.
Currently, skylake also has this implementation for graphics_soc_init() and I suspect that it might be broken too.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... File src/soc/intel/apollolake/graphics.c:
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... PS1, Line 65: if (CONFIG(RUN_FSP_GOP)) : return;
I wonder if this should be done as part of `gma_init` in common/block/graphics/graphics. […]
I don't recall this being an issue on SKL/KBL, but I can re-test there. It's definitely an issue on CML as well
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... File src/soc/intel/apollolake/graphics.c:
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... PS1, Line 65: if (CONFIG(RUN_FSP_GOP)) : return;
I don't recall this being an issue on SKL/KBL, but I can re-test there. […]
We (Michael and myself) had an enlightening moment last night. The problem just seems to be about the order. FSP/GOP runs very early in ramstage, completely out of order with the usual device initialization. What happens here is that this code does not read-modify-write some control registers but assumes the hardware is still disabled and hence we could just write 0 to most settings (including the bits that are already set by FSP to enable the display). IOW, after this code ran, the display is disabled, and in the GOP case nothing would enable it later.
What we need is a RMW for PCH_PP_CONTROL and BXT_BLW_PWM_CTL. I'll try to test things on APL later.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... File src/soc/intel/apollolake/graphics.c:
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... PS1, Line 65: if (CONFIG(RUN_FSP_GOP)) : return;
We (Michael and myself) had an enlightening moment last night. The […]
SKL/KBL are most likely not affected because there we don't write to any control registers.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... File src/soc/intel/apollolake/graphics.c:
https://review.coreboot.org/c/coreboot/+/48858/1/src/soc/intel/apollolake/gr... PS1, Line 65: if (CONFIG(RUN_FSP_GOP)) : return;
What we need is a RMW for PCH_PP_CONTROL and BXT_BLW_PWM_CTL. I'll
try to test things on APL later.
I forgot how much work it is to integrate APL coreboot into the IFWI. Maybe next year.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1: Code-Review+2
Maybe bailing out is the best idea after all. If somebody cares about correct configuration when FSP/GOP is used, they can have a deeper look into it (we just don't know yet which registers to configure before/after the GOP runs).
Maybe leave a comment about that? e.g.
/* In case of RUN_FSP_GOP, the graphics hardware might be enabled already. Proper configuration around FSP/GOP needs further investigation. */ if (CONFIG(RUN_FSP_GOP)) return;
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1: Code-Review+2
Maybe bailing out is the best idea after all. If somebody cares about correct configuration when FSP/GOP is used, they can have a deeper look into it (we just don't know yet which registers to configure before/after the GOP runs).
Maybe leave a comment about that? e.g.
/* In case of RUN_FSP_GOP, the graphics hardware might be enabled already. Proper configuration around FSP/GOP needs further investigation. */ if (CONFIG(RUN_FSP_GOP)) return;
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG@8 PS1, Line 8: : Configuring the panel power and backlight for an internal display : when using FSP/GOP display init causes FSP to turn the backlight off this should be fixed by correcting register programming (it was overridden and panel set to off. see CB:48881
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patch Set 1:
(1 comment)
Patch Set 1: Code-Review+2
Maybe bailing out is the best idea after all. If somebody cares about correct configuration when FSP/GOP is used, they can have a deeper look into it (we just don't know yet which registers to configure before/after the GOP runs).
Maybe leave a comment about that? e.g.
/* In case of RUN_FSP_GOP, the graphics hardware might be enabled already. Proper configuration around FSP/GOP needs further investigation. */ if (CONFIG(RUN_FSP_GOP)) return;
As discussed on IRC:
We do not know enough about GOP (missing source) and programming settings after the panel was enabled is not the right way. Thus, ack for this patch. We should bail out on all platforms.
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG@8 PS1, Line 8: : Configuring the panel power and backlight for an internal display : when using FSP/GOP display init causes FSP to turn the backlight off
this should be fixed by correcting register programming (it was overridden and panel set to off. […]
As discussed on IRC:
We do not know enough about GOP (missing source) and programming settings after the panel was enabled is not the right way. Thus, ack for this patch. We should bail out on all platforms.
CB:48881 will be abandoned for the same reason
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48858/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/apollolake: Fix FSP/GOP display init maybe add some details of our discussion, why we don't try to program the settings when gop is used and call it "Skip panel settings programming"?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1: Code-Review-1
superseded by fix in common/block/graphics/graphics.c
Michael Niewöhner has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Removed Code-Review+2 by Michael Niewöhner foss@mniewoehner.de
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
superseded by fix in common/block/graphics/graphics.c
maybe abandon it?
Matt DeVillier has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48858 )
Change subject: soc/intel/apollolake: Fix FSP/GOP display init ......................................................................
Abandoned
superseded by CB:48884
-
Furquan Shaikh (Code Review) -
Matt DeVillier (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)