Arthur Heymans 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 7:
(4 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 .bss usage (the image is placed in memory that is not reclaimed by the OS)
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 cbfs-files-$(CONFIG_FSP1_1_DISPLAY_LOGO) not work?
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 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?
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 :/