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 9:
(5 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
Ah right.. you were touching common code, that's where cometlake got it from. […]
see my comments below
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@22 PS9, Line 22: Total Time: 910ms
It was not so obvious at first sight that pretty much every x86 platform gets TSEG as WB with this c […]
earlier i have platform specific CLs to enable thus feature 1. CB:34995 as common API 2. CB:35025 make other soc using specific code to refer #1 common API 3. CB:35026 add for CNL and ICL TSEG caching
Aaron had commented to make it common hence i have move everything into 1 CL and here
now looks like you are advocating for split approach like earlier.
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 h […]
will wait for Aaron to reply as its been originally done in that way
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. […]
WB for TSEG been working for almost all platform, CB marking WB has different problem because codes are running from cache and we are going to run invd during tear down. code will only run from TSEG in S3 resume so i don't think we will run into any problem with existing code setup
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.
i have replied in my latest comments