Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36679 )
Change subject: {drivers,mainboard}: Move FSP logo support to fsp1_1 ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/Kc... PS7, Line 107: FSP1_1_DISPLAY_LOGO
Can you add a useful help text? What format does it need to be in, size limits, increased . […]
The message has been changed and the increased .bss usage has been resolved. We don't need the logo to be around during runtime.
We are now used CBMEM for that. We add an entry just before the FSP call that does the display init and remove the entry afterwards. There are no other CBMEM actions so we can do this.
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/Ma... File src/drivers/intel/fsp1_1/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/Ma... PS7, Line 57: ifeq ($(CONFIG_FSP1_1_DISPLAY_LOGO),y
Would […]
This was just done this way as the FSP file is done in the same way.
https://review.coreboot.org/c/coreboot/+/36679/6/src/drivers/intel/fsp1_1/lo... File src/drivers/intel/fsp1_1/logo.c:
https://review.coreboot.org/c/coreboot/+/36679/6/src/drivers/intel/fsp1_1/lo... PS6, Line 26: CBFS_TYPE_RAW);
Done
Done
https://review.coreboot.org/c/coreboot/+/36679/6/src/drivers/intel/fsp1_1/lo... PS6, Line 29: printk(BIOS_DEBUG, "Found a Logo of %d bytes after decompression\n", params->PcdLogoSize);
Is the cbfs_boot_load_file not sufficiently verbose/informative on this?
Done
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/lo... File src/drivers/intel/fsp1_1/logo.c:
https://review.coreboot.org/c/coreboot/+/36679/7/src/drivers/intel/fsp1_1/lo... PS7, Line 19: char logo_data[1 * MiB];
The os does not reclaim this :/
Done
https://review.coreboot.org/c/coreboot/+/36679/8/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/36679/8/src/drivers/intel/fsp1_1/ra... PS8, Line 101: logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, silicon_init_params.PcdLogoSize);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/36679/9/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/36679/9/src/drivers/intel/fsp1_1/ra... PS9, Line 101: logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO,
trailing whitespace
Done
-
Wim Vervoorn (Code Review)