Change in coreboot[master]: {driver,soc}/intel: Move chipset specific logo handling to soc
Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {driver,soc}/intel: Move chipset specific logo handling to soc ......................................................................
{driver,soc}/intel: Move chipset specific logo handling to soc
FSP logo handling used PcdLogoPtr and PcdLogoSize which are elements of the chipset specific FSP structures.
Create soc_load_logo() which will pass the logo pointer and size. This function will call fsp_load_logo which will load the logo.
BUG=NA TEST= Build and verified logo is displayed on Facebook FBG1701
Change-Id: I86943e64ca1ddd05e7e88fc6b882cfd33b98272e Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/include/fsp/ramstage.h M src/drivers/intel/fsp1_1/logo.c M src/drivers/intel/fsp1_1/ramstage.c M src/soc/intel/braswell/chip.c 4 files changed, 34 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37791/1
diff --git a/src/drivers/intel/fsp1_1/include/fsp/ramstage.h b/src/drivers/intel/fsp1_1/include/fsp/ramstage.h index a5eac0e..e50edd8 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/ramstage.h +++ b/src/drivers/intel/fsp1_1/include/fsp/ramstage.h @@ -26,6 +26,7 @@ /* Perform Intel silicon init. */ void intel_silicon_init(void); void fsp_run_silicon_init(FSP_INFO_HEADER *fsp_info_header, int is_s3_wakeup); +const struct cbmem_entry *fsp_load_logo(UINT32 *logo_ptr, UINT32 *logo_size); /* Called after the silicon init code has run. */ void soc_after_silicon_init(void); /* Initialize UPD data before SiliconInit call. */ @@ -33,7 +34,7 @@ void mainboard_silicon_init_params(SILICON_INIT_UPD *params); void soc_display_silicon_init_params(const SILICON_INIT_UPD *old, SILICON_INIT_UPD *new); -void load_logo(SILICON_INIT_UPD *params); +const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params); void load_vbt(uint8_t s3_resume, SILICON_INIT_UPD *params);
#endif /* _INTEL_COMMON_RAMSTAGE_H_ */ diff --git a/src/drivers/intel/fsp1_1/logo.c b/src/drivers/intel/fsp1_1/logo.c index 03b2715..23aad01 100644 --- a/src/drivers/intel/fsp1_1/logo.c +++ b/src/drivers/intel/fsp1_1/logo.c @@ -11,15 +11,24 @@ * GNU General Public License for more details. */
+#include <cbfs.h> +#include <cbmem.h> #include <soc/ramstage.h> -#include <console/console.h> -#include <fsp/ramstage.h> -#include <include/cbfs.h>
-void load_logo(SILICON_INIT_UPD *params) +const struct cbmem_entry *fsp_load_logo(UINT32 *logo_ptr, UINT32 *logo_size) { - params->PcdLogoSize = cbfs_boot_load_file("logo.bmp", (void *)params->PcdLogoPtr, - params->PcdLogoSize, CBFS_TYPE_RAW); - if (!params->PcdLogoSize) - params->PcdLogoPtr = 0; + const struct cbmem_entry *logo_entry = NULL; + void *logo_buffer; + + logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, 1 * MiB); + if (logo_entry) { + logo_buffer = cbmem_entry_start(logo_entry); + if (logo_buffer) { + *logo_size = cbfs_boot_load_file("logo.bmp", (void *)logo_buffer, + 1 * MiB, CBFS_TYPE_RAW); + if (logo_size) + *logo_ptr = (UINT32)logo_buffer; + } + } + return (logo_entry); } diff --git a/src/drivers/intel/fsp1_1/ramstage.c b/src/drivers/intel/fsp1_1/ramstage.c index 9ecdfd6..40e79cc 100644 --- a/src/drivers/intel/fsp1_1/ramstage.c +++ b/src/drivers/intel/fsp1_1/ramstage.c @@ -69,7 +69,7 @@ EFI_STATUS status; UPD_DATA_REGION *upd_ptr; VPD_DATA_REGION *vpd_ptr; - const struct cbmem_entry *logo_entry; + const struct cbmem_entry *logo_entry = NULL;
/* Display the FSP header */ if (fsp_info_header == NULL) { @@ -96,13 +96,8 @@ load_vbt(is_s3_wakeup, &silicon_init_params); mainboard_silicon_init_params(&silicon_init_params);
- if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) { - silicon_init_params.PcdLogoSize = 1 * MiB; - logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, - silicon_init_params.PcdLogoSize); - silicon_init_params.PcdLogoPtr = (UINT32)cbmem_entry_start(logo_entry); - load_logo(&silicon_init_params); - } + if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) + logo_entry = soc_load_logo(&silicon_init_params);
/* Display the UPD data */ if (CONFIG(DISPLAY_UPD_DATA)) @@ -122,7 +117,7 @@ printk(BIOS_DEBUG, "FspSiliconInit returned 0x%08x\n", status);
/* The logo_entry can be freed up now as it is not required any longer */ - if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) + if (logo_entry && !is_s3_wakeup) cbmem_entry_remove(logo_entry);
/* Mark graphics init done after SiliconInit if VBT was provided */ @@ -214,3 +209,9 @@ __weak void soc_silicon_init_params(SILICON_INIT_UPD *params) { } + +/* Load bmp and set FSP parameters, fsp_load_logo can be used */ +__weak const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params) +{ + return NULL; +} diff --git a/src/soc/intel/braswell/chip.c b/src/soc/intel/braswell/chip.c index d179cea..026b281 100644 --- a/src/soc/intel/braswell/chip.c +++ b/src/soc/intel/braswell/chip.c @@ -181,6 +181,11 @@ board_silicon_USB2_override(params); }
+const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params) +{ + return fsp_load_logo(¶ms->PcdLogoPtr, ¶ms->PcdLogoSize); +} + void soc_display_silicon_init_params(const SILICON_INIT_UPD *old, SILICON_INIT_UPD *new) {
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37791
to look at the new patch set (#2).
Change subject: {drivers,soc}/intel: Move chipset specific logo handling to SoC ......................................................................
{drivers,soc}/intel: Move chipset specific logo handling to SoC
FSP logo handling used PcdLogoPtr and PcdLogoSize which are elements of the chipset specific FSP structures.
Create soc_load_logo() which will pass the logo pointer and size. This function will call fsp_load_logo which will load the logo.
BUG=NA TEST= Build and verified logo is displayed on Facebook FBG1701
Change-Id: I86943e64ca1ddd05e7e88fc6b882cfd33b98272e Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/include/fsp/ramstage.h M src/drivers/intel/fsp1_1/logo.c M src/drivers/intel/fsp1_1/ramstage.c M src/soc/intel/braswell/chip.c 4 files changed, 34 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37791/2
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37791
to look at the new patch set (#3).
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
{drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC
FSP logo handling used PcdLogoPtr and PcdLogoSize which are elements of the chipset specific FSP structures.
Create soc_load_logo() which will pass the logo pointer and size. This function will call fsp_load_logo which will load the logo.
BUG=NA TEST= Build and verified logo is displayed on Facebook FBG1701
Change-Id: I86943e64ca1ddd05e7e88fc6b882cfd33b98272e Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/include/fsp/ramstage.h M src/drivers/intel/fsp1_1/logo.c M src/drivers/intel/fsp1_1/ramstage.c M src/soc/intel/braswell/chip.c 4 files changed, 34 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37791/3
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
{drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC
FSP logo handling used PcdLogoPtr and PcdLogoSize which are elements of the chipset specific FSP structures.
Create soc_load_logo() which will pass the logo pointer and size. This function will call fsp_load_logo which will load the logo.
BUG=NA TEST= Build and verified logo is displayed on Facebook FBG1701
Change-Id: I86943e64ca1ddd05e7e88fc6b882cfd33b98272e Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37791 Reviewed-by: Frans Hendriks fhendriks@eltan.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/include/fsp/ramstage.h M src/drivers/intel/fsp1_1/logo.c M src/drivers/intel/fsp1_1/ramstage.c M src/soc/intel/braswell/chip.c 4 files changed, 34 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Frans Hendriks: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/include/fsp/ramstage.h b/src/drivers/intel/fsp1_1/include/fsp/ramstage.h index a5eac0e..e50edd8 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/ramstage.h +++ b/src/drivers/intel/fsp1_1/include/fsp/ramstage.h @@ -26,6 +26,7 @@ /* Perform Intel silicon init. */ void intel_silicon_init(void); void fsp_run_silicon_init(FSP_INFO_HEADER *fsp_info_header, int is_s3_wakeup); +const struct cbmem_entry *fsp_load_logo(UINT32 *logo_ptr, UINT32 *logo_size); /* Called after the silicon init code has run. */ void soc_after_silicon_init(void); /* Initialize UPD data before SiliconInit call. */ @@ -33,7 +34,7 @@ void mainboard_silicon_init_params(SILICON_INIT_UPD *params); void soc_display_silicon_init_params(const SILICON_INIT_UPD *old, SILICON_INIT_UPD *new); -void load_logo(SILICON_INIT_UPD *params); +const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params); void load_vbt(uint8_t s3_resume, SILICON_INIT_UPD *params);
#endif /* _INTEL_COMMON_RAMSTAGE_H_ */ diff --git a/src/drivers/intel/fsp1_1/logo.c b/src/drivers/intel/fsp1_1/logo.c index 03b2715..23aad01 100644 --- a/src/drivers/intel/fsp1_1/logo.c +++ b/src/drivers/intel/fsp1_1/logo.c @@ -11,15 +11,24 @@ * GNU General Public License for more details. */
+#include <cbfs.h> +#include <cbmem.h> #include <soc/ramstage.h> -#include <console/console.h> -#include <fsp/ramstage.h> -#include <include/cbfs.h>
-void load_logo(SILICON_INIT_UPD *params) +const struct cbmem_entry *fsp_load_logo(UINT32 *logo_ptr, UINT32 *logo_size) { - params->PcdLogoSize = cbfs_boot_load_file("logo.bmp", (void *)params->PcdLogoPtr, - params->PcdLogoSize, CBFS_TYPE_RAW); - if (!params->PcdLogoSize) - params->PcdLogoPtr = 0; + const struct cbmem_entry *logo_entry = NULL; + void *logo_buffer; + + logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, 1 * MiB); + if (logo_entry) { + logo_buffer = cbmem_entry_start(logo_entry); + if (logo_buffer) { + *logo_size = cbfs_boot_load_file("logo.bmp", (void *)logo_buffer, + 1 * MiB, CBFS_TYPE_RAW); + if (logo_size) + *logo_ptr = (UINT32)logo_buffer; + } + } + return (logo_entry); } diff --git a/src/drivers/intel/fsp1_1/ramstage.c b/src/drivers/intel/fsp1_1/ramstage.c index 9ecdfd6..40e79cc 100644 --- a/src/drivers/intel/fsp1_1/ramstage.c +++ b/src/drivers/intel/fsp1_1/ramstage.c @@ -69,7 +69,7 @@ EFI_STATUS status; UPD_DATA_REGION *upd_ptr; VPD_DATA_REGION *vpd_ptr; - const struct cbmem_entry *logo_entry; + const struct cbmem_entry *logo_entry = NULL;
/* Display the FSP header */ if (fsp_info_header == NULL) { @@ -96,13 +96,8 @@ load_vbt(is_s3_wakeup, &silicon_init_params); mainboard_silicon_init_params(&silicon_init_params);
- if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) { - silicon_init_params.PcdLogoSize = 1 * MiB; - logo_entry = cbmem_entry_add(CBMEM_ID_FSP_LOGO, - silicon_init_params.PcdLogoSize); - silicon_init_params.PcdLogoPtr = (UINT32)cbmem_entry_start(logo_entry); - load_logo(&silicon_init_params); - } + if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) + logo_entry = soc_load_logo(&silicon_init_params);
/* Display the UPD data */ if (CONFIG(DISPLAY_UPD_DATA)) @@ -122,7 +117,7 @@ printk(BIOS_DEBUG, "FspSiliconInit returned 0x%08x\n", status);
/* The logo_entry can be freed up now as it is not required any longer */ - if (CONFIG(FSP1_1_DISPLAY_LOGO) && !is_s3_wakeup) + if (logo_entry && !is_s3_wakeup) cbmem_entry_remove(logo_entry);
/* Mark graphics init done after SiliconInit if VBT was provided */ @@ -214,3 +209,9 @@ __weak void soc_silicon_init_params(SILICON_INIT_UPD *params) { } + +/* Load bmp and set FSP parameters, fsp_load_logo can be used */ +__weak const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params) +{ + return NULL; +} diff --git a/src/soc/intel/braswell/chip.c b/src/soc/intel/braswell/chip.c index d179cea..026b281 100644 --- a/src/soc/intel/braswell/chip.c +++ b/src/soc/intel/braswell/chip.c @@ -181,6 +181,11 @@ board_silicon_USB2_override(params); }
+const struct cbmem_entry *soc_load_logo(SILICON_INIT_UPD *params) +{ + return fsp_load_logo(¶ms->PcdLogoPtr, ¶ms->PcdLogoSize); +} + void soc_display_silicon_init_params(const SILICON_INIT_UPD *old, SILICON_INIT_UPD *new) {
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/lo... File src/drivers/intel/fsp1_1/logo.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/lo... PS4, Line 29: if (logo_size) Should this be `*logo_size`? A null-pointer check after dereference seems odd.
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... PS4, Line 217: } I doubt this will ever be used.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/lo... File src/drivers/intel/fsp1_1/logo.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/lo... PS4, Line 29: if (logo_size)
Should this be `*logo_size`? A null-pointer check after dereference seems odd.
Thanks for locating this issue. TI will create a patch to correct this issue.
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... PS4, Line 217: }
I doubt this will ever be used.
It is used in the Quark X1000 case. This doesn't provide the option to pass in a logo. Other than that you are right. I don't expect any new FSP1.1 based chipsets and the Braswell is covered.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... PS4, Line 217: }
It is used in the Quark X1000 case. This doesn't provide the option to pass in a logo. […]
Quark is using FSP2.0 only now. I'll just let Jenkins test what happens if I remove this :)
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... PS4, Line 217: }
Quark is using FSP2.0 only now. […]
In this case 124 to 126 and 129 should be removed as well.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37791 )
Change subject: {drivers,soc}/intel/fsp1_1: Move chipset specific logo handling to SoC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37791/4/src/drivers/intel/fsp1_1/ra... PS4, Line 217: }
In this case 124 to 126 and 129 should be removed as well.
Right... and I just realized that we can theoretically drop all the weak functions (if we assume that there won't be any new platforms with FSP1.1).
-
Frans Hendriks (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Wim Vervoorn (Code Review)