Michał Żygowski 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)
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. […]
Stoneyridge kept the amd_initmmio naming and does not carve out the MMIOCONF initialization from that function. Do you mean to create a separate patch to implement common enable_pci_mmconf() for AMD boards? Where exactly? AMD blocks?
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. […]
Yes, we can hardcode the MTRR indexes. I saw they are the same for family14 and binaryPI 16h and probably are for the rest of families currently present in the tree.
Any idea how to avoid duplication?
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();
Do we need the cast here at all?
The compiler doesn't complain and the boards boot, so I guess not.