Angel Pons 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 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.
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).
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?