Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 25: the MTRR are filed. */
And you are assuming there is no overlap of mtrrs.
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 26: uint32_t
why is this fixed width? size_t should be sufficient.
The set_var_mtrr only works uint32_t so that makes things easier.
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 29: u8
int
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 33: msr_t mtrr = rdmsr(MTRR_PHYS_MASK(i));
Add a comment about how we're ignoring caching type and why?
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 58: * all family F models.
So did we conclude that those models have an extended model number of 0? Or that it adds up to 0xf with the extended model number?
yes, Intel Netburst has exactly 0xf as model number with extended model (0) added .
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 82: >
You had < before. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 84: if (cache_used + mtrr_mask_size > MAX_CPU_CACHE)
This doesn't really prevent one from exceeding the MAX_CPU_CACHE as mtrr_mask_size may already have […]
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 104: base
Shouldn't this be mtrr_base ?
Done
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... PS12, Line 25: filed
'filled' […]
Done