Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
drivers/intel/fsp2_0: Add logo support
Add support for the FSP feature to display the logo.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iaaffd2be567861371bbe908c1ef9d7dde483a945 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/Makefile.inc M src/drivers/intel/fsp2_0/include/fsp/api.h A src/drivers/intel/fsp2_0/logo.c M src/drivers/intel/fsp2_0/silicon_init.c 5 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/37515/1
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 77382d3..1d692a8 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -156,6 +156,18 @@ is limited till EFI_PEI_MP_SERVICE_PPI and this option might be useful to add further PPI if required.
+config FSP2_0_DISPLAY_LOGO + bool "Enable logo" + default n + help + Uses the FSP to display the boot logo. This method supports a + BMP file only. The uncompressed size can be up to 1 MB. + +config FSP2_0_LOGO_FILE_NAME + string "Logo file" + depends on FSP2_0_DISPLAY_LOGO + default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/logo.bmp" + if FSP_PEIM_TO_PEIM_INTERFACE source "src/drivers/intel/fsp2_0/ppi/Kconfig" endif diff --git a/src/drivers/intel/fsp2_0/Makefile.inc b/src/drivers/intel/fsp2_0/Makefile.inc index bc00cd4..8658062 100644 --- a/src/drivers/intel/fsp2_0/Makefile.inc +++ b/src/drivers/intel/fsp2_0/Makefile.inc @@ -30,6 +30,7 @@ ramstage-$(CONFIG_DISPLAY_FSP_HEADER) += header_display.c ramstage-$(CONFIG_DISPLAY_HOBS) += hob_display.c ramstage-$(CONFIG_VERIFY_HOBS) += hob_verify.c +ramstage-$(CONFIG_FSP2_0_DISPLAY_LOGO) += logo.c ramstage-y += notify.c ramstage-y += silicon_init.c ramstage-$(CONFIG_DISPLAY_UPD_DATA) += upd_display.c @@ -78,6 +79,12 @@ true endif
+# Add logo to the cbfs image +cbfs-files-$(CONFIG_FSP2_0_DISPLAY_LOGO) += logo.bmp +logo.bmp-file := $(call strip_quotes,$(CONFIG_FSP2_0_LOGO_FILE_NAME)) +logo.bmp-type := raw +logo.bmp-compression := LZMA + ifneq ($(call strip_quotes,$(CONFIG_FSP_HEADER_PATH)),) CPPFLAGS_common+=-I$(CONFIG_FSP_HEADER_PATH) endif diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index b63cd04..fb42c76 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -69,6 +69,9 @@ uint8_t fsp_memory_mainboard_version(void); uint8_t fsp_memory_soc_version(void);
+/* Load logo to be displayed by FSP */ +void load_logo(FSPS_UPD *supd); + /* Callback after processing FSP notify */ void platform_fsp_notify_status(enum fsp_notify_phase phase);
diff --git a/src/drivers/intel/fsp2_0/logo.c b/src/drivers/intel/fsp2_0/logo.c new file mode 100644 index 0000000..feeec3b --- /dev/null +++ b/src/drivers/intel/fsp2_0/logo.c @@ -0,0 +1,27 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <soc/ramstage.h> +#include <console/console.h> +#include <fsp/api.h> +#include <include/cbfs.h> + +void load_logo(FSPS_UPD *supd) +{ + FSP_S_CONFIG *params = &supd->FspsConfig; + + params->LogoSize = cbfs_boot_load_file("logo.bmp", (void *)params->LogoPtr, + params->LogoSize, CBFS_TYPE_RAW); + if (!params->LogoSize) + params->LogoPtr = 0; +} diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index f58851d..4131142 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -34,6 +34,7 @@ fsp_silicon_init_fn silicon_init; uint32_t status; uint8_t postcode; + const struct cbmem_entry *logo_entry;
supd = (FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base);
@@ -56,6 +57,13 @@ /* Give SoC/mainboard a chance to populate entries */ platform_fsp_silicon_init_params_cb(upd);
+ 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); + } + /* Call SiliconInit */ silicon_init = (void *) (hdr->image_base + hdr->silicon_init_entry_offset); @@ -67,6 +75,9 @@ timestamp_add_now(TS_FSP_SILICON_INIT_END); post_code(POST_FSP_SILICON_EXIT);
+ if (CONFIG(FSP2_0_DISPLAY_LOGO)) + cbmem_entry_remove(logo_entry); + fsp_debug_after_silicon_init(status);
/* Handle any errors returned by FspSiliconInit */
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 164: BMP file only. The uncompressed size can be up to 1 MB. mention that it can be compressed as well
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 1:
Since this seems to be one of those "move responsibility into FSP" things: what does this provide over our coreboot-side splash screen feature?
Hello Andrey Petrov, Patrick Rudolph, Arthur Heymans, Patrick Rudolph, Frans Hendriks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37515
to look at the new patch set (#2).
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
drivers/intel/fsp2_0: Add logo support
Add support for the FSP feature to display the logo.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iaaffd2be567861371bbe908c1ef9d7dde483a945 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/Makefile.inc M src/drivers/intel/fsp2_0/include/fsp/api.h A src/drivers/intel/fsp2_0/logo.c M src/drivers/intel/fsp2_0/silicon_init.c 5 files changed, 64 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/37515/2
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
Since this seems to be one of those "move responsibility into FSP" things: what does this provide over our coreboot-side splash screen feature?
Basically it adds BMP support, which is not in the current coreboot implementation. This allows us to use one logo as splash screen for coreboot and a logo published in the BGRT table. The EDK payloads also use the BMP format.
So this is not a replacement, it is just an additional feature that can be used when needed.
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 164: BMP file only. The uncompressed size can be up to 1 MB.
mention that it can be compressed as well
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/37515/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 164: BMP file only. The uncompressed size can be up to 1 MB.
Done
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37515/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/37515/2/src/drivers/intel/fsp2_0/si... PS2, Line 60: #if (CONFIG(HAVE_FSP_GOP)) there's no need for preprocessor here. Linker will remove dead code if !HAVE_FSP_GOP as !FSP2_0_DISPLAY_LOGO in this case.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37515/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/37515/2/src/drivers/intel/fsp2_0/si... PS2, Line 60: #if (CONFIG(HAVE_FSP_GOP))
there's no need for preprocessor here. […]
Compilation breaks if I do that. The Fsp implementations without "HAVE_FSP_GOP" don't have the members involved in the include files so the code doesn't compile and never gets to the linker stage.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
Patch Set 2: Code-Review+1
Patch Set 2:
(1 comment)
Patch Set 1:
Since this seems to be one of those "move responsibility into FSP" things: what does this provide over our coreboot-side splash screen feature?
Basically it adds BMP support, which is not in the current coreboot implementation. This allows us to use one logo as splash screen for coreboot and a logo published in the BGRT table. The EDK payloads also use the BMP format.
So this is not a replacement, it is just an additional feature that can be used when needed.
Out of curiosity, which platform do you plan to use this on? With libgfxinit and tianocore, coreboot can be made to show a .bmp bootsplash already.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37515 )
Change subject: drivers/intel/fsp2_0: Add logo support ......................................................................
drivers/intel/fsp2_0: Add logo support
Add support for the FSP feature to display the logo.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iaaffd2be567861371bbe908c1ef9d7dde483a945 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37515 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/Makefile.inc M src/drivers/intel/fsp2_0/include/fsp/api.h A src/drivers/intel/fsp2_0/logo.c M src/drivers/intel/fsp2_0/silicon_init.c 5 files changed, 64 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, but someone else must approve Frans Hendriks: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 77382d3..d9d7fb2 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -156,6 +156,20 @@ is limited till EFI_PEI_MP_SERVICE_PPI and this option might be useful to add further PPI if required.
+config FSP2_0_DISPLAY_LOGO + bool "Enable logo" + default n + depends on HAVE_FSP_GOP + help + Uses the FSP to display the boot logo. This method supports a + BMP file only. The uncompressed size can be up to 1 MB. The logo can be compressed + using LZMA. + +config FSP2_0_LOGO_FILE_NAME + string "Logo file" + depends on FSP2_0_DISPLAY_LOGO + default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/logo.bmp" + if FSP_PEIM_TO_PEIM_INTERFACE source "src/drivers/intel/fsp2_0/ppi/Kconfig" endif diff --git a/src/drivers/intel/fsp2_0/Makefile.inc b/src/drivers/intel/fsp2_0/Makefile.inc index bc00cd4..8658062 100644 --- a/src/drivers/intel/fsp2_0/Makefile.inc +++ b/src/drivers/intel/fsp2_0/Makefile.inc @@ -30,6 +30,7 @@ ramstage-$(CONFIG_DISPLAY_FSP_HEADER) += header_display.c ramstage-$(CONFIG_DISPLAY_HOBS) += hob_display.c ramstage-$(CONFIG_VERIFY_HOBS) += hob_verify.c +ramstage-$(CONFIG_FSP2_0_DISPLAY_LOGO) += logo.c ramstage-y += notify.c ramstage-y += silicon_init.c ramstage-$(CONFIG_DISPLAY_UPD_DATA) += upd_display.c @@ -78,6 +79,12 @@ true endif
+# Add logo to the cbfs image +cbfs-files-$(CONFIG_FSP2_0_DISPLAY_LOGO) += logo.bmp +logo.bmp-file := $(call strip_quotes,$(CONFIG_FSP2_0_LOGO_FILE_NAME)) +logo.bmp-type := raw +logo.bmp-compression := LZMA + ifneq ($(call strip_quotes,$(CONFIG_FSP_HEADER_PATH)),) CPPFLAGS_common+=-I$(CONFIG_FSP_HEADER_PATH) endif diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index b63cd04..fb42c76 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -69,6 +69,9 @@ uint8_t fsp_memory_mainboard_version(void); uint8_t fsp_memory_soc_version(void);
+/* Load logo to be displayed by FSP */ +void load_logo(FSPS_UPD *supd); + /* Callback after processing FSP notify */ void platform_fsp_notify_status(enum fsp_notify_phase phase);
diff --git a/src/drivers/intel/fsp2_0/logo.c b/src/drivers/intel/fsp2_0/logo.c new file mode 100644 index 0000000..feeec3b --- /dev/null +++ b/src/drivers/intel/fsp2_0/logo.c @@ -0,0 +1,27 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <soc/ramstage.h> +#include <console/console.h> +#include <fsp/api.h> +#include <include/cbfs.h> + +void load_logo(FSPS_UPD *supd) +{ + FSP_S_CONFIG *params = &supd->FspsConfig; + + params->LogoSize = cbfs_boot_load_file("logo.bmp", (void *)params->LogoPtr, + params->LogoSize, CBFS_TYPE_RAW); + if (!params->LogoSize) + params->LogoPtr = 0; +} diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index f58851d..ebdbdbf 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -34,6 +34,7 @@ fsp_silicon_init_fn silicon_init; uint32_t status; uint8_t postcode; + const struct cbmem_entry *logo_entry;
supd = (FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base);
@@ -56,6 +57,15 @@ /* Give SoC/mainboard a chance to populate entries */ platform_fsp_silicon_init_params_cb(upd);
+#if (CONFIG(HAVE_FSP_GOP)) + 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); + } +#endif + /* Call SiliconInit */ silicon_init = (void *) (hdr->image_base + hdr->silicon_init_entry_offset); @@ -67,6 +77,9 @@ timestamp_add_now(TS_FSP_SILICON_INIT_END); post_code(POST_FSP_SILICON_EXIT);
+ if (CONFIG(FSP2_0_DISPLAY_LOGO)) + cbmem_entry_remove(logo_entry); + fsp_debug_after_silicon_init(status);
/* Handle any errors returned by FspSiliconInit */
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
Frans Hendriks 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)
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. […]
Agree specific UPDs should be handled in SoC.
Patrick Rudolph 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); : }
Agree specific UPDs should be handled in SoC.
The easiest solution would be to write a generic bmp decoder and implement it in coreboot that works regardless off the chosen display init method (GOP, native, OptionROM, ...). Decoding uncompressed , non paletted data should be easy.
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Rizwan Qureshi (Code Review)
-
Wim Vervoorn (Code Review)