ap_pwrseq: Use interrupts to maintain power signal mask

Instead of recalculating the entire power mask every change,
use interrupts to update the mask.
Also include signals that need to be polled (such as board provided
signals) and output signals.

BUG=b:201000950
TEST=zmake build nivviks
BRANCH=none

Signed-off-by: Andrew McRae <amcrae@google.com>
Change-Id: I12b8ab1b82ac212581be8a2b6307e46b1395f001
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3516769
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-by: Li1 Feng <li1.feng@intel.com>

7 files changed, 107 insertions, 55 deletions

diff --git a/zephyr/subsys/ap_pwrseq/include/power_signals.h b/zephyr/subsys/ap_pwrseq/include/power_signals.h
index 8c168077f7..cdeb9ec783 100644
--- a/zephyr/subsys/ap_pwrseq/include/power_signals.h
+++ b/zephyr/subsys/ap_pwrseq/include/power_signals.h
@@ -203,20 +203,22 @@ void power_signal_init(void);
  * @brief Power signal interrupt handler
  *
  * Called when an input signal causes an interrupt.
+ *
+ * @param signal The power_signal that has changed.
+ * @param value The new value of the signal
  */
-void power_signal_interrupt(void);
-
-typedef uint32_t power_signal_mask_t;
+void power_signal_interrupt(enum power_signal signal, int value);
 
 /**
- * @brief Update the stored mask of power signals.
+ * Interrupt based signals update a bitfield mask, which can be
+ * used to wait for signal changes.
  */
-void power_update_signals(void);
+typedef uint32_t power_signal_mask_t;
 
 /**
- * @brief Get the current power signals as a mask.
+ * @brief Get the current interrupt bitfield
  *
- * @return Power signals as a mask.
+ * @return Interrupt power signals as a mask.
  */
 power_signal_mask_t power_get_signals(void);
 
diff --git a/zephyr/subsys/ap_pwrseq/power_signals.c b/zephyr/subsys/ap_pwrseq/power_signals.c
index 30961acd38..bbcd7ebcae 100644
--- a/zephyr/subsys/ap_pwrseq/power_signals.c
+++ b/zephyr/subsys/ap_pwrseq/power_signals.c
@@ -6,6 +6,7 @@
 #include <kernel.h>
 #include <toolchain.h>
 #include <logging/log.h>
+#include <sys/atomic.h>
 
 #include <power_signals.h>
 
@@ -75,25 +76,22 @@ DT_FOREACH_STATUS_OKAY_VARGS(intel_ap_pwrseq_adc, GEN_PS_ENTRY,
 			     PWR_SIG_SRC_ADC, PWR_SIG_TAG_ADC)
 };
 
-static power_signal_mask_t power_signals;
-static power_signal_mask_t debug_signals;
+#define PWR_SIGNAL_POLLED(id)	PWR_SIGNAL_ENUM(id),
 
-void power_update_signals(void)
-{
-	power_signal_mask_t n = 0;
+/*
+ * List of power signals that need to be polled.
+ */
+static const uint8_t polled_signals[] = {
+DT_FOREACH_STATUS_OKAY(intel_ap_pwrseq_external, PWR_SIGNAL_POLLED)
+};
 
-	for (int i = 0; i < POWER_SIGNAL_COUNT; i++) {
-		if (power_signal_get(i)) {
-			n |= BIT(i);
-		}
-	}
-	/* Check if any signals flagged for debug have changed. */
-	if ((n ^ power_signals) & debug_signals) {
-		LOG_INF("power update (0x%04x -> 0x%04x, 0x%04x changed)",
-			power_signals, n, n ^ power_signals);
-	}
-	power_signals = n;
-}
+/*
+ * Bitmask of power signals updated via interrupt.
+ */
+static atomic_t interrupt_power_signals;
+
+static power_signal_mask_t output_signals;
+static power_signal_mask_t debug_signals;
 
 void power_set_debug(power_signal_mask_t debug)
 {
@@ -105,14 +103,32 @@ power_signal_mask_t power_get_debug(void)
 	return debug_signals;
 }
 
+static inline void check_debug(power_signal_mask_t mask,
+			       enum power_signal signal,
+			       int value)
+{
+	if (debug_signals & mask) {
+		LOG_INF("%s -> %d", power_signal_name(signal), value);
+	}
+}
+
 power_signal_mask_t power_get_signals(void)
 {
-	return power_signals;
+	power_signal_mask_t mask = 0;
+
+	for (int i = 0; i < ARRAY_SIZE(polled_signals); i++) {
+		if (power_signal_get(polled_signals[i])) {
+			mask |= POWER_SIGNAL_MASK(polled_signals[i]);
+		}
+	}
+	return mask | output_signals |
+		atomic_get(&interrupt_power_signals);
 }
 
-void power_signal_interrupt(void)
+void power_signal_interrupt(enum power_signal signal, int value)
 {
-	power_update_signals();
+	atomic_set_bit_to(&interrupt_power_signals, signal, value);
+	check_debug(POWER_SIGNAL_MASK(signal), signal, value);
 }
 
 int power_wait_mask_signals_timeout(power_signal_mask_t mask,
@@ -124,12 +140,11 @@ int power_wait_mask_signals_timeout(power_signal_mask_t mask,
 	}
 	want &= mask;
 	while (timeout-- > 0) {
-		if ((power_signals & mask) == want) {
+		if ((power_get_signals() & mask) == want) {
 			return 0;
 		}
 		k_msleep(1);
 	}
-	power_update_signals();
 	return -ETIMEDOUT;
 }
 
@@ -166,6 +181,7 @@ int power_signal_get(enum power_signal signal)
 int power_signal_set(enum power_signal signal, int value)
 {
 	const struct ps_config *cp = &sig_config[signal];
+	int ret;
 
 	LOG_DBG("Set %s to %d", power_signal_name(signal), value);
 	switch (cp->source) {
@@ -174,14 +190,32 @@ int power_signal_set(enum power_signal signal, int value)
 
 #if HAS_GPIO_SIGNALS
 	case PWR_SIG_SRC_GPIO:
-		return power_signal_gpio_set(cp->src_enum, value);
+		ret = power_signal_gpio_set(cp->src_enum, value);
+		break;
 #endif
 
 #if HAS_EXT_SIGNALS
 	case PWR_SIG_SRC_EXT:
-		return board_power_signal_set(signal, value);
+		ret = board_power_signal_set(signal, value);
+		break;
 #endif
 	}
+	/*
+	 * Output succeeded, update output mask.
+	 */
+	if (ret == 0) {
+		power_signal_mask_t mask = POWER_SIGNAL_MASK(signal);
+		power_signal_mask_t old = output_signals;
+
+		if (value)
+			output_signals |= mask;
+		else
+			output_signals &= ~mask;
+		if (old != output_signals) {
+			check_debug(mask, signal, value);
+		}
+	}
+	return ret;
 }
 
 int power_signal_enable_interrupt(enum power_signal signal)
diff --git a/zephyr/subsys/ap_pwrseq/signal_adc.c b/zephyr/subsys/ap_pwrseq/signal_adc.c
index 39f1d51a15..9f07b73964 100644
--- a/zephyr/subsys/ap_pwrseq/signal_adc.c
+++ b/zephyr/subsys/ap_pwrseq/signal_adc.c
@@ -19,12 +19,14 @@ LOG_MODULE_DECLARE(ap_pwrseq, CONFIG_AP_PWRSEQ_LOG_LEVEL);
 struct adc_config {
 	const struct device *dev_trig_high;
 	const struct device *dev_trig_low;
+	enum power_signal signal;
 };
 
 #define INIT_ADC_CONFIG(id)	\
 {									\
 	.dev_trig_high = DEVICE_DT_GET(DT_PHANDLE(id, trigger_high)),	\
 	.dev_trig_low = DEVICE_DT_GET(DT_PHANDLE(id, trigger_low)),	\
+	.signal = PWR_SIGNAL_ENUM(id),					\
 },
 
 static const struct adc_config config[] = {
@@ -49,7 +51,7 @@ static void trigger_high(enum pwr_sig_adc adc)
 			SENSOR_ATTR_ALERT,
 			&val);
 	LOG_DBG("power signal adc%d is HIGH", adc);
-	power_update_signals();
+	power_signal_interrupt(config[adc].signal, 1);
 }
 
 static void trigger_low(enum pwr_sig_adc adc)
@@ -68,7 +70,7 @@ static void trigger_low(enum pwr_sig_adc adc)
 			SENSOR_ATTR_ALERT,
 			&val);
 	LOG_DBG("power signal adc%d is LOW", adc);
-	power_update_signals();
+	power_signal_interrupt(config[adc].signal, 0);
 }
 
 int power_signal_adc_get(enum pwr_sig_adc adc)
diff --git a/zephyr/subsys/ap_pwrseq/signal_gpio.c b/zephyr/subsys/ap_pwrseq/signal_gpio.c
index a130bc8abf..c2df5e56c5 100644
--- a/zephyr/subsys/ap_pwrseq/signal_gpio.c
+++ b/zephyr/subsys/ap_pwrseq/signal_gpio.c
@@ -23,6 +23,7 @@ DT_FOREACH_STATUS_OKAY(MY_COMPAT, INIT_GPIO_SPEC)
  */
 struct ps_gpio_int {
 	gpio_flags_t flags;
+	uint8_t signal;
 	unsigned output : 1;
 	unsigned no_enable : 1;
 };
@@ -30,6 +31,7 @@ struct ps_gpio_int {
 #define INIT_GPIO_CONFIG(id)					\
 	{							\
 		.flags = DT_PROP_OR(id, interrupt_flags, 0),	\
+		.signal = PWR_SIGNAL_ENUM(id),			\
 		.no_enable = DT_PROP(id, no_enable),		\
 		.output = DT_PROP(id, output),			\
 	 },
@@ -89,7 +91,10 @@ void power_signal_gpio_interrupt(const struct device *port,
 				 struct gpio_callback *cb,
 				 gpio_port_pins_t pins)
 {
-	power_signal_interrupt();
+	int index = cb - int_cb;
+
+	power_signal_interrupt(gpio_config[index].signal,
+			       gpio_pin_get_dt(&spec[index]));
 }
 
 int power_signal_gpio_get(enum pwr_sig_gpio index)
diff --git a/zephyr/subsys/ap_pwrseq/signal_vw.c b/zephyr/subsys/ap_pwrseq/signal_vw.c
index 6177637eba..b0abe4fe13 100644
--- a/zephyr/subsys/ap_pwrseq/signal_vw.c
+++ b/zephyr/subsys/ap_pwrseq/signal_vw.c
@@ -17,6 +17,7 @@ LOG_MODULE_DECLARE(ap_pwrseq, CONFIG_AP_PWRSEQ_LOG_LEVEL);
 #define INIT_ESPI_SIGNAL(id)					\
 {								\
 	.espi_signal = DT_STRING_UPPER_TOKEN(id, virtual_wire),	\
+	.signal = PWR_SIGNAL_ENUM(id),				\
 	.invert = DT_PROP(id, vw_invert),			\
 },
 
@@ -25,6 +26,7 @@ LOG_MODULE_DECLARE(ap_pwrseq, CONFIG_AP_PWRSEQ_LOG_LEVEL);
  */
 struct vw_config {
 	uint8_t espi_signal;	/* associated VW signal */
+	uint8_t signal;		/* power signal */
 	bool invert;		/* Invert the signal value */
 };
 
@@ -41,7 +43,8 @@ static bool signal_data[ARRAY_SIZE(vw_config)];
  * and it is only when all the signals have been updated that
  * notification is sent that the signals are ready.
  */
-static uint8_t espi_ready;
+static uint8_t espi_mask;
+static bool espi_not_valid;
 BUILD_ASSERT(ARRAY_SIZE(vw_config) <= 8);
 
 static void espi_handler(const struct device *dev,
@@ -62,21 +65,32 @@ static void espi_handler(const struct device *dev,
 		 * the signal mask.
 		 */
 		notify_espi_ready(false);
-		espi_ready = 0;
+		espi_mask = 0;
+		espi_not_valid = true;
 		break;
 
 	case ESPI_BUS_EVENT_VWIRE_RECEIVED:
 		for (int i = 0; i < ARRAY_SIZE(vw_config); i++) {
 			if (event.evt_details == vw_config[i].espi_signal) {
-				signal_data[i] = !!event.evt_data;
-				espi_ready |= BIT(i);
+				int value = vw_config[i].invert
+						? !event.evt_data
+						: !!event.evt_data;
+
+				signal_data[i] = value;
+				if (espi_not_valid) {
+					espi_mask |= BIT(i);
+				}
+				power_signal_interrupt(vw_config[i].signal,
+						       value);
 			}
 		}
 		/*
 		 * When all the signals have been updated, notify that
 		 * the ESPI signals are valid.
 		 */
-		if (espi_ready == BIT_MASK(ARRAY_SIZE(vw_config))) {
+		if (espi_not_valid &&
+		    espi_mask == BIT_MASK(ARRAY_SIZE(vw_config))) {
+			espi_not_valid = false;
 			LOG_DBG("ESPI signals valid");
 			/*
 			 * TODO(b/222946923): Convert to generalised
@@ -90,13 +104,10 @@ static void espi_handler(const struct device *dev,
 
 int power_signal_vw_get(enum pwr_sig_vw vw)
 {
-	int value;
-
 	if (vw < 0 || vw >= ARRAY_SIZE(vw_config)) {
 		return -EINVAL;
 	}
-	value = signal_data[vw];
-	return vw_config[vw].invert ? !value : value;
+	return signal_data[vw];
 }
 
 void power_signal_vw_init(void)
diff --git a/zephyr/subsys/ap_pwrseq/x86_non_dsx_common_pwrseq_sm_handler.c b/zephyr/subsys/ap_pwrseq/x86_non_dsx_common_pwrseq_sm_handler.c
index 47f22f73dc..d712bb008a 100644
--- a/zephyr/subsys/ap_pwrseq/x86_non_dsx_common_pwrseq_sm_handler.c
+++ b/zephyr/subsys/ap_pwrseq/x86_non_dsx_common_pwrseq_sm_handler.c
@@ -41,9 +41,6 @@ const char pwrsm_dbg[][25] = {
 void notify_espi_ready(bool ready)
 {
 	pwrseq_ctx.espi_ready = ready;
-	if (ready) {
-		power_update_signals();
-	}
 }
 #endif
 
@@ -344,6 +341,7 @@ static void pwrseq_loop_thread(void *p1, void *p2, void *p3)
 		 * comes back by the time we update our signals.
 		 */
 		this_in_signals = power_get_signals();
+
 		if (this_in_signals != last_in_signals ||
 				curr_state != last_state) {
 			LOG_INF("power state %d = %s, in 0x%04x",
diff --git a/zephyr/test/ap_power/src/signals.c b/zephyr/test/ap_power/src/signals.c
index 0498a7a6da..8bbf88c2a9 100644
--- a/zephyr/test/ap_power/src/signals.c
+++ b/zephyr/test/ap_power/src/signals.c
@@ -262,9 +262,16 @@ ZTEST(signals, test_gpio_output)
 ZTEST(signals, test_signal_mask)
 {
 	power_signal_mask_t vm = POWER_SIGNAL_MASK(PWR_IMVP9_VRRDY);
+	power_signal_mask_t bm = POWER_SIGNAL_MASK(PWR_ALL_SYS_PWRGD);
 	power_signal_mask_t m;
 
 	/*
+	 * Set board level (polled) signal.
+	 */
+	power_signal_set(PWR_ALL_SYS_PWRGD, 1);
+	zassert_equal(bm, (power_get_signals() & bm),
+		"Expected PWR_ALL_SYS_PWRGD signal to be present in mask");
+	/*
 	 * Use GPIO that does not interrupt to confirm that a pin change
 	 * will not update the power signal mask.
 	 */
@@ -273,16 +280,9 @@ ZTEST(signals, test_signal_mask)
 	zassert_equal(0, (power_get_signals() & vm), "Expected mask to be 0");
 	emul_set(PWR_IMVP9_VRRDY, 1);
 	zassert_equal(0, (power_get_signals() & vm), "Expected mask to be 0");
-	/*
-	 * Calling power_update_signals will read all the signals and
-	 * incorporate them into the mask, even the non-interrupt signals.
-	 */
-	power_update_signals();
-	zassert_equal(vm, (power_get_signals() & vm),
-		"Expected signals to be present in mask");
-	zassert_equal(true, power_signals_match(vm, vm),
+	zassert_equal(true, power_signals_match(bm, bm),
 		"Expected match of mask to signal match");
-	zassert_equal(-ETIMEDOUT, power_wait_mask_signals_timeout(vm, 0, 5),
+	zassert_equal(-ETIMEDOUT, power_wait_mask_signals_timeout(bm, 0, 5),
 		"Expected timeout waiting for mask to be 0");
 	zassert_ok(power_wait_mask_signals_timeout(0, vm, 5),
 		"expected match with a 0 mask (always true)");