<p>Furquan Shaikh has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/27251">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">soc/intel/common: Disable GPEs just before enabling SMIs<br><br>Call to pmc_disable_all_gpe is required before enabling SMIs to ensure<br>that we do not end up in a recursive SMI handler loop as mentioned in<br>change 74145f7 (intel/common/pmc: Disable all GPEs during<br>pmc_init). Thus, this call was added at the end of<br>pmc_fill_power_state as we want to ensure that all the GPE registers<br>are backed up before being cleared for identifying the wake source in<br>ramstage.<br><br>This resulted in a side-effect on APL where pmc_fixup_power_state was<br>called much later in the boot process. Even though we have got rid of<br>pmc_fixup_power_state, this change moves the call to<br>pmc_disable_all_gpe to happen just before enabling SMIs. This helps to<br>keep the disabling of GPEs logically before the enabling of SMIs and<br>any clean ups that happen in pmc or soc-specific code should not<br>affect the state of GPEs.<br><br>BUG=b:110836465<br>TEST=Verified that wake sources are correctly identified on KBL and<br>APL. Also, no SMI handler issues observed when resuming.<br><br>Change-Id: I122a8118edcec117f25beee71a23c0a44ae862ed<br>Signed-off-by: Furquan Shaikh <furquan@google.com><br>---<br>M src/soc/intel/common/block/pmc/pmclib.c<br>M src/soc/intel/common/block/smm/smm.c<br>2 files changed, 9 insertions(+), 12 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/27251/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/soc/intel/common/block/pmc/pmclib.c b/src/soc/intel/common/block/pmc/pmclib.c</span><br><span>index 0795990..339e674 100644</span><br><span>--- a/src/soc/intel/common/block/pmc/pmclib.c</span><br><span>+++ b/src/soc/intel/common/block/pmc/pmclib.c</span><br><span>@@ -416,18 +416,6 @@</span><br><span> ps->prev_sleep_state = pmc_prev_sleep_state(ps);</span><br><span> printk(BIOS_DEBUG, "prev_sleep_state %d\n", ps->prev_sleep_state);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /*</span><br><span style="color: hsl(0, 100%, 40%);">- * GPEs need to be disabled before enabling SMI. Otherwise, it could</span><br><span style="color: hsl(0, 100%, 40%);">- * lead to SMIs being triggered in coreboot preventing the progress of</span><br><span style="color: hsl(0, 100%, 40%);">- * normal boot-up. However, GPEs should not be disabled as part of</span><br><span style="color: hsl(0, 100%, 40%);">- * pmc_gpe_init which happens in bootblock. Otherwise,</span><br><span style="color: hsl(0, 100%, 40%);">- * pmc_fill_power_state would read GPE0_EN registers as all 0s thus</span><br><span style="color: hsl(0, 100%, 40%);">- * losing information about the wake source. Hence,</span><br><span style="color: hsl(0, 100%, 40%);">- * pmc_disable_all_gpe() is placed here after GPE0_EN registers are</span><br><span style="color: hsl(0, 100%, 40%);">- * saved in chipset_power_state.</span><br><span style="color: hsl(0, 100%, 40%);">- */</span><br><span style="color: hsl(0, 100%, 40%);">- pmc_disable_all_gpe();</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> return ps->prev_sleep_state;</span><br><span> }</span><br><span> </span><br><span>diff --git a/src/soc/intel/common/block/smm/smm.c b/src/soc/intel/common/block/smm/smm.c</span><br><span>index e8c5245..6059995 100644</span><br><span>--- a/src/soc/intel/common/block/smm/smm.c</span><br><span>+++ b/src/soc/intel/common/block/smm/smm.c</span><br><span>@@ -46,6 +46,15 @@</span><br><span> pmc_disable_std_gpe(PME_B0_EN);</span><br><span> </span><br><span> /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * GPEs need to be disabled before enabling SMI. Otherwise, it could</span><br><span style="color: hsl(120, 100%, 40%);">+ * lead to SMIs being triggered in coreboot preventing the progress of</span><br><span style="color: hsl(120, 100%, 40%);">+ * normal boot-up. This is done as late as possible so that</span><br><span style="color: hsl(120, 100%, 40%);">+ * pmc_fill_power_state can read the correct state of GPE0_EN* registers</span><br><span style="color: hsl(120, 100%, 40%);">+ * and not lose information about the wake source.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ pmc_disable_all_gpe();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span> * Enable SMI generation:</span><br><span> * - on APMC writes (io 0xb2)</span><br><span> * - on writes to SLP_EN (sleep states)</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/27251">change 27251</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/27251"/><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: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I122a8118edcec117f25beee71a23c0a44ae862ed </div>
<div style="display:none"> Gerrit-Change-Number: 27251 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Furquan Shaikh <furquan@google.com> </div>