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

[James Clark <james.clark@linaro.org>]

On 25/06/2025 3:55 pm, Frank Li wrote:
> 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
> 
> 

Got it, thanks

>>
>>>> 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
>>>>
>>