Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
Patch Set 4:
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Ya. I'm not sure we're in agreement on the best option forward there. Thanks for the pointer.
CB:35029 will hopefully address the concern raised due to set_var_mtrr() usage inside postcar_frame_setup_top_of_dram_usage() function. As we will pass aligned cbmem_top() address now to postcar_frame_setup_top_of_dram_usage(), so set_var_mtrr() should limit into single MTRR itself.
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
I'm thinking that might be true as well. Anyway, we can have the discussion on that commit.
Just now i have finished running 500 cycle each with POSTCAR_STAGE=y and POSTCAR_STAGE=n coreboot image on same unit.
I have analysis the boot average time, here is my observation
1. Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
2. Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Now for the 7ms delta that we were referring above is higher in case of POSTCAR_STAGE=n is due to stage_cache_add() call while running run_ramstage_phase().
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
In case of romstage loading ramstage directly due to stage_cache_add with higher ramstage size we are seeing that delta times, if i remove those stage_cache_add() in normal path and pick stages from cbfs even on S3. i don't see those 7ms extra.
We should discuss the scope of POSTCAR_STAGE=n option in that CL itself rather here but like to highlight that we should also consider the user configuration choice of stages like to load and SPI savings as well.