Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 104: * min base bit set and maximum size bit set. */ https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 108: mtrr_size = 1 << addr_lsb; Some prefere the ternary operator to directly graps that it’s about assignment to `mtrr_size`, but it’s subjective and not important.
mtrr_size = (addr_lsb > size_msb) ? 1 << size_msb : 1 << addr_lsb;
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 117: * a future solution, such as postcar_loader. */ https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 121: int used_var_mtrrs; Can these be made of type unsigned or size_t?
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 127: int size_t/unsigned int
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 135: Returns < 0 Put that on the next line?