bmi160: Keep timestamp and FIFO synchronized

The BMI160 will clear the FIFO watermark/full interrupt when the FIFO
crosses the threshold condition - which happens upon reading from it
in load_fifo(). As a result of this, and the fact that the soft IRQ
handler for the BMI160 tries to loop until it has finished serving all
interrupt causes, there are possibilities for a race condition such as
the following to occur:

read timestamp, value is t0
read interrupt cause: it's a watermark interrupt
load fifo(t0): this clears the interrupt, allowing a new one to happen
read timestamp, value is still t0
a watermark interrupt happens at t1
read interrupt cause: watermark
load fifo(t0)

At this point, the fifo will process events at t1 as-if they had happened
at t0, which is bad and causes incorrect timestamps on the AP side.

This changes moves reading the timestamp value as close as possible
to load_fifo, and in a relative order where a new watermark interrupt
cannot happen, which ensures the timestamp and the FIFO data for it
are synchronized

BUG=b:120508077
TEST=on Bobba and Nocturne, observe that no events happen
     where an identical "a" time matches two distinct (b,c) times
BRANCH=nocturne,bobba

Change-Id: I454b257366bccf2b9e4d78df5dc005a8ad7313a0
Signed-off-by: Enrico Granata <egranata@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1367013
Reviewed-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-by: Diana Z <dzigterman@chromium.org>
(cherry picked from commit e5fc358df19e1c4147d35749b6ddf91e22cebbf2)
Reviewed-on: https://chromium-review.googlesource.com/c/1380711
Reviewed-by: Gwendal Grignou <gwendal@google.com>
Reviewed-by: Philip Chen <philipchen@chromium.org>

1 files changed, 8 insertions, 9 deletions

diff --git a/driver/accelgyro_bmi160.c b/driver/accelgyro_bmi160.c
index 0913ba4731..b2c4a630dc 100644
--- a/driver/accelgyro_bmi160.c
+++ b/driver/accelgyro_bmi160.c
@@ -1095,20 +1095,19 @@ static void irq_set_orientation(struct motion_sensor_t *s,
  */
 static int irq_handler(struct motion_sensor_t *s, uint32_t *event)
 {
-	uint32_t interrupt, last_ts;
+	uint32_t interrupt;
+	int rv;
 
 	if ((s->type != MOTIONSENSE_TYPE_ACCEL) ||
 			(!(*event & CONFIG_ACCELGYRO_BMI160_INT_EVENT)))
 		return EC_ERROR_NOT_HANDLED;
 
 	do {
-		/*
-		 * Collect timestamp before resetting the interrupt line:
-		 * After reading, it is possible for the timestamp to change,
-		 * before we can process the FIFO.
-		 */
-		last_ts = last_interrupt_timestamp;
-		raw_read32(s->port, s->addr, BMI160_INT_STATUS_0, &interrupt);
+		rv = raw_read32(s->port, s->addr, BMI160_INT_STATUS_0,
+				&interrupt);
+		/* Bail out of this loop if the sensor isn't powered. */
+		if (rv == EC_ERROR_NOT_POWERED)
+			return rv;
 
 #ifdef CONFIG_GESTURE_SENSOR_BATTERY_TAP
 		if (interrupt & BMI160_D_TAP_INT)
@@ -1120,7 +1119,7 @@ static int irq_handler(struct motion_sensor_t *s, uint32_t *event)
 #endif
 #ifdef CONFIG_ACCEL_FIFO
 		if (interrupt & (BMI160_FWM_INT | BMI160_FFULL_INT))
-			load_fifo(s, last_ts);
+			load_fifo(s, last_interrupt_timestamp);
 #endif
 #ifdef CONFIG_BMI160_ORIENTATION_SENSOR
 		irq_set_orientation(s, interrupt);