chipset: introduce CONFIG_AP_POWER_CONTROL

Because the new Zephyr power sequencing support is gated on a different
config option than HAS_TASK_CHIPSET which code has historically assumed
implies there is an AP present, it is now easy to introduce bugs when
code that applies in both configurations uses the old option test.

This change decouples the presence of an AP from HAS_TASK_CHIPSET,
introducing a new CONFIG_AP_POWER_CONTROL symbol that is derived from
the power sequencing config options. All existing applicable users of
HAS_TASK_CHIPSET are changed to use the new symbol, fixing several
callers which would not behave correctly under Zephyr with the new
power sequencing code.

The duplicate stub implementations of functions provided by Zephyr's
chipset_api are removed, because they already appear in the header
that declares those functions.

BUG=b:233681784
TEST=make buildall, zmake testall
BRANCH=none

Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Change-Id: I282da30839ca52fcc88c6f9dea2bd00d4811b976
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3670735
Reviewed-by: Keith Short <keithshort@chromium.org>

21 files changed, 57 insertions, 63 deletions

diff --git a/baseboard/hatch/baseboard.c b/baseboard/hatch/baseboard.c
index 683df43251..bdbeb36a3c 100644
--- a/baseboard/hatch/baseboard.c
+++ b/baseboard/hatch/baseboard.c
@@ -178,7 +178,7 @@ void board_hibernate(void)
 	 * To support hibernate from ectool, keyboard, and console,
 	 * ensure that the AP is fully shutdown before hibernating.
 	 */
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 	chipset_force_shutdown(CHIPSET_SHUTDOWN_BOARD_CUSTOM);
 #endif
 
diff --git a/board/host/chipset.c b/board/host/chipset.c
index 5213d06acb..0a7385fc84 100644
--- a/board/host/chipset.c
+++ b/board/host/chipset.c
@@ -5,6 +5,9 @@
 
 /* Chipset module for emulator */
 
+/* Does not run a chipset task, but does emulate an AP chipset */
+#define CONFIG_AP_POWER_CONTROL
+
 #include <stdio.h>
 #include "chipset.h"
 #include "common.h"
diff --git a/common/cbi.c b/common/cbi.c
index 217dffd6f4..e18f15e5a9 100644
--- a/common/cbi.c
+++ b/common/cbi.c
@@ -5,6 +5,7 @@
  * Cros Board Info
  */
 
+#include "chipset.h"
 #include "common.h"
 #include "console.h"
 #include "crc8.h"
@@ -546,7 +547,7 @@ DECLARE_CONSOLE_COMMAND(cbi, cc_cbi, "[set <tag> <value> <size> | "
 			"Print or change Cros Board Info from flash");
 #endif /* CONFIG_CMD_CBI */
 
-#ifndef HAS_TASK_CHIPSET
+#ifndef CONFIG_AP_POWER_CONTROL
 int cbi_set_fw_config(uint32_t fw_config)
 {
 	/* Check write protect status */
diff --git a/common/host_event_commands.c b/common/host_event_commands.c
index 6fa1227272..177e7cb877 100644
--- a/common/host_event_commands.c
+++ b/common/host_event_commands.c
@@ -5,6 +5,7 @@
 
 /* Host event commands for Chrome EC */
 
+#include "chipset.h"
 #include "common.h"
 #include "console.h"
 #include "hooks.h"
diff --git a/common/keyboard_8042.c b/common/keyboard_8042.c
index da09edc91c..755b26f360 100644
--- a/common/keyboard_8042.c
+++ b/common/keyboard_8042.c
@@ -5,8 +5,8 @@
  * 8042 keyboard protocol
  */
 
-#include "chipset.h"
 #include "button.h"
+#include "chipset.h"
 #include "common.h"
 #include "console.h"
 #include "device_event.h"
diff --git a/common/keyboard_backlight.c b/common/keyboard_backlight.c
index ef72e93fc8..62da361d73 100644
--- a/common/keyboard_backlight.c
+++ b/common/keyboard_backlight.c
@@ -3,6 +3,7 @@
  * found in the LICENSE file.
  */
 
+#include "chipset.h"
 #include "console.h"
 #include "ec_commands.h"
 #include "gpio.h"
@@ -121,7 +122,7 @@ DECLARE_HOOK(HOOK_CHIPSET_STARTUP, keyboard_backlight_init, HOOK_PRIO_DEFAULT);
 DECLARE_HOOK(HOOK_INIT, keyboard_backlight_init, HOOK_PRIO_DEFAULT);
 #endif
 
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 static void kblight_suspend(void)
 {
 	kblight_enable(0);
@@ -136,7 +137,7 @@ static void kblight_resume(void)
 	}
 }
 DECLARE_HOOK(HOOK_CHIPSET_RESUME, kblight_resume, HOOK_PRIO_DEFAULT);
-#endif  // HAS_TASK_CHIPSET
+#endif  /* CONFIG_AP_POWER_CONTROL */
 
 #ifdef CONFIG_LID_SWITCH
 static void kblight_lid_change(void)
diff --git a/common/led_policy_std.c b/common/led_policy_std.c
index b18dc1bc27..65bf8cedbd 100644
--- a/common/led_policy_std.c
+++ b/common/led_policy_std.c
@@ -124,7 +124,7 @@ int led_set_brightness(enum ec_led_id led_id, const uint8_t *brightness)
 	return EC_SUCCESS;
 }
 
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 static void std_led_shutdown(void)
 {
 	pwr_led_set_color(LED_OFF);
diff --git a/common/system.c b/common/system.c
index f53d444733..013452c21a 100644
--- a/common/system.c
+++ b/common/system.c
@@ -1055,13 +1055,12 @@ void system_enter_hibernate(uint32_t seconds, uint32_t microseconds)
 	 * On ChromeOS devices, if AC is present, don't hibernate.
 	 * It might trigger an immediate wake up (since AC is present),
 	 * resulting in an AP reboot.
-	 * Hibernate when AC is present never occurs in normal circumstantces,
+	 * Hibernate when AC is present never occurs in normal circumstances,
 	 * this is to prevent an action triggered by developers.
 	 * See: b/192259035
 	 */
-	if (IS_ENABLED(CONFIG_EXTPOWER) &&
-	    (IS_ENABLED(HAS_TASK_CHIPSET) || IS_ENABLED(CONFIG_AP_PWRSEQ)) &&
-	    extpower_is_present()) {
+	if (IS_ENABLED(CONFIG_EXTPOWER) && IS_ENABLED(CONFIG_AP_POWER_CONTROL)
+	    && extpower_is_present()) {
 		CPRINTS("AC on, skip hibernate");
 		return;
 	}
diff --git a/common/usb_pd_alt_mode_dfp.c b/common/usb_pd_alt_mode_dfp.c
index feda92c74e..1de01f9c48 100644
--- a/common/usb_pd_alt_mode_dfp.c
+++ b/common/usb_pd_alt_mode_dfp.c
@@ -1174,7 +1174,7 @@ __overridable int svdm_enter_dp_mode(int port, uint32_t mode_caps)
 	 * could cause a wake up.)  When in S5->S3 transition state, we
 	 * should treat it as a SoC off state.
 	 */
-#if defined(HAS_TASK_CHIPSET) || defined(CONFIG_AP_PWRSEQ)
+#ifdef CONFIG_AP_POWER_CONTROL
 	if (!chipset_in_state(CHIPSET_STATE_ANY_SUSPEND | CHIPSET_STATE_ON))
 		return -1;
 #endif
diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c
index 5dc133cb45..3f0408eedf 100644
--- a/common/usb_pd_protocol.c
+++ b/common/usb_pd_protocol.c
@@ -2754,7 +2754,7 @@ static void pd_init_tasks(void)
 	if (initialized)
 		return;
 
-#if defined(HAS_TASK_CHIPSET) && defined(CONFIG_USB_PD_DUAL_ROLE)
+#if defined(CONFIG_AP_POWER_CONTROL) && defined(CONFIG_USB_PD_DUAL_ROLE)
 	/* Set dual-role state based on chipset power state */
 	if (chipset_in_state(CHIPSET_STATE_ANY_OFF))
 		for (i = 0; i < board_get_usb_pd_port_count(); i++)
diff --git a/common/usbc/usb_pd_dpm.c b/common/usbc/usb_pd_dpm.c
index 745454b647..159331171e 100644
--- a/common/usbc/usb_pd_dpm.c
+++ b/common/usbc/usb_pd_dpm.c
@@ -278,7 +278,7 @@ static void dpm_attempt_mode_entry(int port)
 		return;
 	}
 
-#if defined(HAS_TASK_CHIPSET) || defined(CONFIG_AP_PWRSEQ)
+#ifdef CONFIG_AP_POWER_CONTROL
 	/*
 	 * Do not try to enter mode while CPU is off.
 	 * CPU transitions (e.g b/158634281) can occur during the discovery
@@ -887,7 +887,7 @@ static uint8_t get_status_power_state_change(void)
 {
 	enum pd_sdb_power_state ret = PD_SDB_POWER_STATE_NOT_SUPPORTED;
 
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 	if (chipset_in_or_transitioning_to_state(CHIPSET_STATE_HARD_OFF)) {
 		ret = PD_SDB_POWER_STATE_G3;
 	} else if (chipset_in_or_transitioning_to_state(
@@ -906,7 +906,7 @@ static uint8_t get_status_power_state_change(void)
 			   CHIPSET_STATE_STANDBY)) {
 		ret = PD_SDB_POWER_STATE_MODERN_STANDBY;
 	}
-#endif /* HAS_TASK_CHIPSET */
+#endif /* CONFIG_AP_POWER_CONTROL */
 
 	return ret | board_get_pd_sdb_power_indicator(ret);
 }
diff --git a/common/usbc/usb_tc_drp_acc_trysrc_sm.c b/common/usbc/usb_tc_drp_acc_trysrc_sm.c
index 046461b7a8..2da6b59f0a 100644
--- a/common/usbc/usb_tc_drp_acc_trysrc_sm.c
+++ b/common/usbc/usb_tc_drp_acc_trysrc_sm.c
@@ -547,7 +547,7 @@ int pd_get_rev(int port, enum tcpci_msg_type type)
 
 #endif /* !CONFIG_USB_PR_SM */
 
-#if !defined(HAS_TASK_CHIPSET) && !defined(CONFIG_AP_PWRSEQ)
+#ifndef CONFIG_AP_POWER_CONTROL
 __overridable enum pd_dual_role_states board_tc_get_initial_drp_mode(int port)
 {
 	/*
@@ -1577,7 +1577,7 @@ void tc_state_init(int port)
 	 */
 	tc_policy_pd_enable(port, pd_comm_allowed_by_policy());
 
-#if defined(HAS_TASK_CHIPSET) || defined(CONFIG_AP_PWRSEQ)
+#ifdef CONFIG_AP_POWER_CONTROL
 	/* Set dual-role state based on chipset power state */
 	if (chipset_in_state(CHIPSET_STATE_ANY_OFF))
 		pd_set_dual_role_and_event(port, PD_DRP_FORCE_SINK, 0);
diff --git a/driver/bc12/max14637.c b/driver/bc12/max14637.c
index 60e9be8054..4c7cbffd18 100644
--- a/driver/bc12/max14637.c
+++ b/driver/bc12/max14637.c
@@ -147,7 +147,7 @@ static void detect_or_power_down_ic(const int port)
 #endif /* !defined(CONFIG_USB_PD_VBUS_DETECT_TCPC) */
 
 	if (vbus_present) {
-#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(HAS_TASK_CHIPSET)
+#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(CONFIG_AP_POWER_CONTROL)
 		/* Turn on the 5V rail to allow the chip to be powered. */
 		power_5v_enable(task_get_current(), 1);
 #endif
@@ -167,7 +167,7 @@ static void detect_or_power_down_ic(const int port)
 		 * switch of USB2.0 can be kept close from now on.
 		 */
 		bc12_detect(port);
-#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(HAS_TASK_CHIPSET)
+#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(CONFIG_AP_POWER_CONTROL)
 		/* Issue a request to turn off the rail. */
 		power_5v_enable(task_get_current(), 0);
 #endif
diff --git a/driver/bc12/pi3usb9201.c b/driver/bc12/pi3usb9201.c
index 6d47ed5d04..9e60c9b4fd 100644
--- a/driver/bc12/pi3usb9201.c
+++ b/driver/bc12/pi3usb9201.c
@@ -203,7 +203,7 @@ static void bc12_power_down(int port)
 	if (pi3usb9201_bc12_chips[port].flags & PI3USB9201_ALWAYS_POWERED)
 		return;
 
-#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(HAS_TASK_CHIPSET)
+#if defined(CONFIG_POWER_PP5000_CONTROL) && defined(CONFIG_AP_POWER_CONTROL)
 	/* Indicate PP5000_A rail is not required by USB_CHG task. */
 	power_5v_enable(task_get_current(), 0);
 #endif
@@ -212,7 +212,7 @@ static void bc12_power_down(int port)
 static void bc12_power_up(int port)
 {
 	if (IS_ENABLED(CONFIG_POWER_PP5000_CONTROL) &&
-	    IS_ENABLED(HAS_TASK_CHIPSET) &&
+	    IS_ENABLED(CONFIG_AP_POWER_CONTROL) &&
 	    !(pi3usb9201_bc12_chips[port].flags & PI3USB9201_ALWAYS_POWERED)) {
 		/* Turn on the 5V rail to allow the chip to be powered. */
 		power_5v_enable(task_get_current(), 1);
diff --git a/include/chipset.h b/include/chipset.h
index 51a9018dfb..840db3aa60 100644
--- a/include/chipset.h
+++ b/include/chipset.h
@@ -48,7 +48,7 @@ enum critical_shutdown {
 	CRITICAL_SHUTDOWN_CUTOFF,
 };
 
-#if defined(HAS_TASK_CHIPSET) || defined(CONFIG_ZEPHYR)
+#ifdef CONFIG_AP_POWER_CONTROL
 
 /**
  * Check if chipset is in a given state.
@@ -125,7 +125,7 @@ void chipset_pre_init_callback(void);
  */
 void init_reset_log(void);
 
-#else /* !HAS_TASK_CHIPSET */
+#else /* !CONFIG_AP_POWER_CONTROL */
 
 /* When no chipset is present, assume it is always off. */
 static inline int chipset_in_state(int state_mask)
@@ -156,7 +156,7 @@ static inline void chipset_watchdog_interrupt(enum gpio_signal signal) { }
 
 static inline void init_reset_log(void) { }
 
-#endif /* !HAS_TASK_CHIPSET */
+#endif /* !CONFIG_AP_POWER_CONTROL */
 
 /**
  * Optional chipset check if PLTRST# is valid.
diff --git a/include/config.h b/include/config.h
index 5b74d1e80d..523d94a8a8 100644
--- a/include/config.h
+++ b/include/config.h
@@ -6279,6 +6279,17 @@
 #endif
 
 /*
+ * If the chipset task is enabled, this implies there is an AP to manage power
+ * for. In Zephyr this can be implied by multiple options, so we provide the
+ * same symbol here instead of making code examine HAS_TASK_CHIPSET.
+ */
+#ifndef CONFIG_AP_POWER_CONTROL
+#ifdef HAS_TASK_CHIPSET
+#define CONFIG_AP_POWER_CONTROL
+#endif	/* HAS_TASK_CHIPSET */
+#endif /* CONFIG_AP_POWER_CONTROL */
+
+/*
  * If a board has a chipset task, set the minimum charger power required for
  * powering on to 15W.  This is also the highest power discovered over Type-C by
  * analog signaling.  The EC normally does not communicate using USB PD when the
diff --git a/include/cros_board_info.h b/include/cros_board_info.h
index 7a79cfee2a..9ed5e1777b 100644
--- a/include/cros_board_info.h
+++ b/include/cros_board_info.h
@@ -191,9 +191,9 @@ int cbi_board_override(enum cbi_data_tag tag, uint8_t *buf, uint8_t *size);
 /**
  * Set and update FW_CONFIG tag field
  *
- * This function is only included when HAS_TASK_CHIPSET is not defined. It is
- * intended to be used for projects which want CBI functions, but do not have an
- * AP and ectool host command access.
+ * This function is only included when CONFIG_AP_POWER_CONTROL is disabled. It
+ * is intended to be used for projects which want CBI functions, but do not
+ * have an AP and ectool host command access.
  *
  * @param fw_config	updated value for FW_CONFIG tag
  * @return EC_SUCCESS to indicate the field was written correctly.
diff --git a/include/power.h b/include/power.h
index 547a4d93ab..6200392b95 100644
--- a/include/power.h
+++ b/include/power.h
@@ -8,6 +8,7 @@
 #ifndef __CROS_EC_POWER_H
 #define __CROS_EC_POWER_H
 
+#include "chipset.h"
 #include "common.h"
 #include "compiler.h"
 #include "gpio_signal.h"
@@ -173,7 +174,7 @@ void power_set_state(enum power_state new_state);
  *
  * @return Current chipset power state
  */
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 enum power_state power_get_state(void);
 #else
 static inline enum power_state power_get_state(void) {
@@ -204,11 +205,11 @@ enum power_state power_handle_state(enum power_state state);
 /**
  * Interrupt handler for power signal GPIOs.
  */
-#ifdef HAS_TASK_CHIPSET
+#ifdef CONFIG_AP_POWER_CONTROL
 void power_signal_interrupt(enum gpio_signal signal);
 #else
 static inline void power_signal_interrupt(enum gpio_signal signal) { }
-#endif /* !HAS_TASK_CHIPSET */
+#endif /* !CONFIG_AP_POWER_CONTROL */
 
 /**
  * Interrupt handler for rsmrst signal GPIO. This interrupt handler should be
diff --git a/zephyr/Kconfig.ap_power b/zephyr/Kconfig.ap_power
index 3db5a1fef8..03c92759f0 100644
--- a/zephyr/Kconfig.ap_power
+++ b/zephyr/Kconfig.ap_power
@@ -2,6 +2,15 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+config AP_POWER_CONTROL
+    bool
+    default y if PLATFORM_EC_POWERSEQ || AP_PWRSEQ
+    help
+      Whether the EC has control over AP power states.
+
+      This is automatically enabled if an implementation of AP power
+      sequencing is enabled.
+
 menuconfig PLATFORM_EC_BOOT_AP_POWER_REQUIREMENTS
 	bool "Power requirements to boot AP"
 	default y
diff --git a/zephyr/shim/src/CMakeLists.txt b/zephyr/shim/src/CMakeLists.txt
index 9273a41fde..dee8d0af71 100644
--- a/zephyr/shim/src/CMakeLists.txt
+++ b/zephyr/shim/src/CMakeLists.txt
@@ -2,7 +2,6 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-zephyr_library_sources(chipset_api.c)
 zephyr_library_sources(console.c)
 zephyr_library_sources(crc.c)
 zephyr_library_sources(gpio.c)
@@ -27,6 +26,7 @@ zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_BATTERY
 zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_CHARGER_RT9490
                                                             bc12_rt9490.c)
 zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_CHARGER     charger.c)
+zephyr_library_sources_ifdef(CONFIG_AP_PWRSEQ               chipset_api.c)
 zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_HOST_INTERFACE_ESPI
                                                             espi.c)
 zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_FAN         fan.c)
diff --git a/zephyr/shim/src/chipset_api.c b/zephyr/shim/src/chipset_api.c
index 2804de8e78..3bfa420980 100644
--- a/zephyr/shim/src/chipset_api.c
+++ b/zephyr/shim/src/chipset_api.c
@@ -7,7 +7,6 @@
 
 #include "common.h"
 
-#if defined(CONFIG_AP_PWRSEQ)
 #include "ap_power/ap_power_interface.h"
 #include "chipset_state_check.h"
 
@@ -45,34 +44,3 @@ void init_reset_log(void)
 {
 	ap_power_init_reset_log();
 }
-
-#else
-
-#if !defined(HAS_TASK_CHIPSET)
-#include "chipset.h"
-
-/* When no chipset is present, assume it is always off. */
-int chipset_in_state(int state_mask)
-{
-	return state_mask & CHIPSET_STATE_ANY_OFF;
-}
-int chipset_in_or_transitioning_to_state(int state_mask)
-{
-	return state_mask & CHIPSET_STATE_ANY_OFF;
-}
-void chipset_exit_hard_off(void) { }
-void chipset_throttle_cpu(int throttle) { }
-void chipset_force_shutdown(enum chipset_shutdown_reason reason) { }
-void chipset_reset(enum chipset_shutdown_reason reason) { }
-void power_interrupt(enum gpio_signal signal) { }
-void chipset_handle_espi_reset_assert(void) { }
-void chipset_handle_reboot(void) { }
-void chipset_reset_request_interrupt(enum gpio_signal signal) { }
-void chipset_warm_reset_interrupt(enum gpio_signal signal) { }
-void chipset_ap_rst_interrupt(enum gpio_signal signal) { }
-void chipset_power_good_interrupt(enum gpio_signal signal) { }
-void chipset_watchdog_interrupt(enum gpio_signal signal) { }
-void init_reset_log(void) { }
-
-#endif /* !defined(HAS_TASK_CHIPSET) */
-#endif /*defined(CONFIG_AP_PWRSEQ) */