1. Patchew
  2. linux
  3. [PATCH v3 0/6] spi: spi-fsl-dspi: Target mode improvements
  4. View patch

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

James Clark posted 6 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by James Clark 3 months, 2 weeks ago 
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. 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;
 		} 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
Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by kernel test robot 3 months, 2 weeks ago 
Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 4f326fa6236787ca516ea6eab8e5e9dc5c236f03]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-dspi-Clear-completion-counter-before-initiating-transfer/20250624-183952
base:   4f326fa6236787ca516ea6eab8e5e9dc5c236f03
patch link:    https://lore.kernel.org/r/20250624-james-nxp-spi-dma-v3-6-e7d574f5f62c%40linaro.org
patch subject: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250625/202506251415.Cj026uIP-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250625/202506251415.Cj026uIP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506251415.Cj026uIP-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-dspi.c: In function 'dspi_dma_xfer':
   drivers/spi/spi-fsl-dspi.c:792:9: error: 'sdpi' undeclared (first use in this function); did you mean 'dspi'?
     792 |         sdpi->cur_msg->status = -EINVAL;
         |         ^~~~
         |         dspi
   drivers/spi/spi-fsl-dspi.c:792:9: note: each undeclared identifier is reported only once for each function it appears in
   drivers/spi/spi-fsl-dspi.c: In function 'dspi_poll':
>> drivers/spi/spi-fsl-dspi.c:1086:49: error: implicit declaration of function 'dspi_fifo_error'; did you mean 'dspi_fifo_write'? [-Wimplicit-function-declaration]
    1086 |                         dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
         |                                                 ^~~~~~~~~~~~~~~
         |                                                 dspi_fifo_write
   drivers/spi/spi-fsl-dspi.c: At top level:
   drivers/spi/spi-fsl-dspi.c:474:12: warning: 'dspi_pop_tx_pushr' defined but not used [-Wunused-function]
     474 | static u32 dspi_pop_tx_pushr(struct fsl_dspi *dspi)
         |            ^~~~~~~~~~~~~~~~~


vim +1086 drivers/spi/spi-fsl-dspi.c

  1072	
  1073	static void dspi_poll(struct fsl_dspi *dspi)
  1074	{
  1075		int tries = 1000;
  1076		u32 spi_sr;
  1077	
  1078		while (dspi->len) {
  1079			do {
  1080				regmap_read(dspi->regmap, SPI_SR, &spi_sr);
  1081				regmap_write(dspi->regmap, SPI_SR, spi_sr);
  1082	
  1083				if (spi_sr & SPI_SR_CMDTCF)
  1084					break;
  1085	
> 1086				dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
  1087				if (dspi->cur_msg->status)
  1088					return;
  1089			} while (--tries);
  1090	
  1091			if (!tries) {
  1092				dspi->cur_msg->status = -ETIMEDOUT;
  1093				return;
  1094			}
  1095	
  1096			dspi_rxtx(dspi);
  1097		}
  1098	
  1099		dspi->cur_msg->status = 0;
  1100	}
  1101	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by Frank Li 3 months, 2 weeks ago 
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.

Add checking FIFO error status at target mode in a DMA transfer since PIO
mode already do it. It help catch some host mode ...

> 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

>  		} 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
>
Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by James Clark 3 months, 2 weeks ago 

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.

>> 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
>>
Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by Frank Li 3 months, 2 weeks ago 
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
> > >
>
Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
Posted by James Clark 3 months, 1 week ago 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.