Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/1
diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig index e449409..b09ea05 100644 --- a/src/soc/intel/xeon_sp/Kconfig +++ b/src/soc/intel/xeon_sp/Kconfig @@ -56,6 +56,7 @@ select MICROCODE_BLOB_NOT_HOOKED_UP select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FSP_CAR + select NO_SMM
config MAINBOARD_USES_FSP2_0 bool diff --git a/src/soc/intel/xeon_sp/Makefile.inc b/src/soc/intel/xeon_sp/Makefile.inc index ea5f253..c2b8c40 100644 --- a/src/soc/intel/xeon_sp/Makefile.inc +++ b/src/soc/intel/xeon_sp/Makefile.inc @@ -7,6 +7,7 @@
bootblock-y += bootblock.c spi.c lpc.c gpio.c pch.c romstage-y += romstage.c reset.c util.c spi.c gpio.c pmutil.c memmap.c +romstage-y += ../../../cpu/intel/car/romstage.c ramstage-y += uncore.c reset.c util.c lpc.c spi.c gpio.c memmap.c ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_PMC) += pmc.c postcar-y += spi.c diff --git a/src/soc/intel/xeon_sp/memmap.c b/src/soc/intel/xeon_sp/memmap.c index 08ea732..674675d 100644 --- a/src/soc/intel/xeon_sp/memmap.c +++ b/src/soc/intel/xeon_sp/memmap.c @@ -2,6 +2,9 @@
#define __SIMPLE_DEVICE__
+#include <arch/romstage.h> +#include <cbmem.h> +#include <console/console.h> #include <device/pci.h> #include <cpu/x86/smm.h> #include <soc/pci_devs.h> @@ -24,3 +27,21 @@ *start = tseg_base; *size = tseg_limit - tseg_base; } + +void fill_postcar_frame(struct postcar_frame *pcf) +{ + /* + * We need to make sure ramstage will be run cached. At this + * point exact location of ramstage in cbmem is not known. + * Instruct postcar to cache 16 megs under cbmem top which is + * a safe bet to cover ramstage. + */ + uintptr_t top_of_ram = (uintptr_t) cbmem_top(); + + printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); + top_of_ram -= 16*MiB; + postcar_frame_add_mtrr(pcf, top_of_ram, 16*MiB, MTRR_TYPE_WRBACK); + /* Cache the TSEG region */ + if (CONFIG(TSEG_STAGE_CACHE)) + postcar_enable_tseg_cache(pcf); +} diff --git a/src/soc/intel/xeon_sp/romstage.c b/src/soc/intel/xeon_sp/romstage.c index f3e32fd..2540c5c 100644 --- a/src/soc/intel/xeon_sp/romstage.c +++ b/src/soc/intel/xeon_sp/romstage.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/romstage.h> -#include <cbmem.h> #include <intelblocks/rtc.h> #include <console/console.h> #include <cpu/x86/mtrr.h> @@ -9,14 +8,8 @@ #include <soc/romstage.h> #include <soc/util.h>
-asmlinkage void car_stage_entry(void) +void mainboard_romstage_entry(void) { - struct postcar_frame pcf; - uintptr_t top_of_ram; - - printk(BIOS_DEBUG, "FSP TempRamInit was successful...\n"); - - console_init(); rtc_init(); if (soc_get_rtc_failed()) mainboard_rtc_failed(); @@ -26,23 +19,7 @@
unlock_pam_regions();
- if (postcar_frame_init(&pcf, 1 * KiB)) - die("Unable to initialize postcar frame.\n"); - - /* - * We need to make sure ramstage will be run cached. At this point exact - * location of ramstage in cbmem is not known. Instruct postcar to cache - * 16 megs under cbmem top which is a safe bet to cover ramstage. - */ - top_of_ram = (uintptr_t)cbmem_top(); - printk(BIOS_DEBUG, "top_of_ram: 0x%lx\n", top_of_ram); - postcar_frame_add_mtrr(&pcf, top_of_ram - 16 * MiB, 16 * MiB, - MTRR_TYPE_WRBACK); - - /* Cache the memory-mapped boot media. */ - postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); save_dimm_info(); - run_postcar_phase(&pcf); }
__weak void mainboard_memory_init_params(FSPM_UPD *mupd)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... PS1, Line 39: uintptr_t top_of_ram = (uintptr_t) cbmem_top(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... PS1, Line 39: uintptr_t top_of_ram = (uintptr_t) cbmem_top(); please, no spaces at the start of a line
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... PS1, Line 42: * nit: spacing around operators?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46658
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46658
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/1/src/soc/intel/xeon_sp/memma... PS1, Line 42: *
nit: spacing around operators?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46658
to look at the new patch set (#5).
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/5
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 5: Code-Review+2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46658
to look at the new patch set (#6).
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... PS6, Line 37: cbmem_top I guess FSP provides this pointer? On SKL and CFL, the value for this pointer is MTRR poison (trying to cache it will exhaust all MTRRs and more). I made CB:45930 to fix this on client platforms.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... PS6, Line 37: cbmem_top
I guess FSP provides this pointer? On SKL and CFL, the value for this pointer is MTRR poison (trying […]
I don't recall seeing bad things on that. The behaviour is not changed in this patch so it can be tackled later on if it proves to be a problem.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... File src/soc/intel/xeon_sp/memmap.c:
https://review.coreboot.org/c/coreboot/+/46658/6/src/soc/intel/xeon_sp/memma... PS6, Line 37: cbmem_top
I don't recall seeing bad things on that. […]
Sounds good
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46658
to look at the new patch set (#8).
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/46658/8
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 8: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
soc/intel/xeon_sp: Use common cpu/intel romstage entry
This removes some boilerplate like starting the console and also adds a "start of romstage" timestamp.
Change-Id: Ie85df5d244fa37c41f0b3177ca325c607fa54593 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/46658 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/Makefile.inc M src/soc/intel/xeon_sp/memmap.c M src/soc/intel/xeon_sp/romstage.c 4 files changed, 24 insertions(+), 24 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig index a83a3c3..3d3e803 100644 --- a/src/soc/intel/xeon_sp/Kconfig +++ b/src/soc/intel/xeon_sp/Kconfig @@ -55,6 +55,7 @@ select MICROCODE_BLOB_NOT_HOOKED_UP select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FSP_CAR + select NO_SMM
config MAINBOARD_USES_FSP2_0 bool diff --git a/src/soc/intel/xeon_sp/Makefile.inc b/src/soc/intel/xeon_sp/Makefile.inc index 07fe3de..a10075a 100644 --- a/src/soc/intel/xeon_sp/Makefile.inc +++ b/src/soc/intel/xeon_sp/Makefile.inc @@ -7,6 +7,7 @@
bootblock-y += bootblock.c spi.c lpc.c gpio.c pch.c romstage-y += romstage.c reset.c util.c spi.c gpio.c pmutil.c memmap.c +romstage-y += ../../../cpu/intel/car/romstage.c ramstage-y += uncore.c reset.c util.c lpc.c spi.c gpio.c nb_acpi.c ramstage.c chip_common.c ramstage-y += memmap.c ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_PMC) += pmc.c diff --git a/src/soc/intel/xeon_sp/memmap.c b/src/soc/intel/xeon_sp/memmap.c index 79ab47e..edc62cf 100644 --- a/src/soc/intel/xeon_sp/memmap.c +++ b/src/soc/intel/xeon_sp/memmap.c @@ -1,5 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <arch/romstage.h> +#include <cbmem.h> +#include <console/console.h> #include <device/pci_ops.h> #include <cpu/x86/smm.h> #include <soc/pci_devs.h> @@ -20,3 +23,21 @@ *start = tseg_base; *size = tseg_limit - tseg_base; } + +void fill_postcar_frame(struct postcar_frame *pcf) +{ + /* + * We need to make sure ramstage will be run cached. At this + * point exact location of ramstage in cbmem is not known. + * Instruct postcar to cache 16 megs under cbmem top which is + * a safe bet to cover ramstage. + */ + uintptr_t top_of_ram = (uintptr_t)cbmem_top(); + + printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); + top_of_ram -= 16 * MiB; + postcar_frame_add_mtrr(pcf, top_of_ram, 16 * MiB, MTRR_TYPE_WRBACK); + /* Cache the TSEG region */ + if (CONFIG(TSEG_STAGE_CACHE)) + postcar_enable_tseg_cache(pcf); +} diff --git a/src/soc/intel/xeon_sp/romstage.c b/src/soc/intel/xeon_sp/romstage.c index f3e32fd..2540c5c 100644 --- a/src/soc/intel/xeon_sp/romstage.c +++ b/src/soc/intel/xeon_sp/romstage.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/romstage.h> -#include <cbmem.h> #include <intelblocks/rtc.h> #include <console/console.h> #include <cpu/x86/mtrr.h> @@ -9,14 +8,8 @@ #include <soc/romstage.h> #include <soc/util.h>
-asmlinkage void car_stage_entry(void) +void mainboard_romstage_entry(void) { - struct postcar_frame pcf; - uintptr_t top_of_ram; - - printk(BIOS_DEBUG, "FSP TempRamInit was successful...\n"); - - console_init(); rtc_init(); if (soc_get_rtc_failed()) mainboard_rtc_failed(); @@ -26,23 +19,7 @@
unlock_pam_regions();
- if (postcar_frame_init(&pcf, 1 * KiB)) - die("Unable to initialize postcar frame.\n"); - - /* - * We need to make sure ramstage will be run cached. At this point exact - * location of ramstage in cbmem is not known. Instruct postcar to cache - * 16 megs under cbmem top which is a safe bet to cover ramstage. - */ - top_of_ram = (uintptr_t)cbmem_top(); - printk(BIOS_DEBUG, "top_of_ram: 0x%lx\n", top_of_ram); - postcar_frame_add_mtrr(&pcf, top_of_ram - 16 * MiB, 16 * MiB, - MTRR_TYPE_WRBACK); - - /* Cache the memory-mapped boot media. */ - postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); save_dimm_info(); - run_postcar_phase(&pcf); }
__weak void mainboard_memory_init_params(FSPM_UPD *mupd)
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/9/src/soc/intel/xeon_sp/Kconf... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/46658/9/src/soc/intel/xeon_sp/Kconf... PS9, Line 58: select NO_SMM Why do you add this? What does it to do with this commit subject?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46658 )
Change subject: soc/intel/xeon_sp: Use common cpu/intel romstage entry ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46658/9/src/soc/intel/xeon_sp/Kconf... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/46658/9/src/soc/intel/xeon_sp/Kconf... PS9, Line 58: select NO_SMM
Why do you add this? What does it to do with this commit subject?
The common cpu/intel/car/romstage.c code has different codepaths depending on CONFIG_TSEG. So in this CL CONFIG_NO_SMM needs to be explicitly set. It is removed in one of the next CL when SMM is properly set up. I'll update the commit message to reflect this.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Christian Walter (Code Review)
-
Jonathan Zhang (Code Review)