pd: if our request is rejected, go to SNK_READY

If our request is rejected, go to SNK_READY, but don't set
explicit contract flag.

This also changes charge manager slightly to avoid new power
request loops. A new power request is only requested if the
charge port changes, or if the active charge port changes its
voltage/current offering. A new power request does not occur
if the current ceiling changes, since the existing contract
still suffices.

BUG=chrome-os-partner:33692, chrome-os-partner:28332
BRANCH=samus
TEST=make buildall. use samus and make sure we negotiate for 20V
as normal. modify zinger to send a REJECT and make sure we go from
PD_STATE_SNK_REQUESTED to PD_STATE_SNK_READY and explicit contract
bit is 0.

Signed-off-by: Alec Berg <alecaberg@chromium.org>
Change-Id: Iec02663364dcdc4aa66c681ec08911db7424abbc
Reviewed-on: https://chromium-review.googlesource.com/230522
Reviewed-by: Shawn Nematbakhsh <shawnn@chromium.org>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

3 files changed, 78 insertions, 74 deletions

diff --git a/common/charge_manager.c b/common/charge_manager.c
index ba0afd5958..d8990e56da 100644
--- a/common/charge_manager.c
+++ b/common/charge_manager.c
@@ -26,6 +26,8 @@ static int charge_ceil[PD_PORT_COUNT];
 /* Store current state of port enable / charge current. */
 static int charge_port = CHARGE_PORT_NONE;
 static int charge_current = CHARGE_CURRENT_UNINITIALIZED;
+static int charge_current_uncapped = CHARGE_CURRENT_UNINITIALIZED;
+static int charge_voltage;
 static int charge_supplier = CHARGE_SUPPLIER_NONE;
 static int override_port = OVERRIDE_OFF;
 
@@ -82,7 +84,10 @@ static void charge_manager_refresh(void)
 {
 	int new_supplier = CHARGE_SUPPLIER_NONE;
 	int new_port = CHARGE_PORT_NONE;
-	int new_charge_current, new_charge_voltage, i, j, old_port;
+	int new_charge_current, new_charge_current_uncapped;
+	int new_charge_voltage, i, j;
+	int updated_new_port = CHARGE_PORT_NONE;
+	int updated_old_port = CHARGE_PORT_NONE;
 
 	/* Skip port selection on OVERRIDE_DONT_CHARGE. */
 	if (override_port != OVERRIDE_DONT_CHARGE) {
@@ -135,15 +140,19 @@ static void charge_manager_refresh(void)
 			override_port = OVERRIDE_OFF;
 	}
 
-	if (new_supplier == CHARGE_SUPPLIER_NONE)
-		new_charge_current = new_charge_voltage = 0;
-	else {
-		new_charge_current =
+	if (new_supplier == CHARGE_SUPPLIER_NONE) {
+		new_charge_current = 0;
+		new_charge_current_uncapped = 0;
+		new_charge_voltage = 0;
+	} else {
+		new_charge_current_uncapped =
 			available_charge[new_supplier][new_port].current;
 		/* Enforce port charge ceiling. */
-		if (charge_ceil[new_port] != CHARGE_CEIL_NONE &&
-		    charge_ceil[new_port] < new_charge_current)
-			new_charge_current = charge_ceil[new_port];
+		if (charge_ceil[new_port] != CHARGE_CEIL_NONE)
+			new_charge_current = MIN(charge_ceil[new_port],
+						 new_charge_current_uncapped);
+		else
+			new_charge_current = new_charge_current_uncapped;
 
 		new_charge_voltage =
 			available_charge[new_supplier][new_port].voltage;
@@ -156,17 +165,35 @@ static void charge_manager_refresh(void)
 			new_charge_current, new_charge_voltage);
 		board_set_charge_limit(new_charge_current);
 		board_set_active_charge_port(new_port);
-
-		charge_current = new_charge_current;
-		charge_supplier = new_supplier;
-		old_port = charge_port;
-		charge_port = new_port;
-
-		if (new_port != CHARGE_PORT_NONE)
-			pd_set_new_power_request(new_port);
-		if (old_port != CHARGE_PORT_NONE)
-			pd_set_new_power_request(old_port);
 	}
+
+	/*
+	 * Signal new power request only if the port changed, the voltage
+	 * on the same port changed, or the actual uncapped current
+	 * on the same port changed (don't consider ceil).
+	 */
+	if (new_port != CHARGE_PORT_NONE &&
+	    (new_port != charge_port ||
+	     new_charge_current_uncapped != charge_current_uncapped ||
+	     new_charge_voltage != charge_voltage))
+		updated_new_port = new_port;
+
+	/* Signal new power request on old port if we're switching away. */
+	if (charge_port != new_port && charge_port != CHARGE_PORT_NONE)
+		updated_old_port = charge_port;
+
+	/* Update globals to reflect current state. */
+	charge_current = new_charge_current;
+	charge_current_uncapped = new_charge_current_uncapped;
+	charge_voltage = new_charge_voltage;
+	charge_supplier = new_supplier;
+	charge_port = new_port;
+
+	/* New power requests must be set only after updating the globals. */
+	if (updated_new_port != CHARGE_PORT_NONE)
+		pd_set_new_power_request(updated_new_port);
+	if (updated_old_port != CHARGE_PORT_NONE)
+		pd_set_new_power_request(updated_old_port);
 }
 DECLARE_DEFERRED(charge_manager_refresh);
 
diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c
index e32ea6ea5c..aecb9ee4e3 100644
--- a/common/usb_pd_protocol.c
+++ b/common/usb_pd_protocol.c
@@ -273,10 +273,6 @@ static struct pd_protocol {
 	uint32_t supply_voltage;
 	/* Signal charging update that affects the port */
 	int new_power_request;
-#ifdef CONFIG_CHARGE_MANAGER
-	/* Track last requested charge type (min / max) */
-	enum pd_request_types last_charge_request;
-#endif
 #endif
 
 	/* PD state for Vendor Defined Messages */
@@ -773,54 +769,28 @@ static void pd_send_request_msg(int port, enum pd_request_types request)
 	/* we were waiting for them, let's process them */
 	res = pd_choose_voltage(pd_src_cap_cnt[port], pd_src_caps[port], &rdo,
 				&curr_limit, &supply_voltage);
-	if (res == EC_SUCCESS) {
-#ifdef CONFIG_CHARGE_MANAGER
-		/* Set max. limit, but apply 500mA ceiling */
-		charge_manager_set_ceil(port, PD_MIN_MA);
-		pd_set_input_current_limit(port,
-					   curr_limit,
-					   supply_voltage);
-		/* Negotiate for Vsafe5V, if requested */
-		if (request == PD_REQUEST_MIN) {
-			res = pd_choose_voltage_min(pd_src_cap_cnt[port],
-						    pd_src_caps[port],
-						    &rdo,
-						    &curr_limit,
-						    &supply_voltage);
-			if (res != EC_SUCCESS)
-				/*
-				 * Successfully requested max. voltage mode,
-				 * but unable to request min. for some reason.
-				 * We have no choice but to negotiate for max.
-				 * Update last_charge_request accordingly.
-				 */
-				request = PD_REQUEST_MAX;
-		}
+	if (res != EC_SUCCESS)
 		/*
-		 * The request message may be later rejected, in which case
-		 * last_charge_request may not reflect reality.
-		 * TODO(shawnn): Handle last_charge_request correctly for this
-		 * case. crosbug.com/p/33692.
+		 * If fail to choose voltage, do nothing, let source re-send
+		 * source cap
 		 */
-		pd[port].last_charge_request = request;
+		return;
+
+#ifdef CONFIG_CHARGE_MANAGER
+	/* Set max. limit, but apply 500mA ceiling */
+	charge_manager_set_ceil(port, PD_MIN_MA);
+	pd_set_input_current_limit(port, curr_limit, supply_voltage);
+	/* Negotiate for Vsafe5V, if requested */
+	if (request == PD_REQUEST_MIN)
+		pd_choose_voltage_min(pd_src_cap_cnt[port], pd_src_caps[port],
+				      &rdo, &curr_limit, &supply_voltage);
 #endif
-		pd[port].curr_limit = curr_limit;
-		pd[port].supply_voltage = supply_voltage;
-		res = send_request(port, rdo);
-		if (res >= 0)
-			set_state(port, PD_STATE_SNK_REQUESTED);
-		else
-			/*
-			 * for now: ignore failure here,
-			 * we will retry ...
-			 * TODO(crosbug.com/p/28332)
-			 */
-			set_state(port, PD_STATE_SNK_REQUESTED);
-	}
-	/*
-	 * TODO(crosbug.com/p/28332): if pd_choose_voltage
-	 * returns an error, ignore failure for now.
-	 */
+	pd[port].curr_limit = curr_limit;
+	pd[port].supply_voltage = supply_voltage;
+	res = send_request(port, rdo);
+	if (res >= 0)
+		set_state(port, PD_STATE_SNK_REQUESTED);
+	/* If fail send request, do nothing, let source re-send source cap */
 }
 #endif
 
@@ -987,8 +957,9 @@ static void handle_ctrl_request(int port, uint16_t head,
 			set_state(port, PD_STATE_SRC_READY);
 		else if (pd[port].task_state == PD_STATE_SNK_SWAP_INIT)
 			set_state(port, PD_STATE_SNK_READY);
-		else
-			set_state(port, PD_STATE_SNK_DISCOVERY);
+		else if (pd[port].task_state == PD_STATE_SNK_REQUESTED)
+			/* no explicit contract */
+			set_state(port, PD_STATE_SNK_READY);
 #endif
 		break;
 	case PD_CTRL_ACCEPT:
@@ -1939,13 +1910,11 @@ void pd_task(void)
 				pd[port].new_power_request = 0;
 #ifdef CONFIG_CHARGE_MANAGER
 				if (charge_manager_get_active_charge_port()
-				    != port && pd[port].last_charge_request
-				    == PD_REQUEST_MAX)
+				    != port)
 					pd_send_request_msg(port,
 							    PD_REQUEST_MIN);
 				else if (charge_manager_get_active_charge_port()
-					 == port && pd[port].last_charge_request
-					 == PD_REQUEST_MIN)
+					 == port)
 #endif
 					pd_send_request_msg(port,
 							    PD_REQUEST_MAX);
diff --git a/test/charge_manager.c b/test/charge_manager.c
index 6c05d8e0b6..78fbd7bfae 100644
--- a/test/charge_manager.c
+++ b/test/charge_manager.c
@@ -257,10 +257,18 @@ static int test_new_power_request(void)
 	TEST_ASSERT(new_power_request[1] == 1);
 	clear_new_power_requests();
 
-	/* Reduce port 1 limit and verify NPR on port 1 only */
+	/* Reduce port 1 through ceil and verify no NPR */
 	charge_manager_set_ceil(1, 500);
 	wait_for_charge_manager_refresh();
 	TEST_ASSERT(new_power_request[0] == 0);
+	TEST_ASSERT(new_power_request[1] == 0);
+	clear_new_power_requests();
+
+	/* Change port 1 voltage and verify NPR on port 1 */
+	charge.voltage = 4000;
+	charge_manager_update(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();