charge_manager: Wait for dualrole determination before charging

If a dual-role charger is plugged, we will not realize it is dual-role
until after we see a type-C charge source. It can cause us to briefly
charge from a dual-role charger, which has bad side effects related to
charge override and the lightbar. Fix this by not charging from a port
until we are fairly certain that it is a dedicated charger (based upon
PD discovery timeout).

BUG=chrome-os-partner:36390
TEST=Manual on Samus. Insert 1A Apple charger, verify correct detection.
Run 'chgoverride -2' to prevent charging, then repeatedly insert +
remove a dual-role charger on the other charge port. Verify that
charging is still prevented. Finally, insert a dedicated charger and
verify that the override is removed. Also, pass unit tests and verify
correct detection in various scenarios with various chargers.
BRANCH=Samus

Change-Id: Ia4154f34dd0a850b6e72bebadbd938f034532f14
Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/247130
Reviewed-by: Alec Berg <alecaberg@chromium.org>

9 files changed, 369 insertions, 152 deletions

diff --git a/board/ryu/board.c b/board/ryu/board.c
index baf633fa96..57e2d2e2ed 100644
--- a/board/ryu/board.c
+++ b/board/ryu/board.c
@@ -110,19 +110,29 @@ void usb_charger_task(void)
 		if (device_type || PI3USB9281_CHG_STATUS_ANY(charger_status)) {
 			charge.current = pi3usb9281_get_ilim(device_type,
 							     charger_status);
-			charge_manager_update(type, 0, &charge);
+			charge_manager_update_charge(type, 0, &charge);
 		} else { /* Detachment: update available charge to 0 */
 			charge.current = 0;
-			charge_manager_update(CHARGE_SUPPLIER_PROPRIETARY, 0,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_CDP, 0,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_DCP, 0,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_SDP, 0,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_OTHER, 0,
-					      &charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_PROPRIETARY,
+						0,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_CDP,
+						0,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_DCP,
+						0,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_SDP,
+						0,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_OTHER,
+						0,
+						&charge);
 		}
 
 		/* notify host of power info change */
@@ -158,12 +168,13 @@ static void board_init(void)
 	/* Initialize all pericom charge suppliers to 0 */
 	charge.voltage = USB_BC12_CHARGE_VOLTAGE;
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_PROPRIETARY, 0,
-			      &charge);
-	charge_manager_update(CHARGE_SUPPLIER_BC12_CDP, 0, &charge);
-	charge_manager_update(CHARGE_SUPPLIER_BC12_DCP, 0, &charge);
-	charge_manager_update(CHARGE_SUPPLIER_BC12_SDP, 0, &charge);
-	charge_manager_update(CHARGE_SUPPLIER_OTHER, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_PROPRIETARY,
+				     0,
+				     &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_BC12_CDP, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_BC12_DCP, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_BC12_SDP, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_OTHER, 0, &charge);
 
 	/* Enable pericom BC1.2 interrupts. */
 	gpio_enable_interrupt(GPIO_USBC_BC12_INT_L);
diff --git a/board/ryu/usb_pd_policy.c b/board/ryu/usb_pd_policy.c
index 7556c86ba6..596e8c003b 100644
--- a/board/ryu/usb_pd_policy.c
+++ b/board/ryu/usb_pd_policy.c
@@ -38,7 +38,7 @@ void pd_set_input_current_limit(int port, uint32_t max_ma,
 	struct charge_port_info charge;
 	charge.current = max_ma;
 	charge.voltage = supply_voltage;
-	charge_manager_update(CHARGE_SUPPLIER_PD, port, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_PD, port, &charge);
 }
 
 void typec_set_input_current_limit(int port, uint32_t max_ma,
@@ -47,7 +47,7 @@ void typec_set_input_current_limit(int port, uint32_t max_ma,
 	struct charge_port_info charge;
 	charge.current = max_ma;
 	charge.voltage = supply_voltage;
-	charge_manager_update(CHARGE_SUPPLIER_TYPEC, port, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TYPEC, port, &charge);
 }
 
 int pd_is_valid_input_voltage(int mv)
diff --git a/board/samus_pd/board.c b/board/samus_pd/board.c
index 3fd3f0e219..63b435bc33 100644
--- a/board/samus_pd/board.c
+++ b/board/samus_pd/board.c
@@ -214,24 +214,29 @@ void usb_charger_task(void)
 		if (device_type || PI3USB9281_CHG_STATUS_ANY(charger_status)) {
 			charge.current = pi3usb9281_get_ilim(device_type,
 							     charger_status);
-			charge_manager_update(type, port, &charge);
+			charge_manager_update_charge(type, port, &charge);
 		} else { /* Detachment: update available charge to 0 */
 			charge.current = 0;
-			charge_manager_update(CHARGE_SUPPLIER_PROPRIETARY,
-					      port,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_CDP,
-					      port,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_DCP,
-					      port,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_BC12_SDP,
-					      port,
-					      &charge);
-			charge_manager_update(CHARGE_SUPPLIER_OTHER,
-					      port,
-					      &charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_PROPRIETARY,
+						port,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_CDP,
+						port,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_DCP,
+						port,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_BC12_SDP,
+						port,
+						&charge);
+			charge_manager_update_charge(
+						CHARGE_SUPPLIER_OTHER,
+						port,
+						&charge);
 		}
 
 		/* notify host of power info change */
@@ -371,12 +376,21 @@ static void board_init(void)
 	charge.voltage = USB_BC12_CHARGE_VOLTAGE;
 	charge.current = 0;
 	for (i = 0; i < PD_PORT_COUNT; i++) {
-		charge_manager_update(CHARGE_SUPPLIER_PROPRIETARY, i,
-				      &charge);
-		charge_manager_update(CHARGE_SUPPLIER_BC12_CDP, i, &charge);
-		charge_manager_update(CHARGE_SUPPLIER_BC12_DCP, i, &charge);
-		charge_manager_update(CHARGE_SUPPLIER_BC12_SDP, i, &charge);
-		charge_manager_update(CHARGE_SUPPLIER_OTHER, i, &charge);
+		charge_manager_update_charge(CHARGE_SUPPLIER_PROPRIETARY,
+					     i,
+					     &charge);
+		charge_manager_update_charge(CHARGE_SUPPLIER_BC12_CDP,
+					     i,
+					     &charge);
+		charge_manager_update_charge(CHARGE_SUPPLIER_BC12_DCP,
+					     i,
+					     &charge);
+		charge_manager_update_charge(CHARGE_SUPPLIER_BC12_SDP,
+					     i,
+					     &charge);
+		charge_manager_update_charge(CHARGE_SUPPLIER_OTHER,
+					     i,
+					     &charge);
 	}
 
 	/* Enable pericom BC1.2 interrupts. */
diff --git a/board/samus_pd/usb_pd_policy.c b/board/samus_pd/usb_pd_policy.c
index 6a5a0043c0..7d638dfd0c 100644
--- a/board/samus_pd/usb_pd_policy.c
+++ b/board/samus_pd/usb_pd_policy.c
@@ -111,7 +111,7 @@ void pd_set_input_current_limit(int port, uint32_t max_ma,
 	struct charge_port_info charge;
 	charge.current = max_ma;
 	charge.voltage = supply_voltage;
-	charge_manager_update(CHARGE_SUPPLIER_PD, port, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_PD, port, &charge);
 
 	/* notify host of power info change */
 	pd_send_host_event(PD_EVENT_POWER_CHANGE);
@@ -123,7 +123,7 @@ void typec_set_input_current_limit(int port, uint32_t max_ma,
 	struct charge_port_info charge;
 	charge.current = max_ma;
 	charge.voltage = supply_voltage;
-	charge_manager_update(CHARGE_SUPPLIER_TYPEC, port, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TYPEC, port, &charge);
 
 	/* notify host of power info change */
 	pd_send_host_event(PD_EVENT_POWER_CHANGE);
diff --git a/common/charge_manager.c b/common/charge_manager.c
index d11fef91d8..c3e5b4d0ce 100644
--- a/common/charge_manager.c
+++ b/common/charge_manager.c
@@ -43,6 +43,11 @@ static int override_port = OVERRIDE_OFF;
 static int delayed_override_port = OVERRIDE_OFF;
 static timestamp_t delayed_override_deadline;
 
+enum charge_manager_change_type {
+	CHANGE_CHARGE,
+	CHANGE_DUALROLE,
+};
+
 /**
  * Initialize available charge. Run before board init, so board init can
  * initialize data, if needed.
@@ -127,7 +132,7 @@ static void charge_manager_fill_power_info(int port,
 		r->role = USB_PD_PORT_POWER_DISCONNECTED;
 
 	/* Is port partner dual-role capable */
-	r->dualrole = pd_get_partner_dualrole_capable(port);
+	r->dualrole = (pd_get_partner_dualrole_capable(port) == CAP_DUALROLE);
 
 	if (sup == CHARGE_SUPPLIER_NONE ||
 	    r->role == USB_PD_PORT_POWER_SOURCE) {
@@ -226,7 +231,7 @@ static void charge_manager_cleanup_override_port(int port)
 	if (port < 0 || port >= PD_PORT_COUNT)
 		return;
 
-	if (pd_get_partner_dualrole_capable(port) &&
+	if (pd_get_partner_dualrole_capable(port) == CAP_DUALROLE &&
 	    pd_get_role(port) == PD_ROLE_SINK)
 		pd_request_power_swap(port);
 }
@@ -277,7 +282,8 @@ static void charge_manager_get_best_charge_port(int *new_port,
 				 * Don't charge from a dual-role port unless
 				 * it is our override port.
 				 */
-				if (pd_get_partner_dualrole_capable(j) &&
+				if (pd_get_partner_dualrole_capable(j) !=
+				    CAP_DEDICATED &&
 				    override_port != j)
 					continue;
 
@@ -425,60 +431,117 @@ static void charge_manager_refresh(void)
 }
 DECLARE_DEFERRED(charge_manager_refresh);
 
-/**
- * Update available charge for a given port / supplier.
- *
- * @param supplier		Charge supplier to update.
- * @param port			Charge port to update.
- * @param charge		Charge port current / voltage.
- */
-void charge_manager_update(int supplier,
-			   int port,
-			   struct charge_port_info *charge)
+static void charge_manager_make_change(enum charge_manager_change_type change,
+				       int supplier,
+				       int port,
+				       struct charge_port_info *charge)
 {
-	ASSERT(supplier >= 0 && supplier < CHARGE_SUPPLIER_COUNT);
-	ASSERT(port >= 0 && port < PD_PORT_COUNT);
-
-	/* Update charge table if needed. */
-	if (available_charge[supplier][port].current != charge->current ||
-		available_charge[supplier][port].voltage != charge->voltage) {
-		/* Remove override when a dedicated charger is plugged */
-		if (available_charge[supplier][port].current == 0 &&
-		    charge->current > 0 &&
-		    !pd_get_partner_dualrole_capable(port)) {
-			charge_manager_cleanup_override_port(override_port);
-			override_port = OVERRIDE_OFF;
-			if (delayed_override_port != OVERRIDE_OFF) {
-				charge_manager_cleanup_override_port(
-					delayed_override_port);
-				delayed_override_port = OVERRIDE_OFF;
-				hook_call_deferred(
-					board_charge_manager_override_timeout,
-					-1);
+	int i;
+	int clear_override = 0;
+
+	/* Determine if this is a change which can affect charge status */
+	switch (change) {
+	case CHANGE_CHARGE:
+		/* Ignore changes where charge is identical */
+		if (available_charge[supplier][port].current ==
+		    charge->current &&
+		    available_charge[supplier][port].voltage ==
+		    charge->voltage)
+			return;
+		if (charge->current > 0 &&
+		    available_charge[supplier][port].current == 0)
+			clear_override = 1;
+		break;
+	case CHANGE_DUALROLE:
+		/*
+		 * Ignore all except for transition to non-dualrole,
+		 * which may occur some time after we see a charge
+		 */
+		if (pd_get_partner_dualrole_capable(port) != CAP_DEDICATED)
+			return;
+		/* Clear override only if a charge is present on the port */
+		for (i = 0; i < CHARGE_SUPPLIER_COUNT; ++i)
+			if (available_charge[i][port].current > 0) {
+				clear_override = 1;
+				break;
 			}
+		/*
+		 * If there is no charge present on the port, the dualrole
+		 * change is meaningless to charge_manager.
+		 */
+		if (!clear_override)
+			return;
+		break;
+	}
+
+	/* Remove override when a dedicated charger is plugged */
+	if (clear_override && override_port != port &&
+	    pd_get_partner_dualrole_capable(port) == CAP_DEDICATED) {
+		charge_manager_cleanup_override_port(override_port);
+		override_port = OVERRIDE_OFF;
+		if (delayed_override_port != OVERRIDE_OFF) {
+			charge_manager_cleanup_override_port(
+				delayed_override_port);
+			delayed_override_port = OVERRIDE_OFF;
+			hook_call_deferred(
+				board_charge_manager_override_timeout,
+				-1);
 		}
+	}
+
+	if (change == CHANGE_CHARGE) {
 		available_charge[supplier][port].current = charge->current;
 		available_charge[supplier][port].voltage = charge->voltage;
 
 		/*
 		 * If we have a charge on our delayed override port within
 		 * the deadline, make it our override port.
-		 */
-		if (port == delayed_override_port &&
-		    charge->current > 0 &&
+		*/
+		if (port == delayed_override_port && charge->current > 0 &&
 		    pd_get_role(delayed_override_port) == PD_ROLE_SINK &&
 		    get_time().val < delayed_override_deadline.val)
 			charge_manager_set_override(port);
-
-		/*
-		 * Don't call charge_manager_refresh unless all ports +
-		 * suppliers have reported in. We don't want to make changes
-		 * to our charge port until we are certain we know what is
-		 * attached.
-		 */
-		if (charge_manager_is_seeded())
-			hook_call_deferred(charge_manager_refresh, 0);
 	}
+
+	/*
+	 * Don't call charge_manager_refresh unless all ports +
+	 * suppliers have reported in. We don't want to make changes
+	 * to our charge port until we are certain we know what is
+	 * attached.
+	 */
+	if (charge_manager_is_seeded())
+		hook_call_deferred(charge_manager_refresh, 0);
+}
+
+/**
+ * Update available charge for a given port / supplier.
+ *
+ * @param supplier		Charge supplier to update.
+ * @param port			Charge port to update.
+ * @param charge		Charge port current / voltage.
+ */
+void charge_manager_update_charge(int supplier,
+				  int port,
+				  struct charge_port_info *charge)
+{
+	ASSERT(supplier >= 0 && supplier < CHARGE_SUPPLIER_COUNT);
+	ASSERT(port >= 0 && port < PD_PORT_COUNT);
+	ASSERT(charge != NULL);
+
+	charge_manager_make_change(CHANGE_CHARGE, supplier, port, charge);
+}
+
+/**
+ * Notify charge_manager of a partner dualrole capability change. There is
+ * no capability parameter to this function, since the capability can be
+ * checked with pd_get_partner_dualrole_capable().
+ *
+ * @param port			Charge port which changed.
+ */
+void charge_manager_update_dualrole(int port)
+{
+	ASSERT(port >= 0 && port < PD_PORT_COUNT);
+	charge_manager_make_change(CHANGE_DUALROLE, 0, port, NULL);
 }
 
 /**
@@ -540,7 +603,7 @@ int charge_manager_set_override(int port)
 	 * power swap and set the delayed override for swap completion.
 	 */
 	else if (pd_get_role(port) != PD_ROLE_SINK &&
-		 pd_get_partner_dualrole_capable(port)) {
+		 pd_get_partner_dualrole_capable(port) == CAP_DUALROLE) {
 		delayed_override_deadline.val = get_time().val +
 						POWER_SWAP_TIMEOUT;
 		delayed_override_port = port;
diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c
index 049e53f13f..77329ef5a4 100644
--- a/common/usb_pd_protocol.c
+++ b/common/usb_pd_protocol.c
@@ -280,6 +280,11 @@ static struct pd_protocol {
 	int prev_request_mv;
 #endif
 
+#ifdef CONFIG_CHARGE_MANAGER
+	/* Dual-role capability of attached partner port */
+	enum dualrole_capabilities dualrole_capability;
+#endif
+
 	/* PD state for Vendor Defined Messages */
 	enum vdm_states vdm_state;
 	/* Timeout for the current vdm state.  Set to 0 for no timeout. */
@@ -399,6 +404,10 @@ static inline void set_state(int port, enum pd_states next_state)
 #endif
 		pd[port].dev_id = 0;
 		pd[port].flags &= ~PD_FLAGS_RESET_ON_DISCONNECT_MASK;
+#ifdef CONFIG_CHARGE_MANAGER
+		pd[port].dualrole_capability = CAP_UNKNOWN;
+		charge_manager_update_dualrole(port);
+#endif
 #ifdef CONFIG_USB_PD_ALT_MODE_DFP
 		pd_dfp_exit_mode(port, 0, 0);
 #endif
@@ -968,10 +977,19 @@ static void pd_update_pdo_flags(int port, uint32_t pdo)
 {
 	/* can only parse PDO flags if type is fixed */
 	if ((pdo & PDO_TYPE_MASK) == PDO_TYPE_FIXED) {
-		if (pdo & PDO_FIXED_DUAL_ROLE)
+		if (pdo & PDO_FIXED_DUAL_ROLE) {
 			pd[port].flags |= PD_FLAGS_PARTNER_DR_POWER;
-		else
+#ifdef CONFIG_CHARGE_MANAGER
+			pd[port].dualrole_capability = CAP_DUALROLE;
+			charge_manager_update_dualrole(port);
+#endif
+		} else {
 			pd[port].flags &= ~PD_FLAGS_PARTNER_DR_POWER;
+#ifdef CONFIG_CHARGE_MANAGER
+			pd[port].dualrole_capability = CAP_DEDICATED;
+			charge_manager_update_dualrole(port);
+#endif
+		}
 
 		if (pdo & PDO_FIXED_DATA_SWAP)
 			pd[port].flags |= PD_FLAGS_PARTNER_DR_DATA;
@@ -1655,11 +1673,13 @@ int pd_get_polarity(int port)
 	return pd[port].polarity;
 }
 
-int pd_get_partner_dualrole_capable(int port)
+#ifdef CONFIG_CHARGE_MANAGER
+enum dualrole_capabilities pd_get_partner_dualrole_capable(int port)
 {
 	/* return dualrole status of port partner */
-	return pd[port].flags & PD_FLAGS_PARTNER_DR_POWER;
+	return pd[port].dualrole_capability;
 }
+#endif /* CONFIG_CHARGE_MANAGER */
 
 int pd_get_partner_data_swap_capable(int port)
 {
@@ -1791,6 +1811,7 @@ void pd_task(void)
 #ifdef CONFIG_CHARGE_MANAGER
 	/* Initialize PD supplier current limit to 0 */
 	pd_set_input_current_limit(port, 0, 0);
+	pd[port].dualrole_capability = CAP_UNKNOWN;
 #ifdef CONFIG_USB_PD_DUAL_ROLE
 	/* If sink, set initial type-C current limit based on vbus */
 	if (pd_snk_is_vbus_provided(port)) {
@@ -2673,6 +2694,21 @@ void pd_task(void)
 			hard_reset_count++;
 			if (pd[port].last_state != pd[port].task_state)
 				hard_reset_sent = 0;
+#ifdef CONFIG_CHARGE_MANAGER
+			if (pd[port].last_state == PD_STATE_SNK_DISCOVERY) {
+				/*
+				 * If discovery timed out, assume that we
+				 * have a dedicated charger attached. This
+				 * may not be a correct assumption according
+				 * to the specification, but it generally
+				 * works in practice and the harmful
+				 * effects of a wrong assumption here
+				 * are minimal.
+				 */
+				pd[port].dualrole_capability = CAP_DEDICATED;
+				charge_manager_update_dualrole(port);
+			}
+#endif
 
 			/* try sending hard reset until it succeeds */
 			if (!hard_reset_sent) {
diff --git a/include/charge_manager.h b/include/charge_manager.h
index 0b1de3186d..70afd2135e 100644
--- a/include/charge_manager.h
+++ b/include/charge_manager.h
@@ -22,9 +22,12 @@ struct charge_port_info {
 };
 
 /* Called by charging tasks to update their available charge */
-void charge_manager_update(int supplier,
-			   int charge_port,
-			   struct charge_port_info *charge);
+void charge_manager_update_charge(int supplier,
+				  int port,
+				  struct charge_port_info *charge);
+
+/* Called by charging tasks to indicate partner dualrole capability change */
+void charge_manager_update_dualrole(int port);
 
 /* Update charge ceiling for a given port */
 void charge_manager_set_ceil(int port, int ceil);
diff --git a/include/usb_pd.h b/include/usb_pd.h
index 410251775f..d177007239 100644
--- a/include/usb_pd.h
+++ b/include/usb_pd.h
@@ -1283,12 +1283,19 @@ int pd_is_connected(int port);
  */
 int pd_get_polarity(int port);
 
+/* Partner port dualrole capabilities */
+enum dualrole_capabilities {
+	CAP_UNKNOWN,
+	CAP_DUALROLE,
+	CAP_DEDICATED,
+};
+
 /**
  * Get port partner dual-role capable status
  *
  * @param port USB-C port number
  */
-int pd_get_partner_dualrole_capable(int port);
+enum dualrole_capabilities pd_get_partner_dualrole_capable(int port);
 
 /**
  * Get port partner data swap capable status
diff --git a/test/charge_manager.c b/test/charge_manager.c
index b0c9423632..977f516496 100644
--- a/test/charge_manager.c
+++ b/test/charge_manager.c
@@ -34,14 +34,9 @@ static unsigned int active_charge_limit = CHARGE_SUPPLIER_NONE;
 static unsigned int active_charge_port = CHARGE_PORT_NONE;
 static unsigned int charge_port_to_reject = CHARGE_PORT_NONE;
 static int new_power_request[PD_PORT_COUNT];
-static int dual_role_capable[PD_PORT_COUNT];
+static int dual_role_capable[PD_PORT_COUNT] = { CAP_DEDICATED, CAP_DEDICATED };
 static int power_role[PD_PORT_COUNT];
 
-enum {
-	DEDICATED_CHARGER = 0,
-	DUAL_ROLE_CHARGER = 1,
-};
-
 /* Callback functions called by CM on state change */
 void board_set_charge_limit(int charge_ma)
 {
@@ -85,12 +80,12 @@ static void clear_new_power_requests(void)
  * does not change dynamically, and thus won't trigger a charge manager
  * refresh, in test code and in production.
  */
-static void set_charger_role(int port, int role)
+static void set_charger_role(int port, enum dualrole_capabilities cap)
 {
-	dual_role_capable[port] = role;
+	dual_role_capable[port] = cap;
 }
 
-int pd_get_partner_dualrole_capable(int port)
+enum dualrole_capabilities pd_get_partner_dualrole_capable(int port)
 {
 	return dual_role_capable[port];
 }
@@ -130,10 +125,10 @@ static void initialize_charge_table(int current, int voltage, int ceil)
 
 	for (i = 0; i < PD_PORT_COUNT; ++i) {
 		charge_manager_set_ceil(i, ceil);
-		set_charger_role(i, DEDICATED_CHARGER);
+		set_charger_role(i, CAP_DEDICATED);
 		pd_set_role(i, PD_ROLE_SINK);
 		for (j = 0; j < CHARGE_SUPPLIER_COUNT; ++j)
-			charge_manager_update(j, i, &charge);
+			charge_manager_update_charge(j, i, &charge);
 	}
 	wait_for_charge_manager_refresh();
 }
@@ -157,7 +152,7 @@ static int test_initialization(void)
 			if (i == CHARGE_SUPPLIER_COUNT - 1 &&
 			    j == PD_PORT_COUNT - 1)
 				break;
-			charge_manager_update(i, j, &charge);
+			charge_manager_update_charge(i, j, &charge);
 		}
 
 	/* Verify no active charge port, since all pairs haven't updated */
@@ -165,9 +160,9 @@ static int test_initialization(void)
 	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
 
 	/* Update last pair and verify a charge port has been selected */
-	charge_manager_update(CHARGE_SUPPLIER_COUNT-1,
-			      PD_PORT_COUNT-1,
-			      &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_COUNT-1,
+				     PD_PORT_COUNT-1,
+				     &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port != CHARGE_PORT_NONE);
 
@@ -188,9 +183,9 @@ static int test_priority(void)
 	 */
 	charge.current = 2000;
 	charge.voltage = 5000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 0, &charge);
 	charge.current = 1000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 1000);
@@ -200,7 +195,7 @@ static int test_priority(void)
 	 * lower charge.
 	 */
 	charge.current = 1500;
-	charge_manager_update(CHARGE_SUPPLIER_TEST7, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST7, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 1000);
@@ -210,20 +205,20 @@ static int test_priority(void)
 	 * which happens to be a different port.
 	 */
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 2000);
 
 	/* Add a charge at equal priority and verify highest charge selected */
 	charge.current = 2500;
-	charge_manager_update(CHARGE_SUPPLIER_TEST5, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST5, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 2500);
 
 	charge.current = 3000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 3000);
@@ -234,7 +229,7 @@ static int test_priority(void)
 	 * selected as the tiebreaker.
 	 */
 	charge.current = 3000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 3000);
@@ -268,9 +263,9 @@ static int test_charge_ceil(void)
 	/* Verify that ceiling is ignored in determining active charge port */
 	charge.current = 2000;
 	charge.voltage = 5000;
-	charge_manager_update(0, 0, &charge);
+	charge_manager_update_charge(0, 0, &charge);
 	charge.current = 2500;
-	charge_manager_update(0, 1, &charge);
+	charge_manager_update_charge(0, 1, &charge);
 	charge_manager_set_ceil(1, 750);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
@@ -294,7 +289,7 @@ static int test_new_power_request(void)
 	/* Charge from port 1 and verify NPR on port 1 only */
 	charge.current = 1000;
 	charge.voltage = 5000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(new_power_request[0] == 0);
 	TEST_ASSERT(new_power_request[1] == 1);
@@ -309,14 +304,14 @@ static int test_new_power_request(void)
 
 	/* Change port 1 voltage and verify NPR on port 1 */
 	charge.voltage = 4000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(new_power_request[0] == 0);
 	TEST_ASSERT(new_power_request[1] == 1);
 	clear_new_power_requests();
 
 	/* Add low-priority source and verify no NPRs */
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(new_power_request[0] == 0);
 	TEST_ASSERT(new_power_request[1] == 0);
@@ -326,7 +321,7 @@ static int test_new_power_request(void)
 	 * Add higher-priority source and verify NPR on both ports,
 	 * since we're switching charge ports.
 	 */
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(new_power_request[0] == 1);
 	TEST_ASSERT(new_power_request[1] == 1);
@@ -348,8 +343,8 @@ static int test_override(void)
 	 */
 	charge.current = 500;
 	charge.voltage = 5000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 500);
@@ -369,14 +364,14 @@ static int test_override(void)
 	 * is again selected.
 	 */
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	charge_manager_set_override(0);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 
 	/* Set non-zero charge on port 0 and verify override was auto-removed */
 	charge.current = 250;
-	charge_manager_update(CHARGE_SUPPLIER_TEST5, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST5, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 
@@ -385,13 +380,13 @@ static int test_override(void)
 	 * priority on the override port.
 	 */
 	charge.current = 300;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	charge_manager_set_override(0);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 300);
 	charge.current = 100;
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 0, &charge);
 	charge_manager_set_override(0);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
@@ -403,12 +398,12 @@ static int test_override(void)
 	 * is successful.
 	 */
 	charge_manager_set_override(OVERRIDE_DONT_CHARGE);
-	set_charger_role(0, DUAL_ROLE_CHARGER);
+	set_charger_role(0, CAP_DUALROLE);
 	pd_set_role(0, PD_ROLE_SOURCE);
 	charge_manager_set_override(0);
 	wait_for_charge_manager_refresh();
 	charge.current = 200;
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 200);
@@ -423,7 +418,7 @@ static int test_override(void)
 
 	/* Update a charge supplier, verify that we still aren't charging */
 	charge.current = 200;
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
 	TEST_ASSERT(active_charge_limit == 0);
@@ -450,10 +445,10 @@ static int test_dual_role(void)
 	 * Mark P0 as dual-role and set a charge. Verify that we don't charge
 	 * from the port.
 	 */
-	set_charger_role(0, DUAL_ROLE_CHARGER);
+	set_charger_role(0, CAP_DUALROLE);
 	charge.current = 500;
 	charge.voltage = 5000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
 	TEST_ASSERT(active_charge_limit == 0);
@@ -475,7 +470,7 @@ static int test_dual_role(void)
 	/* Mark P0 as the override port, verify that we again charge. */
 	charge_manager_set_override(0);
 	charge.current = 550;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 550);
@@ -485,10 +480,10 @@ static int test_dual_role(void)
 	 * Insert a dual-role charger into P1 and set the override. Verify
 	 * that the override correctly changes.
 	 */
-	set_charger_role(1, DUAL_ROLE_CHARGER);
+	set_charger_role(1, CAP_DUALROLE);
 	charge_manager_set_override(1);
 	charge.current = 500;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 500);
@@ -498,7 +493,7 @@ static int test_dual_role(void)
 	/* Set override back to P0 and verify switch */
 	charge_manager_set_override(0);
 	charge.current = 600;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 600);
@@ -507,11 +502,11 @@ static int test_dual_role(void)
 
 	/* Insert a dedicated charger and verify override is removed */
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 1, &charge);
 	wait_for_charge_manager_refresh();
-	set_charger_role(1, DEDICATED_CHARGER);
+	set_charger_role(1, CAP_DEDICATED);
 	charge.current = 400;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 400);
@@ -521,11 +516,11 @@ static int test_dual_role(void)
 	 * Verify the port is handled normally if the dual-role source is
 	 * unplugged and replaced with a dedicated source.
 	 */
-	set_charger_role(0, DEDICATED_CHARGER);
+	set_charger_role(0, CAP_DEDICATED);
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	charge.current = 500;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 500);
@@ -534,13 +529,13 @@ static int test_dual_role(void)
 	 * Verify that we charge from the dedicated port if a dual-role
 	 * source is also attached.
 	 */
-	set_charger_role(0, DUAL_ROLE_CHARGER);
+	set_charger_role(0, CAP_DUALROLE);
 	charge.current = 0;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	charge.current = 500;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	charge.current = 200;
-	charge_manager_update(CHARGE_SUPPLIER_TEST6, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST6, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 200);
@@ -560,7 +555,7 @@ static int test_rejected_port(void)
 	/* Set a charge on P0. */
 	charge.current = 500;
 	charge.voltage = 5000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 500);
@@ -568,14 +563,14 @@ static int test_rejected_port(void)
 	/* Set P0 as rejected, and verify that it doesn't become active. */
 	set_charge_port_to_reject(1);
 	charge.current = 1000;
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 0);
 	TEST_ASSERT(active_charge_limit == 500);
 
 	/* Don't reject P0, and verify it can become active. */
 	set_charge_port_to_reject(CHARGE_PORT_NONE);
-	charge_manager_update(CHARGE_SUPPLIER_TEST1, 1, &charge);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST1, 1, &charge);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(active_charge_port == 1);
 	TEST_ASSERT(active_charge_limit == 1000);
@@ -583,6 +578,93 @@ static int test_rejected_port(void)
 	return EC_SUCCESS;
 }
 
+static int test_unknown_dualrole_capability(void)
+{
+	struct charge_port_info charge;
+
+	/* Initialize table to no charge. */
+	initialize_charge_table(0, 5000, 1000);
+	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
+
+	/*
+	 * Set a charge on P0 with unknown dualrole capability,
+	 * verify that we don't charge from the port.
+	 */
+	charge.current = 500;
+	charge.voltage = 5000;
+	set_charger_role(0, CAP_UNKNOWN);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
+
+	/* Toggle to dedicated and verify port becomes active. */
+	set_charger_role(0, CAP_DEDICATED);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 0);
+
+	/* Add dualrole charger in port 1 */
+	charge.current = 1000;
+	set_charger_role(1, CAP_DUALROLE);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 1, &charge);
+	charge_manager_update_dualrole(1);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 0);
+
+	/* Remove charger on port 0 */
+	charge.current = 0;
+	set_charger_role(0, CAP_UNKNOWN);
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == CHARGE_PORT_NONE);
+
+	/* Set override to charge on port 1 */
+	charge_manager_set_override(1);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 1);
+
+	/*
+	 * Toggle port 0 to dedicated, verify that override is still kept
+	 * because there's no charge on the port.
+	 */
+	set_charger_role(0, CAP_DEDICATED);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 1);
+
+	/*
+	 * Insert UNKNOWN capability charger on port 0, verify that override
+	 * is still kept.
+	 */
+	set_charger_role(0, CAP_UNKNOWN);
+	charge.current = 2000;
+	charge_manager_update_charge(CHARGE_SUPPLIER_TEST2, 0, &charge);
+	wait_for_charge_manager_refresh();
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 1);
+
+	/* Toggle to dualrole, verify that override is still kept. */
+	set_charger_role(0, CAP_DUALROLE);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 1);
+
+	/* Toggle to dedicated, verify that override is removed. */
+	set_charger_role(0, CAP_UNKNOWN);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 1);
+	set_charger_role(0, CAP_DEDICATED);
+	charge_manager_update_dualrole(0);
+	wait_for_charge_manager_refresh();
+	TEST_ASSERT(active_charge_port == 0);
+
+	return EC_SUCCESS;
+}
+
 void run_test(void)
 {
 	test_reset();
@@ -594,6 +676,7 @@ void run_test(void)
 	RUN_TEST(test_override);
 	RUN_TEST(test_dual_role);
 	RUN_TEST(test_rejected_port);
+	RUN_TEST(test_unknown_dualrole_capability);
 
 	test_print_result();
 }