charge_state: Add EC_CMD_CHARGE_CONTROL V3

Currently, only if sustain_soc.lower == sustain_soc.upper, the battery
sustainer uses IDLE for discharging. This forces a host to set lower <
upper to avoid IDLE (and use DISCHARGE only). This is suboptimal not
only because the behavior is obscure but also because a host can't
specify lower < upper for boards which support IDLE. (That means there
is no hysteresis.)

This CL adds flags field in EC_CMD_CHARGE_CONTROL so that a host can
explicitly disable IDLE.

This CL also adds dedicated tests for EC_CMD_CHARGE_CONTROL to increase
the coverage.

There is no functionality change for the hosts speaking V2.

BUG=b:222619859,b:188457962
TEST=make run-sbs_charging
TEST=ectool chargecontrol normal 79 80 0x1
LOW_COVERAGE_REASON=All false positives except one trigger which is
deliberately not tested. Comments are added for each trigger.

Change-Id: I9ca2d5f68f9c1d84baff9feb5eefd7b60e98e474
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4824703
Reviewed-by: Derek Basehore <[email protected]>
Commit-Queue: Daisuke Nojiri <[email protected]>
Auto-Submit: Daisuke Nojiri <[email protected]>
Code-Coverage: Zoss <[email protected]>
Tested-by: Daisuke Nojiri <[email protected]>

Author: dnojiri

common/charge_state.c

@@ -207,19 +207,13 @@ void reset_prev_disp_charge(void)
 	prev_disp_charge = -1;
 }
 
-static bool battery_sustainer_enabled(void)
-{
-	return sustain_soc.lower != -1 && sustain_soc.upper != -1;
-}
-
 int battery_sustainer_set(int8_t lower, int8_t upper)
 {
 	if (lower == -1 || upper == -1) {
-		if (battery_sustainer_enabled()) {
-			CPRINTS("Sustainer disabled");
-			sustain_soc.lower = -1;
-			sustain_soc.upper = -1;
-		}
+		CPRINTS("Sustainer disabled");
+		sustain_soc.lower = -1;
+		sustain_soc.upper = -1;
+		sustain_soc.flags = 0;
 		return EC_SUCCESS;
 	}
 
@@ -242,6 +236,11 @@ static void battery_sustainer_disable(void)
 	battery_sustainer_set(-1, -1);
 }
 
+test_export_static bool battery_sustainer_enabled(void)
+{
+	return sustain_soc.lower != -1 && sustain_soc.upper != -1;
+}
+
 static const char *const state_list[] = { "idle", "discharge", "charge",
 					  "precharge" };
 BUILD_ASSERT(ARRAY_SIZE(state_list) == NUM_STATES_V2);
@@ -910,58 +909,115 @@ int battery_outside_charging_temperature(void)
 	return 0;
 }
 
-static void sustain_battery_soc(void)
+static enum ec_charge_control_mode
+sustain_switch_mode(enum ec_charge_control_mode mode)
 {
-	enum ec_charge_control_mode mode = get_chg_ctrl_mode();
-	int soc;
-	int rv;
-
-	/* If either AC or battery is not present, nothing to do. */
-	if (!curr.ac || curr.batt.is_present != BP_YES ||
-	    !battery_sustainer_enabled())
-		return;
-
-	soc = charge_get_display_charge() / 10;
+	enum ec_charge_control_mode new_mode = mode;
+	int soc = charge_get_display_charge() / 10;
 
 	/*
-	 * When lower < upper, the sustainer discharges using DISCHARGE. When
-	 * lower == upper, the sustainer discharges using IDLE. The following
-	 * switch statement handle both cases but in reality either DISCHARGE
-	 * or IDLE is used but not both.
+	 * The sustain range is defined by 'lower' and 'upper' where the equal
+	 * values are inclusive:
+	 *
+	 * |------------NORMAL------------+--IDLE--+---DISCHARGE---|
+	 * 0%                             ^        ^              100%
+	 *                              lower    upper
+	 *
+	 * The switch statement below allows the sustainer to start with any soc
+	 * (0% ~ 100%) and any previous lower & upper limits. It sets mode to
+	 * NORMAL to charge till the soc hits the upper limit or sets mode to
+	 * DISCHARGE to discharge till the soc hits the upper limit.
+	 *
+	 * Once the soc enters in the sustain range, it'll switch to IDLE. In
+	 * IDLE mode, the system power is supplied from the AC. Thus, the soc
+	 * normally should stay in the sustain range unless there is high load
+	 * on the system or the charger is too weak.
+	 *
+	 * Some boards have a sing capacitor problem with mode == IDLE. For such
+	 * boards, a host can specify EC_CHARGE_CONTROL_FLAG_NO_IDLE, which
+	 * makes the sustainer use DISCHARGE instead of IDLE. This is done by
+	 * setting lower != upper in V2, which doesn't support the flag.
 	 */
 	switch (mode) {
 	case CHARGE_CONTROL_NORMAL:
-		/* Going up. Always DISCHARGE if the soc is above upper. */
-		if (sustain_soc.lower == soc && soc == sustain_soc.upper) {
-			mode = CHARGE_CONTROL_IDLE;
-		} else if (sustain_soc.upper < soc) {
-			mode = CHARGE_CONTROL_DISCHARGE;
+		/* Currently charging */
+		if (sustain_soc.upper < soc) {
+			/*
+			 * We come here only if the soc is already above the
+			 * upper limit at the time the sustainer started.
+			 */
+			new_mode = CHARGE_CONTROL_DISCHARGE;
+		} else if (sustain_soc.upper == soc) {
+			/*
+			 * We've been charging and finally reached the upper.
+			 * Let's switch to IDLE to stay.
+			 */
+			if (sustain_soc.flags & EC_CHARGE_CONTROL_FLAG_NO_IDLE)
+				new_mode = CHARGE_CONTROL_DISCHARGE;
+			else
+				new_mode = CHARGE_CONTROL_IDLE;
 		}
 		break;
 	case CHARGE_CONTROL_IDLE:
 		/* Discharging naturally */
 		if (soc < sustain_soc.lower)
-			mode = CHARGE_CONTROL_NORMAL;
+			/*
+			 * Presumably, we stayed in the sustain range for a
+			 * while but finally fell off the range. Let's charge to
+			 * the upper.
+			 */
+			new_mode = CHARGE_CONTROL_NORMAL;
+		else if (sustain_soc.upper < soc)
+			/*
+			 * This can happen only if sustainer is restarted with
+			 * decreased upper limit. Let's discharge to the upper.
+			 */
+			new_mode = CHARGE_CONTROL_DISCHARGE;
 		break;
 	case CHARGE_CONTROL_DISCHARGE:
 		/* Discharging actively. */
-		if (sustain_soc.lower == soc && soc == sustain_soc.upper) {
-			mode = CHARGE_CONTROL_IDLE;
-		} else if (soc < sustain_soc.lower) {
-			mode = CHARGE_CONTROL_NORMAL;
-		}
+		if (soc <= sustain_soc.upper &&
+		    !(sustain_soc.flags & EC_CHARGE_CONTROL_FLAG_NO_IDLE))
+			/*
+			 * Normal case. We've been discharging and finally
+			 * reached the upper. Let's switch to IDLE to stay.
+			 */
+			new_mode = CHARGE_CONTROL_IDLE;
+		else if (soc < sustain_soc.lower)
+			/*
+			 * This can happen only if sustainer is restarted with
+			 * increase lower limit. Let's charge to the upper (then
+			 * switch to IDLE).
+			 */
+			new_mode = CHARGE_CONTROL_NORMAL;
 		break;
 	default:
-		return;
+		break;
 	}
 
-	if (mode == get_chg_ctrl_mode())
+	return new_mode;
+}
+
+static void sustain_battery_soc(void)
+{
+	enum ec_charge_control_mode mode = get_chg_ctrl_mode();
+	enum ec_charge_control_mode new_mode;
+	int rv;
+
+	/* If either AC or battery is not present, nothing to do. */
+	if (!curr.ac || curr.batt.is_present != BP_YES ||
+	    !battery_sustainer_enabled())
 		return;
 
-	rv = set_chg_ctrl_mode(mode);
+	new_mode = sustain_switch_mode(mode);
+
+	if (new_mode == mode)
+		return;
+
+	rv = set_chg_ctrl_mode(new_mode);
 	CPRINTS("%s: %s control mode to %s", __func__,
 		rv == EC_SUCCESS ? "Switched" : "Failed to switch",
-		mode_text[mode]);
+		mode_text[new_mode]);
 }
 
 static void current_limit_battery_soc(void)
@@ -2093,13 +2149,27 @@ charge_command_charge_control(struct host_cmd_handler_args *args)
 				return EC_RES_UNAVAILABLE;
 			if (rv)
 				return EC_RES_INVALID_PARAM;
+			if (args->version == 2) {
+				/*
+				 * V2 uses lower == upper to indicate NO_IDLE.
+				 * TODO: Remove this if-branch once all OS-side
+				 * components are updated to v3.
+				 */
+				if (sustain_soc.lower < sustain_soc.upper)
+					sustain_soc.flags =
+						EC_CHARGE_CONTROL_FLAG_NO_IDLE;
+			} else {
+				sustain_soc.flags = p->flags;
+			}
 		} else {
 			battery_sustainer_disable();
 		}
 	} else if (p->cmd == EC_CHARGE_CONTROL_CMD_GET) {
 		r->mode = get_chg_ctrl_mode();
 		r->sustain_soc.lower = sustain_soc.lower;
 		r->sustain_soc.upper = sustain_soc.upper;
+		if (args->version > 2)
+			r->flags = sustain_soc.flags;
 		args->response_size = sizeof(*r);
 		return EC_RES_SUCCESS;
 	} else {
@@ -2113,7 +2183,7 @@ charge_command_charge_control(struct host_cmd_handler_args *args)
 	return EC_RES_SUCCESS;
 }
 DECLARE_HOST_COMMAND(EC_CMD_CHARGE_CONTROL, charge_command_charge_control,
-		     EC_VER_MASK(2));
+		     EC_VER_MASK(2) | EC_VER_MASK(3));
 
 static enum ec_status
 charge_command_current_limit(struct host_cmd_handler_args *args)

include/charge_state.h

@@ -116,6 +116,7 @@ struct charge_state_data {
 struct sustain_soc {
 	int8_t lower;
 	int8_t upper;
+	uint8_t flags; /* enum ec_charge_control_flag */
 };
 
 #define BAT_MAX_DISCHG_CURRENT 5000 /* mA */

include/ec_commands.h

@@ -4428,7 +4428,7 @@ struct ec_params_i2c_write {
  * discharge the battery.
  */
 #define EC_CMD_CHARGE_CONTROL 0x0096
-#define EC_VER_CHARGE_CONTROL 2
+#define EC_VER_CHARGE_CONTROL 3
 
 enum ec_charge_control_mode {
 	CHARGE_CONTROL_NORMAL = 0,
@@ -4450,12 +4450,16 @@ enum ec_charge_control_cmd {
 	EC_CHARGE_CONTROL_CMD_GET,
 };
 
+enum ec_charge_control_flag {
+	EC_CHARGE_CONTROL_FLAG_NO_IDLE = BIT(0),
+};
+
 struct ec_params_charge_control {
 	uint32_t mode; /* enum charge_control_mode */
 
 	/* Below are the fields added in V2. */
 	uint8_t cmd; /* enum ec_charge_control_cmd. */
-	uint8_t reserved;
+	uint8_t flags; /* enum ec_charge_control_flag (v3+) */
 	/*
 	 * Lower and upper thresholds for battery sustainer. This struct isn't
 	 * named to avoid tainting foreign projects' name spaces.
@@ -4477,7 +4481,8 @@ struct ec_response_charge_control {
 		int8_t lower;
 		int8_t upper;
 	} sustain_soc;
-	uint16_t reserved;
+	uint8_t flags; /* enum ec_charge_control_flag (v3+) */
+	uint8_t reserved;
 } __ec_align4;
 
 /*****************************************************************************/