Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37515/3/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/37515/3/src/drivers/intel/fsp2_0/si... PS3, Line 61: if (CONFIG(FSP2_0_DISPLAY_LOGO)) { : upd->FspsConfig.LogoSize = 1 * MiB; : logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, upd->FspsConfig.LogoSize); : upd->FspsConfig.LogoPtr = (UINT32)cbmem_entry_start(logo_entry); : load_logo(upd); : } I just saw this patch. I realized that LogoSize and LogoPtr are platform/SoC specific UPDs. The struct members/configs under FspsConfig may change for a platform i.e., they may get renamed or they may get moved to a nested structure. If that happens this will break. Also fsp2_0 driver is accessing the architecture UPDs only, which do not change. Changes in them is accompanied by the spec change.
Ideally this change should be part of the platform code i.e., src/soc/intel/cannonlake/fsp_params.c