From ef71b1a3e56efb86b888edc6c04240f3eb970bd2 Mon Sep 17 00:00:00 2001 From: Bill Richardson Date: Fri, 21 Sep 2012 14:23:09 -0700 Subject: [PATCH] Prevent I2C interrupts from consuming pending task events This manifested as the lightbar task missing transitions between CPU states. The underlying cause was that when a task talks over the I2C bus, the I2C communication was using the task scheduler to wait for an interrupt to signal completed I2C traffic without blocking the other threads, but while doing so it was not preserving pending events. This CL seems to fix it. BUG=chrome-os-partner:12431 BRANCH=all TEST=manual The original bug is tricky to reproduce without adding some delay to the I2C task code, but you can do it. Boot the CPU, then from the EC console repeatedly alternate these two commands: lightbar seq s0 lightbar seq s3 You should see the lightbar pattern turn off and on, but occasionally you'll type the command and the EC won't change the pattern. With this change applied, it should *always* work. Change-Id: Ie6819a4a36162a8760455c71c41ab8a468656af1 Signed-off-by: Bill Richardson Reviewed-on: https://gerrit.chromium.org/gerrit/33805 Reviewed-by: Randall Spangler --- chip/lm4/i2c.c | 26 +++++++++++++++++++++----- include/task.h | 3 ++- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/chip/lm4/i2c.c b/chip/lm4/i2c.c index 996cdd2b49..94ae391712 100644 --- a/chip/lm4/i2c.c +++ b/chip/lm4/i2c.c @@ -41,22 +41,38 @@ extern const struct i2c_port_t i2c_ports[I2C_PORTS_USED]; static int wait_idle(int port) { int i; - int event; + int event = 0; i = LM4_I2C_MCS(port); while (i & 0x01) { /* Port is busy, so wait for the interrupt */ task_waiting_on_port[port] = task_get_current(); LM4_I2C_MIMR(port) = 0x03; - event = task_wait_event(1000000); + /* We want to wait here quietly until the I2C interrupt comes + * along, but we don't want to lose any pending events that + * will be needed by the task that started the I2C transaction + * in the first place. So we save them up and restore them when + * the I2C is either completed or timed out. Refer to the + * implementation of usleep() for a similar situation. + */ + event |= (task_wait_event(1000000) & ~TASK_EVENT_I2C_IDLE); LM4_I2C_MIMR(port) = 0x00; task_waiting_on_port[port] = TASK_ID_INVALID; - if (event == TASK_EVENT_TIMER) + if (event & TASK_EVENT_TIMER) { + /* Restore any events that we saw while waiting */ + task_set_event(task_get_current(), + (event & ~TASK_EVENT_TIMER), 0); return EC_ERROR_TIMEOUT; + } i = LM4_I2C_MCS(port); } + /* Restore any events that we saw while waiting. TASK_EVENT_TIMER isn't + * one, because we've handled it above. + */ + task_set_event(task_get_current(), event, 0); + /* Check for errors */ if (i & 0x02) return EC_ERROR_UNKNOWN; @@ -308,9 +324,9 @@ static void handle_interrupt(int port) /* Clear the interrupt status */ LM4_I2C_MICR(port) = LM4_I2C_MMIS(port); - /* Wake up the task which was waiting on the interrupt, if any */ + /* Wake up the task which was waiting on the I2C interrupt, if any. */ if (id != TASK_ID_INVALID) - task_wake(id); + task_set_event(id, TASK_EVENT_I2C_IDLE, 0); } diff --git a/include/task.h b/include/task.h index 2362d077c8..c6b8b4591d 100644 --- a/include/task.h +++ b/include/task.h @@ -13,7 +13,8 @@ #include "task_id.h" /* Task event bitmasks */ -#define TASK_EVENT_CUSTOM(x) (x & 0x1fffffff) +#define TASK_EVENT_CUSTOM(x) (x & 0x0fffffff) +#define TASK_EVENT_I2C_IDLE (1 << 28) /* I2C interrupt handler event. */ #define TASK_EVENT_WAKE (1 << 29) /* task_wake() called on task */ #define TASK_EVENT_MUTEX (1 << 30) /* Mutex unlocking */ #define TASK_EVENT_TIMER (1 << 31) /* Timer expired. For example,