Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Can you maybe share a bootlog with DISPLAY_MTRR set?
https://pastebin.ubuntu.com/p/6yFNJQwK6g/
build is dirty since apu2 dos not have DISPLAY_MTRR and HAVE_DISPLAY_MTRR configured by default and also added APIC ID before each MTRR dump. Here's the diff: https://pastebin.ubuntu.com/p/567t6zHDsZ/
I see what is going on. The BSP MTRR are modified in postcar, but the AP ones are not. Then during the CPU init no syncing is done. So the BSP MTRR don't match the AP ones. The parallel mp init handles this automatically. So moving away from the lapic init would automatically fix it.
Another option is to save the BSP MTRR in model_xx_init() and apply it to the APs.
It would be better to do the other way around: restore the lost UC entry on BSP, since it seems to be some HW init part needed for functional C6 state.
Anyway, all this is not really related to this patch, is it?
No it isn't. Sorry if I started a too long discussion in an inappropriate place. Thank you for your opinions.