power/cometlake-discrete: don't treat orderly shutdown as power failure

The common Intel code looks at IN_PGOOD_ALL_CORE in steady-state S3 and
S0, forcing a shutdown if any part of it goes away. By including the
DRAM rails, we make it think there's a power failure during orderly
shutdown because the PCH turns off the things it controls before the EC
notices it's transitioning (they're not in lockstep, the PCH goes as
fast as it wants and the EC must catch up).

This means the only time we can actually monitor the DRAM rails is when
bringing them up, since the PCH may turn them off.

BUG=b:147461413
TEST=puff now responds to power button press in S5 after shutdown, and
     does not log power failure.
BRANCH=None

Change-Id: Ia18c06ea2eec54c4ff0ccad3c91ba29547014f9c
Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2009541
Reviewed-by: Andrew McRae <amcrae@chromium.org>

1 files changed, 12 insertions, 6 deletions

diff --git a/power/cometlake-discrete.h b/power/cometlake-discrete.h
index 3e9a01baaa..14e18e91e6 100644
--- a/power/cometlake-discrete.h
+++ b/power/cometlake-discrete.h
@@ -48,11 +48,16 @@
  *
  * Transition S5->S3 only involves turning on the DRAM power rails which are
  * controlled directly from the PCH, so this condition doesn't require any
- * special code- just check that the DRAM rails are good.
+ * special code, except this collection of signals is also polled in POWER_S3
+ * and POWER_S0 states.
+ *
+ * During normal shutdown the PCH will turn off the DRAM rails before the EC
+ * notices, so if this collection includes those rails a normal shutdown will be
+ * treated as a power failure so the system immediately drops to G3 rather than
+ * doing an orderly shutdown. This must only include those signals that are
+ * EC-controlled, not those controlled by the PCH.
  */
-#define IN_PGOOD_ALL_CORE                                                     \
-	(CHIPSET_G3S5_POWERUP_SIGNAL | POWER_SIGNAL_MASK(PP2500_DRAM_PGOOD) | \
-	 POWER_SIGNAL_MASK(PP1200_DRAM_PGOOD))
+#define IN_PGOOD_ALL_CORE CHIPSET_G3S5_POWERUP_SIGNAL
 
 /*
  * intel_x86 power mask for S0 all-OK.
@@ -60,8 +65,9 @@
  * This is only used on power task init to check whether the system is powered
  * up and already in S0, to correctly handle switching from RO to RW firmware.
  */
-#define IN_ALL_S0                                       \
-	(IN_PGOOD_ALL_CORE | IN_ALL_PM_SLP_DEASSERTED)
+#define IN_ALL_S0                                                   \
+	(IN_PGOOD_ALL_CORE | POWER_SIGNAL_MASK(PP2500_DRAM_PGOOD) | \
+	 POWER_SIGNAL_MASK(PP1200_DRAM_PGOOD) | IN_ALL_PM_SLP_DEASSERTED)
 
 #define CHARGER_INITIALIZED_DELAY_MS 100
 #define CHARGER_INITIALIZED_TRIES 40