Cr50: Clear the TX FIFO when the SPS CS deasserts

When the SPI slave chip select is deasserted, it means that the
SPI master doesn't want to hear any more from the EC. We need to
clear any bytes left in the TX FIFO, so that the next SPI
transaction doesn't send those leftover bytes out.

Since the EC's SPI protocol for host commands uses software flow
control, those leftover bytes could screw up the messages. I
expanded a comment explaining how that works.

BUG=chrome-os-partner:40969
BRANCH=none
TEST=make buildall

And, with the EC connected to the build host via an FTDI USB-to-SPI
adapater, I used the extra/sps_errs/ test program to see the
original problem and that this CL fixed it:

  cd extra/sps_errs
  make
  ./prog -v
  ./prog -v -c 22
  ./prog -v

This sends a complete EC_CMD_HELLO message, then a truncated
message, then sends the whole message. Before this change to
sps.c, the third message response begins with the leftover bytes
from the aborted second message.

Bad third message:

  Transfer(12) =>
     03 64 01 00 00 00 04 00 a5 a5 a5 a5
  Transfer(12) <=
     a9 a8 a7 a6 f8 f8 f8 f8 f8 f8 f9 f9
     ^^ ^^ ^^ ^^

Good third message:

  Transfer(12) =>
     03 64 01 00 00 00 04 00 a5 a5 a5 a5
  Transfer(12) <=
     f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f9 f9
     ^^ ^^ ^^ ^^

Change-Id: Id6e431f91be0204921edee2f774b6c487966ddff
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/284746
Reviewed-by: Vadim Bendebury <vbendeb@google.com>

2 files changed, 28 insertions, 9 deletions

diff --git a/chip/g/sps.c b/chip/g/sps.c
index b4152da076..c68e3b0c60 100644
--- a/chip/g/sps.c
+++ b/chip/g/sps.c
@@ -167,17 +167,18 @@ static void sps_enable(void)
 	GWRITE_FIELD(SPS, FIFO_CTRL, TXFIFO_EN, 1);
 
 	/*
-	 * Wait until we have a few bytes in the FIFO before waking up. Note
-	 * that if the master wants to read bytes from us, it may have to clock
-	 * in at least RXFIFO_THRESHOLD + 1 bytes before we notice that it's
-	 * asking.
+	 * Wait until we have a few bytes in the FIFO before waking up. There's
+	 * a tradeoff here: If the master wants to talk to us it will have to
+	 * clock in at least RXFIFO_THRESHOLD + 1 bytes before we notice. On
+	 * the other hand, if we set this too low we waste a lot of time
+	 * handling interrupts before we have enough bytes to know what the
+	 * master is saying.
 	 */
-	GREG32(SPS, RXFIFO_THRESHOLD) = 8;
+	GREG32(SPS, RXFIFO_THRESHOLD) = 7;
 	GWRITE_FIELD(SPS, ICTRL, RXFIFO_LVL, 1);
 
 	/* Also wake up when the master has finished talking to us, so we can
-	 * drain any remaining bytes in the RX FIFO. Too late for TX, of
-	 * course. */
+	 * drain any remaining bytes in the RX FIFO. */
 	GWRITE_FIELD(SPS, ISTATE_CLR, CS_DEASSERT, 1);
 	GWRITE_FIELD(SPS, ICTRL, CS_DEASSERT, 1);
 }
@@ -245,8 +246,11 @@ DECLARE_IRQ(GC_IRQNUM_SPS0_RXFIFO_LVL_INTR, _sps0_interrupt, 1);
 
 void _sps0_cs_deassert_interrupt(void)
 {
-	/* Make sure the receive FIFO is drained. */
+	/* Notify the registered handler (and drain the RX FIFO) */
 	sps_rx_interrupt(0);
+	/* Clear the TX FIFO manually, so the next transaction doesn't
+	 * start by clocking out any bytes left over from this one. */
+	GREG32(SPS, TXFIFO_WPTR) = GREG32(SPS, TXFIFO_RPTR);
 	/* Clear the interrupt bit */
 	GWRITE_FIELD(SPS, ISTATE_CLR, CS_DEASSERT, 1);
 }
diff --git a/chip/g/sps_hc.c b/chip/g/sps_hc.c
index 1b302a5e17..d0c16ba2fe 100644
--- a/chip/g/sps_hc.c
+++ b/chip/g/sps_hc.c
@@ -13,6 +13,22 @@
 /*
  * This implements EC host commands over the SPI bus, using the Cr50 SPS (SPI
  * slave) controller.
+ *
+ * Host commands are communicated using software flow-control, because most of
+ * the embedded controllers either aren't fast enough or don't have any support
+ * for hardware flow-control.
+ *
+ * Every SPI transaction is bi-directional, so when the AP sends commands to
+ * the EC, a default "dummy" byte is returned at the same time. The EC
+ * preconfigures that default response byte to indicate its status (ready,
+ * busy, waiting for more input, etc). Once the AP has sent a complete command
+ * message, it continues clocking bytes to the EC (which the EC ignores) and
+ * just looks at the response byte that comes back. Once the EC has parsed the
+ * command and is ready to reply, it sends a "start of frame" byte, followed by
+ * the actual response. The AP continues to read and ignore bytes from the EC
+ * until it sees the start of frame byte, and then it knows that the EC's
+ * response is starting with the next byte. Refer to include/ec_commands.h for
+ * more details.
  */
 
 /* Console output macros */
@@ -126,7 +142,6 @@ static void hc_rx_handler(uint8_t *data, size_t data_size, int cs_enabled)
 		}
 
 		/* Otherwise, just go back to waiting for new input */
-		/* HEY: should we discard any unsent TX bytes? How? */
 		state = SPI_STATE_READY_TO_RX;
 		sps_tx_status(EC_SPI_RX_READY);
 		return;