Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36914 )
Change subject: binaryPI: implement C bootblock ......................................................................
Patch Set 15:
(7 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 25: #define SOC_EARLY_VMTRR_TEMPRAM 3
unused?
Done
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;
get_free_var_mtrr()? or is something else touching these MTRR's later on such that you want to use a […]
These particular MTRRs are used by AGESA, so they should stay as they are.
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 39: CONFIG_ROM_SIZE
OPTIMAL_CACHE_ROM_SIZE
Done
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 39: FLASH_BASE_ADDR
OPTIMAL_CACHE_ROM_BASE
Done
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
You might want to do a buildtime assertion that CONFIG_PI_AGESA_HEAP_SIZE is a power of 2 and that C […]
Stoneyridge does not guard it either. These values are fixed and should not be modified/configured since they are related to AGESA
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 46: msr_t msr = rdmsr(0x1B); : msr.lo |= 1 << 11; : wrmsr(0x1B, msr);
just enable_lapic() here?
Done
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 52: bootblock_c_entry
I'd adda ap_bootblock_c_entry and call it from assembly to avoid branching twice on the LAPIC_BASE_M […]
Done