Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36914 )
Change subject: AGESA, binaryPI: implement C bootblock ......................................................................
Patch Set 28:
(3 comments)
See comments in CB:37440, we may need to do couple more things in fam14 and binaryPI bootblocks.
https://review.coreboot.org/c/coreboot/+/36914/28/src/drivers/amd/agesa/boot... File src/drivers/amd/agesa/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/28/src/drivers/amd/agesa/boot... PS28, Line 23: static void init_mmio(void) The name is bad, enable_pci_mmconf() would be accurate. And this should be shared from amd/stoneyridge instead of a new copy.
https://review.coreboot.org/c/coreboot/+/36914/28/src/drivers/amd/agesa/boot... PS28, Line 40: set_var_mtrr(mtrr, OPTIMAL_CACHE_ROM_BASE, OPTIMAL_CACHE_ROM_SIZE, I think there was previous comment this fails when cores share MTRRs. That is, this creates unnecessary duplicate VAR MTRR usage. Also, I believe vendorcode side uses fixed indexes, so dynamic ones here seem like bad idea to me.
https://review.coreboot.org/c/coreboot/+/36914/28/src/drivers/amd/agesa/boot... PS28, Line 57: void (*ap_romstage_entry)(void) = (void (*)(void))get_ap_entry_ptr();
function definition argument 'void' should also have an identifier name
Do we need the cast here at all?