Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[Frank Li <Frank.li@nxp.com>]

On Wed, Jun 25, 2025 at 11:09:57AM +0100, James Clark wrote:
>
>
> On 24/06/2025 5:50 pm, Frank Li wrote:
> > On Tue, Jun 24, 2025 at 11:35:36AM +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 because we need to add a
> > > register read.
> >
> > "We need to add a register read" is not reason.
> >
>
> Maybe: "It's not likely to catch any errors in host mode so optimize by
> avoiding an extra register read"?
>
> > Add checking FIFO error status at target mode in a DMA transfer since PIO
> > mode already do it. It help catch some host mode ...
> >
>
> Are you suggesting that we check for FIFO errors in host mode too? It
> requires an extra read and check in dspi_tx_dma_callback(), but I'm not sure
> what it could catch. Realistically as long as everything is setup correctly
> then neither of those flags will be set. It will either always work or never
> work.
>
> It might be better to add it later if a use becomes apparent otherwise it's
> extra noise in the code.

I think your origial last phrase is not good enough. You may rephrase it
to make it clear.

for example: according to your patch

"Only do this for target mode in a DMA transfer because we need to add a register read."

"add a register read" is result, not a reason. the reason should be "you want
host side capture such error."

Frank


>
> > > In IRQ and polling modes always do it because SPI_SR was
> > > already read and it might catch some host mode programming/buffer
> > > management errors too.
> > >
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
> > >   1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > > index 58881911e74a..16a9769f518d 100644
> > > --- a/drivers/spi/spi-fsl-dspi.c
> > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
> > >   	complete(&dma->cmd_rx_complete);
> > >   }
> > >
> > > +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;
> > > +}
> > > +
> > >   static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
> > >   {
> > >   	size_t size = dspi_dma_transfer_size(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,
> > > @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
> > >
> > >   			if (spi_sr & SPI_SR_CMDTCF)
> > >   				break;
> > > +
> > > +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
> > > +			if (dspi->cur_msg->status)
> > > +				return;
> >
> >
> > Although fifo error may happen after you check, it may reduce some possilbity
> > and catch some problems.
> >
> > Frak
> >
>
> Not sure what you mean by this one. But I've seen a few small errors now
> that I look again. The error check should be before the transfer complete
> break. And tries should be reset for each part of the message.
>
> > >   		} while (--tries);
> > >
> > >   		if (!tries) {
> > > @@ -1085,6 +1102,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 +1111,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
> > >
>