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

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde </div>
<div style="display:none"> Gerrit-Change-Number: 25921 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-CC: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 01 May 2018 12:22:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>