Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33927
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
drivers/intel/fsp1_1: Adjust postcar MTRRs
Use of romstage_ram_stack_bottom() was invalid, it potentially uses a different ROMSTAGE_RAM_STACK_SIZE from the postcar_frame_init() call.
If alignment evaluated to 1 MiB, that WB MTRR may not have covered all of CBMEM range, having some impact on boot speeds.
There is no need to accurately describe write-back MTRR ranges for postcar.
Change-Id: Icb65cef079df56fadcc292c648cab8bdbb667f47 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 7 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/33927/1
diff --git a/src/drivers/intel/fsp1_1/car.c b/src/drivers/intel/fsp1_1/car.c index bd3f478..b10a410 100644 --- a/src/drivers/intel/fsp1_1/car.c +++ b/src/drivers/intel/fsp1_1/car.c @@ -14,6 +14,7 @@ */
#include <arch/symbols.h> +#include <cbmem.h> #include <console/console.h> #include <commonlib/helpers.h> #include <cpu/intel/romstage.h> @@ -32,8 +33,7 @@ void platform_enter_postcar(void) { struct postcar_frame pcf; - size_t alignment; - uint32_t aligned_ram; + uintptr_t top_of_ram;
if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) die("Unable to initialize postcar frame.\n"); @@ -44,53 +44,12 @@ /* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ postcar_frame_add_mtrr(&pcf, 0, CACHE_TMP_RAMTOP, MTRR_TYPE_WRBACK);
- /* - * +-------------------------+ Top of RAM (aligned) - * | System Management Mode | - * | code and data | Length: CONFIG_TSEG_SIZE - * | (TSEG) | - * +-------------------------+ SMM base (aligned) - * | | - * | Chipset Reserved Memory | Length: Multiple of CONFIG_TSEG_SIZE - * | | - * +-------------------------+ top_of_ram (aligned) - * | | - * | CBMEM Root | - * | | - * +-------------------------+ - * | | - * | FSP Reserved Memory | - * | | - * +-------------------------+ - * | | - * | Various CBMEM Entries | - * | | - * +-------------------------+ top_of_stack (8 byte aligned) - * | | - * | stack (CBMEM Entry) | - * | | - * +-------------------------+ + /* Cache at least 8 MiB below the top of ram, and at most 8 MiB + * above top of the ram. This satisfies MTRR alignment requirement + * with different TSEG size configurations. */ - - alignment = mmap_region_granularity(); - aligned_ram = ALIGN_DOWN(romstage_ram_stack_bottom(), alignment); - postcar_frame_add_mtrr(&pcf, aligned_ram, alignment, MTRR_TYPE_WRBACK); - - if (CONFIG(HAVE_SMI_HANDLER)) { - void *smm_base; - size_t smm_size; - - /* - * Cache the TSEG region at the top of ram. This region is not - * restricted to SMM mode until SMM has been relocated. By - * setting the region to cacheable it provides faster access - * when relocating the SMM handler as well as using the TSEG - * region for other purposes. - */ - smm_region(&smm_base, &smm_size); - postcar_frame_add_mtrr(&pcf, (uintptr_t)smm_base, alignment, - MTRR_TYPE_WRBACK); - } + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8*MiB); + postcar_frame_add_mtrr(&pcf, top_of_ram - 8*MiB, 16*MiB, MTRR_TYPE_WRBACK);
run_postcar_phase(&pcf); }
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 1:
untested
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c@a76 PS1, Line 76: Was the only use of this function afaict.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c@a76 PS1, Line 76:
Was the only use of this function afaict.
Seems like it. How about rebasing CB:33936 on this and remove _bottom() too, if this commit works.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/#/c/33927/1/src/drivers/intel/fsp1_1/car.c@47 PS1, Line 47: /* Cache at least 8 MiB below the top of ram, and at most 8 MiB Please use the comment style from the coding style (either concise or normal (blank comment line at top)).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 1: Code-Review+2
Hello Aaron Durbin, Arthur Heymans, Frans Hendriks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33927
to look at the new patch set (#2).
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
drivers/intel/fsp1_1: Adjust postcar MTRRs
Use of romstage_ram_stack_bottom() was invalid, it potentially uses a different ROMSTAGE_RAM_STACK_SIZE from the postcar_frame_init() call.
If alignment evaluated to 1 MiB, that WB MTRR may not have covered all of CBMEM range, having some impact on boot speeds.
There is no need to accurately describe write-back MTRR ranges for postcar.
Change-Id: Icb65cef079df56fadcc292c648cab8bdbb667f47 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 7 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/33927/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
Patch Set 2: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33927 )
Change subject: drivers/intel/fsp1_1: Adjust postcar MTRRs ......................................................................
drivers/intel/fsp1_1: Adjust postcar MTRRs
Use of romstage_ram_stack_bottom() was invalid, it potentially uses a different ROMSTAGE_RAM_STACK_SIZE from the postcar_frame_init() call.
If alignment evaluated to 1 MiB, that WB MTRR may not have covered all of CBMEM range, having some impact on boot speeds.
There is no need to accurately describe write-back MTRR ranges for postcar.
Change-Id: Icb65cef079df56fadcc292c648cab8bdbb667f47 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33927 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 7 insertions(+), 49 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/car.c b/src/drivers/intel/fsp1_1/car.c index bd3f478..84ea7d1 100644 --- a/src/drivers/intel/fsp1_1/car.c +++ b/src/drivers/intel/fsp1_1/car.c @@ -14,6 +14,7 @@ */
#include <arch/symbols.h> +#include <cbmem.h> #include <console/console.h> #include <commonlib/helpers.h> #include <cpu/intel/romstage.h> @@ -32,8 +33,7 @@ void platform_enter_postcar(void) { struct postcar_frame pcf; - size_t alignment; - uint32_t aligned_ram; + uintptr_t top_of_ram;
if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) die("Unable to initialize postcar frame.\n"); @@ -44,53 +44,11 @@ /* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ postcar_frame_add_mtrr(&pcf, 0, CACHE_TMP_RAMTOP, MTRR_TYPE_WRBACK);
- /* - * +-------------------------+ Top of RAM (aligned) - * | System Management Mode | - * | code and data | Length: CONFIG_TSEG_SIZE - * | (TSEG) | - * +-------------------------+ SMM base (aligned) - * | | - * | Chipset Reserved Memory | Length: Multiple of CONFIG_TSEG_SIZE - * | | - * +-------------------------+ top_of_ram (aligned) - * | | - * | CBMEM Root | - * | | - * +-------------------------+ - * | | - * | FSP Reserved Memory | - * | | - * +-------------------------+ - * | | - * | Various CBMEM Entries | - * | | - * +-------------------------+ top_of_stack (8 byte aligned) - * | | - * | stack (CBMEM Entry) | - * | | - * +-------------------------+ - */ - - alignment = mmap_region_granularity(); - aligned_ram = ALIGN_DOWN(romstage_ram_stack_bottom(), alignment); - postcar_frame_add_mtrr(&pcf, aligned_ram, alignment, MTRR_TYPE_WRBACK); - - if (CONFIG(HAVE_SMI_HANDLER)) { - void *smm_base; - size_t smm_size; - - /* - * Cache the TSEG region at the top of ram. This region is not - * restricted to SMM mode until SMM has been relocated. By - * setting the region to cacheable it provides faster access - * when relocating the SMM handler as well as using the TSEG - * region for other purposes. - */ - smm_region(&smm_base, &smm_size); - postcar_frame_add_mtrr(&pcf, (uintptr_t)smm_base, alignment, - MTRR_TYPE_WRBACK); - } + /* Cache at least 8 MiB below the top of ram, and at most 8 MiB + * above top of the ram. This satisfies MTRR alignment requirement + * with different TSEG size configurations. */ + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8*MiB); + postcar_frame_add_mtrr(&pcf, top_of_ram - 8*MiB, 16*MiB, MTRR_TYPE_WRBACK);
run_postcar_phase(&pcf); }
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)