<p><a href="https://review.coreboot.org/28932">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/28932/2//COMMIT_MSG">Commit Message:</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/28932/2//COMMIT_MSG@9">Patch Set #2, Line 9:</a> <code style="font-family:monospace,monospace">The logic for setting PCIEXP_WAKE_DIS bit is backwards</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The function you are changing does not enable or disable PCIEXP_WAKE_DIS.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28932/2//COMMIT_MSG@21">Patch Set #2, Line 21:</a> <code style="font-family:monospace,monospace">TEST</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am really surprised that the changes in this CL actually help.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c">File src/soc/intel/skylake/acpi.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/28932/2/src/soc/intel/skylake/acpi.c@691">Patch Set #2, Line 691:</a> <code style="font-family:monospace,monospace">soc_fill_acpi_wake</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This function, as the comment says above, is used to only identify the wake source during resume. It does not actually write to PM1_EN_STS register to enable/disable any wake bits.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@719">Patch Set #2, Line 719:</a> <code style="font-family:monospace,monospace">pm1_en |= PCIEXPWAK_STS;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The intent of this code is to check if DSX_EN_WAKE_PIN was enabled for wake and if yes, then set PCIEXPWAK_STS to mask the pm1_sts bits in order to identify the true wake source. It is not used to enable or disable wake using the WAKE# pin. That is the reason why it is using PCIEXPWAK_STS and not PCIEXPWAK_DIS macro.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28932/2/src/soc/intel/skylake/acpi.c@722">Patch Set #2, Line 722:</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;">*pm1 = ps->pm1_sts & pm1_en;<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is where it masks the status read from PM1_STS using the valid bits that were set in pm1_en. </p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>The logic here is that some bits in PM1_EN get lost when waking from Deep Sx. This is because the bits reside in different power wells as indicated by the comment in line 710. Hence, the code in this function:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1. Reads current pm1_en from ps->pm1_en into pm1_en<br>2. ORs pm1_en with PWRBTN_STS and PCIEXPWAK_STS if DSX_EN_WAKE_PIN is set.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This mask obtained using #1 and #2 is used to identify the wake source from pm1_sts.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/28932">change 28932</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/28932"/><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: Id8b14ae2ae4d97e184906dec468b405134d590da </div>
<div style="display:none"> Gerrit-Change-Number: 28932 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Nick Vaccaro <nvaccaro@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Duncan Laurie <dlaurie@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Lijian Zhao <lijian.zhao@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nick Vaccaro <nvaccaro@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 05 Oct 2018 06:30:13 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>