Hi Arthur,

Regarding the last point, I looked a little bit more into it and I feel like objcopy may indeed be needed 😞

Regards,

–
Jeremy
One Emacs to rule them all

From: Compostella, Jeremy
Subject: Re: [coreboot] [RFC] VGA Text mode in romstage
To: Arthur Heymans
Cc: <coreboot@coreboot.org>
Date: Tue, 22 Nov 2022 21:08:17 -0700
Organization: Intel Corporation - 2200 Mission College Blvd. Santa Clara, CA 95052. USA

Hi Arthur,

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

That's also what I guessed and this is what I did. I actually reused the existing CONFIG_GFX_GMA_DEFAULT_MMIO Kconfig for that purpose. I had to make a twist in the Makefile to convert to ADA number representation though (see drivers/intel/gma/Makefile.inc#65).

I'd would have the option to run libgfxinit in romstage at the soc level but still depend on MAINBOARD_USE_LIBGFXINIT.

Could you clarify your thought ? I could not make sense of this.

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.

I also have exactly that (see arch/x86/car.ld#70).

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.

This is something I have not explored. I thought about trying something like that but I could not even find any .rodata section in the romstage binary or car.ld script. But this is good suggestion, if my current proposal is not accepted or lead to too many issues I'll have a look into this.

Instead of involving objcpy, I I could try to create a region in .car.data of the size of the .data section, make the linker script move all the .data symbols to a .init_data section and modify src/arch/x86/assembly_entry.S to perform the copy at runtime before jumping into car_stage_entry after clearing the .bss section.

–
Jeremy
One Emacs to rule them all

From: Arthur Heymans
Subject: Re: [coreboot] [RFC] VGA Text mode in romstage
To: Compostella, Jeremy
Cc: coreboot@coreboot.org
Date: Tue, 22 Nov 2022 06:07:10 -0700

Re: [coreboot] [RFC] VGA Text mode in romstage
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/romstage/graphics.c#64>

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/Kconfig>
<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 <jeremy.compostella@intel.com> 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/romstage/graphics.c#64>

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/Kconfig>
<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,

--
*Jeremy*
/One Emacs to rule them all/
_______________________________________________
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-leave@coreboot.org