Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
Patch Set 3:
Patch Set 2:
The thing with MTRRs is that there's a limited number of them. If we run out of MTRRs, we can't properly describe memory regions.
Valid point, today we have 6 MTRR max to provide the BIOS resource snapshot which should be able to manage the required BIOS used memory description
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
That's both not true.
i don't understand your comments here. i have provided the value so a simple math will tell that the assumption made by CB:36216 is not true
TSEG = 0x7b000000 Assuming you don't know the value of DPR and reseved top_of_memory = 0x77000000 so the result would be 0x4000000 its not 0 right ?
The thing with these numbers is that they may change between platforms and between different configurations (e.g. iGPU enabled/disabled).
yes, also based on debug options like tracehub, PRMRR etc those number would chance.
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
Making a bigger memory range cacheable has usually better results, though. Unless something must not be cached. Which nobody said yet.
But do we have that much bigger cache either right?
Marking a memory range as cacheable does not mean it will always be put in cache. The cache size does not matter when marking a memory range as cacheable. For things like memory-mapped registers, we do not want caching: if writes hit the cache, they don't reach the actual registers; for reads, the value of some registers can be changed by the hardware and the cache would not be updated.
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Not true.
I have given the calculation above, can you please help to understand why its still not true ?
AFAIUI, you said that "TSEG - DPR - reserved - top_of_memory == 0" is not a valid assumption. Why so? In which case does this assumption not hold?
this reserve size should accounted for all reserve range available (this was old implementation which got stabled after CB:36216 hence we can ignore this part) if that not happen then this assumption won't hold good.