Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36914 )
Change subject: binaryPI: implement C bootblock ......................................................................
Patch Set 14:
(10 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?
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 specific one?
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 39: FLASH_BASE_ADDR OPTIMAL_CACHE_ROM_BASE
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 39: CONFIG_ROM_SIZE OPTIMAL_CACHE_ROM_SIZE
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 CONFIG_PI_AGESA_CAR_HEAP_BASE is aligned to CONFIG_PI_AGESA_HEAP_SIZE. Not sure if that is already done somewhere.
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?
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_MSR reads.
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/cach... File src/drivers/amd/agesa/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/cach... PS14, Line 40: remove change?
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... PS14, Line 57: #if !CONFIG(ROMCC_BOOTBLOCK) : set_ap_entry_ptr(agesa_call); : #endif You can probably use C code instead of CPP. It should get garbage collected on the ROMCC_BOOTBLOCK path.
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... PS14, Line 62: : #if CONFIG(ROMCC_BOOTBLOCK) : void *asmlinkage romstage_main(unsigned long bist) : #else : asmlinkage void car_stage_entry(void) : #endif you can avoid the ifdefs if car_stage_entry just calls romstage_main.