Change in coreboot[master]: soc/intel/cannonlake: Add provision to skip postcar and load ramstage
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/1
diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index 94b9899..3aefb97 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -127,6 +127,23 @@ printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); }
+#if !CONFIG(HAVE_POSTCAR) +/* + * Make sure we are enabling intermediate caching to speed up + * ramstage.elf loading and decompression as we are still in romstage + * and car tear down will be handled by ramstage at its entry. + */ +static void enable_ramstage_caching(uintptr_t base, size_t size) +{ + int mtrr = get_free_var_mtrr(); + + if (mtrr == -1) + return; + + set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT); +} +#endif + asmlinkage void car_stage_entry(void) { bool s3wake; @@ -164,5 +181,10 @@ /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT);
+#if CONFIG(HAVE_POSTCAR) run_postcar_phase(&pcf); +#else + enable_ramstage_caching(top_of_ram, 16*MiB); + run_ramstage_phase(&pcf); +#endif }
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/romstage/romstage.c 2 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT); Well the stack frame created for postcar would be exactly want you want to apply prior to run_ramstage() call, isn't it? You probably could replay those MTRRs, just need to keep in mind that you cannot do cache-disable / invd in the sequence since active stack is in CAR?
MTRR manipulations are maybe over-cautios in IA manuals. If you make the sequence right (clear VALID, program new mask and size, set VALID) maybe you can even just overwrite MTRR that covers CAR region. The sensitive cache-lines were allocated with no-evict so will they ever be evicted without explicit invd command?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
Well the stack frame created for postcar would be exactly want you want to apply prior to run_ramsta […]
yes you are right, in fact i have done in that way first but later i saw the code changes was too much/too complicated (where i can't team down CAR, because i'm inside CAR but still wish to program new sets of MTRRs in romstage and later once i have reached ramstage, again doing the tear down and reprogramming those MTRRs) and decided to make most of code re usability from exit_car.S which does the same thing in simplest form using postcar_frame
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
yes you are right, in fact i have done in that way first but later i saw the code changes was too mu […]
Was this 'first way' available for review? I am explicitly saying to program the MTRRs (even in C) without tearing down the CAR. And then reuse the same frame in ramstage to teardown CAR after stack switch.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
Was this 'first way' available for review?
no, i didn't pushed that CL
I am explicitly saying to program the MTRRs (even in C) without tearing down the CAR.
yes, got your point, you are telling to perform below MTRRs in romstage itself before tearing down the car. 1. set_var_mtrr(<number>, top_of_stack, size, MTRR_TYPE_WRPROT); 2. set_var_mtrr(<number>, ROM start address, ROM size, MTRR_TYPE_WRPROT);
if i look at romstage MTRR snapshot, i could see it always has #2 MTRR programmed also additionally it has DACHE_RAM_BASE/SIZE in another MTRR.
So, additionally i only need #1 MTRR programming which i have done here. Did i miss anything ?
And then reuse the same frame in ramstage to teardown CAR after stack switch.
yes, doing the same using exit_car.S in front of ramstage
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
Was this 'first way' available for review? […]
Well my suggestion was to do the complete frame exactly, even overwriting the VAR MTRR that covers CAR region.
All this, even to add new MTRRs without going through cache-disable-enable sequence, is a grey area. Or I don't have the detailed docs about the non-evict mode to try estimate what will work and what not.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
even overwriting the VAR MTRR that covers CAR region.
This cause sluggishness in romstage code with overridden CAR region
All this, even to add new MTRRs without going through cache-disable-enable sequence, is a grey area. Or I don't have the detailed docs about the non-evict mode to try estimate what will work and what not.
I thought there is good white paper in public domain, i couldn't find that now. never mind i will check how do we make that paper in public. meantime, you can refer to this document, it has some idea about NEM/CQOS mode. https://www.coreboot.org/images/2/23/Apollolake_SoC.pdf
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... PS3, Line 184: #if CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR) There's no need for #if here or above.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... PS3, Line 184: #if CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR)
There's no need for #if here or above.
oh yes, my bad.
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 133: * and car tear down will be handled by ramstage at its entry. This seems orthogonal to the change, and I'm not understanding why we need this. Presumably the same problem exists while loading postcar from romstage, but it's smaller so less noticeable?
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 165: if (postcar_frame_init(&pcf, 0)) It should be noted somewhere that there's a stack being added that is only used for a very short amount of time but eating 4KiB.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 133: * and car tear down will be handled by ramstage at its entry.
This seems orthogonal to the change, and I'm not understanding why we need this. […]
yes Aaron, postcar is very small hence you might not notice the impact but with romstage loading ramstage directly, it might regress ~50ms without this enable_ramstage_caching()
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 165: if (postcar_frame_init(&pcf, 0))
It should be noted somewhere that there's a stack being added that is only used for a very short amo […]
yes agree, we should do this as comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 133: * and car tear down will be handled by ramstage at its entry.
yes Aaron, postcar is very small hence you might not notice the impact but with romstage loading ram […]
So what are the timings like for postcar loading doing this? Why wouldn't this be unconditional just below line 177?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 133: * and car tear down will be handled by ramstage at its entry.
So what are the timings like for postcar loading doing this? Why wouldn't this be unconditional just […]
i can try this and let you know by tomorrow ? this should provide some savings for sure
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 133: * and car tear down will be handled by ramstage at its entry.
i can try this and let you know by tomorrow ? this should provide some savings for sure
https://review.coreboot.org/c/coreboot/+/34791/5 already in review
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/5/src/soc/intel/cannonlake/ro... PS5, Line 188: if (CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR)) braces {} are not necessary for any arm of this statement
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/5/src/soc/intel/cannonlake/ro... PS5, Line 188: if (CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR))
braces {} are not necessary for any arm of this statement
Ack
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/7/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/7/src/soc/intel/cannonlake/ro... PS7, Line 188: if (CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR)) braces {} are not necessary for any arm of this statement
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 165: if (postcar_frame_init(&pcf, 0))
yes agree, we should do this as comment
Aaron, should we have comments in postcar_loader.c file itself to avoid commenting on every soc code?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 165: if (postcar_frame_init(&pcf, 0))
Aaron, should we have comments in postcar_loader. […]
The problem is that the temporary stack is only short lived (in comparison to postcar stage using it) when RAMSTAGE_LOADS_FROM_POSTCAR is enabled because ramstage very quickly switches to its own stacks allocated in bss.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/4/src/soc/intel/cannonlake/ro... PS4, Line 165: if (postcar_frame_init(&pcf, 0))
The problem is that the temporary stack is only short lived (in comparison to postcar stage using it […]
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/2/src/soc/intel/cannonlake/ro... PS2, Line 143: set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT);
even overwriting the VAR MTRR that covers CAR region. […]
Ack
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/34754/3/src/soc/intel/cannonlake/ro... PS3, Line 184: #if CONFIG(RAMSTAGE_LOADS_FROM_POSTCAR)
oh yes, my bad.
Done
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/9
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
soc/intel/cannonlake: Add provision to skip postcar and load ramstage
This patch adds required provision in soc code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
Also remove CAR global migration when ramstage stage is used (without intermediate postcar stage)
When a platform is not using postcar stage (!HAVE_POSTCAR) it will use ramstage hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/10
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 10: Code-Review+1
My guess is that we'd need to combine this logic into some other location so we don't duplicate the if/else all over the code base.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 10:
Patch Set 10: Code-Review+1
My guess is that we'd need to combine this logic into some other location so we don't duplicate the if/else all over the code base.
we can do the same inside postcar_loader.c run_postcar_phase() and check like below ?
void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog = PROG_INIT(PROG_POSTCAR, CONFIG_CBFS_PREFIX "/postcar");
if (CONFIG(RAMSTAGE_LOADS_FROM_RAMSTAGE)) { run_ramstage_phase(&pcf); /* control shouldn't come here */ }
run_phase(pcf, prog, STAGE_POSTCAR, CBMEM_ID_AFTER_CAR); }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: soc/intel/cannonlake: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 11:
Patch Set 10:
Patch Set 10: Code-Review+1
My guess is that we'd need to combine this logic into some other location so we don't duplicate the if/else all over the code base.
we can do the same inside postcar_loader.c run_postcar_phase() and check like below ?
void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog = PROG_INIT(PROG_POSTCAR, CONFIG_CBFS_PREFIX "/postcar");
if (CONFIG(RAMSTAGE_LOADS_FROM_RAMSTAGE)) { run_ramstage_phase(&pcf); /* control shouldn't come here */ } run_phase(pcf, prog, STAGE_POSTCAR, CBMEM_ID_AFTER_CAR);}
That would work.
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#12).
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
arch/x86: Add provision to skip postcar and load ramstage
This patch adds required provision in postcar_loader code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
When a platform is not using postcar stage it will use ramstage, hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/12
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 12:
Patch Set 11:
Patch Set 10:
Patch Set 10: Code-Review+1
My guess is that we'd need to combine this logic into some other location so we don't duplicate the if/else all over the code base.
we can do the same inside postcar_loader.c run_postcar_phase() and check like below ?
void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog = PROG_INIT(PROG_POSTCAR, CONFIG_CBFS_PREFIX "/postcar");
if (CONFIG(RAMSTAGE_LOADS_FROM_RAMSTAGE)) { run_ramstage_phase(&pcf); /* control shouldn't come here */ } run_phase(pcf, prog, STAGE_POSTCAR, CBMEM_ID_AFTER_CAR);}
That would work.
Done.
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34754
to look at the new patch set (#13).
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
arch/x86: Add provision to skip postcar and load ramstage
This patch adds required provision in postcar_loader code to pick ramstage directly from romstage and avoid postcar as intermediate stage for car tear down.
When a platform is not using postcar stage it will use ramstage, hence it's by definition not tearing down cache-as-ram from within romstage prior to loading ramstage. Because of this property there's no need to migrate CAR_GLOBAL variables to cbmem.
Change-Id: I6f1d93ae0f8d957bf9c15e358bc13039a300c4ca Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/34754/13
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/13/src/arch/x86/postcar_loade... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34754/13/src/arch/x86/postcar_loade... PS13, Line 227: void run_ramstage_phase(struct postcar_frame *pcf) Shouldn't this be static now? No need to expose anything beyond run_postcar_phase(). This change and https://review.coreboot.org/c/coreboot/+/34751 should be combined, I think.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34754/13/src/arch/x86/postcar_loade... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34754/13/src/arch/x86/postcar_loade... PS13, Line 227: void run_ramstage_phase(struct postcar_frame *pcf)
Shouldn't this be static now? No need to expose anything beyond run_postcar_phase(). […]
Done
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34754 )
Change subject: arch/x86: Add provision to skip postcar and load ramstage ......................................................................
Abandoned
clubbed with https://review.coreboot.org/c/coreboot/+/34751
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Subrata Banik (Code Review)