Attention is currently required from: Bora Guvendik, Anil Kumar K, Hannah Williams, Cliff Huang, Paul Menzel, Angel Pons, Arthur Heymans, Andrey Petrov, Zhixing Ma, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Nick Vaccaro.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70300 )
Change subject: soc/intel/alderlake: Inform user of memory training ......................................................................
Patch Set 39: Code-Review+1
(5 comments)
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/70300/comment/660e7ac6_f75bf0e4 PS26, Line 42: config INFORM_USER_OF_MEMORY_TRAINING_ON_SCREEN : bool : default n : depends on EARLY_GFX_GMA : help : Display a message on-screen to inform the end-user that : memory training is in progress. : : config MEMORY_TRAINING_TEXT : string "Text to display during memory training" : default "Memory training in progress, please wait..." : depends on INFORM_USER_OF_MEMORY_TRAINING_ON_SCREEN : help : Message to be displayed while memory training is in : progress. It can be a multi-line string by using the '' : character as a line break. The text is rendered centered : vertically and horizontally on the screen and due to VGA : text mode constraints it is limited to 80 columns and 25 : lines.
Same question applied here.
I implemented Subrata suggestion and as a result got rid of these configuration flags.
File src/drivers/intel/fsp2_0/intel_early_graphics.h:
https://review.coreboot.org/c/coreboot/+/70300/comment/0e34566d_8e46487d PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
Where do you suggest to put this code precisely ? `drivers/gfx/early_graphics` seems odd as this is […]
As requested by Subrata I moved this "generic code" and put it in a SoC specific code tree. As a result the question of the host directory is now irrelevant.
File src/drivers/intel/fsp2_0/intel_early_graphics.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/d884d15d_6676e1bd PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
Where do you suggest to put this code precisely ? […]
As requested I moved this "generic code" and put it in a SoC specific code tree. As also requested, I also hardcoded the text string.
File src/drivers/intel/fsp2_0/intel_early_graphics.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/5a4b152d_0068957f PS36, Line 78:
Thinking about this, I too suggest dropping the Kconfig in favor of a const char[] somewhere. […]
As requested I moved this "generic code" and put it in a SoC specific code tree. As also requested, I also hardcoded the text string.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/d2cc0a6f_b7727d6b PS27, Line 308:
The current implementation is using the same PCI BAR 0 that is being used at ramstage and as such […]
Regarding the use of your temporary MMIO region instead of the use of the use of a range in the memory hole we need to re-visit in the light of Angel interesting suggestion (https://review.coreboot.org/c/coreboot/+/70276/15/src/drivers/intel/gma/Kcon...)
Does it make sense to re-use a SPI map region and to have to release it if we consider `CONFIG_GFX_GMA_DEFAULT_MMIO` reserved for graphics MMIO operation and that we make it clear to the coreboot resource allocator that this is reserved for this purpose ?