samus: pd: fix potential junk at end of tx transmissionstabilize-6670.B

Fix potential junk at end of PD TX transmit by adding to the DMA
transmit complete interrupt a blocking wait for SPI to finish and
then immediately disable SPI clock. This means we block in an
interrupt function for approximately 45us at the end of every
transmit. But, this is the highest priority thing going on anyway.

Note, there is still a potential for junk if both ports are
transmitting at the same time and finish very close to the same time.

BUG=chrome-os-partner:34600
BRANCH=samus
TEST=load onto samus and test communications with zinger. tested
specifically with an old zinger CL,
https://chromium-review.googlesource.com/#/c/226118/11,
which watchdogs when samus has junk at end of transmit. Tested
without this CL and verified we could never successfully flash zinger
over PD due to this watchdog and verified on scope presence of junk.
Then tested with this change and was able to successfully flash
zinger using ectool on both ports in both polarities.

Change-Id: If0cd9ab0551d36a7d7dc10232b6476dd56735972
Signed-off-by: Alec Berg <alecaberg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/239244
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

5 files changed, 94 insertions, 93 deletions

diff --git a/chip/stm32/dma.c b/chip/stm32/dma.c
index 0f2e89bc14..096ec45186 100644
--- a/chip/stm32/dma.c
+++ b/chip/stm32/dma.c
@@ -16,8 +16,11 @@
 #define CPUTS(outstr) cputs(CC_DMA, outstr)
 #define CPRINTF(format, args...) cprintf(CC_DMA, format, ## args)
 
-/* Task IDs for the interrupt handlers to wake up */
-static task_id_t id[STM32_DMAC_COUNT];
+/* Callback data to use when IRQ fires */
+static struct {
+	void (*cb)(void *);	/* Callback function to call */
+	void *cb_data;		/* Callback data for callback function */
+} dma_irq[STM32_DMAC_COUNT];
 
 /**
  * Return the IRQ for the DMA channel
@@ -206,14 +209,8 @@ void dma_test(void)
 
 void dma_init(void)
 {
-	int i;
-
 	/* Enable DMA1; current chips don't have DMA2 */
 	STM32_RCC_AHBENR |= STM32_RCC_HB_DMA1;
-
-	/* Initialize data for interrupt handlers */
-	for (i = 0; i < STM32_DMAC_COUNT; i++)
-		id[i] = TASK_ID_INVALID;
 }
 
 int dma_wait(enum dma_channel channel)
@@ -232,12 +229,27 @@ int dma_wait(enum dma_channel channel)
 	return EC_SUCCESS;
 }
 
+static inline void _dma_wake_callback(void *cb_data)
+{
+	task_id_t id = (task_id_t)(int)cb_data;
+	if (id != TASK_ID_INVALID)
+		task_set_event(id, TASK_EVENT_DMA_TC, 0);
+}
+
 void dma_enable_tc_interrupt(enum dma_channel channel)
 {
+	dma_enable_tc_interrupt_callback(channel, _dma_wake_callback,
+					 (void *)(int)task_get_current());
+}
+
+void dma_enable_tc_interrupt_callback(enum dma_channel channel,
+				      void (*callback)(void *),
+				      void *callback_data)
+{
 	stm32_dma_chan_t *chan = dma_get_channel(channel);
 
-	/* Store task ID so the ISR knows which task to wake */
-	id[channel] = task_get_current();
+	dma_irq[channel].cb = callback;
+	dma_irq[channel].cb_data = callback_data;
 
 	chan->ccr |= STM32_DMA_CCR_TCIE;
 	task_enable_irq(dma_get_irq(channel));
@@ -247,10 +259,11 @@ void dma_disable_tc_interrupt(enum dma_channel channel)
 {
 	stm32_dma_chan_t *chan = dma_get_channel(channel);
 
-	id[channel] = TASK_ID_INVALID;
-
 	chan->ccr &= ~STM32_DMA_CCR_TCIE;
 	task_disable_irq(dma_get_irq(channel));
+
+	dma_irq[channel].cb = NULL;
+	dma_irq[channel].cb_data = NULL;
 }
 
 void dma_clear_isr(enum dma_channel channel)
@@ -266,11 +279,12 @@ void dma_event_interrupt_channel_1(void)
 {
 	if (STM32_DMA1_REGS->isr & STM32_DMA_ISR_TCIF(STM32_DMAC_CH1)) {
 		dma_clear_isr(STM32_DMAC_CH1);
-		if (id[STM32_DMAC_CH1] != TASK_ID_INVALID)
-			task_wake(id[STM32_DMAC_CH1]);
+		if (dma_irq[STM32_DMAC_CH1].cb != NULL)
+			(*dma_irq[STM32_DMAC_CH1].cb)
+				(dma_irq[STM32_DMAC_CH1].cb_data);
 	}
 }
-DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_1, dma_event_interrupt_channel_1, 3);
+DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_1, dma_event_interrupt_channel_1, 1);
 
 void dma_event_interrupt_channel_2_3(void)
 {
@@ -279,12 +293,12 @@ void dma_event_interrupt_channel_2_3(void)
 	for (i = STM32_DMAC_CH2; i <= STM32_DMAC_CH3; i++) {
 		if (STM32_DMA1_REGS->isr & STM32_DMA_ISR_TCIF(i)) {
 			dma_clear_isr(i);
-			if (id[i] != TASK_ID_INVALID)
-				task_wake(id[i]);
+			if (dma_irq[i].cb != NULL)
+				(*dma_irq[i].cb)(dma_irq[i].cb_data);
 		}
 	}
 }
-DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_2_3, dma_event_interrupt_channel_2_3, 3);
+DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_2_3, dma_event_interrupt_channel_2_3, 1);
 
 void dma_event_interrupt_channel_4_7(void)
 {
@@ -293,12 +307,12 @@ void dma_event_interrupt_channel_4_7(void)
 	for (i = STM32_DMAC_CH4; i <= STM32_DMAC_CH7; i++) {
 		if (STM32_DMA1_REGS->isr & STM32_DMA_ISR_TCIF(i)) {
 			dma_clear_isr(i);
-			if (id[i] != TASK_ID_INVALID)
-				task_wake(id[i]);
+			if (dma_irq[i].cb != NULL)
+				(*dma_irq[i].cb)(dma_irq[i].cb_data);
 		}
 	}
 }
-DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_4_7, dma_event_interrupt_channel_4_7, 3);
+DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_4_7, dma_event_interrupt_channel_4_7, 1);
 
 #else /* !CHIP_FAMILY_STM32F0 */
 
@@ -306,11 +320,12 @@ DECLARE_IRQ(STM32_IRQ_DMA_CHANNEL_4_7, dma_event_interrupt_channel_4_7, 3);
 	void CONCAT2(dma_event_interrupt_channel_, x)(void) \
 	{ \
 		dma_clear_isr(CONCAT2(STM32_DMAC_CH, x)); \
-		if (id[CONCAT2(STM32_DMAC_CH, x)] != TASK_ID_INVALID) \
-			task_wake(id[CONCAT2(STM32_DMAC_CH, x)]); \
+		if (dma_irq[CONCAT2(STM32_DMAC_CH, x)].cb != NULL) \
+			(*dma_irq[CONCAT2(STM32_DMAC_CH, x)].cb) \
+				(dma_irq[CONCAT2(STM32_DMAC_CH, x)].cb_data); \
 	} \
 	DECLARE_IRQ(CONCAT2(STM32_IRQ_DMA_CHANNEL_, x), \
-		    CONCAT2(dma_event_interrupt_channel_, x), 3);
+		    CONCAT2(dma_event_interrupt_channel_, x), 1);
 
 DECLARE_DMA_IRQ(1);
 DECLARE_DMA_IRQ(2);
diff --git a/chip/stm32/usb_pd_phy.c b/chip/stm32/usb_pd_phy.c
index 55c465986d..1a09b8e07a 100644
--- a/chip/stm32/usb_pd_phy.c
+++ b/chip/stm32/usb_pd_phy.c
@@ -76,6 +76,9 @@ static struct pd_physical {
 static timestamp_t rx_edge_ts[PD_PORT_COUNT][PD_RX_TRANSITION_COUNT];
 static int rx_edge_ts_idx[PD_PORT_COUNT];
 
+/* keep track of transmit polarity for DMA interrupt */
+static int tx_dma_polarities[PD_PORT_COUNT];
+
 void pd_init_dequeue(int port)
 {
 	/* preamble ends with 1 */
@@ -273,26 +276,10 @@ void pd_tx_spi_init(int port)
 	/* Enable Tx DMA for our first transaction */
 	spi->cr2 = STM32_SPI_CR2_TXDMAEN | STM32_SPI_CR2_DATASIZE(8);
 
-#ifdef CONFIG_USB_PD_TX_USES_SPI_MASTER
-	/*
-	 * Enable the master SPI: LSB first, force NSS, TX only, CPOL and CPHA
-	 * high.
-	 */
-	spi->cr1 = STM32_SPI_CR1_LSBFIRST | STM32_SPI_CR1_BIDIMODE
-		 | STM32_SPI_CR1_SSM | STM32_SPI_CR1_SSI
-		 | STM32_SPI_CR1_BIDIOE | STM32_SPI_CR1_MSTR
-		 | STM32_SPI_CR1_BR_DIV64R | STM32_SPI_CR1_SPE
-		 | STM32_SPI_CR1_CPOL | STM32_SPI_CR1_CPHA;
-
-#if CPU_CLOCK != 38400000
-#error "CPU_CLOCK must be 38.4MHz to use SPI master for USB PD Tx"
-#endif
-#else
 	/* Enable the slave SPI: LSB first, force NSS, TX only, CPHA */
 	spi->cr1 = STM32_SPI_CR1_SPE | STM32_SPI_CR1_LSBFIRST
 		 | STM32_SPI_CR1_SSM | STM32_SPI_CR1_BIDIMODE
 		 | STM32_SPI_CR1_BIDIOE | STM32_SPI_CR1_CPHA;
-#endif
 }
 
 void pd_tx_set_circular_mode(int port)
@@ -300,6 +287,28 @@ void pd_tx_set_circular_mode(int port)
 	pd_phy[port].dma_tx_option.flags |= STM32_DMA_CCR_CIRC;
 }
 
+static void tx_dma_done(void *data)
+{
+	int port = (int)data;
+	int polarity = tx_dma_polarities[port];
+	stm32_spi_regs_t *spi = SPI_REGS(port);
+
+	while (spi->sr & STM32_SPI_SR_FTLVL)
+		; /* wait for TX FIFO empty */
+	while (spi->sr & STM32_SPI_SR_BSY)
+		; /* wait for BSY == 0 */
+
+	/* put TX pins and reference in Hi-Z */
+	pd_tx_disable(port, polarity);
+
+	/* Stop counting */
+	pd_phy[port].tim_tx->cr1 &= ~1;
+
+#if defined(CONFIG_COMMON_RUNTIME) && defined(CONFIG_DMA_DEFAULT_HANDLERS)
+	task_set_event(PORT_TO_TASK_ID(port), TASK_EVENT_DMA_TC, 0);
+#endif
+}
+
 int pd_start_tx(int port, int polarity, int bit_len)
 {
 	stm32_dma_chan_t *tx = dma_get_channel(DMAC_SPI_TX(port));
@@ -329,8 +338,10 @@ int pd_start_tx(int port, int polarity, int bit_len)
 
 	/* Kick off the DMA to send the data */
 	dma_clear_isr(DMAC_SPI_TX(port));
-#ifdef CONFIG_COMMON_RUNTIME
-	dma_enable_tc_interrupt(DMAC_SPI_TX(port));
+#if defined(CONFIG_COMMON_RUNTIME) && defined(CONFIG_DMA_DEFAULT_HANDLERS)
+	tx_dma_polarities[port] = polarity;
+	dma_enable_tc_interrupt_callback(DMAC_SPI_TX(port), &tx_dma_done,
+					 (void *)port);
 #endif
 	dma_go(tx);
 
@@ -343,65 +354,29 @@ int pd_start_tx(int port, int polarity, int bit_len)
 	 */
 	pd_tx_enable(port, polarity);
 
-#ifndef CONFIG_USB_PD_TX_USES_SPI_MASTER
 	/* Start counting at 300Khz*/
 	pd_phy[port].tim_tx->cr1 |= 1;
-#endif
 
 	return bit_len;
 }
 
 void pd_tx_done(int port, int polarity)
 {
-	stm32_spi_regs_t *spi = SPI_REGS(port);
+#if defined(CONFIG_COMMON_RUNTIME) && defined(CONFIG_DMA_DEFAULT_HANDLERS)
+	int rv;
 
-	/* wait for DMA */
-#ifdef CONFIG_COMMON_RUNTIME
-	task_wait_event(DMA_TRANSFER_TIMEOUT_US);
+	/* wait for DMA, DMA interrupt will stop the SPI clock */
+	do {
+		rv = task_wait_event(DMA_TRANSFER_TIMEOUT_US);
+	} while (!(rv & (TASK_EVENT_TIMER | TASK_EVENT_DMA_TC)));
 	dma_disable_tc_interrupt(DMAC_SPI_TX(port));
-#endif
-
-	/* wait for real end of transmission */
-#if defined(CHIP_FAMILY_STM32F0) || defined(CHIP_FAMILY_STM32F3)
-	while (spi->sr & STM32_SPI_SR_FTLVL)
-		; /* wait for TX FIFO empty */
-#else
-	while (!(spi->sr & STM32_SPI_SR_TXE))
-		; /* wait for TXE == 1 */
-#endif
-
-	/*
-	 * At the end of transmitting, the last bit is guaranteed by the
-	 * protocol to be low, and it is necessary that the TX line stay low
-	 * until pd_tx_disable().
-	 *
-	 * When using SPI slave mode for TX, this is done by writing out dummy
-	 * 0 byte at end.
-	 * When using SPI master mode, the CPOL and CPHA are set high, which
-	 * means that after the last bit is transmitted there are no more
-	 * clock edges. Hopefully, this is sufficient to guarantee that the
-	 * MOSI line does not change before pd_tx_disable().
-	 */
-#ifndef CONFIG_USB_PD_TX_USES_SPI_MASTER
-	/* ensure that we are not pushing out junk */
-	*(uint8_t *)&spi->dr = 0;
-	while (spi->sr & STM32_SPI_SR_FTLVL)
-		; /* wait for TX FIFO empty */
 #else
-	while (spi->sr & STM32_SPI_SR_BSY)
-		; /* wait for BSY == 0 */
+	tx_dma_polarities[port] = polarity;
+	tx_dma_done((void *)port);
 #endif
 
-	/* put TX pins and reference in Hi-Z */
-	pd_tx_disable(port, polarity);
-
-#ifndef CONFIG_USB_PD_TX_USES_SPI_MASTER
-	/* Stop counting */
-	pd_phy[port].tim_tx->cr1 &= ~1;
-
 	/* Reset SPI to clear remaining data in buffer */
 	pd_tx_spi_reset(port);
-#endif
 }
 
 /* --- RX operation using comparator linked to timer --- */
@@ -542,7 +517,6 @@ void pd_hw_init(int port)
 	phy->tim_tx = (void *)TIM_REG_TX(port);
 	phy->tim_rx = (void *)TIM_REG_RX(port);
 
-#ifndef CONFIG_USB_PD_TX_USES_SPI_MASTER
 	/* --- set the TX timer with updates at 600KHz (BMC frequency) --- */
 	__hw_timer_enable_clock(TIM_CLOCK_PD_TX(port), 1);
 	/* Timer configuration */
@@ -567,7 +541,6 @@ void pd_hw_init(int port)
 	phy->tim_tx->psc = 0;
 	/* Reload the pre-scaler and reset the counter */
 	phy->tim_tx->egr = 0x0001;
-#endif
 #ifndef CONFIG_USB_PD_TX_PHY_ONLY
 	/* configure RX DMA */
 	phy->dma_tim_option.channel = DMAC_TIM_RX(port);
diff --git a/include/config.h b/include/config.h
index e1ae4750d4..2270277265 100644
--- a/include/config.h
+++ b/include/config.h
@@ -1174,9 +1174,6 @@
 /* Use comparator module for PD RX interrupt */
 #define CONFIG_USB_PD_RX_COMP_IRQ
 
-/* USB PD transmit uses SPI master */
-#undef CONFIG_USB_PD_TX_USES_SPI_MASTER
-
 /* Support for USB type-c superspeed mux */
 #undef CONFIG_USBC_SS_MUX
 
diff --git a/include/dma.h b/include/dma.h
index 5e5553a794..3ecef70361 100644
--- a/include/dma.h
+++ b/include/dma.h
@@ -118,13 +118,27 @@ void dma_test(void);
 void dma_clear_isr(enum dma_channel channel);
 
 /**
- * Enable "Transfer Complete" interrupt for a DMA channel
+ * Enable "Transfer Complete" interrupt for a DMA channel.
+ * Will Wake up calling task when complete.
  *
  * @param channel	Which channel's interrupts to change
  */
 void dma_enable_tc_interrupt(enum dma_channel channel);
 
 /**
+ * Enable "Transfer Complete" interrupt for a DMA channel with callback
+ * NOTE: The callback is run at highest interrupt priority so should be
+ * fast and not depend on get_time().
+ *
+ * @param channel	Which channel's interrupts to change
+ * @param callback	Pointer to callback function to call on interrupt
+ * @param callback_data Data to pass through to callback function
+ */
+void dma_enable_tc_interrupt_callback(enum dma_channel channel,
+				      void (*callback)(void *),
+				      void *callback_data);
+
+/**
  * Disable "Transfer Complete" interrupt for a DMA channel
  *
  * @param channel	Which channel's interrupts to change
diff --git a/include/task.h b/include/task.h
index 1ec546fdb1..0c71e804ba 100644
--- a/include/task.h
+++ b/include/task.h
@@ -13,7 +13,9 @@
 
 /* Task event bitmasks */
 /* Tasks may use the bits in TASK_EVENT_CUSTOM for their own events */
-#define TASK_EVENT_CUSTOM(x)	(x & 0x07ffffff)
+#define TASK_EVENT_CUSTOM(x)	(x & 0x03ffffff)
+/* DMA transmit complete event */
+#define TASK_EVENT_DMA_TC       (1 << 26)
 /* ADC interrupt handler event */
 #define TASK_EVENT_ADC_DONE	(1 << 27)
 /* I2C interrupt handler event */