charge_state_v2: Store battery information in new structures

On dual battery systems, this allows to keep both batteries
information in similar structures. This also means that battery
information can only be fetched via host commands
EC_CMD_BATTERY_GET_STATIC/DYNAMIC (next CL will make it possible
to fetch the information via shared memory/ACPI).

BRANCH=none
BUG=b:65697620
TEST=Boot lux/wand, dual-battery algorithm works, AP can fetch
     both battery information via host commands.

Change-Id: I3c087e8f378c5cef0006f6bfe58335228a880e5b
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/888381
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Furquan Shaikh <furquan@chromium.org>

8 files changed, 126 insertions, 98 deletions

diff --git a/common/battery.c b/common/battery.c
index 65a375c02e..711d7f7913 100644
--- a/common/battery.c
+++ b/common/battery.c
@@ -21,6 +21,15 @@
 #define CPRINTF(format, args...) cprintf(CC_CHARGER, format, ## args)
 #define CPRINTS(format, args...) cprints(CC_CHARGER, format, ## args)
 
+#ifdef CONFIG_BATTERY_V2
+/*
+ * Store battery information in these 2 structures. Main (lid) battery is always
+ * at index 0, and secondary (base) battery at index 1.
+ */
+struct ec_response_battery_static_info battery_static[CONFIG_BATTERY_COUNT];
+struct ec_response_battery_dynamic_info battery_dynamic[CONFIG_BATTERY_COUNT];
+#endif
+
 #ifdef CONFIG_BATTERY_CUT_OFF
 
 #ifndef CONFIG_BATTERY_CUTOFF_DELAY_US
@@ -433,21 +442,20 @@ DECLARE_HOST_COMMAND(EC_CMD_BATTERY_VENDOR_PARAM,
 		     EC_VER_MASK(0));
 #endif /* CONFIG_BATTERY_VENDOR_PARAM */
 
-#ifdef CONFIG_EC_EC_COMM_BATTERY_MASTER
-/*
- * TODO(b:65697620): Add support for returning main battery information through
- * these commands, as well.
- */
+#ifdef CONFIG_HOSTCMD_BATTERY_V2
+#ifndef CONFIG_BATTERY_V2
+#error "CONFIG_HOSTCMD_BATTERY_V2 cannot be set without CONFIG_BATTERY_V2."
+#endif
 static int host_command_battery_get_static(struct host_cmd_handler_args *args)
 {
 	const struct ec_params_battery_static_info *p = args->params;
 	struct ec_response_battery_static_info *r = args->response;
 
-	if (p->index != 1)
+	if (p->index < 0 || p->index >= CONFIG_BATTERY_COUNT)
 		return EC_RES_INVALID_PARAM;
 
 	args->response_size = sizeof(*r);
-	memcpy(r, &base_battery_static, sizeof(*r));
+	memcpy(r, &battery_static[p->index], sizeof(*r));
 
 	return EC_RES_SUCCESS;
 }
@@ -460,15 +468,15 @@ static int host_command_battery_get_dynamic(struct host_cmd_handler_args *args)
 	const struct ec_params_battery_dynamic_info *p = args->params;
 	struct ec_response_battery_dynamic_info *r = args->response;
 
-	if (p->index != 1)
+	if (p->index < 0 || p->index >= CONFIG_BATTERY_COUNT)
 		return EC_RES_INVALID_PARAM;
 
 	args->response_size = sizeof(*r);
-	memcpy(r, &base_battery_dynamic, sizeof(*r));
+	memcpy(r, &battery_dynamic[p->index], sizeof(*r));
 
 	return EC_RES_SUCCESS;
 }
 DECLARE_HOST_COMMAND(EC_CMD_BATTERY_GET_DYNAMIC,
 		     host_command_battery_get_dynamic,
 		     EC_VER_MASK(0));
-#endif /* CONFIG_EC_EC_COMM_BATTERY */
+#endif /* CONFIG_HOSTCMD_BATTERY_V2 */
diff --git a/common/charge_state_v2.c b/common/charge_state_v2.c
index d4ab11ca8b..608e1e92e9 100644
--- a/common/charge_state_v2.c
+++ b/common/charge_state_v2.c
@@ -219,13 +219,15 @@ static const struct dual_battery_policy db_policy = {
 
 static int charge_get_base_percent(void)
 {
-	if (base_battery_dynamic.flags & (BATT_FLAG_BAD_FULL_CAPACITY |
-					  BATT_FLAG_BAD_REMAINING_CAPACITY))
+	const struct ec_response_battery_dynamic_info *const bd =
+		&battery_dynamic[BATT_IDX_BASE];
+
+	if (bd->flags & (BATT_FLAG_BAD_FULL_CAPACITY |
+			 BATT_FLAG_BAD_REMAINING_CAPACITY))
 		return -1;
 
-	if (base_battery_dynamic.full_capacity > 0)
-		return 100 * base_battery_dynamic.remaining_capacity
-			/ base_battery_dynamic.full_capacity;
+	if (bd->full_capacity > 0)
+		return 100 * bd->remaining_capacity / bd->full_capacity;
 
 	return 0;
 }
@@ -396,6 +398,10 @@ static void charge_allocate_input_current_limit(void)
 
 	int charge_lid = charge_get_percent();
 
+	const struct ec_response_battery_dynamic_info *const base_bd =
+		&battery_dynamic[BATT_IDX_BASE];
+
+
 	if (!base_connected) {
 		set_base_lid_current(0, 0, curr.desired_input_current, 1);
 		prev_base_battery_power = -1;
@@ -480,11 +486,11 @@ static void charge_allocate_input_current_limit(void)
 	lid_battery_power = MIN(lid_battery_power, lid_battery_power_max);
 
 	/* Estimate base battery power. */
-	if (!(base_battery_dynamic.flags & EC_BATT_FLAG_INVALID_DATA)) {
-		base_battery_power = base_battery_dynamic.actual_current *
-				     base_battery_dynamic.actual_voltage / 1000;
-		base_battery_power_max = base_battery_dynamic.desired_current *
-				    base_battery_dynamic.desired_voltage / 1000;
+	if (!(base_bd->flags & EC_BATT_FLAG_INVALID_DATA)) {
+		base_battery_power = base_bd->actual_current *
+				     base_bd->actual_voltage / 1000;
+		base_battery_power_max = base_bd->desired_current *
+					 base_bd->desired_voltage / 1000;
 	}
 	if (base_battery_power < prev_base_battery_power)
 		base_battery_power = smooth_value(prev_base_battery_power,
@@ -548,7 +554,7 @@ static void charge_allocate_input_current_limit(void)
 }
 #endif /* CONFIG_EC_EC_COMM_BATTERY_MASTER */
 
-#ifdef HAS_TASK_HOSTCMD
+#ifndef CONFIG_BATTERY_V2
 /* Returns zero if every item was updated. */
 static int update_static_battery_info(void)
 {
@@ -696,12 +702,7 @@ static void update_dynamic_battery_info(void)
 	if (send_batt_status_event)
 		host_set_single_event(EC_HOST_EVENT_BATTERY_STATUS);
 }
-#elif defined(CONFIG_EC_EC_COMM_BATTERY_SLAVE)
-/*
- * TODO(b:65697620): Make this depend on a separate config option instead, so
- * that, even on systems that would typically use memmap, we can decide to use
- * these structures, and pass battery information via a host command instead.
- */
+#else /* CONFIG_BATTERY_V2 */
 static int update_static_battery_info(void)
 {
 	int batt_serial;
@@ -712,48 +713,48 @@ static int update_static_battery_info(void)
 	 */
 	int rv, ret;
 
+	struct ec_response_battery_static_info *const bs =
+		&battery_static[BATT_IDX_MAIN];
+
 	/* Clear all static information. */
-	memset(&base_battery_static, 0, sizeof(base_battery_static));
+	memset(bs, 0, sizeof(*bs));
 
 	/* Smart battery serial number is 16 bits */
 	rv = battery_serial_number(&batt_serial);
 	if (!rv)
-		snprintf(base_battery_static.serial,
-			sizeof(base_battery_static.serial),
-			"%04X", batt_serial);
+		snprintf(bs->serial, sizeof(bs->serial), "%04X", batt_serial);
 
 	/* Design Capacity of Full */
 	ret = battery_design_capacity(&val);
 	if (!ret)
-		base_battery_static.design_capacity = val;
+		bs->design_capacity = val;
 	rv |= ret;
 
 	/* Design Voltage */
 	ret = battery_design_voltage(&val);
 	if (!ret)
-		base_battery_static.design_voltage = val;
+		bs->design_voltage = val;
 	rv |= ret;
 
 	/* Cycle Count */
 	ret = battery_cycle_count(&val);
 	if (!ret)
-		base_battery_static.cycle_count = val;
+		bs->cycle_count = val;
 	rv |= ret;
 
 	/* Battery Manufacturer string */
-	rv |= battery_manufacturer_name(base_battery_static.manufacturer,
-				sizeof(base_battery_static.manufacturer));
+	rv |= battery_manufacturer_name(bs->manufacturer,
+					sizeof(bs->manufacturer));
 
 	/* Battery Model string */
-	rv |= battery_device_name(base_battery_static.model,
-				sizeof(base_battery_static.model));
+	rv |= battery_device_name(bs->model, sizeof(bs->model));
 
 	/* Battery Type string */
-	rv |= battery_device_chemistry(base_battery_static.type,
-				sizeof(base_battery_static.type));
+	rv |= battery_device_chemistry(bs->type, sizeof(bs->type));
 
 	/* Zero the dynamic entries. They'll come next. */
-	memset(&base_battery_dynamic, 0, sizeof(base_battery_dynamic));
+	memset(&battery_dynamic[BATT_IDX_MAIN], 0,
+	       sizeof(battery_dynamic[BATT_IDX_MAIN]));
 
 	if (rv)
 		problem(PR_STATIC_UPDATE, rv);
@@ -771,6 +772,9 @@ static void update_dynamic_battery_info(void)
 	static int __bss_slow batt_present;
 	uint8_t tmp;
 
+	struct ec_response_battery_dynamic_info *const bd =
+		&battery_dynamic[BATT_IDX_MAIN];
+
 	tmp = 0;
 	if (curr.ac)
 		tmp |= EC_BATT_FLAG_AC_PRESENT;
@@ -792,18 +796,16 @@ static void update_dynamic_battery_info(void)
 		tmp |= EC_BATT_FLAG_INVALID_DATA;
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_VOLTAGE))
-		base_battery_dynamic.actual_voltage = curr.batt.voltage;
+		bd->actual_voltage = curr.batt.voltage;
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_CURRENT))
-		base_battery_dynamic.actual_current = curr.batt.current;
+		bd->actual_current = curr.batt.current;
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_DESIRED_VOLTAGE))
-		base_battery_dynamic.desired_voltage =
-			curr.batt.desired_voltage;
+		bd->desired_voltage = curr.batt.desired_voltage;
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_DESIRED_CURRENT))
-		base_battery_dynamic.desired_current =
-			curr.batt.desired_current;
+		bd->desired_current = curr.batt.desired_current;
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_REMAINING_CAPACITY)) {
 		/*
@@ -812,18 +814,17 @@ static void update_dynamic_battery_info(void)
 		 * to Chrome OS powerd.
 		 */
 		if (curr.batt.remaining_capacity == 0 && !curr.batt_is_charging)
-			base_battery_dynamic.remaining_capacity = 1;
+			bd->remaining_capacity = 1;
 		else
-			base_battery_dynamic.remaining_capacity =
-				curr.batt.remaining_capacity;
+			bd->remaining_capacity = curr.batt.remaining_capacity;
 	}
 
 	if (!(curr.batt.flags & BATT_FLAG_BAD_FULL_CAPACITY) &&
 		(curr.batt.full_capacity <=
-		   (base_battery_dynamic.full_capacity - LFCC_EVENT_THRESH) ||
+			(bd->full_capacity - LFCC_EVENT_THRESH) ||
 		 curr.batt.full_capacity >=
-		   (base_battery_dynamic.full_capacity + LFCC_EVENT_THRESH))) {
-		base_battery_dynamic.full_capacity = curr.batt.full_capacity;
+			(bd->full_capacity + LFCC_EVENT_THRESH))) {
+		bd->full_capacity = curr.batt.full_capacity;
 	}
 
 	if (curr.batt.is_present == BP_YES &&
@@ -834,9 +835,9 @@ static void update_dynamic_battery_info(void)
 	tmp |= curr.batt_is_charging ? EC_BATT_FLAG_CHARGING :
 				       EC_BATT_FLAG_DISCHARGING;
 
-	base_battery_dynamic.flags = tmp;
+	bd->flags = tmp;
 }
-#endif
+#endif /* CONFIG_BATTERY_V2 */
 
 static const char * const state_list[] = {
 	"idle", "discharge", "charge", "precharge"
@@ -1238,7 +1239,7 @@ void charger_task(void *u)
 #ifdef CONFIG_EC_EC_COMM_BATTERY_MASTER
 	base_responsive = 0;
 	curr.input_voltage = CHARGE_VOLTAGE_UNINITIALIZED;
-	base_battery_dynamic.flags = EC_BATT_FLAG_INVALID_DATA;
+	battery_dynamic[BATT_IDX_BASE].flags = EC_BATT_FLAG_INVALID_DATA;
 	charge_base = -1;
 #endif
 
@@ -1302,18 +1303,19 @@ void charger_task(void *u)
 
 		if (!base_connected) {
 			/* Invalidate static/dynamic information */
-			base_battery_dynamic.flags = EC_BATT_FLAG_INVALID_DATA;
+			battery_dynamic[BATT_IDX_BASE].flags =
+				EC_BATT_FLAG_INVALID_DATA;
 			charge_base = -1;
 			base_responsive = 0;
 			prev_current_base = 0;
 			prev_allow_charge_base = 0;
 		} else if (base_responsive) {
-			int old_flags = base_battery_dynamic.flags;
+			int old_flags = battery_dynamic[BATT_IDX_BASE].flags;
 
 			ec_ec_master_base_get_dynamic_info();
 
 			/* Fetch static information when flags change. */
-			if (old_flags != base_battery_dynamic.flags)
+			if (old_flags != battery_dynamic[BATT_IDX_BASE].flags)
 				ec_ec_master_base_get_static_info();
 
 			charge_base = charge_get_base_percent();
diff --git a/common/ec_ec_comm_master.c b/common/ec_ec_comm_master.c
index 9a742ea40b..a30be6d329 100644
--- a/common/ec_ec_comm_master.c
+++ b/common/ec_ec_comm_master.c
@@ -5,6 +5,7 @@
  * EC-EC communication, functions and definitions for master.
  */
 
+#include "battery.h"
 #include "common.h"
 #include "console.h"
 #include "crc8.h"
@@ -18,13 +19,6 @@
 #define CPRINTF(format, args...) cprintf(CC_CHARGER, format, ## args)
 
 /*
- * TODO(b:65697620): Move these to some the second position of some battery
- * array, depending on a config option.
- */
-struct ec_response_battery_static_info base_battery_static;
-struct ec_response_battery_dynamic_info base_battery_dynamic;
-
-/*
  * TODO(b:65697962): The packed structures below do not play well if we force EC
  * host commands structures to be aligned on 32-bit boundary. There are ways to
  * fix that, possibly requiring copying data around, or modifying
@@ -281,8 +275,8 @@ int ec_ec_master_base_get_dynamic_info(void)
 	CPRINTF("I-desired:  %d mA\n", data.resp.info.desired_current);
 #endif
 
-	memcpy(&base_battery_dynamic, &data.resp.info,
-				sizeof(base_battery_dynamic));
+	memcpy(&battery_dynamic[BATT_IDX_BASE], &data.resp.info,
+				sizeof(battery_dynamic[BATT_IDX_BASE]));
 	return EC_RES_SUCCESS;
 }
 
@@ -321,8 +315,8 @@ int ec_ec_master_base_get_static_info(void)
 	CPRINTF("C-count:    %d\n", data.resp.info.cycle_count);
 #endif
 
-	memcpy(&base_battery_static, &data.resp.info,
-				sizeof(base_battery_static));
+	memcpy(&battery_static[BATT_IDX_BASE], &data.resp.info,
+				sizeof(battery_static[BATT_IDX_BASE]));
 	return EC_RES_SUCCESS;
 }
 
diff --git a/common/ec_ec_comm_slave.c b/common/ec_ec_comm_slave.c
index c9696a52d4..91687bb79a 100644
--- a/common/ec_ec_comm_slave.c
+++ b/common/ec_ec_comm_slave.c
@@ -26,13 +26,6 @@
 /* Print extra debugging information */
 #undef EXTRA_DEBUG
 
-/*
- * TODO(b:65697620): Move these to some other C file, depending on a config
- * option.
- */
-struct ec_response_battery_static_info base_battery_static;
-struct ec_response_battery_dynamic_info base_battery_dynamic;
-
 /* Set if the master allows the slave to charge the battery. */
 static int charging_allowed;
 
@@ -274,14 +267,14 @@ void ec_ec_comm_slave_task(void *u)
 		case EC_CMD_BATTERY_GET_STATIC:
 			/* Note that we ignore the battery index parameter. */
 			write_response(EC_RES_SUCCESS, seq,
-				&base_battery_static,
-				sizeof(base_battery_static));
+				&battery_static[BATT_IDX_MAIN],
+				sizeof(battery_static[BATT_IDX_MAIN]));
 			break;
 		case EC_CMD_BATTERY_GET_DYNAMIC:
 			/* Note that we ignore the battery index parameter. */
 			write_response(EC_RES_SUCCESS, seq,
-				&base_battery_dynamic,
-				sizeof(base_battery_dynamic));
+				&battery_dynamic[BATT_IDX_MAIN],
+				sizeof(battery_dynamic[BATT_IDX_MAIN]));
 			break;
 		case EC_CMD_CHARGER_CONTROL: {
 			handle_cmd_charger_control((void *)params,
diff --git a/include/battery.h b/include/battery.h
index 43425fcd8c..b783432ded 100644
--- a/include/battery.h
+++ b/include/battery.h
@@ -9,6 +9,20 @@
 #define __CROS_EC_BATTERY_H
 
 #include "common.h"
+#include "host_command.h"
+
+/* Battery index, only used with CONFIG_BATTERY_V2. */
+enum battery_index {
+	BATT_IDX_MAIN,
+	BATT_IDX_BASE,
+};
+
+#ifdef CONFIG_BATTERY_V2
+extern struct ec_response_battery_static_info
+	battery_static[CONFIG_BATTERY_COUNT];
+extern struct ec_response_battery_dynamic_info
+	battery_dynamic[CONFIG_BATTERY_COUNT];
+#endif
 
 /* Stop charge when charging and battery level >= this percentage */
 #define BATTERY_LEVEL_FULL		100
diff --git a/include/config.h b/include/config.h
index 243b51ca33..f091694ec1 100644
--- a/include/config.h
+++ b/include/config.h
@@ -335,6 +335,23 @@
 #undef CONFIG_BATTERY_LEVEL_NEAR_FULL
 
 /*
+ * Use an alternative method to store battery information: Instead of writing
+ * directly to host memory mapped region, this keeps the battery information in
+ * ec_response_battery_static/dynamic_info structures, that can then be fetched
+ * using host commands, or via EC_ACPI_MEM_BATTERY_INDEX command, which tells
+ * the EC to update the shared memory.
+ *
+ * This is required on dual-battery systems, and on on hostless bases with a
+ * battery.
+ */
+#undef CONFIG_BATTERY_V2
+
+/*
+ * Number of batteries, only matters when CONFIG_BATTERY_V2 is used.
+ */
+#undef CONFIG_BATTERY_COUNT
+
+/*
  * Expose some data when it is needed.
  * For example, battery disconnect state
  */
@@ -1546,6 +1563,13 @@
  */
 #undef CONFIG_HOSTCMD_ALIGNED
 
+/*
+ * Include host commands to fetch battery information from
+ * ec_response_battery_static/dynamic_info structures, only makes sense when
+ * CONFIG_BATTERY_V2 is enabled.
+ */
+#undef CONFIG_HOSTCMD_BATTERY_V2
+
 /* Default hcdebug mode, e.g. HCDEBUG_OFF or HCDEBUG_NORMAL */
 #define CONFIG_HOSTCMD_DEBUG_MODE HCDEBUG_NORMAL
 
@@ -3303,10 +3327,14 @@
 #ifdef CONFIG_EC_EC_COMM_BATTERY
 #ifdef CONFIG_EC_EC_COMM_MASTER
 #define CONFIG_EC_EC_COMM_BATTERY_MASTER
+#define CONFIG_BATTERY_V2
+#define CONFIG_BATTERY_COUNT 2
 #endif
 
 #ifdef CONFIG_EC_EC_COMM_SLAVE
 #define CONFIG_EC_EC_COMM_BATTERY_SLAVE
+#define CONFIG_BATTERY_V2
+#define CONFIG_BATTERY_COUNT 1
 #endif
 #endif /* CONFIG_EC_EC_COMM_BATTERY */
 
diff --git a/include/ec_ec_comm_master.h b/include/ec_ec_comm_master.h
index 98b8011b95..adaf564c65 100644
--- a/include/ec_ec_comm_master.h
+++ b/include/ec_ec_comm_master.h
@@ -11,19 +11,12 @@
 #include <stdint.h>
 #include "config.h"
 
-/*
- * TODO(b:65697620): Move these to some other C file, depending on a config
- * option.
- */
-extern struct ec_response_battery_static_info base_battery_static;
-extern struct ec_response_battery_dynamic_info base_battery_dynamic;
-
 /**
  * Sends EC_CMD_BATTERY_GET_DYNAMIC command to slave, and writes the
- * battery dynamic information into base_battery_dynamic.
+ * battery dynamic information into battery_dynamic[BATT_IDX_BASE].
  *
- * Leaves base_battery_dynamic intact on error: it is the callers responsability
- * to clear the data or ignore it.
+ * Leaves battery_dynamic[BATT_IDX_BASE] intact on error: it is the callers
+ * responsibility to clear the data or ignore it.
 
  * @return EC_RES_SUCCESS on success, EC_RES_ERROR on communication error,
  * else forwards the error code from the slave.
@@ -32,10 +25,10 @@ int ec_ec_master_base_get_dynamic_info(void);
 
 /**
  * Sends EC_CMD_BATTERY_GET_STATIC command to slave, and writes the
- * battery static information into base_static_dynamic.
+ * battery static information into battery_static[BATT_IDX_BASE].
  *
- * Leaves base_battery_static intact on error: it is the callers responsability
- * to clear the data or ignore it.
+ * Leaves battery_static[BATT_IDX_BASE] intact on error: it is the callers
+ * responsibility to clear the data or ignore it.
  *
  * @return EC_RES_SUCCESS on success, EC_RES_ERROR on communication error,
  * else forwards the error code from the slave.
diff --git a/include/ec_ec_comm_slave.h b/include/ec_ec_comm_slave.h
index e3501400e2..19e1912d94 100644
--- a/include/ec_ec_comm_slave.h
+++ b/include/ec_ec_comm_slave.h
@@ -12,10 +12,6 @@
 #include "consumer.h"
 #include "queue.h"
 
-/* TODO(b:65697620): Move these to battery.h, depending on a config option. */
-extern struct ec_response_battery_static_info base_battery_static;
-extern struct ec_response_battery_dynamic_info base_battery_dynamic;
-
 extern struct queue const ec_ec_comm_slave_input;
 extern struct queue const ec_ec_comm_slave_output;