Attention is currently required from: Bora Guvendik, Hannah Williams, Cliff Huang, Jérémy Compostella, Paul Menzel, Arthur Heymans, Andrey Petrov, Zhixing Ma, Tarun Tuli, Nico Huber, Michał Żygowski, Subrata Banik, Nick Vaccaro.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70300 )
Change subject: drivers/intel/fsp2_0/memory_init: Inform user of memory training ......................................................................
Patch Set 26:
(7 comments)
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/70300/comment/d0fb7541_203e60e7 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. Even though this isn't used anywhere else yet, it's not specific to FSP 2.0 and it'd be nice to move these Kconfig options elsewhere. For example, in the same place as the `early_graphics` files (see comments over there).
File src/drivers/intel/fsp2_0/intel_early_graphics.h:
https://review.coreboot.org/c/coreboot/+/70300/comment/661fe89d_161a9256 PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
can u please move intel_early_graphics. […]
Something like `drivers/gfx/early_graphics` or similar should work, the code isn't specific to Intel graphics anyway.
https://review.coreboot.org/c/coreboot/+/70300/comment/79bfb7fd_a7ac5e03 PS26, Line 9: /* Display CONFIG_MEMORY_TRAINING_TEXT centered on screen if start is : true. Clear the text otherwise */
please use multiline comment
While at it, balance the length of the two lines?
``` /* * Display CONFIG_MEMORY_TRAINING_TEXT centered on * screen if start is true. Clear the text otherwise. */ ```
File src/drivers/intel/fsp2_0/intel_early_graphics.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/e7ee2aab_da990192 PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
looking at this file, I don't find any good reason to keep this file inside FSP2.0 driver. […]
This "early_graphics" stuff doesn't even belong in the FSP 2.0 driver, it should be moved elsewhere so that it can be reused for non-FSP platforms. For instance, we want to try using this on Haswell, and it doesn't use FSP.
https://review.coreboot.org/c/coreboot/+/70300/comment/ae942e73_e0cd54db PS26, Line 39: inform_user_of_memory_training
suggestion: set_memory_training_text(boot is_required)
How about `set_memory_training_text(bool show_text)`? Same function name, different parameter name.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/d4161f22_21dfa1b0 PS26, Line 306: if (!arch_upd->NvsBufferPtr) Can FSP change the value of `NvsBufferPtr`? If so, it would be a good idea to store the value in a constant so that both checks always evaluate to the same value. Otherwise, the text could show up but never be cleared.
https://review.coreboot.org/c/coreboot/+/70300/comment/4b997882_75cd364b PS26, Line 313: "FspMemoryInit returned with error 0x%08x!\n", status); Idea for future patches: We could show BIOS_EMERG log messages on the screen.