From c5e79ca4d3a3991886cc260155ce806c6427acda Mon Sep 17 00:00:00 2001 From: Jonathon Reinhart Date: Tue, 27 Aug 2024 18:10:30 +0000 Subject: [PATCH] pw_spi_mcuxpresso: Add check_fifo_error to responder config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test: Enabled in downstream project, verified no false positives Change-Id: I42aed460d5f4ab026999c593e4e6567ba48bf278 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/232215 Reviewed-by: Eli Lipsitz Commit-Queue: Jonathon Reinhart Lint: Lint 🤖 --- .../public/pw_spi_mcuxpresso/responder.h | 16 +++++++++- pw_spi_mcuxpresso/responder.cc | 29 ++++++++++--------- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/responder.h b/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/responder.h index 6e78cc2e9a..5edb0969d5 100644 --- a/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/responder.h +++ b/pw_spi_mcuxpresso/public/pw_spi_mcuxpresso/responder.h @@ -27,6 +27,12 @@ namespace pw::spi { +enum class FifoErrorCheck { + kNone, // Don't check for FIFO error. + kLogOnly, // Only log on FIFO error. + kError, // Log and return DATA_LOSS. +}; + class McuxpressoResponder : public Responder { public: struct Config { @@ -37,7 +43,15 @@ class McuxpressoResponder : public Responder { uint32_t base_address; // Flexcomm peripheral base address. // True if the driver should handle Chip Select (CS) assertion and // deassertion. When set, transfers will complete on CS deassertion. - bool handle_cs; + bool handle_cs = true; + + // If enabled, the FIFO status registers are checked for error + // (underflow/overflow) upon transfer completion, returning DATA_LOSS if + // detected. + // + // NOTE: A false positive could be triggered if this is enabled and the + // initiator clocks more bytes than the transfer is set up to send+receive. + FifoErrorCheck check_fifo_error = FifoErrorCheck::kNone; }; McuxpressoResponder(const Config config, diff --git a/pw_spi_mcuxpresso/responder.cc b/pw_spi_mcuxpresso/responder.cc index 18a4569b36..24bc0f9cde 100644 --- a/pw_spi_mcuxpresso/responder.cc +++ b/pw_spi_mcuxpresso/responder.cc @@ -170,7 +170,6 @@ bool SPI_RxFifoIsEmpty(SPI_Type* base) { return !(base->FIFOSTAT & SPI_FIFOSTAT_RXNOTEMPTY_MASK); } -#if 0 bool SPI_RxError(SPI_Type* base) { return (base->FIFOSTAT & SPI_FIFOSTAT_RXERR_MASK); } @@ -178,7 +177,6 @@ bool SPI_RxError(SPI_Type* base) { bool SPI_TxError(SPI_Type* base) { return (base->FIFOSTAT & SPI_FIFOSTAT_TXERR_MASK); } -#endif // Non-FIFO interrupt sources enum _spi_interrupt_sources { @@ -279,24 +277,29 @@ void McuxpressoResponder::TransferComplete(Status status, SPI_SlaveTransferAbortDMA(base_, &handle_); // Check for TX underflow / RX overflow - // TODO(jrreinhart): Unfortunately we can't do this. We want to check for - // FIFO under/overflow *while* the transfer is running, but if the initiator - // sent more bytes than the DMA was set up to receive, both of these errors - // will happen (after the DMA is complete). We would need to find a way to - // capture this status immediate when the DMA is complete, or otherwise - // monitor it during the transfer. -#if 0 - if (status.ok()) { + // + // Ideally we want to check for FIFO under/overflow only *while* the transfer + // is running. But if the initiator sent more bytes than the DMA was set up + // to tx/rx, both of these errors will happen (after the DMA is complete). + // + // To do this without risk of false positives, we would need to find a way to + // capture this status immediately when the DMA is complete, or otherwise + // monitor it during the transfer. Perhaps with a custom DMA chain, we could + // capture the status registers via DMA, but that might still be too late. + if (status.ok() && config_.check_fifo_error != FifoErrorCheck::kNone) { if (SPI_RxError(base_)) { PW_LOG_ERROR("RX FIFO overflow detected!"); - status = Status::DataLoss(); + if (config_.check_fifo_error == FifoErrorCheck::kError) { + status = Status::DataLoss(); + } } if (SPI_TxError(base_)) { PW_LOG_ERROR("TX FIFO underflow detected!"); - status = Status::DataLoss(); + if (config_.check_fifo_error == FifoErrorCheck::kError) { + status = Status::DataLoss(); + } } } -#endif // TODO(jrreinhart) Remove these safety checks. if (rx_dma_.IsBusy()) {