diff options
author | Caveh Jalali <caveh@google.com> | 2017-06-20 17:44:55 -0700 |
---|---|---|
committer | chrome-bot <chrome-bot@chromium.org> | 2017-06-28 19:54:26 -0700 |
commit | 7771c52368e03bb63fd49eb303b3152b557b897b (patch) | |
tree | c8ac0f29274dcdeb59eb3d4ad59293dd3ee2041a | |
parent | e85d3e982597a9bfc20bb7ef820ef93db1927de1 (diff) | |
download | chrome-ec-7771c52368e03bb63fd49eb303b3152b557b897b.tar.gz |
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
Change-Id: I363803ff60db31ccf84d592f8c9d1610fbe0f9ce
Signed-off-by: Caveh Jalali <caveh@google.com>
Reviewed-on: https://chromium-review.googlesource.com/544659
Reviewed-by: Shawn N <shawnn@chromium.org>
-rw-r--r-- | common/usb_pd_protocol.c | 44 |
1 files changed, 35 insertions, 9 deletions
diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c index ee4ae31ab0..d3cfa0da73 100644 --- a/common/usb_pd_protocol.c +++ b/common/usb_pd_protocol.c @@ -118,6 +118,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; /* Timeout for the current state. Set to 0 for no timeout. */ @@ -1803,6 +1805,10 @@ void pd_task(void) 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; @@ -2299,6 +2305,7 @@ void pd_task(void) break; case PD_STATE_SUSPENDED: 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); @@ -3108,19 +3115,38 @@ DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, dual_role_force_sink, 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); + } } #if defined(CONFIG_CMD_PD) && defined(CONFIG_CMD_PD_FLASH) |