sm5803: disable GPADCs when in LPM

It's safe to disable the GPADCs when the SM5803 is not active and very
dangerous to disable them when it is active, but they consume about 7mW
of power that we want to save. Add support for disabling GPADCs, taking
care to never disable them when the charger is active.

This requires changes to users that inspect the VBUS voltage, because
analog VBUS readings (as used by the USB-C state machine to detect
when a source is connected) do not update when the VBUS GPADC is disabled
(as happens when the charger goes to runtime LPM). sm5803_get_vbus_voltage
is changed to return an error if the VBUS ADC is disabled and a new
function is added to use an alternate path if necessary, which is now
adopted by all users of the SM5803.

BUG=b:237697900
TEST=Idle chargers now disable GPADCs when in runtime LPM
     (i2c read_byte I2C_SUB_USB_C1_TCPC 0x31 1 => 0) on Nereid, and VBUS
     detection still works following CC detection (which did not work
     when ADCs were disabled without any further changes).
BRANCH=none

Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Change-Id: I40ba9b80e5503b240cdf67025a2c33817a337adb
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3750543
Reviewed-by: Diana Z <dzigterman@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

11 files changed, 140 insertions, 140 deletions

diff --git a/board/drawcia/usb_pd_policy.c b/board/drawcia/usb_pd_policy.c
index 042adc0a86..ef687f0f9c 100644
--- a/board/drawcia/usb_pd_policy.c
+++ b/board/drawcia/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/drawcia_riscv/usb_pd_policy.c b/board/drawcia_riscv/usb_pd_policy.c
index 6c3370ca2f..54fa6f6933 100644
--- a/board/drawcia_riscv/usb_pd_policy.c
+++ b/board/drawcia_riscv/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/haboki/usb_pd_policy.c b/board/haboki/usb_pd_policy.c
index 6c3370ca2f..54fa6f6933 100644
--- a/board/haboki/usb_pd_policy.c
+++ b/board/haboki/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/kracko/usb_pd_policy.c b/board/kracko/usb_pd_policy.c
index 6c3370ca2f..54fa6f6933 100644
--- a/board/kracko/usb_pd_policy.c
+++ b/board/kracko/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/lantis/usb_pd_policy.c b/board/lantis/usb_pd_policy.c
index 042adc0a86..ef687f0f9c 100644
--- a/board/lantis/usb_pd_policy.c
+++ b/board/lantis/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/shotzo/usb_pd_policy.c b/board/shotzo/usb_pd_policy.c
index 4caa440cc2..69984e9b6f 100644
--- a/board/shotzo/usb_pd_policy.c
+++ b/board/shotzo/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/board/waddledee/usb_pd_policy.c b/board/waddledee/usb_pd_policy.c
index 042adc0a86..ef687f0f9c 100644
--- a/board/waddledee/usb_pd_policy.c
+++ b/board/waddledee/usb_pd_policy.c
@@ -65,18 +65,7 @@ int pd_set_power_supply_ready(int port)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage))
-		return false;
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 int pd_snk_is_vbus_provided(int port)
diff --git a/driver/charger/sm5803.c b/driver/charger/sm5803.c
index e107f54dad..821666fc50 100644
--- a/driver/charger/sm5803.c
+++ b/driver/charger/sm5803.c
@@ -142,31 +142,45 @@ test_update8(int chgnum, const int offset, const uint8_t mask,
 }
 
 /*
- * Ensure the charger clocks are at normal operating speed, setting them to
- * that speed if not already.
+ * Ensure the charger configuration is safe for operation, updating registers
+ * as necessary to become safe.
  *
  * The SM5803 runs multiple digital control loops that are important to correct
- * operation. The CLOCK_SEL_LOW register reduced their speed by about 10x, which
+ * operation. The CLOCK_SEL_LOW register reduces their speed by about 10x, which
  * is dangerous when either sinking or sourcing is to be enabled because the
  * control loops will respond much more slowly. Leaving clocks at low speed can
  * cause incorrect operation or even hardware damage.
  *
+ * The GPADCs are inputs to the control loops, and disabling them can also cause
+ * incorrect operation or hardware damage. They must be enabled for the charger
+ * to be safe to operate.
+ *
  * This function is used by the functions that enable sinking or sourcing to
- * ensure the control loops are running at full speed before enabling switching
- * on the charger.
+ * ensure the current configuration is safe before enabling switching on the
+ * charger.
  */
-static int sm5803_set_full_clock_speed(int chgnum)
+static int sm5803_set_active_safe(int chgnum)
 {
 	int rv, val;
 
+	/*
+	 * Set clocks to full speed.
+	 *
+	 * This should occur first because enabling GPADCs with clocks slowed
+	 * can cause spurious acquisition.
+	 */
 	rv = main_read8(chgnum, SM5803_REG_CLOCK_SEL, &val);
-	if (rv) {
-		goto out;
-	}
-	if (val & SM5803_CLOCK_SEL_LOW) {
+	if (rv == 0 && val & SM5803_CLOCK_SEL_LOW) {
 		rv = main_write8(chgnum, SM5803_REG_CLOCK_SEL,
 				 val & ~SM5803_CLOCK_SEL_LOW);
 	}
+	if (rv) {
+		goto out;
+	}
+
+	/* Enable default GPADCs */
+	rv = meas_write8(chgnum, SM5803_REG_GPADC_CONFIG1,
+			 SM5803_GPADCC1_DEFAULT_ENABLE);
 
 out:
 	if (rv) {
@@ -326,7 +340,7 @@ enum ec_error_list sm5803_vbus_sink_enable(int chgnum, int enable)
 		return rv;
 
 	if (enable) {
-		rv = sm5803_set_full_clock_speed(chgnum);
+		rv = sm5803_set_active_safe(chgnum);
 		if (rv) {
 			return rv;
 		}
@@ -663,20 +677,8 @@ static void sm5803_init(int chgnum)
 	reg &= ~(BIT(0) | BIT(1));
 	rv |= main_write8(chgnum, SM5803_REG_REFERENCE, reg);
 
-	/* Set a higher clock speed in case it was lowered for z-state */
-	rv |= main_read8(chgnum, SM5803_REG_CLOCK_SEL, &reg);
-	reg &= ~SM5803_CLOCK_SEL_LOW;
-	rv |= main_write8(chgnum, SM5803_REG_CLOCK_SEL, reg);
-
-	/*
-	 * Turn on GPADCs to default.  Enable the IBAT_CHG ADC in order to
-	 * measure battery current and calculate system resistance.
-	 */
-	reg = SM5803_GPADCC1_TINT_EN | SM5803_GPADCC1_VSYS_EN |
-	      SM5803_GPADCC1_VCHGPWR_EN | SM5803_GPADCC1_VBUS_EN |
-	      SM5803_GPADCC1_IBAT_CHG_EN | SM5803_GPADCC1_IBAT_DIS_EN |
-	      SM5803_GPADCC1_VBATSNSP_EN;
-	rv |= meas_write8(chgnum, SM5803_REG_GPADC_CONFIG1, reg);
+	/* Reset clocks and enable GPADCs */
+	rv |= sm5803_set_active_safe(chgnum);
 
 	/* Enable Psys DAC */
 	rv |= meas_read8(chgnum, SM5803_REG_PSYS1, &reg);
@@ -870,10 +872,9 @@ static void sm5803_disable_runtime_low_power_mode(void)
 			chgnum);
 		return;
 	}
-	/* Set a higher clock speed */
-	rv |= main_read8(chgnum, SM5803_REG_CLOCK_SEL, &reg);
-	reg &= ~SM5803_CLOCK_SEL_LOW;
-	rv |= main_write8(chgnum, SM5803_REG_CLOCK_SEL, reg);
+
+	/* Reset clocks and enable GPADCs */
+	rv = sm5803_set_active_safe(chgnum);
 
 	/* Enable ADC sigma delta */
 	rv |= chg_read8(chgnum, SM5803_REG_CC_CONFIG1, &reg);
@@ -950,6 +951,22 @@ static void sm5803_enable_runtime_low_power_mode(void)
 		return;
 	}
 
+	/*
+	 * Turn off GPADCs.
+	 *
+	 * This is only safe to do if the charger is inactive. We ensure that
+	 * they are enabled again in sm5803_set_active_safe() before the charger
+	 * is enabled, and verify here that the charger is not currently active.
+	 */
+	rv |= chg_read8(chgnum, SM5803_REG_FLOW1, &reg);
+	if (rv == 0 && (reg & SM5803_FLOW1_MODE) == CHARGER_MODE_DISABLED) {
+		rv |= meas_write8(chgnum, SM5803_REG_GPADC_CONFIG1, 0);
+		rv |= meas_write8(chgnum, SM5803_REG_GPADC_CONFIG2, 0);
+	} else {
+		CPRINTS("%s %d: FLOW1 %x is active! Not disabling GPADCs",
+			CHARGER_NAME, chgnum, reg);
+	}
+
 	/* Disable ADC sigma delta */
 	rv |= chg_read8(chgnum, SM5803_REG_CC_CONFIG1, &reg);
 	reg &= ~SM5803_CC_CONFIG1_SD_PWRUP;
@@ -1536,6 +1553,14 @@ static enum ec_error_list sm5803_get_vbus_voltage(int chgnum, int port,
 	int reg;
 	int volt_bits;
 
+	rv = meas_read8(chgnum, SM5803_REG_GPADC_CONFIG1, &reg);
+	if (rv)
+		return rv;
+	if ((reg & SM5803_GPADCC1_VBUS_EN) == 0) {
+		/* VBUS ADC is currently disabled */
+		return EC_ERROR_NOT_POWERED;
+	}
+
 	rv = meas_read8(chgnum, SM5803_REG_VBUS_MEAS_MSB, &reg);
 	if (rv)
 		return rv;
@@ -1551,6 +1576,54 @@ static enum ec_error_list sm5803_get_vbus_voltage(int chgnum, int port,
 	return rv;
 }
 
+bool sm5803_check_vbus_level(int chgnum, enum vbus_level level)
+{
+	int rv, vbus_voltage;
+
+	/*
+	 * Analog reading of VBUS is more accurate and helps reliability when
+	 * doing power role swaps, but if the charger is in LPM with the GPADCs
+	 * disabled then the reading won't update.
+	 *
+	 * Digital VBUS presence (with transitions flagged by STATUS1_CHG_DET
+	 * interrupt) still works when GPADCs are off, and shouldn't otherwise
+	 * impact performance because the GPADCs should be enabled in any
+	 * situation where we're doing a PRS.
+	 */
+	rv = sm5803_get_vbus_voltage(chgnum, chgnum, &vbus_voltage);
+	if (rv == EC_ERROR_NOT_POWERED) {
+		/* VBUS ADC is disabled, use digital presence */
+		switch (level) {
+		case VBUS_PRESENT:
+			return sm5803_is_vbus_present(chgnum);
+		case VBUS_SAFE0V:
+		case VBUS_REMOVED:
+			return !sm5803_is_vbus_present(chgnum);
+		default:
+			CPRINTS("%s: unrecognized vbus_level value: %d",
+				__func__, level);
+			return false;
+		}
+	}
+	if (rv != EC_SUCCESS) {
+		/* Unhandled communication error; assume unsatisfied */
+		return false;
+	}
+
+	switch (level) {
+	case VBUS_PRESENT:
+		return vbus_voltage > PD_V_SAFE5V_MIN;
+	case VBUS_SAFE0V:
+		return vbus_voltage < PD_V_SAFE0V_MAX;
+	case VBUS_REMOVED:
+		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	default:
+		CPRINTS("%s: unrecognized vbus_level value: %d", __func__,
+			level);
+		return false;
+	}
+}
+
 static enum ec_error_list sm5803_set_input_current_limit(int chgnum,
 							 int input_current)
 {
@@ -1718,7 +1791,7 @@ static enum ec_error_list sm5803_enable_otg_power(int chgnum, int enabled)
 	if (enabled) {
 		int selected_current;
 
-		rv = sm5803_set_full_clock_speed(chgnum);
+		rv = sm5803_set_active_safe(chgnum);
 		if (rv) {
 			return rv;
 		}
diff --git a/driver/charger/sm5803.h b/driver/charger/sm5803.h
index 5a03ce37d4..174f0684ae 100644
--- a/driver/charger/sm5803.h
+++ b/driver/charger/sm5803.h
@@ -9,6 +9,7 @@
 #define __CROS_EC_SM5803_H
 
 #include "common.h"
+#include "usb_pd_tcpm.h"
 
 /* Note: configure charger struct with CHARGER_FLAGS */
 #define SM5803_ADDR_MAIN_FLAGS 0x30
@@ -129,6 +130,18 @@ enum sm5803_gpio0_modes {
 #define SM5803_GPADCC1_VSYS_EN BIT(6) /* NOTE: DO NOT CLEAR */
 #define SM5803_GPADCC1_TINT_EN BIT(7)
 
+/*
+ * Default value for GPADCC1, set at initialization: the normal operating state.
+ *
+ * IBAT_CHG is enabled in order to measure battery current and calculate system
+ * resistance.
+ */
+#define SM5803_GPADCC1_DEFAULT_ENABLE                              \
+	(SM5803_GPADCC1_TINT_EN | SM5803_GPADCC1_VSYS_EN |         \
+	 SM5803_GPADCC1_VCHGPWR_EN | SM5803_GPADCC1_VBUS_EN |      \
+	 SM5803_GPADCC1_IBAT_CHG_EN | SM5803_GPADCC1_IBAT_DIS_EN | \
+	 SM5803_GPADCC1_VBATSNSP_EN)
+
 #define SM5803_REG_GPADC_CONFIG2 0x02
 
 #define SM5803_REG_PSYS1 0x04
@@ -418,6 +431,21 @@ void sm5803_interrupt(int chgnum);
  */
 enum ec_error_list sm5803_is_acok(int chgnum, bool *acok);
 
+/**
+ * Test whether the current voltage on VBUS corresponds to the given range.
+ *
+ * Users should prefer this function to manually evaluating the result of
+ * charger_get_vbus_voltage because that function may behave incorrectly when
+ * the charger is in low power mode. This function will return correct results
+ * regardless of the charger state.
+ *
+ * @param chgnum charger index to test
+ * @param level VBUS range
+ * @return true if the current VBUS voltage is in the given range, false if it
+ *         is not or if there is a problem communicating with the charger.
+ */
+bool sm5803_check_vbus_level(int chgnum, enum vbus_level level);
+
 /* Expose low power mode functions */
 void sm5803_disable_low_power_mode(int chgnum);
 void sm5803_enable_low_power_mode(int chgnum);
diff --git a/zephyr/projects/nissa/joxer/src/usbc.c b/zephyr/projects/nissa/joxer/src/usbc.c
index 9e12f05188..606d673afa 100644
--- a/zephyr/projects/nissa/joxer/src/usbc.c
+++ b/zephyr/projects/nissa/joxer/src/usbc.c
@@ -63,19 +63,7 @@ void board_pd_vconn_ctrl(int port, enum usbpd_cc_pin cc_pin, int enabled)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage)) {
-		return false;
-	}
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 /*
diff --git a/zephyr/projects/nissa/nereid/src/usbc.c b/zephyr/projects/nissa/nereid/src/usbc.c
index b352d578f2..653c5b511e 100644
--- a/zephyr/projects/nissa/nereid/src/usbc.c
+++ b/zephyr/projects/nissa/nereid/src/usbc.c
@@ -63,19 +63,7 @@ void board_pd_vconn_ctrl(int port, enum usbpd_cc_pin cc_pin, int enabled)
 
 __override bool pd_check_vbus_level(int port, enum vbus_level level)
 {
-	int vbus_voltage;
-
-	/* If we're unable to speak to the charger, best to guess false */
-	if (charger_get_vbus_voltage(port, &vbus_voltage)) {
-		return false;
-	}
-
-	if (level == VBUS_SAFE0V)
-		return vbus_voltage < PD_V_SAFE0V_MAX;
-	else if (level == VBUS_PRESENT)
-		return vbus_voltage > PD_V_SAFE5V_MIN;
-	else
-		return vbus_voltage < PD_V_SINK_DISCONNECT_MAX;
+	return sm5803_check_vbus_level(port, level);
 }
 
 /*