[coreboot] Re: [RFC] VGA Text mode in romstage

[Arthur Heymans]

Hi

It's surprising this works before raminit, nice find!
I'm not so convinced it's an improvement for enduser friendliness.
Is waiting at a screen that tells you raminit is being done better than
waiting at a black screen?
It might just be more confusing? But I'm not an enduser so I can't really
say.

About the implementation details:

PCI resource allocation
> =======================
>
> <
> https://review.coreboot.org/c/coreboot/+/69757/2/src/soc/intel/alderlake/rom...
> >
>
> We need to configure the graphics PCI device to perform graphic
> initialization. It requires to perform an early romstage PCI resource
> allocation. For the proof-of-concept, we assigned the same resources
> than the coreboot resource allocator would assign during ramstage. We
> cannot leave hardcoded values like these. Any suggestion on how to
> cleanly handle early PCI resources allocation ?

Hardcode it with a Kconfig constant is good enough I think.

<
> https://review.coreboot.org/c/coreboot/+/69757/2/src/drivers/intel/fsp2_0/Kc...
> >
>
> https://review.coreboot.org/c/coreboot/+/69757/2/src/lib/Kconfig
> https://review.coreboot.org/c/coreboot/+/69757/2/src/device/Kconfig
>
> We had to "duplicate" some configuration flags such as
> `MAINBOARD_USE_LIBGFXINIT' as a `ROMSTAGE_MAINBOARD_USE_LIBGFXINIT' for
> instance. We want to separate configuration between romstage and
> ramstage to offer flexibility to use to have VGA text mode in romstage
> without any graphics initialization in ramstage or a graphic
> initialization with the GOP driver in ramstage ...
>
> We would appreciate some review and feedback on our clearly not perfect
> `Kconfig' changes. Any suggestion is welcome.
>
> I'd would have the option to run libgfxinit in romstage at the soc level

but still depend on MAINBOARD_USE_LIBGFXINIT.

libgfxinit and global initialized variables
> ===========================================
>
> https://review.coreboot.org/c/coreboot/+/69757/1/src/arch/x86/car.ld
>
> `libgfxinit' has some initialized global variables ending up in the
> `.data' section.
>
> Global *initialized* variables are not permitted in cache-as-ram and
> with the current `car.ld' linker script I hit compilation link errors
> due the following list of variables. Using objdump I collected their
> initial value (note that I did not exhaustively checked all the
> `[...]_E' ones but only a sample of them).
>
> Address   Type  Name                                            Value
>
> ffffff60  d     `hw__debug__register_write_delay_nanoseconds'   00
>
> ffffff68  d     `hw__debug__start_of_line'                      01
>
> ffffff2c  d     `hw__gfx__gma__registers__gtt_32__range_aXnnn'  00 20 00
> 00
>  ffffff28  d     `hw__gfx__gma__registers__gtt_64__range_aXnnn'  00 80 00
> 00
>  ffffff30  d     `hw__gfx__gma__registers__regs__range_aXnnn'    00 00 00
> 00
>  ffffff6e  D     `gma_E'                                         00 ...
>
> ffffff70  D     `gma__gfx_init_E'                               00 ...
>
> ffffff6a  D     `hw__debug_E'                                   00 ...
>
> ffffff6c  D     `hw__debug_sink_E'                              00 ...
>
> ffffff02  D     `hw__gfx__dp_aux_ch_E'                          00 ...
>
> ffffff04  D     `hw__gfx__dp_dual_mode_E'                       00 ...
>
> ffffff06  D     `hw__gfx__dp_info_E'                            00 ...
>
> ffffff08  D     `hw__gfx__dp_training_E'                        00 ...
>
> ffffff0a  D     `hw__gfx__edid_E'                               00 ...
>
> ffffff0c  D     `hw__gfx__framebuffer_filler_E'                 00 ...
>
> ffffff3a  D     `hw__gfx__gma_E'                                00 ...
>
> ffffff3c  D     `hw__gfx__gma__combo_phy_E'                     00 ...
>
> ffffff00  D     `hw__gfx__gma__config_E'                        00 ...
>
> ffffff0e  D     `hw__gfx__gma__config_helpers_E'                00 ...
>
> ffffff10  D     `hw__gfx__gma__connector_info_E'                00 ...
>
> ffffff42  D     `hw__gfx__gma__connectors_E'                    00 ...
>
> ffffff3e  D     `hw__gfx__gma__connectors__combo_phy_E'         00 ...
>
> ffffff40  D     `hw__gfx__gma__connectors__tc_E'                00 ...
>
> ffffff12  D     `hw__gfx__gma__display_probing_E'               00 ...
>
> ffffff14  D     `hw__gfx__gma__dp_aux_request_E'                00 ...
>
> ffffff16  D     `hw__gfx__gma__i2c_E'                           00 ...
>
> ffffff38  D     `hw__gfx__gma__initialized'                     00 ...
>
> ffffff18  D     `hw__gfx__gma__panel_E'                         00 ...
>
> ffffff1a  D     `hw__gfx__gma__pch__fdi_E'                      00 ...
>
> ffffff1c  D     `hw__gfx__gma__pch__sideband_E'                 00 ...
>
> ffffff1e  D     `hw__gfx__gma__pch__transcoder_E'               00 ...
>
> ffffff20  D     `hw__gfx__gma__pch__vga_E'                      00 ...
>
> ffffff22  D     `hw__gfx__gma__pcode_E'                         00 ...
>
> ffffff24  D     `hw__gfx__gma__pipe_setup_E'                    00 ...
>
> ffffff48  D     `hw__gfx__gma__plls_E'                          00 ...
>
> ffffff44  D     `hw__gfx__gma__plls__combo_phy_E'               00 ...
>
> ffffff46  D     `hw__gfx__gma__plls__dkl_E'                     00 ...
>
> ffffff4a  D     `hw__gfx__gma__port_detect_E'                   00 ...
>
> ffffff50  D     `hw__gfx__gma__power_and_clocks_E'              00 ...
>
> ffffff4c  D     `hw__gfx__gma__power_and_clocks__tgl_E'         00 ...
>
> ffffff4e  D     `hw__gfx__gma__power_and_clocks__xelpd_E'       00 ...
>
> ffffff34  D     `hw__gfx__gma__registers_E'                     00 ...
>
> ffffff36  D     `hw__gfx__gma__transcoder_E'                    00 ...
>
> ffffff52  D     `hw__mmio_range_E'                              00 ...
>
> ffffff56  D     `hw__mmio_regs_E'                               00 ...
>
> ffffff54  D     `hw__pci__dev_E'                                00 ...
>
> ffffff58  D     `hw__pci__mmconf_E'                             00 ...
>
> ffffff5a  D     `hw__port_io_E'                                 00 ...
>
> ffffff5c  D     `hw__time_E'                                    00 ...
>
> ffffff72  D     `hw__time__timer_E'                             00 ...
>
> The symbols ending with `_E' are empty and initialized by GNAT generated
> code (cf. `___elabs' functions) which is called using the
> `$stage_adainit()' function. Therefor, I think it is safe to places
> these into the `_bss' region of the `.car.data' section. What I don't
> understand though is why isn't the GNAT compiler placing them into the
> `.bss' region of the object file. Any idea ? I could not find any GNAT
> compiler option for this but I am clearly not an ADA compiler expert
> :-)

If you are going to have the linker use .data in .bss try to only do that
for those symbols and not the whole data section as the illegal globals
check is a good one to keep.
> Now we are left with the following symbols:
> `hw__debug__register_write_delay_nanoseconds' is zero so installing it
> in the `_bss' region of the `.car.data' section is "technically" fine.
> `hw__debug__start_of_line': Reverting the logic of this boolean makes it
>
> work
> The following three symbols are the most challenging ones:
> `hw__gfx__gma__registers__gtt_32__range_aXnnn'
> `hw__gfx__gma__registers__gtt_64__range_aXnnn'
> `hw__gfx__gma__registers__regs__range_aXnnn'
> They seem to be entirely by the compiler and I don't know how to
> control them. Surprisingly, putting them in the `_bss' region of the
> `.car.data' section which is making them zeros is not causing any
> apparent issue while their initial value should not be zeros. I am
> thinking that these could be used for code verification purposes
> only. Does anyone knows before I adventure in to assembly code to
> uncover their role ?

One trick you could use is to put a backup of the .data section in .rodata
with objcopy and copy it back at runtime very early in the stage.
This would make it possible to have a .data section on CAR stages in
general.

Kind regards

Arthur

On Tue, Nov 22, 2022 at 3:52 AM Compostella, Jeremy <
[e-mail]> wrote:
> Dear coreboot developers,
>
> In order to improve the user experience we would like to display a
> message on the screen while memory training is in progress. As a first
> step, we are working on Alder Lake and Raptor Lake platforms and are
> leveraging the `libgfxinit' and VGA text mode. We have libgfxinit
> patches for Tiger Lake, Alder Lake (and Raptor Lake) support under
> review but this is not the object of this email (see
> https://review.coreboot.org/c/libgfxinit/+/65087 if you are
> interested)
>
> In the last few weeks, we have been working on a proof-of-concept to
> ensure that we could overcome all the technical hardware challenges to
> initialize VGA text mode and setup the display before we jump into FSP
> memory.
>
> Even though we are still working on some stability issues, we have a
> "working" but hacky patch :-). But we have an aggressive timeline and
> it works good enough that we believe it is time to start an open
> discussion as the surface of the patch is important (linker scripts,
> configuration flags logic ...). The changes have been reduced to a
> unique patch after a lot of clean-up
> (https://review.coreboot.org/c/coreboot/+/69757).  This patch will be
> split up in a couple of patches once we are done with some in progress
> development.
>
> We identified three majors "issues" for which we are seeking feedback,
> guidance or help.
> PCI resource allocation
>
> <
> https://review.coreboot.org/c/coreboot/+/69757/2/src/soc/intel/alderlake/rom...
> >
>
> We need to configure the graphics PCI device to perform graphic
> initialization. It requires to perform an early romstage PCI resource
> allocation. For the proof-of-concept, we assigned the same resources
> than the coreboot resource allocator would assign during ramstage. We
> cannot leave hardcoded values like these. Any suggestion on how to
> cleanly handle early PCI resources allocation ?
> Configuration flags and "duplicated"
>
> <
> https://review.coreboot.org/c/coreboot/+/69757/2/src/drivers/intel/fsp2_0/Kc...
> >
>
> https://review.coreboot.org/c/coreboot/+/69757/2/src/lib/Kconfig
> https://review.coreboot.org/c/coreboot/+/69757/2/src/device/Kconfig
>
> We had to "duplicate" some configuration flags such as
> `MAINBOARD_USE_LIBGFXINIT' as a `ROMSTAGE_MAINBOARD_USE_LIBGFXINIT' for
> instance. We want to separate configuration between romstage and
> ramstage to offer flexibility to use to have VGA text mode in romstage
> without any graphics initialization in ramstage or a graphic
> initialization with the GOP driver in ramstage ...
>
> We would appreciate some review and feedback on our clearly not perfect
> `Kconfig' changes. Any suggestion is welcome.
> libgfxinit and global initialized variables
>
> https://review.coreboot.org/c/coreboot/+/69757/1/src/arch/x86/car.ld
>
> `libgfxinit' has some initialized global variables ending up in the
> `.data' section.
>
> Global *initialized* variables are not permitted in cache-as-ram and
> with the current `car.ld' linker script I hit compilation link errors
> due the following list of variables. Using objdump I collected their
> initial value (note that I did not exhaustively checked all the
> `[...]_E' ones but only a sample of them).
>
> Address   Type  Name                                            Value
>
> ffffff60  d     `hw__debug__register_write_delay_nanoseconds'   00
>
> ffffff68  d     `hw__debug__start_of_line'                      01
>
> ffffff2c  d     `hw__gfx__gma__registers__gtt_32__range_aXnnn'  00 20 00
> 00
>  ffffff28  d     `hw__gfx__gma__registers__gtt_64__range_aXnnn'  00 80 00
> 00
>  ffffff30  d     `hw__gfx__gma__registers__regs__range_aXnnn'    00 00 00
> 00
>  ffffff6e  D     `gma_E'                                         00 ...
>
> ffffff70  D     `gma__gfx_init_E'                               00 ...
>
> ffffff6a  D     `hw__debug_E'                                   00 ...
>
> ffffff6c  D     `hw__debug_sink_E'                              00 ...
>
> ffffff02  D     `hw__gfx__dp_aux_ch_E'                          00 ...
>
> ffffff04  D     `hw__gfx__dp_dual_mode_E'                       00 ...
>
> ffffff06  D     `hw__gfx__dp_info_E'                            00 ...
>
> ffffff08  D     `hw__gfx__dp_training_E'                        00 ...
>
> ffffff0a  D     `hw__gfx__edid_E'                               00 ...
>
> ffffff0c  D     `hw__gfx__framebuffer_filler_E'                 00 ...
>
> ffffff3a  D     `hw__gfx__gma_E'                                00 ...
>
> ffffff3c  D     `hw__gfx__gma__combo_phy_E'                     00 ...
>
> ffffff00  D     `hw__gfx__gma__config_E'                        00 ...
>
> ffffff0e  D     `hw__gfx__gma__config_helpers_E'                00 ...
>
> ffffff10  D     `hw__gfx__gma__connector_info_E'                00 ...
>
> ffffff42  D     `hw__gfx__gma__connectors_E'                    00 ...
>
> ffffff3e  D     `hw__gfx__gma__connectors__combo_phy_E'         00 ...
>
> ffffff40  D     `hw__gfx__gma__connectors__tc_E'                00 ...
>
> ffffff12  D     `hw__gfx__gma__display_probing_E'               00 ...
>
> ffffff14  D     `hw__gfx__gma__dp_aux_request_E'                00 ...
>
> ffffff16  D     `hw__gfx__gma__i2c_E'                           00 ...
>
> ffffff38  D     `hw__gfx__gma__initialized'                     00 ...
>
> ffffff18  D     `hw__gfx__gma__panel_E'                         00 ...
>
> ffffff1a  D     `hw__gfx__gma__pch__fdi_E'                      00 ...
>
> ffffff1c  D     `hw__gfx__gma__pch__sideband_E'                 00 ...
>
> ffffff1e  D     `hw__gfx__gma__pch__transcoder_E'               00 ...
>
> ffffff20  D     `hw__gfx__gma__pch__vga_E'                      00 ...
>
> ffffff22  D     `hw__gfx__gma__pcode_E'                         00 ...
>
> ffffff24  D     `hw__gfx__gma__pipe_setup_E'                    00 ...
>
> ffffff48  D     `hw__gfx__gma__plls_E'                          00 ...
>
> ffffff44  D     `hw__gfx__gma__plls__combo_phy_E'               00 ...
>
> ffffff46  D     `hw__gfx__gma__plls__dkl_E'                     00 ...
>
> ffffff4a  D     `hw__gfx__gma__port_detect_E'                   00 ...
>
> ffffff50  D     `hw__gfx__gma__power_and_clocks_E'              00 ...
>
> ffffff4c  D     `hw__gfx__gma__power_and_clocks__tgl_E'         00 ...
>
> ffffff4e  D     `hw__gfx__gma__power_and_clocks__xelpd_E'       00 ...
>
> ffffff34  D     `hw__gfx__gma__registers_E'                     00 ...
>
> ffffff36  D     `hw__gfx__gma__transcoder_E'                    00 ...
>
> ffffff52  D     `hw__mmio_range_E'                              00 ...
>
> ffffff56  D     `hw__mmio_regs_E'                               00 ...
>
> ffffff54  D     `hw__pci__dev_E'                                00 ...
>
> ffffff58  D     `hw__pci__mmconf_E'                             00 ...
>
> ffffff5a  D     `hw__port_io_E'                                 00 ...
>
> ffffff5c  D     `hw__time_E'                                    00 ...
>
> ffffff72  D     `hw__time__timer_E'                             00 ...
>
> The symbols ending with `_E' are empty and initialized by GNAT generated
> code (cf. `___elabs' functions) which is called using the
> `$stage_adainit()' function. Therefor, I think it is safe to places
> these into the `_bss' region of the `.car.data' section. What I don't
> understand though is why isn't the GNAT compiler placing them into the
> `.bss' region of the object file. Any idea ? I could not find any GNAT
> compiler option for this but I am clearly not an ADA compiler expert
> :-)
>
> Now we are left with the following symbols:
> `hw__debug__register_write_delay_nanoseconds' is zero so installing it
> in the `_bss' region of the `.car.data' section is "technically" fine.
> `hw__debug__start_of_line': Reverting the logic of this boolean makes it
>
> work
> The following three symbols are the most challenging ones:
> `hw__gfx__gma__registers__gtt_32__range_aXnnn'
> `hw__gfx__gma__registers__gtt_64__range_aXnnn'
> `hw__gfx__gma__registers__regs__range_aXnnn'
> They seem to be entirely by the compiler and I don't know how to
> control them. Surprisingly, putting them in the `_bss' region of the
> `.car.data' section which is making them zeros is not causing any
> apparent issue while their initial value should not be zeros. I am
> thinking that these could be used for code verification purposes
> only. Does anyone knows before I adventure in to assembly code to
> uncover their role ?
>
> Regards,