rdd: use rdd_interrupt for rdd_connect/disconnect

Occasionally cc1 and cc2 might be in a valid debug range for a short
time even if a debug accesssory isn't connected. The rdd hardware will
debounce the signals long enough to make sure they're stable before
triggering the Rdd interrupt. This change moves all rdd
connect/disconnect logic to the interrupt instead of sampling the
voltages in detect_rdd. In detect_rdd the samples of the cc lines
falsely triggered rdd connect whenever the cc voltages happened to be in
the valid debug range. We weren't waiting for Rdd to declare the voltage
stable before running rdd connect. Moving the logic from rdd_detect to
rdd_interrupt will ensure the signals are actually stable before we
change the rdd state.

BUG=b:113064204
BRANCH=cr50
TEST=manual
	connect suzyq
	reboot cr50
	verify rdd is connected

	disconnect suzyq
	verify rdd is disconnected

	boot nami
	leave it up for a while
	make sure there aren't any false connects

	reconnect suzyq
	verify rdd is connected

	disconnect suzyq
	verify rdd is disconnected

	reboot cr50
	verify rdd is disconnected

	run firmware_Cr50CCDServoCap and firmware_Cr50DeviceState

Change-Id: I3eeaa39f79d2ccc3e997da291d8aae1f12103dc8
Signed-off-by: Mary Ruthven <mruthven@google.com>
Reviewed-on: https://chromium-review.googlesource.com/1185967
Tested-by: Mary Ruthven <mruthven@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Mary Ruthven <mruthven@chromium.org>

2 files changed, 26 insertions, 46 deletions

diff --git a/chip/g/rdd.c b/chip/g/rdd.c
index 9febad81c6..9460ad35e6 100644
--- a/chip/g/rdd.c
+++ b/chip/g/rdd.c
@@ -36,17 +36,20 @@ static enum device_state state = DEVICE_STATE_DISCONNECTED;
 /* Force detecting a debug accessory (ignore RDD CC detect hardware) */
 static int force_detected;
 
+/*
+ * The Rdd state. This is saved in the rdd_interrupt to make sure the state is
+ * stable.
+ */
+static uint8_t rdd_is_detected_shadow;
+
 /**
  * Get instantaneous cable detect state
  *
  * @return 1 if debug accessory is detected, 0 if not detected
  */
-int rdd_is_detected(void)
+uint8_t rdd_is_detected(void)
 {
-	uint8_t cc1 = GREAD_FIELD(RDD, INPUT_PIN_VALUES, CC1);
-	uint8_t cc2 = GREAD_FIELD(RDD, INPUT_PIN_VALUES, CC2);
-
-	return (cc1 == cc2 && (cc1 == 3 || cc1 == 1));
+	return rdd_is_detected_shadow;
 }
 
 void print_rdd_state(void)
@@ -73,6 +76,7 @@ static void rdd_disconnect(void)
 	 */
 	gpio_set_flags(GPIO_CCD_MODE_L, GPIO_INPUT);
 }
+DECLARE_DEFERRED(rdd_disconnect);
 
 /**
  * Handle debug accessory connecting
@@ -105,6 +109,12 @@ DECLARE_DEFERRED(rdd_connect);
  */
 static void rdd_interrupt(void)
 {
+	uint8_t cc1 = GREAD_FIELD(RDD, INPUT_PIN_VALUES, CC1);
+	uint8_t cc2 = GREAD_FIELD(RDD, INPUT_PIN_VALUES, CC2);
+
+	/* Save the rdd state while the cc lines are stable. */
+	rdd_is_detected_shadow = (cc1 == cc2 && (cc1 == 3 || cc1 == 1));
+
 	/*
 	 * The Rdd detector is level-sensitive with debounce.  That is, it
 	 * samples the RDCCx pin states.  If they're different, it resets the
@@ -128,12 +138,13 @@ static void rdd_interrupt(void)
 	 * unlikely to actually trigger the deferred function twice, and it
 	 * doesn't care if we do anyway because on the second call it'll
 	 * already be in the connected state.
-	 *
 	 */
 	if (rdd_is_detected()) {
 		/* Accessory detected; toggle to looking for disconnect */
 		GWRITE(RDD, PROG_DEBUG_STATE_MAP, DETECT_DISCONNECT);
 
+		/* Cancel any pending disconnects */
+		hook_call_deferred(&rdd_disconnect_data, -1);
 		/*
 		 * Trigger the deferred handler so that we move back into the
 		 * connected state before our debounce interval expires.
@@ -141,10 +152,15 @@ static void rdd_interrupt(void)
 		hook_call_deferred(&rdd_connect_data, 0);
 	} else {
 		/*
-		 * Not detected; toggle to looking for connect.  We'll start
-		 * debouncing disconnect the next time HOOK_SECOND triggers
-		 * rdd_detect() below.
+		 * Skip disconnecting Rdd, if rdd is force detected. If Rdd is
+		 * already disconnected, no need to do it again.
 		 */
+		if (!force_detected && state != DEVICE_STATE_DISCONNECTED) {
+			/* Debounce disconnect for 1 second */
+			state = DEVICE_STATE_DEBOUNCING;
+			hook_call_deferred(&rdd_disconnect_data, SECOND);
+		}
+		/* Not detected; toggle to looking for connect. */
 		GWRITE(RDD, PROG_DEBUG_STATE_MAP, DETECT_DEBUG);
 	}
 
@@ -156,42 +172,6 @@ static void rdd_interrupt(void)
 }
 DECLARE_IRQ(GC_IRQNUM_RDD0_INTR_DEBUG_STATE_DETECTED_INT, rdd_interrupt, 1);
 
-/**
- * RDD CC detect state machine
- */
-static void rdd_detect(void)
-{
-	/* Handle detecting device */
-	if (force_detected || rdd_is_detected()) {
-		rdd_connect();
-		return;
-	}
-
-	/* CC wasn't detected.  If we're already disconnected, done. */
-	if (state == DEVICE_STATE_DISCONNECTED)
-		return;
-
-	/* If we were debouncing, we're now sure we're disconnected */
-	if (state == DEVICE_STATE_DEBOUNCING) {
-		rdd_disconnect();
-		return;
-	}
-
-	/*
-	 * Otherwise, we were connected but the accessory seems to be
-	 * disconnected right now.  PD negotiation (e.g. during EC reset or
-	 * sysjump) can alter the RDCCx voltages, so we need to debounce this
-	 * signal for longer than the Rdd hardware does to make sure it's
-	 * really disconnected before we deassert CCD_MODE_L.
-	 */
-	state = DEVICE_STATE_DEBOUNCING;
-}
-/*
- * Bump up priority so this runs before the CCD_MODE_L state machine, because
- * we can change CCD_MODE_L.
- */
-DECLARE_HOOK(HOOK_SECOND, rdd_detect, HOOK_PRIO_DEFAULT - 1);
-
 void init_rdd_state(void)
 {
 	/* Enable RDD hardware */
diff --git a/chip/g/rdd.h b/chip/g/rdd.h
index 159801b203..e30dc0c80f 100644
--- a/chip/g/rdd.h
+++ b/chip/g/rdd.h
@@ -21,6 +21,6 @@ void print_rdd_state(void);
  *
  * @return 1 if debug accessory is detected, 0 if not detected.
  */
-int rdd_is_detected(void);
+uint8_t rdd_is_detected(void);
 
 #endif  /* __CROS_RDD_H */