Attention is currently required from: Bora Guvendik, Zhixing Ma, Hannah Williams, Cliff Huang, Tarun Tuli, Nico Huber, Michał Żygowski, Jérémy Compostella, Paul Menzel, Arthur Heymans, Nick Vaccaro, Andrey Petrov.
Subrata Banik 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)
Patchset:
PS26: c
File src/drivers/intel/fsp2_0/intel_early_graphics.h:
https://review.coreboot.org/c/coreboot/+/70300/comment/1b01e925_a5fe1f10 PS26, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */ can u please move intel_early_graphics.* files into a directory ?
https://review.coreboot.org/c/coreboot/+/70300/comment/8516f4f2_ba092390 PS26, Line 9: /* Display CONFIG_MEMORY_TRAINING_TEXT centered on screen if start is : true. Clear the text otherwise */ please use multiline comment
File src/drivers/intel/fsp2_0/intel_early_graphics.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/e0e76d39_02b991a7 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. why we decided to keep the code here?
https://review.coreboot.org/c/coreboot/+/70300/comment/78e264c7_af207da2 PS26, Line 39: inform_user_of_memory_training suggestion: set_memory_training_text(boot is_required)
https://review.coreboot.org/c/coreboot/+/70300/comment/64d81920_44193054 PS26, Line 56: return; assume due to so reason the code is either returning from here or line#50. Meaning you actually haven't display anything on the screen?
In that case does it make sense to again calling into this function with argument as `false` to clean things up?
I would use a bool flag as return status. and use the same status for further calling into this function post returning from FSP-M for clearing things.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/70300/comment/682897f0_297c37c1 PS26, Line 306: if (!arch_upd->NvsBufferPtr) : inform_user_of_memory_training(false); i have shared some suggestion here https://review.coreboot.org/c/coreboot/+/70300/26/src/drivers/intel/fsp2_0/i...