Nico Huber has posted comments on this change. ( https://review.coreboot.org/25921 )
Change subject: Documentation/Intel: Add MultiProcessorInit documentation ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 7: for Intel 9th Gen (Cannon Lake) and beyond CPUs
i have made this clear why we need to create this API interface in this section.
Although i agree this is very generic implementation and dependent on mp_init.c so going forward we might feel a feed to make it generic, today we can't do because it has some EDK2 header dependencies which is only available for intel UDK2017 platform (CNL onwards).
I don't care. If somebody made a mess with these headers, fix it please. Something like "vendor x does weird stuff with API headers y" shouldn't stop progress of coreboot.
On second thought, you don't have to do anything. A correct solu- tion would let the user decide, based on a properly guarded Kconfig option. And then you don't have any compilation conflicts because the code would simply not be compiled.
btw, why you might need to have 2 document, i'm not going to explain how mp_init works on x86 platform, those are already in SDM, this document is to only talk about why we are creating EDK2 interface in coreboot and how intel thinks this can help our partners design.
That's ok, I didn't mean a general document about MP init. Just in case you can figure out the header problem, the API documen- tation should be separated from the justification / Intel speci- fics.
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 25: closed source in nature
i don;t understand what you mean by can't be trusted ?
It is my try to interpret English (I'm not a native speaker). I tried to make something up that "closed source in nature" could mean.
If you would publish it, would it change its nature? That seems wrong to me. But I might just need help how the term "nature" is used in English.
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 35: It’s also possible for coreboot to extend its own : support model and create a sets of APIs which later can be used by FSP to run CPU feature : programming using coreboot published APIs
i guess you have missed problem statement, you are referring to proposal and try to understand why i […]
Sorry I think we're talking past each other.
Just this sentence alone. You say "coreboot [can] create a set of APIs which later can be used by FSP". I say, coreboot has created a set of APIs (cpu/x86/mp.h) and they can be published and used by FSP now.
Those are two conflicting statements no matter the problem state- ment, AFAICS.
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 68: 1. coreboot was using SkipMpInit=1 which will skip entire FSP CPU feature programming.
not sure what you mean by coreboot? this is a UPD which is intel fsp specific. […]
I meant "coreboot was using SkipMpInit=1" isn't true for all boards. coreboot has a setting "FspSkipMpInit" that is set per board in the devicetree. So a board maintainer already had the choice to enable MpInit in FSP.
-
Nico Huber (Code Review)