dma: separate out DMA enable status from wait_for_bytes

When wait_for_bytes returns 0 when DMA is disabled, we can't
differentiate between DMA being disabled and a transfer having completed
when it has reached the end of the requested transfer. Separating out
into separate functions lets us distinguish the two cases.

The reason we didn't hit this in the past is that the requested receive
size is generally larger than the actual amount we're sending. Since we
know the amount that we're waiting for from the header, we would stop
the transfer in software.

BRANCH=none
BUG=b:132444384
TEST=On DUTs with bloonchipper and dartmonkey:
     ectool --name=cros_fp testmaxtransfer
TEST=make buildall -j

Change-Id: I885161a3e04b7a12d597d8dc8691f599990bda8b
Signed-off-by: Tom Hughes <tomhughes@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1734010
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>

7 files changed, 64 insertions, 28 deletions

diff --git a/chip/mchp/dma.c b/chip/mchp/dma.c
index 1c491729cd..6c9ed8dd47 100644
--- a/chip/mchp/dma.c
+++ b/chip/mchp/dma.c
@@ -194,28 +194,32 @@ void dma_xfr_start_rx(const struct dma_option *option,
 
 /*
  * Return the number of bytes transferred.
- * The number of bytes transferred can be easily determinted
+ * The number of bytes transferred can be easily determined
  * from the difference in DMA memory start address register
  * and memory end address register. No need to look at DMA
  * transfer size field because the hardware increments memory
- * start address by unit size on each unit tranferred.
+ * start address by unit size on each unit transferred.
  * Why is a signed integer being used for a count value?
  */
 int dma_bytes_done(dma_chan_t *chan, int orig_count)
 {
-	int bcnt = 0;
+	int bcnt;
 
-	if (chan != NULL) {
-		if (chan->ctrl & MCHP_DMA_RUN) {
-			bcnt = (int)chan->mem_end;
-			bcnt -= (int)chan->mem_start;
-			bcnt = orig_count - bcnt;
-		}
-	}
+	if (chan == NULL)
+		return 0;
+
+	bcnt = (int)chan->mem_end;
+	bcnt -= (int)chan->mem_start;
+	bcnt = orig_count - bcnt;
 
 	return bcnt;
 }
 
+bool dma_is_enabled(dma_chan_t *chan)
+{
+	return (chan->ctrl & MCHP_DMA_RUN);
+}
+
 int dma_bytes_done_chan(enum dma_channel ch, uint32_t orig_count)
 {
 	uint32_t cnt;
diff --git a/chip/mec1322/dma.c b/chip/mec1322/dma.c
index ff5a0385c0..a6c6fed5ad 100644
--- a/chip/mec1322/dma.c
+++ b/chip/mec1322/dma.c
@@ -119,11 +119,14 @@ int dma_bytes_done(mec1322_dma_chan_t *chan, int orig_count)
 {
 	int xfer_size = (chan->ctrl >> 20) & 0x7;
 
-	if (!(chan->ctrl & MEC1322_DMA_RUN))
-		return 0;
 	return orig_count - (chan->mem_end - chan->mem_start) / xfer_size;
 }
 
+bool dma_is_enabled(mec1322_dma_chan_t *chan)
+{
+	return (chan->ctrl & MEC1322_DMA_RUN);
+}
+
 void dma_init(void)
 {
 	mec1322_dma_regs_t *dma = MEC1322_DMA_REGS;
diff --git a/chip/stm32/dma-stm32f4.c b/chip/stm32/dma-stm32f4.c
index a821ee3d08..abb33befda 100644
--- a/chip/stm32/dma-stm32f4.c
+++ b/chip/stm32/dma-stm32f4.c
@@ -15,6 +15,7 @@
 /* Console output macros */
 #define CPUTS(outstr) cputs(CC_DMA, outstr)
 #define CPRINTF(format, args...) cprintf(CC_DMA, format, ## args)
+#define CPRINTS(format, args...) cprints(CC_DMA, format, ## args)
 
 stm32_dma_regs_t *STM32_DMA_REGS[] = { STM32_DMA1_REGS, STM32_DMA2_REGS };
 
@@ -144,11 +145,29 @@ void dma_start_rx(const struct dma_option *option, unsigned count,
 
 int dma_bytes_done(stm32_dma_stream_t *stream, int orig_count)
 {
-	if (!(stream->scr & STM32_DMA_CCR_EN))
-		return 0;
+	/*
+	 * Note that we're intentionally not checking that DMA is enabled here
+	 * because there is a race when the hardware stops the transfer:
+	 *
+	 * From Section 9.3.14 DMA transfer completion in RM0402 Rev 5
+	 * https://www.st.com/resource/en/reference_manual/dm00180369.pdf:
+	 * If the stream is configured in non-circular mode, after the end of
+	 * the transfer (that is when the number of data to be transferred
+	 * reaches zero), the DMA is stopped (EN bit in DMA_SxCR register is
+	 * cleared by Hardware) and no DMA request is served unless the software
+	 * reprograms the stream and re-enables it (by setting the EN bit in the
+	 * DMA_SxCR register).
+	 *
+	 * See http://b/132444384 for full details.
+	 */
 	return orig_count - stream->sndtr;
 }
 
+bool dma_is_enabled(stm32_dma_stream_t *stream)
+{
+	return (stream->scr & STM32_DMA_CCR_EN);
+}
+
 #ifdef CONFIG_DMA_HELP
 void dma_dump(enum dma_channel stream)
 {
diff --git a/chip/stm32/dma.c b/chip/stm32/dma.c
index 031b8566bd..b09213d039 100644
--- a/chip/stm32/dma.c
+++ b/chip/stm32/dma.c
@@ -154,11 +154,14 @@ void dma_start_rx(const struct dma_option *option, unsigned count,
 
 int dma_bytes_done(stm32_dma_chan_t *chan, int orig_count)
 {
-	if (!(chan->ccr & STM32_DMA_CCR_EN))
-		return 0;
 	return orig_count - chan->cndtr;
 }
 
+bool dma_is_enabled(stm32_dma_chan_t *chan)
+{
+	return (chan->ccr & STM32_DMA_CCR_EN);
+}
+
 #ifdef CONFIG_DMA_HELP
 void dma_dump(enum dma_channel channel)
 {
diff --git a/chip/stm32/spi_master-stm32h7.c b/chip/stm32/spi_master-stm32h7.c
index af4c3a30f2..6f661b72bf 100644
--- a/chip/stm32/spi_master-stm32h7.c
+++ b/chip/stm32/spi_master-stm32h7.c
@@ -211,10 +211,9 @@ static int spi_dma_start(int port, const uint8_t *txdata,
 	return EC_SUCCESS;
 }
 
-static inline int dma_is_enabled(const struct dma_option *option)
+static inline bool dma_is_enabled_(const struct dma_option *option)
 {
-	/* dma_bytes_done() returns 0 if channel is not enabled */
-	return dma_bytes_done(dma_get_channel(option->channel), -1);
+	return dma_is_enabled(dma_get_channel(option->channel));
 }
 
 static int spi_dma_wait(int port)
@@ -224,7 +223,7 @@ static int spi_dma_wait(int port)
 	int rv = EC_SUCCESS;
 
 	/* Wait for DMA transmission to complete */
-	if (dma_is_enabled(&dma_tx_option[port])) {
+	if (dma_is_enabled_(&dma_tx_option[port])) {
 		rv = dma_wait(dma_tx_option[port].channel);
 		if (rv)
 			return rv;
@@ -240,7 +239,7 @@ static int spi_dma_wait(int port)
 	}
 
 	/* Wait for DMA reception to complete */
-	if (dma_is_enabled(&dma_rx_option[port])) {
+	if (dma_is_enabled_(&dma_rx_option[port])) {
 		rv = dma_wait(dma_rx_option[port].channel);
 		if (rv)
 			return rv;
diff --git a/chip/stm32/spi_master.c b/chip/stm32/spi_master.c
index 40002643c1..47c5be3979 100644
--- a/chip/stm32/spi_master.c
+++ b/chip/stm32/spi_master.c
@@ -297,10 +297,9 @@ static int spi_dma_start(int port, const uint8_t *txdata,
 	return EC_SUCCESS;
 }
 
-static int dma_is_enabled(const struct dma_option *option)
+static bool dma_is_enabled_(const struct dma_option *option)
 {
-	/* dma_bytes_done() returns 0 if channel is not enabled */
-	return dma_bytes_done(dma_get_channel(option->channel), -1);
+	return dma_is_enabled(dma_get_channel(option->channel));
 }
 
 static int spi_dma_wait(int port)
@@ -308,7 +307,7 @@ static int spi_dma_wait(int port)
 	int rv = EC_SUCCESS;
 
 	/* Wait for DMA transmission to complete */
-	if (dma_is_enabled(&dma_tx_option[port])) {
+	if (dma_is_enabled_(&dma_tx_option[port])) {
 		/*
 		 * In TX mode, SPI only generates clock when we write to FIFO.
 		 * Therefore, even though `dma_wait` polls with interval 0.1ms,
@@ -322,7 +321,7 @@ static int spi_dma_wait(int port)
 	}
 
 	/* Wait for DMA reception to complete */
-	if (dma_is_enabled(&dma_rx_option[port])) {
+	if (dma_is_enabled_(&dma_rx_option[port])) {
 		/*
 		 * Because `dma_wait` polls with interval 0.1ms, we will read at
 		 * least ~100 bytes (with 8MHz clock).  If you don't want this
diff --git a/include/dma.h b/include/dma.h
index a6be40a031..1687b5f899 100644
--- a/include/dma.h
+++ b/include/dma.h
@@ -13,6 +13,8 @@
 #include "common.h"
 #include "registers.h"
 
+#include <stdbool.h>
+
 /* DMA channel options */
 struct dma_option {
 	enum dma_channel channel;	/* DMA channel */
@@ -76,12 +78,19 @@ void dma_disable_all(void);
  *
  * @param chan		DMA channel to check, from dma_get_channel()
  * @param orig_count	Original number of bytes requested on channel
- * @return number of bytes completed on a channel, or 0 if this channel is
- *		not enabled
+ * @return number of bytes completed on a channel
  */
 int dma_bytes_done(dma_chan_t *chan, int orig_count);
 
 /**
+ * Check if DMA for a given channel is enabled.
+ *
+ * @param chan  DMA channel to check, from dma_get_channel()
+ * @return true if DMA is enabled on the channel, false otherwise
+ */
+bool dma_is_enabled(dma_chan_t *chan);
+
+/**
  * Start a previously-prepared dma channel
  *
  * @param chan	Channel to start, from dma_get_channel()