In target mode, the host sending more data than can be consumed would be
a common problem for any message exceeding the FIFO or DMA buffer size.
Cancel the whole message as soon as this condition is hit as the message
will be corrupted.
Only do this for target mode in a DMA transfer, it's not likely these
flags will be set in host mode so it's not worth adding extra checks. In
IRQ and polling modes we use the same transfer functions for hosts and
targets so the error flags always get checked. This is slightly
inconsistent but it's not worth doing the check conditionally because it
may catch some host programming errors in the future.
Signed-off-by: James Clark <james.clark@linaro.org>
---
drivers/spi/spi-fsl-dspi.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 46d3cae9efed..2c2a263c5276 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -478,6 +478,17 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
dspi->dev_to_host(dspi, rxdata);
}
+static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
+{
+ if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
+ dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
+ spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
+ spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
+ return -EIO;
+ }
+ return 0;
+}
+
#if IS_ENABLED(CONFIG_DMA_ENGINE)
/* Prepare one TX FIFO entry (txdata plus cmd) */
@@ -566,6 +577,7 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
struct device *dev = &dspi->pdev->dev;
struct fsl_dspi_dma *dma = dspi->dma;
int time_left;
+ u32 spi_sr;
int i;
for (i = 0; i < dspi->words_in_flight; i++)
@@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
if (spi_controller_is_target(dspi->ctlr)) {
wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
- return 0;
+ regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+ return dspi_fifo_error(dspi, spi_sr);
}
time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
@@ -1067,6 +1080,9 @@ static void dspi_poll(struct fsl_dspi *dspi)
regmap_read(dspi->regmap, SPI_SR, &spi_sr);
regmap_write(dspi->regmap, SPI_SR, spi_sr);
+ dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
+ if (dspi->cur_msg->status)
+ return;
if (spi_sr & SPI_SR_CMDTCF)
break;
}
@@ -1085,6 +1101,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
static irqreturn_t dspi_interrupt(int irq, void *dev_id)
{
struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
+ int status;
u32 spi_sr;
regmap_read(dspi->regmap, SPI_SR, &spi_sr);
@@ -1093,6 +1110,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
if (!(spi_sr & SPI_SR_CMDTCF))
return IRQ_NONE;
+ status = dspi_fifo_error(dspi, spi_sr);
+ if (status) {
+ if (dspi->cur_msg)
+ WRITE_ONCE(dspi->cur_msg->status, status);
+ complete(&dspi->xfer_done);
+ return IRQ_HANDLED;
+ }
+
dspi_rxtx(dspi);
if (!dspi->len) {
--
2.34.1On Fri, Jun 27, 2025 at 11:21:42AM +0100, James Clark wrote:
> In target mode, the host sending more data than can be consumed would be
> a common problem for any message exceeding the FIFO or DMA buffer size.
> Cancel the whole message as soon as this condition is hit as the message
> will be corrupted.
>
> Only do this for target mode in a DMA transfer, it's not likely these
> flags will be set in host mode
"not likely", I think SPI controller should stop CLK if FIFO empty and full.
It should be "never" happen.
Only check FIFO error flags at target mode because it never happen at host mode.
You can remove below whole paragram.
Frank
> so it's not worth adding extra checks. In
> IRQ and polling modes we use the same transfer functions for hosts and
> targets so the error flags always get checked. This is slightly
> inconsistent but it's not worth doing the check conditionally because it
> may catch some host programming errors in the future.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> drivers/spi/spi-fsl-dspi.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 46d3cae9efed..2c2a263c5276 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -478,6 +478,17 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
> dspi->dev_to_host(dspi, rxdata);
> }
>
> +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
> +{
> + if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
> + dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
> + spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
> + spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
> + return -EIO;
> + }
> + return 0;
> +}
> +
> #if IS_ENABLED(CONFIG_DMA_ENGINE)
>
> /* Prepare one TX FIFO entry (txdata plus cmd) */
> @@ -566,6 +577,7 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
> struct device *dev = &dspi->pdev->dev;
> struct fsl_dspi_dma *dma = dspi->dma;
> int time_left;
> + u32 spi_sr;
> int i;
>
> for (i = 0; i < dspi->words_in_flight; i++)
> @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>
> if (spi_controller_is_target(dspi->ctlr)) {
> wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
> - return 0;
> + regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> + return dspi_fifo_error(dspi, spi_sr);
> }
>
> time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
> @@ -1067,6 +1080,9 @@ static void dspi_poll(struct fsl_dspi *dspi)
> regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> regmap_write(dspi->regmap, SPI_SR, spi_sr);
>
> + dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
> + if (dspi->cur_msg->status)
> + return;
> if (spi_sr & SPI_SR_CMDTCF)
> break;
> }
> @@ -1085,6 +1101,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
> static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> {
> struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
> + int status;
> u32 spi_sr;
>
> regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> @@ -1093,6 +1110,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> if (!(spi_sr & SPI_SR_CMDTCF))
> return IRQ_NONE;
>
> + status = dspi_fifo_error(dspi, spi_sr);
> + if (status) {
> + if (dspi->cur_msg)
> + WRITE_ONCE(dspi->cur_msg->status, status);
> + complete(&dspi->xfer_done);
> + return IRQ_HANDLED;
> + }
> +
> dspi_rxtx(dspi);
>
> if (!dspi->len) {
>
> --
> 2.34.1
>
On Fri, Jun 27, 2025 at 03:56:43PM -0400, Frank Li wrote: > On Fri, Jun 27, 2025 at 11:21:42AM +0100, James Clark wrote: > > In target mode, the host sending more data than can be consumed would be > > a common problem for any message exceeding the FIFO or DMA buffer size. > > Cancel the whole message as soon as this condition is hit as the message > > will be corrupted. > > Only do this for target mode in a DMA transfer, it's not likely these > > flags will be set in host mode > "not likely", I think SPI controller should stop CLK if FIFO empty and full. > It should be "never" happen. > Only check FIFO error flags at target mode because it never happen at host mode. > You can remove below whole paragram. I agree it *should* never happen in host mode but it can be good practice to look in case something gets messed up. That said extra device accesses in the hot path are probably going to be noticable so likely not worth it, but in the dspi_poll() case: > @@ -1067,6 +1080,9 @@ static void dspi_poll(struct fsl_dspi *dspi) > regmap_read(dspi->regmap, SPI_SR, &spi_sr); > regmap_write(dspi->regmap, SPI_SR, spi_sr); > > + dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr); > + if (dspi->cur_msg->status) > + return; we're reading the register anyway so the overhead is much lower.
On 27/06/2025 10:41 pm, Mark Brown wrote: > On Fri, Jun 27, 2025 at 03:56:43PM -0400, Frank Li wrote: >> On Fri, Jun 27, 2025 at 11:21:42AM +0100, James Clark wrote: > >>> In target mode, the host sending more data than can be consumed would be >>> a common problem for any message exceeding the FIFO or DMA buffer size. >>> Cancel the whole message as soon as this condition is hit as the message >>> will be corrupted. > >>> Only do this for target mode in a DMA transfer, it's not likely these >>> flags will be set in host mode > >> "not likely", I think SPI controller should stop CLK if FIFO empty and full. >> It should be "never" happen. > >> Only check FIFO error flags at target mode because it never happen at host mode. > >> You can remove below whole paragram. > > I agree it *should* never happen in host mode but it can be good > practice to look in case something gets messed up. That said extra > device accesses in the hot path are probably going to be noticable so > likely not worth it, but in the dspi_poll() case: > Yeah the point was to be defensive. Even though it shouldn't happen I don't see an issue with checking error flags. We could add a condition to only do it in target mode, but it doesn't make the code more readable, and it wouldn't be any faster than just checking the flags. So I'm not sure what problem we're trying to solve by removing it. >> @@ -1067,6 +1080,9 @@ static void dspi_poll(struct fsl_dspi *dspi) >> regmap_read(dspi->regmap, SPI_SR, &spi_sr); >> regmap_write(dspi->regmap, SPI_SR, spi_sr); >> >> + dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr); >> + if (dspi->cur_msg->status) >> + return; > > we're reading the register anyway so the overhead is much lower. In both poll and interrupt mode we're already reading it. Only DMA mode didn't have a read and I didn't add a check for error flags there anyway.
© 2016 - 2025 Red Hat, Inc.