pd_suspend: coordinate with pd_task().

looks like we had a bit of a race condition: set_state() was
effectively just an assignment opration to pd[port].task_state.  it's
called asynchronously from pd_set_suspend() in response to a
PD_SUSPEND message from the AP as well as from pd_task() before it
enters its main event loop.  this can take a long time because
tcpci_tcpm_init() has a 300ms timeout.  last one wins.

similarly, when pd_task() is running its main loop, pd_set_suspend()
really needs to wait for pd_task() to actually enter the
PD_STATE_SUSPENDED state before the caller can assume that the
pd_task() has stopped accessing the TCPC.

the particular failure case was when depthcharge would decide to do a
TCPC firmware update.  it starts by sending a PD_SUSPEND to the EC,
then accessing the TCPC.  unfortunately, the pd_task() hadn't gotten
out of the way yet, thus causing TCPC access chaos.

so, i'm adding a req_suspend_state flag to the pd_protocol struct so
we can tell pd_task() to suspend itself in a controlled manner.  when
pd_task() is ready to do a state change - basically at the top of the
main event loop -  it'll change to PD_STATE_SUSPENDED and clear the
req_suspend_state flag.

in any case, pd_set_suspend() still needs to wait around for pd_task()
to enter the suspended state as we don't have a fancy handshake
mechanism between these tasks.

TEST=in combination with some follow-on CLs, ps8751 firmware update
        works properly where previously it needed a ~2 second delay
        for the EC pd_task() to settle.  the way to trigger the
        failure was to insert or remove the power brick.

BRANCH=none

BUG=b:62356808

Signed-off-by: Duncan Laurie <dlaurie@google.com>
Original-Commit-Id: 7771c52368e03bb63fd49eb303b3152b557b897b
Original-Change-Id: I363803ff60db31ccf84d592f8c9d1610fbe0f9ce
Original-Signed-off-by: Caveh Jalali <caveh@google.com>
Original-Reviewed-on: https://chromium-review.googlesource.com/544659
Original-Reviewed-by: Shawn N <shawnn@chromium.org>
Original-(cherry picked from commit 7771c52368e03bb63fd49eb303b3152b557b897b)

Change-Id: I023400c6a9a83c3f9a1a883be2832abb7ee15d60
Reviewed-on: https://chromium-review.googlesource.com/1038049
Reviewed-by: Duncan Laurie <dlaurie@google.com>
Commit-Queue: Duncan Laurie <dlaurie@google.com>
Tested-by: Duncan Laurie <dlaurie@google.com>
Trybot-Ready: Duncan Laurie <dlaurie@google.com>

1 files changed, 35 insertions, 9 deletions

diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c
index 59875b1a9e..48c682d7e5 100644
--- a/common/usb_pd_protocol.c
+++ b/common/usb_pd_protocol.c
@@ -144,6 +144,8 @@ static struct pd_protocol {
 	enum pd_states task_state;
 	/* PD state when we run state handler the last time */
 	enum pd_states last_state;
+	/* bool: request state change to SUSPENDED */
+	uint8_t req_suspend_state;
 	/* The state to go to after timeout */
 	enum pd_states timeout_state;
 	/* port flags, see PD_FLAGS_* */
@@ -2243,6 +2245,10 @@ void pd_task(void *u)
 			if (!tcpm_get_message(port, payload, &head))
 				handle_request(port, head, payload);
 		}
+
+		if (pd[port].req_suspend_state)
+			set_state(port, PD_STATE_SUSPENDED);
+
 		/* if nothing to do, verify the state of the world in 500ms */
 		this_state = pd[port].task_state;
 		timeout = 500*MSEC;
@@ -2702,6 +2708,7 @@ void pd_task(void *u)
 			int rstatus;
 #endif
 			CPRINTS("TCPC p%d suspended!", port);
+			pd[port].req_suspend_state = 0;
 #ifdef CONFIG_USB_PD_TCPC
 			pd_rx_disable_monitoring(port);
 			pd_hw_release(port);
@@ -3501,19 +3508,38 @@ DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, pd_chipset_shutdown, HOOK_PRIO_DEFAULT);
 #endif /* CONFIG_USB_PD_DUAL_ROLE */
 
 #ifdef CONFIG_COMMON_RUNTIME
+
+/*
+ * (enable=1) request pd_task transition to the suspended state.  hang
+ * around for a while until we observe the state change.  this can
+ * take a while (like 300ms) on startup when pd_task is sleeping in
+ * tcpci_tcpm_init.
+ *
+ * (enable=0) force pd_task out of the suspended state and into the
+ * port's default state.
+ */
+
 void pd_set_suspend(int port, int enable)
 {
-	int tries = 3;
+	int tries = 300;
 
-	do {
-		set_state(port, enable ? PD_STATE_SUSPENDED :
-			  PD_DEFAULT_STATE(port));
+	if (enable) {
+		pd[port].req_suspend_state = 1;
+		do {
+			task_wake(PD_PORT_TO_TASK_ID(port));
+			if (pd[port].task_state == PD_STATE_SUSPENDED)
+				break;
+			msleep(1);
+		} while (--tries != 0);
+		if (!tries)
+			CPRINTS("TCPC p%d set_suspend failed!", port);
+	} else {
+		if (pd[port].task_state != PD_STATE_SUSPENDED)
+			CPRINTS("TCPC p%d suspend disable request "
+				"while not suspended!", port);
+		set_state(port, PD_DEFAULT_STATE(port));
 		task_wake(PD_PORT_TO_TASK_ID(port));
-	} while (enable && pd[port].task_state != PD_STATE_SUSPENDED
-			&& --tries);
-
-	if (!tries)
-		CPRINTS("TCPC p%d set_suspend failed!", port);
+	}
 }
 
 int pd_is_port_enabled(int port)