[coreboot-gerrit] Change in coreboot[master]: soc/intel/common: Disable GPEs just before enabling SMIs

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/27251


Change subject: soc/intel/common: Disable GPEs just before enabling SMIs
......................................................................

soc/intel/common: Disable GPEs just before enabling SMIs

Call to pmc_disable_all_gpe is required before enabling SMIs to ensure
that we do not end up in a recursive SMI handler loop as mentioned in
change 74145f7 (intel/common/pmc: Disable all GPEs during
pmc_init). Thus, this call was added at the end of
pmc_fill_power_state as we want to ensure that all the GPE registers
are backed up before being cleared for identifying the wake source in
ramstage.

This resulted in a side-effect on APL where pmc_fixup_power_state was
called much later in the boot process. Even though we have got rid of
pmc_fixup_power_state, this change moves the call to
pmc_disable_all_gpe to happen just before enabling SMIs. This helps to
keep the disabling of GPEs logically before the enabling of SMIs and
any clean ups that happen in pmc or soc-specific code should not
affect the state of GPEs.

BUG=b:110836465
TEST=Verified that wake sources are correctly identified on KBL and
APL. Also, no SMI handler issues observed when resuming.

Change-Id: I122a8118edcec117f25beee71a23c0a44ae862ed
Signed-off-by: Furquan Shaikh <furquan at google.com>
---
M src/soc/intel/common/block/pmc/pmclib.c
M src/soc/intel/common/block/smm/smm.c
2 files changed, 9 insertions(+), 12 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/27251/1

diff --git a/src/soc/intel/common/block/pmc/pmclib.c b/src/soc/intel/common/block/pmc/pmclib.c
index 0795990..339e674 100644
--- a/src/soc/intel/common/block/pmc/pmclib.c
+++ b/src/soc/intel/common/block/pmc/pmclib.c
@@ -416,18 +416,6 @@
 	ps->prev_sleep_state = pmc_prev_sleep_state(ps);
 	printk(BIOS_DEBUG, "prev_sleep_state %d\n", ps->prev_sleep_state);
 
-	/*
-	 * GPEs need to be disabled before enabling SMI. Otherwise, it could
-	 * lead to SMIs being triggered in coreboot preventing the progress of
-	 * normal boot-up. However, GPEs should not be disabled as part of
-	 * pmc_gpe_init which happens in bootblock. Otherwise,
-	 * pmc_fill_power_state would read GPE0_EN registers as all 0s thus
-	 * losing information about the wake source. Hence,
-	 * pmc_disable_all_gpe() is placed here after GPE0_EN registers are
-	 * saved in chipset_power_state.
-	 */
-	pmc_disable_all_gpe();
-
 	return ps->prev_sleep_state;
 }
 
diff --git a/src/soc/intel/common/block/smm/smm.c b/src/soc/intel/common/block/smm/smm.c
index e8c5245..6059995 100644
--- a/src/soc/intel/common/block/smm/smm.c
+++ b/src/soc/intel/common/block/smm/smm.c
@@ -46,6 +46,15 @@
 	pmc_disable_std_gpe(PME_B0_EN);
 
 	/*
+	 * GPEs need to be disabled before enabling SMI. Otherwise, it could
+	 * lead to SMIs being triggered in coreboot preventing the progress of
+	 * normal boot-up. This is done as late as possible so that
+	 * pmc_fill_power_state can read the correct state of GPE0_EN* registers
+	 * and not lose information about the wake source.
+	 */
+	pmc_disable_all_gpe();
+
+	/*
 	 * Enable SMI generation:
 	 *  - on APMC writes (io 0xb2)
 	 *  - on writes to SLP_EN (sleep states)

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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I122a8118edcec117f25beee71a23c0a44ae862ed
Gerrit-Change-Number: 27251
Gerrit-PatchSet: 1
Gerrit-Owner: Furquan Shaikh <furquan at google.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180627/66609b09/attachment-0001.html>