Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
drivers/intel/fsp2_0: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols:
Change-Id: I8d79f97f8c12c63ce215935353717855442a8290 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp2_0/Makefile.inc A src/drivers/intel/fsp2_0/fspt_report.c M src/drivers/intel/fsp2_0/include/fsp/util.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S 4 files changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/46884/1
diff --git a/src/drivers/intel/fsp2_0/Makefile.inc b/src/drivers/intel/fsp2_0/Makefile.inc index 32140f4..46299ee 100644 --- a/src/drivers/intel/fsp2_0/Makefile.inc +++ b/src/drivers/intel/fsp2_0/Makefile.inc @@ -2,6 +2,8 @@
ifeq ($(CONFIG_PLATFORM_USES_FSP2_0),y)
+bootblock-$(CONFIG_FSP_CAR) += fspt_report.c + romstage-y += debug.c romstage-y += hand_off_block.c romstage-$(CONFIG_DISPLAY_FSP_HEADER) += header_display.c diff --git a/src/drivers/intel/fsp2_0/fspt_report.c b/src/drivers/intel/fsp2_0/fspt_report.c new file mode 100644 index 0000000..7fa3205 --- /dev/null +++ b/src/drivers/intel/fsp2_0/fspt_report.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/symbols.h> +#include <console/console.h> +#include <fsp/util.h> + +/* filled in assembly after FSP-T ran */ +uintptr_t temp_memory_start; +uintptr_t temp_memory_end; + +void report_fspt_output(void) +{ + const struct region fsp_car_region = { + .offset = temp_memory_start, + .size = temp_memory_end - temp_memory_start, + }; + const struct region coreboot_car_region = { + .offset = (uintptr_t)_car_region_start, + .size = (uintptr_t)_car_region_size, + }; + printk(BIOS_DEBUG, "FSP-T: reported temp_mem region: [0x%08lx,0x%08lx)\n", + temp_memory_start, temp_memory_end); + if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { + printk(BIOS_ERR, "Wrong CAR region used!\n"); + printk(BIOS_ERR, "Adapt CONFIG_DCACHE_RAM_BASE and CONFIG_DCACHE_RAM_SIZE to match FSP-T\n"); + } +} diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index f154a34..a57b1bb 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -89,6 +89,7 @@ void fsp_find_bootloader_tolum(struct range_entry *re); void fsp_get_version(char *buf); void lb_string_platform_blob_version(struct lb_header *header); +void report_fspt_output(void);
/* Fill in header and validate sanity of component within region device. */ enum cb_err fsp_validate_component(struct fsp_header *hdr, diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index b9daf08..573f5ea 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -72,6 +72,8 @@
/* Setup bootblock stack */ mov %edx, %esp + push %ecx + push %edx
/* clear .bss section as it is not shared */ cld @@ -82,6 +84,11 @@ shrl $2, %ecx rep stosl
+ pop %edx + movl %edx, temp_memory_end + pop %ecx + movl %ecx, temp_memory_start + /* Restore the timestamp from bootblock_crt0.S (ebp:mm1) */ push %ebp movd %mm1, %eax
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S:
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... PS1, Line 75: push %ecx : push %edx This is pushed on the stack instead of directly writing the variables as bss is cleared below. Is that clear or does it deserve a comment?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S:
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... PS1, Line 75: push %ecx : push %edx
This is pushed on the stack instead of directly writing the variables as bss is cleared below. […]
a comment would be nice
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46884
to look at the new patch set (#2).
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
drivers/intel/fsp2_0: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols:
Change-Id: I8d79f97f8c12c63ce215935353717855442a8290 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp2_0/Makefile.inc A src/drivers/intel/fsp2_0/fspt_report.c M src/drivers/intel/fsp2_0/include/fsp/util.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S 4 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/46884/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S:
https://review.coreboot.org/c/coreboot/+/46884/1/src/soc/intel/common/block/... PS1, Line 75: push %ecx : push %edx
a comment would be nice
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46884 )
Change subject: drivers/intel/fsp2_0: Add function to report FSP-T output ......................................................................
drivers/intel/fsp2_0: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols:
Change-Id: I8d79f97f8c12c63ce215935353717855442a8290 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/46884 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/intel/fsp2_0/Makefile.inc A src/drivers/intel/fsp2_0/fspt_report.c M src/drivers/intel/fsp2_0/include/fsp/util.h M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S 4 files changed, 42 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/Makefile.inc b/src/drivers/intel/fsp2_0/Makefile.inc index 32140f4..46299ee 100644 --- a/src/drivers/intel/fsp2_0/Makefile.inc +++ b/src/drivers/intel/fsp2_0/Makefile.inc @@ -2,6 +2,8 @@
ifeq ($(CONFIG_PLATFORM_USES_FSP2_0),y)
+bootblock-$(CONFIG_FSP_CAR) += fspt_report.c + romstage-y += debug.c romstage-y += hand_off_block.c romstage-$(CONFIG_DISPLAY_FSP_HEADER) += header_display.c diff --git a/src/drivers/intel/fsp2_0/fspt_report.c b/src/drivers/intel/fsp2_0/fspt_report.c new file mode 100644 index 0000000..7fa3205 --- /dev/null +++ b/src/drivers/intel/fsp2_0/fspt_report.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/symbols.h> +#include <console/console.h> +#include <fsp/util.h> + +/* filled in assembly after FSP-T ran */ +uintptr_t temp_memory_start; +uintptr_t temp_memory_end; + +void report_fspt_output(void) +{ + const struct region fsp_car_region = { + .offset = temp_memory_start, + .size = temp_memory_end - temp_memory_start, + }; + const struct region coreboot_car_region = { + .offset = (uintptr_t)_car_region_start, + .size = (uintptr_t)_car_region_size, + }; + printk(BIOS_DEBUG, "FSP-T: reported temp_mem region: [0x%08lx,0x%08lx)\n", + temp_memory_start, temp_memory_end); + if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { + printk(BIOS_ERR, "Wrong CAR region used!\n"); + printk(BIOS_ERR, "Adapt CONFIG_DCACHE_RAM_BASE and CONFIG_DCACHE_RAM_SIZE to match FSP-T\n"); + } +} diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index f154a34..a57b1bb 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -89,6 +89,7 @@ void fsp_find_bootloader_tolum(struct range_entry *re); void fsp_get_version(char *buf); void lb_string_platform_blob_version(struct lb_header *header); +void report_fspt_output(void);
/* Fill in header and validate sanity of component within region device. */ enum cb_err fsp_validate_component(struct fsp_header *hdr, diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index b9daf08..e77b841 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -72,6 +72,13 @@
/* Setup bootblock stack */ mov %edx, %esp + /* + * temp_memory_start/end reside in the .bss section, which gets cleared + * below. Save the FSP return value to the stack before writing those + * variables. + */ + push %ecx + push %edx
/* clear .bss section as it is not shared */ cld @@ -82,6 +89,11 @@ shrl $2, %ecx rep stosl
+ pop %edx + movl %edx, temp_memory_end + pop %ecx + movl %ecx, temp_memory_start + /* Restore the timestamp from bootblock_crt0.S (ebp:mm1) */ push %ebp movd %mm1, %eax
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)