Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35025 )
Change subject: soc/{amd, intel}: Make use of common enable_tseg_cache() API ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Subrata, you realise this is conflicting CB:34893
CB:34893 is supposed to refactoring code to fit into common romstage.c file if i understood that correctly.
And together with CB:34894 we want to consistently have any postcar_frame creations across all platforms dealt with in a file called memmap.c that is unconditionally built for romstage, postcar and ramstage. This makes it easier to review and apply fixes for MTRR alignment and gives you one common point in cpu/intel/car/romstage.c where to enable TSEG cache. Or, use the already existing postcar_frame_add_common_mtrrs().
A lot of these decisions about API depends on whether Aaron or Furguain will accept the use of set_var_mtrr() late in romstage. And whether 'cbmem -t' shows any significant performance boost with your changes. TSEG WB 7ms was a 90% enhancement for memcpy() in stage cache, so it will be accepted in one form or other.
Also, as you don't have stack guard checking in place for intel/cannonlake, I believe your POSTCAR_STAGE=n ipmlementation currently smashes the BSP stack, overwrites VBOOT2_WORK and VBOOT2_TPM_LOG during run_ramstage(). See CB:34882. Just one more reason I think your effort should be on getting CB:34893 and CB:34894 reviewed.