drivers: spi_nxp_lpspi: Fix extra byte issue on v1

The interrupt handling was not deterministic before because it relied
on "guessing" if the lpspi was a v1 type that was stalling due to design
errata. This obviously ended up being wrong in some cases. So we really
need to fix this so that it is deterministic, and my idea to do that is
to explicitly count how many words we have written to the TX fifo
throughout the whole transfer.

As a side effect of making the IRQ more deterministic, we can't support
the SPI_HOLD_ON_CS flag for v1 LPSPI anymore. It is fundamentally
impossible due to the fact that the transfer can only complete in
hardware as a result of CS deasserting. If this is absolutely a
requirement to support this flag in the future, my recommendation is to
update the driver so that it muxes the pin to a gpio before ending the
transfer, but that would kind of defeat the point of having a native CS,
at least at the end of the xfer.

Signed-off-by: Declan Snyder <[email protected]>

Author: decsny

drivers/spi/spi_nxp_lpspi/spi_nxp_lpspi.c

@@ -12,6 +12,8 @@ LOG_MODULE_DECLARE(spi_lpspi, CONFIG_SPI_LOG_LEVEL);
 #include "spi_nxp_lpspi_priv.h"
 
 struct lpspi_driver_data {
+	size_t total_words_to_clock;
+	size_t words_clocked;
 	uint8_t word_size_bytes;
 	uint8_t lpspi_op_mode;
 };
@@ -72,10 +74,10 @@ static inline void lpspi_handle_rx_irq(const struct device *dev)
 	struct lpspi_data *data = dev->data;
 	struct lpspi_driver_data *lpspi_data = (struct lpspi_driver_data *)data->driver_data;
 	struct spi_context *ctx = &data->ctx;
-	uint8_t rx_fsr = rx_fifo_cur_len(base);
 	uint8_t total_words_written = 0;
 	uint8_t total_words_read = 0;
 	uint8_t words_read;
+	uint8_t rx_fsr;
 
 	base->SR = LPSPI_SR_RDF_MASK;
 
@@ -125,18 +127,22 @@ static inline void lpspi_fill_tx_fifo(const struct device *dev, const uint8_t *b
 		buf_len -= word_size;
 	}
 
+	lpspi_data->words_clocked += fill_len;
 	LOG_DBG("Filled TX FIFO to %d words (%d bytes)", fill_len, offset);
 }
 
 /* just fills TX fifo with the specified amount of NOPS */
 static void lpspi_fill_tx_fifo_nop(const struct device *dev, size_t fill_len)
 {
 	LPSPI_Type *base = (LPSPI_Type *)DEVICE_MMIO_NAMED_GET(dev, reg_base);
+	struct lpspi_data *data = dev->data;
+	struct lpspi_driver_data *lpspi_data = (struct lpspi_driver_data *)data->driver_data;
 
 	for (int i = 0; i < fill_len; i++) {
 		base->TDR = 0;
 	}
 
+	lpspi_data->words_clocked += fill_len;
 	LOG_DBG("Filled TX fifo with %d NOPs", fill_len);
 }
 
@@ -243,15 +249,14 @@ static void lpspi_isr(const struct device *dev)
 	struct lpspi_data *data = dev->data;
 	struct lpspi_driver_data *lpspi_data = (struct lpspi_driver_data *)data->driver_data;
 	uint8_t word_size_bytes = lpspi_data->word_size_bytes;
-	uint8_t op_mode = lpspi_data->lpspi_op_mode;
 	struct spi_context *ctx = &data->ctx;
 	uint32_t status_flags = base->SR;
 
-	if (status_flags & LPSPI_SR_RDF_MASK) {
+	if (status_flags & LPSPI_SR_RDF_MASK && base->IER & LPSPI_IER_RDIE_MASK) {
 		lpspi_handle_rx_irq(dev);
 	}
 
-	if (status_flags & LPSPI_SR_TDF_MASK) {
+	if (status_flags & LPSPI_SR_TDF_MASK && base->IER & LPSPI_IER_TDIE_MASK) {
 		lpspi_handle_tx_irq(dev);
 	}
 
@@ -264,65 +269,59 @@ static void lpspi_isr(const struct device *dev)
 		return;
 	}
 
-	/* the lpspi v1 has an errata where it doesn't clock the last bit
-	 * in continuous master mode until you write the TCR
-	 */
-	bool likely_stalling_v1 = (data->major_version < 2) && (op_mode == SPI_OP_MODE_MASTER) &&
-		(DIV_ROUND_UP(spi_context_rx_len_left(ctx, word_size_bytes), word_size_bytes) == 1);
-
-	if (spi_context_rx_on(ctx)) {
-		/* capture these values because they could change during this code block */
-		size_t rx_fifo_len = rx_fifo_cur_len(base);
-		size_t tx_fifo_len = tx_fifo_cur_len(base);
-
-		/*
-		 * Goal here is to provide the number of TX NOPS to match the amount of RX
-		 * we have left to receive, so that we provide the correct number of
-		 * clocks to the bus, since the clocks only happen when TX data is being sent.
-		 *
-		 * The expected RX left is essentially what is left in the spi transfer
-		 * minus what we have just got in the fifo, but prevent underflow of this
-		 * subtraction since it is unsigned.
-		 */
-		size_t expected_rx_left = rx_fifo_len < ctx->rx_len ? ctx->rx_len - rx_fifo_len : 0;
+	/* Both receive and transmit parts disable their interrupt once finished. */
+	if (base->IER == 0) {
+		lpspi_end_xfer(dev);
+		return;
+	}
 
+	/* only explanation at this point is one of two things:
+	 * 1) that rx is bigger than tx and we need to clock nops
+	 * 2) this is a version of LPSPI which will not clock the last bit of the transfer
+	 *    in continuous mode until the TCR is written to end the XFER
+	 */
 
-		/*
-		 * We know the expected amount of RX we have left but only fill up to the
-		 * max of the RX fifo size so that we don't have some overflow of the FIFO later.
-		 * Similarly, we shouldn't overfill the TX fifo with too many NOPs.
+	if (lpspi_data->words_clocked >= lpspi_data->total_words_to_clock) {
+		/* Due to stalling behavior on older LPSPI, if we know we already wrote all the
+		 * words into the fifo, then we need to end xfer manually by writing TCR
+		 * in order to get last bit clocked out on bus. So all we need to do is touch the
+		 * TCR by writing to fifo through TCR register and wait for final RX interrupt.
 		 */
-		size_t tx_fifo_space_left = config->tx_fifo_size - tx_fifo_len;
-		size_t rx_fifo_space_left = config->rx_fifo_size - rx_fifo_len;
-		size_t max_fifo_fill = MIN(tx_fifo_space_left, rx_fifo_space_left);
-		size_t max_fill = MIN(max_fifo_fill, expected_rx_left);
-
-		if (likely_stalling_v1 && max_fill > 0) {
-			max_fill -= 1;
-		}
+		base->TCR = base->TCR;
+		return;
+	}
 
-		/* If we already have some words in the tx fifo, we should count those */
-		if (max_fill > tx_fifo_len) {
-			max_fill -= tx_fifo_len;
-		} else {
-			max_fill = 0;
-		}
+	/* At this point we know only case is that we need to put NOPs in the TX fifo
+	 * in order to get the rest of the RX
+	 */
 
-		/* So now we want to fill the fifo with the max amount of NOPs */
-		lpspi_fill_tx_fifo_nop(dev, max_fill);
-	}
+	size_t rx_fifo_len = rx_fifo_cur_len(base);
+	size_t tx_fifo_len = tx_fifo_cur_len(base);
+	size_t words_really_left = lpspi_data->total_words_to_clock - lpspi_data->words_clocked;
+
+	/*
+	 * Goal here is to provide the number of TX NOPS to match the amount of RX
+	 * we have left to receive, so that we provide the correct number of
+	 * clocks to the bus, since the clocks only happen when TX data is being sent.
+	 *
+	 * The expected RX left is essentially what is left in the spi transfer
+	 * minus what we have just got in the fifo, but prevent underflow of this
+	 * subtraction since it is unsigned.
+	 */
+	size_t expected_rx_left = rx_fifo_len < words_really_left ?
+					words_really_left - rx_fifo_len : 0;
 
-	if (likely_stalling_v1) {
-		/* Due to stalling behavior on older LPSPI,
-		 * need to end xfer in order to get last bit clocked out on bus.
-		 */
-		base->TCR |= LPSPI_TCR_CONT_MASK;
-	}
+	/*
+	 * We know the expected amount of RX we have left but only fill up to the
+	 * max of the RX fifo size so that we don't have some overflow of the FIFO later.
+	 * Similarly, we shouldn't overfill the TX fifo with too many NOPs.
+	 */
+	size_t tx_fifo_space_left = config->tx_fifo_size - tx_fifo_len;
+	size_t rx_fifo_space_left = config->rx_fifo_size - rx_fifo_len;
+	size_t max_fifo_fill = MIN(tx_fifo_space_left, rx_fifo_space_left);
+	size_t max_fill = MIN(max_fifo_fill, expected_rx_left);
 
-	/* Both receive and transmit parts disable their interrupt once finished. */
-	if (base->IER == 0) {
-		lpspi_end_xfer(dev);
-	}
+	lpspi_fill_tx_fifo_nop(dev, max_fill);
 }
 
 static void lpspi_master_setup_native_cs(const struct device *dev, const struct spi_config *spi_cfg)
@@ -372,6 +371,14 @@ static int transceive(const struct device *dev, const struct spi_config *spi_cfg
 		goto error;
 	}
 
+	if (data->major_version < 2 && spi_cfg->operation & SPI_HOLD_ON_CS) {
+		/* on this version of LPSPI, due to errata in design
+		 * CS must be deasserted in order to clock all words,
+		 * so HOLD_ON_CS flag cannot be supported.
+		 */
+		return -EINVAL;
+	}
+
 	spi_context_buffers_setup(ctx, tx_bufs, rx_bufs, lpspi_data->word_size_bytes);
 	lpspi_data->lpspi_op_mode = op_mode;
 
@@ -384,6 +391,12 @@ static int transceive(const struct device *dev, const struct spi_config *spi_cfg
 	base->IER = 0;
 	base->SR |= LPSPI_INTERRUPT_BITS;
 
+	size_t max_side_clocks = MAX(spi_context_total_tx_len(ctx), spi_context_total_rx_len(ctx));
+
+	lpspi_data->total_words_to_clock =
+				DIV_ROUND_UP(max_side_clocks, lpspi_data->word_size_bytes);
+	lpspi_data->words_clocked = 0;
+
 	LOG_DBG("Starting LPSPI transfer");
 	spi_context_cs_control(ctx, true);