Attention is currently required from: Bora Guvendik, Zhixing Ma, Anil Kumar K, Hannah Williams, Cliff Huang, Nico Huber, Michał Żygowski, Jérémy Compostella, Paul Menzel, Angel Pons, Arthur Heymans.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70276 )
Change subject: drivers/intel/gma: Hook up libgfxinit in romstage ......................................................................
Patch Set 13:
(9 comments)
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/70276/comment/cdd39d47_fb1129dc PS13, Line 61: MAINBOARD_HAS_ROMSTAGE_LIBGFXINIT MAINBOARD_HAS_EARLY_LIBGFXINIT
https://review.coreboot.org/c/coreboot/+/70276/comment/437247a8_a48b8f94 PS13, Line 127: choice I'm guessing the best would have been if you could reuse the `choice` belongs to the line#68 but mostly due to RUN_FSP_GOP, you don't like to add `MAINBOARD_USE_EARLY_LIBGFXINIT` config there ?
https://review.coreboot.org/c/coreboot/+/70276/comment/13d523e8_17a068dd PS13, Line 132: ROMSTAGE EARLY
https://review.coreboot.org/c/coreboot/+/70276/comment/382f4f91_71e9cc1b PS13, Line 138: MAINBOARD_USE_ROMSTAGE_LIBGFXINIT MAINBOARD_USE_EARLY_LIBGFXINIT
https://review.coreboot.org/c/coreboot/+/70276/comment/c7610ba3_a6a21625 PS13, Line 147: endchoice do we really need a choice here.
I was thinking what should be the flow here
1. MB user selects `MAINBOARD_HAS_EARLY_LIBGFXINIT` config 2. SoC user selects `HAVE_EARLY_LIBGFXINIT` based on #1. (assuming Brya variants would like to enable MAINBOARD_HAS_EARLY_LIBGFXINIT then HAVE_EARLY_LIBGFXINIT is also selected. 3. Have a config name `EARLY_GFX_INIT` (in this file instead MAINBOARD_USE_ROMSTAGE_LIBGFXINIT) depends on `HAVE_EARLY_LIBGFXINIT`. Assume Brya scenario, EARLY_GFX_INIT=y where else for Nissa, EARLY_GFX_INIT=n 4. HAVE_VGA_TEXT_FRAMEBUFFER config would be enable if !NO_GFX_INIT OR EARLY_GFX_INIT is enabled.
File src/drivers/intel/gma/Kconfig:
https://review.coreboot.org/c/coreboot/+/70276/comment/14f704c3_674d6964 PS13, Line 93: ROMSTAGE I would avoid calling it `ROMSTAGE` instead may be `EARLY`?
https://review.coreboot.org/c/coreboot/+/70276/comment/b5db5020_f6347d60 PS13, Line 95: SOC_INTEL_ALDERLAKE this is no no for sure. in common code, u can't (shouldn't use SoC config, over the year this list will grow and it would be difficult to maintain)
I would have relying on a `EARLY_GFX_INIT` config which would be selected by ADL Kconfig now and further by target mainboard.
https://review.coreboot.org/c/coreboot/+/70276/comment/b0709cb3_e7b4d797 PS13, Line 96: depends on MAINBOARD_USE_ROMSTAGE_LIBGFXINIT u don't need this Kconfig IMO.
https://review.coreboot.org/c/coreboot/+/70276/comment/63117d7a_df693850 PS13, Line 103: Graphic MMIO address we might need little elaborated help text. I'm wondering which memory u r referring here
1. Legacy video area 2. GTTMMADR 3. GMADR
-
Subrata Banik (Code Review)