Kyösti Mälkki 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 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@16 PS9, Line 16: TEST=Verified normal boot time on CML-Hatch with latest coreboot That's SOC_INTEL_COMETLAKE, building from soc/intel/cannonlake, and this commit does not even touch that?
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@22 PS9, Line 22: Total Time: 910ms Huh? You only move the function call from one file to another and you get this improvement??
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@28 PS9, Line 28: Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Remove my signed-off-by.
While I may have suggested that we need some common API around TSEG, I never co-authored this commit or the source file changes you have made here.
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return; I did see Aaron had made a comment around these lines on patchset #7 and I choose to disagree with him here.
Use of conditional here is not logical and seems like a workaround. If you are allowing TSEG as WB with TSEG_STAGE_CACHE, what is the reasoning for disallowing it without TSEG_STAGE_CACHE? I feel this is just creating conditions where you test platform with HAVE_ACPI_RESUME=n but they potentially start to have random boot failures (even on cold boots) with HAVE_ACPI_RESUME=y,TSEG_STAGE_CACHE=y. That's what we saw when we had marked low-ram 0..CACHE_TMP_RAMTOP as WB...
I think it is better to have fill_postcar_frame() in platform code explicitly call this instead of embedding it in run_postcar_phase():
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 210: MTRR_TYPE_WRBACK); There were many iterations, I don't know which were valid for which platform. My conclusion was WB for CBMEM is sometimes harmful, so I would have assumed WB for TSEG is equally sometimes harmful. If WC provided perfomance boost while WB was unstable, the type can be provided as parameter.
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 220: See comments above. I think this needs to be called from platform code not common code.
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... PS9, Line 89: run_postcar_phase(&pcf); Everything about postcar in amd/picasso is wrong and work-in-progress. Since you don't need to, better not make soc/amd changes any dependency to this work, just let them catch up later when API is ready.