Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36914 )
Change subject: binaryPI: implement C bootblock ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... File src/drivers/amd/agesa/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 38: mtrr = (mtrr_cap.lo & MTRR_CAP_VCNT) - SOC_EARLY_VMTRR_FLASH;
These particular MTRRs are used by AGESA, so they should stay as they are.
Looks another reason is that MTRR's are shared accross cores. You might want to copy that comment from stoneyridge code.
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 42: CONFIG_PI_AGESA_CAR_HEAP_BASE, : CONFIG_PI_AGESA_HEAP_SIZE
Stoneyridge does not guard it either. These values are fixed and should not be modified/configured since they are related to AGESA
Sure, but it's not because values are fixed by something else that you don't want a (compiletime) check to see if what you are doing makes sense.
https://review.coreboot.org/c/coreboot/+/36914/16/src/drivers/amd/agesa/boot... File src/drivers/amd/agesa/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/16/src/drivers/amd/agesa/boot... PS16, Line 58: void maybe gcc optimises things if you add the __noreturn flag?