[coreboot-gerrit] Change in coreboot[master]: Documentation/Intel: Add MultiProcessorInit documentation

[Nico Huber (Code Review)]

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/MultiProcessorInit/MultiProcessorInit.md
File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:

https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@7
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/MultiProcessorInit/MultiProcessorInit.md@25
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/MultiProcessorInit/MultiProcessorInit.md@35
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/MultiProcessorInit/MultiProcessorInit.md@68
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.



-- 
To view, visit https://review.coreboot.org/25921
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde
Gerrit-Change-Number: 25921
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi at google.com>
Gerrit-Comment-Date: Tue, 01 May 2018 12:22:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180501/08be726d/attachment.html>