Aaron Durbin 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 4:
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you want it conditionally enabled even within the platform, intel/cannonlake here?
By 'this' I meant enabling WB MTRR to speed up loading. I think there's merit to adding smm region to the postcar solution -- just like apollolake. That is definitely not there in cannonlake. And yes we should probably not open code that everywhere...
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you mean, we should check if TSEG_STAGE_CACHE Kconfig is enable by platform, if yes then only cache tseg region else not ? thus might be good feedback.
I thought keeping inside common x86 code might help platform to call the required function ?
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
All that said, I don't think the top of ram nomenclature is helpful here any longer nor do I think we need an API that doesn't save us anything over postcar_frame_add_mtrr() for the top of ram WB case.
I know these patches have churned a bit, but that's ok. I think we've sorted out previous assumptions and learned what's currently problematic.