Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34805 )
Change subject: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34805/6/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34805/6/src/arch/x86/postcar_loader... PS6, Line 162: postcar_frame_add_mtrr(pcf, addr, size, type);
Implementation of postcar_frame_add_mtrr() is capable of splitting the region to multiple MTRRs. Implementation of enable_top_of_dram_cache() is not.
enable_top_of_dram_cache() is to just enable intermediate caching, anyway the range we are passing into enable_top_of_dram_cache() always existed into postcar frame so we shouldn't be worried for next stage as post car frame will pass the number of MTRR to reset and set while setting up next stage stack top/setting up MTRR range.
The approach proposed here adds new alignment requirements, so you cannot simply change calls from postcar_frame_add_mtrr() to postcar_frame_setup_top_of_dram_usage(), like your followup work seems to propose.
postcar_frame_setup_top_of_dram_usage() functionally will additionally enable caching for top of DRAM along with existing postcar_frame_add_mtrr() work what it was doing previously.
I believe that as we discussed dropping CPU_ADDR_BITS and cpu_phys_address_size() I recommended not to use set_var_mtrr() but to replay the values from the generated frame instead.
set_var_mtrr() been used here to enable immediate caching of give range. if we had to relying on "generated frame", it would be taken care during next stage entry point while setting up new MTRR range. In that way, we won't be getting any benefit as mentioned here.