<p><a href="https://review.coreboot.org/23580">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h">File src/soc/intel/cannonlake/chip.h:</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/23580/3/src/soc/intel/cannonlake/chip.h@89">Patch Set #3, Line 89:</a> <code style="font-family:monospace,monospace">power limit</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the unit?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h@89">Patch Set #3, Line 89:</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;">Package PL1 - Package long duration turbo mode power limit<br>    * Package PL2 - Short Duration Turbo Mode<br>     * Package PL1 - Package short duration turbo mode power limit */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These comments don't seem to match the fields below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h@92">Patch Set #3, Line 92:</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;">uint32_t PPL1Power;<br>     uint8_t PPL2_enable;<br>  uint32_t PPL2Power;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I know this file is not very consistent about the naming, but it would be better to use:<br>uint32_t package_pl1;<br>uint8_t package_pl2_enable;<br>uint32_t package_pl2;</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, would it be safe to assume that package_pl2_enable should be set to 1 if package_pl2 is provided? i.e. do we need to accept a separate package_pl2_enable from mainboard?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.c">File src/soc/intel/cannonlake/chip.c:</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/23580/3/src/soc/intel/cannonlake/chip.c@183">Patch Set #3, Line 183:</a> <code style="font-family:monospace,monospace">params_t</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can we name this tparams or test_params? params_t looks like a typedef.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.c@273">Patch Set #3, Line 273:</a> <code style="font-family:monospace,monospace">Powermanagement</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">space after power?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/23580">change 23580</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/23580"/><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: I81bd04f5de05543ad00ce2562839a51a5c0ae047 </div>
<div style="display:none"> Gerrit-Change-Number: 23580 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Lijian Zhao <lijian.zhao@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 03 Feb 2018 05:50:57 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>