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

[PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status

James Clark posted 6 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status
Posted by James Clark 3 months, 2 weeks ago 
This will allow us to return a status from the interrupt handler in a
later commit and avoids copying it at the end of
dspi_transfer_one_message(). For consistency make polling and DMA modes
use the same mechanism.

Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
this isn't actually a status that was ever returned to the core layer
but some internal state. Wherever that was used we can look at dspi->len
instead.

No functional changes intended.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 744dfc561db2..feb29bb92a77 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 static void dspi_setup_accel(struct fsl_dspi *dspi);
 
-static int dspi_dma_xfer(struct fsl_dspi *dspi)
+static void dspi_dma_xfer(struct fsl_dspi *dspi)
 {
 	struct spi_message *message = dspi->cur_msg;
 	struct device *dev = &dspi->pdev->dev;
-	int ret = 0;
 
 	/*
 	 * dspi->len gets decremented by dspi_pop_tx_pushr in
@@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 		message->actual_length += dspi->words_in_flight *
 					  dspi->oper_word_size;
 
-		ret = dspi_next_xfer_dma_submit(dspi);
-		if (ret) {
+		message->status = dspi_next_xfer_dma_submit(dspi);
+		if (message->status) {
 			dev_err(dev, "DMA transfer failed\n");
 			break;
 		}
 	}
-
-	return ret;
 }
 
 static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
@@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
 				dspi->progress, !dspi->irq);
 }
 
-static int dspi_rxtx(struct fsl_dspi *dspi)
+static void dspi_rxtx(struct fsl_dspi *dspi)
 {
 	dspi_fifo_read(dspi);
 
 	if (!dspi->len)
 		/* Success! */
-		return 0;
+		return;
 
 	dspi_fifo_write(dspi);
-
-	return -EINPROGRESS;
 }
 
-static int dspi_poll(struct fsl_dspi *dspi)
+static void dspi_poll(struct fsl_dspi *dspi)
 {
 	int tries = 1000;
 	u32 spi_sr;
 
-	do {
-		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
-		regmap_write(dspi->regmap, SPI_SR, spi_sr);
+	while (dspi->len) {
+		do {
+			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+			regmap_write(dspi->regmap, SPI_SR, spi_sr);
 
-		if (spi_sr & SPI_SR_CMDTCF)
-			break;
-	} while (--tries);
+			if (spi_sr & SPI_SR_CMDTCF)
+				break;
+		} while (--tries);
 
-	if (!tries)
-		return -ETIMEDOUT;
+		if (!tries) {
+			dspi->cur_msg->status = -ETIMEDOUT;
+			return;
+		}
 
-	return dspi_rxtx(dspi);
+		dspi_rxtx(dspi);
+	}
+
+	dspi->cur_msg->status = 0;
 }
 
 static irqreturn_t dspi_interrupt(int irq, void *dev_id)
@@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	if (!(spi_sr & SPI_SR_CMDTCF))
 		return IRQ_NONE;
 
-	if (dspi_rxtx(dspi) == 0)
+	dspi_rxtx(dspi);
+
+	if (!dspi->len) {
+		if (dspi->cur_msg)
+			WRITE_ONCE(dspi->cur_msg->status, 0);
 		complete(&dspi->xfer_done);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 	struct spi_device *spi = message->spi;
 	struct spi_transfer *transfer;
 	bool cs = false;
-	int status = 0;
 	u32 val = 0;
 	bool cs_change = false;
 
@@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 				       dspi->progress, !dspi->irq);
 
 		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
-			status = dspi_dma_xfer(dspi);
+			dspi_dma_xfer(dspi);
 		} else {
 			/*
 			 * Reset completion counter to clear any extra
@@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 
 			dspi_fifo_write(dspi);
 
-			if (dspi->irq) {
+			if (dspi->irq)
 				wait_for_completion(&dspi->xfer_done);
-			} else {
-				do {
-					status = dspi_poll(dspi);
-				} while (status == -EINPROGRESS);
-			}
+			else
+				dspi_poll(dspi);
 		}
-		if (status)
+		if (READ_ONCE(message->status))
 			break;
 
 		spi_transfer_delay_exec(transfer);
@@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 			dspi_deassert_cs(spi, &cs);
 	}
 
-	if (status || !cs_change) {
+	dspi->cur_msg = NULL;
+	if (message->status || !cs_change) {
 		/* Put DSPI in stop mode */
 		regmap_update_bits(dspi->regmap, SPI_MCR,
 				   SPI_MCR_HALT, SPI_MCR_HALT);
@@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 			;
 	}
 
-	message->status = status;
 	spi_finalize_current_message(ctlr);
 
-	return status;
+	return message->status;
 }
 
 static int dspi_set_mtf(struct fsl_dspi *dspi)

-- 
2.34.1
Re: [PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status
Posted by Frank Li 3 months, 2 weeks ago 
On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
> This will allow us to return a status from the interrupt handler in a
> later commit and avoids copying it at the end of
> dspi_transfer_one_message(). For consistency make polling and DMA modes
> use the same mechanism.
>
> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
> this isn't actually a status that was ever returned to the core layer
> but some internal state. Wherever that was used we can look at dspi->len
> instead.
>
> No functional changes intended.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 744dfc561db2..feb29bb92a77 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>
>  static void dspi_setup_accel(struct fsl_dspi *dspi);
>
> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>  {
>  	struct spi_message *message = dspi->cur_msg;
>  	struct device *dev = &dspi->pdev->dev;
> -	int ret = 0;
>
>  	/*
>  	 * dspi->len gets decremented by dspi_pop_tx_pushr in
> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>  		message->actual_length += dspi->words_in_flight *
>  					  dspi->oper_word_size;
>
> -		ret = dspi_next_xfer_dma_submit(dspi);
> -		if (ret) {
> +		message->status = dspi_next_xfer_dma_submit(dspi);
> +		if (message->status) {
>  			dev_err(dev, "DMA transfer failed\n");
>  			break;
>  		}
>  	}
> -
> -	return ret;
>  }
>
>  static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>  				dspi->progress, !dspi->irq);
>  }
>
> -static int dspi_rxtx(struct fsl_dspi *dspi)
> +static void dspi_rxtx(struct fsl_dspi *dspi)
>  {
>  	dspi_fifo_read(dspi);
>
>  	if (!dspi->len)
>  		/* Success! */
> -		return 0;
> +		return;
>
>  	dspi_fifo_write(dspi);
> -
> -	return -EINPROGRESS;
>  }
>
> -static int dspi_poll(struct fsl_dspi *dspi)
> +static void dspi_poll(struct fsl_dspi *dspi)
>  {
>  	int tries = 1000;
>  	u32 spi_sr;
>
> -	do {
> -		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> -		regmap_write(dspi->regmap, SPI_SR, spi_sr);
> +	while (dspi->len) {

Preivous have not checked dspi->len.

Not sure if it is logical equivalence

> +		do {
> +			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> +			regmap_write(dspi->regmap, SPI_SR, spi_sr);
>
> -		if (spi_sr & SPI_SR_CMDTCF)
> -			break;
> -	} while (--tries);
> +			if (spi_sr & SPI_SR_CMDTCF)
> +				break;
> +		} while (--tries);
>
> -	if (!tries)
> -		return -ETIMEDOUT;
> +		if (!tries) {
> +			dspi->cur_msg->status = -ETIMEDOUT;
> +			return;
> +		}
>
> -	return dspi_rxtx(dspi);
> +		dspi_rxtx(dspi);
> +	}
> +
> +	dspi->cur_msg->status = 0;
>  }
>
>  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>  	if (!(spi_sr & SPI_SR_CMDTCF))
>  		return IRQ_NONE;
>
> -	if (dspi_rxtx(dspi) == 0)
> +	dspi_rxtx(dspi);
> +
> +	if (!dspi->len) {
> +		if (dspi->cur_msg)
> +			WRITE_ONCE(dspi->cur_msg->status, 0);
>  		complete(&dspi->xfer_done);
> +	}
>
>  	return IRQ_HANDLED;
>  }
> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  	struct spi_device *spi = message->spi;
>  	struct spi_transfer *transfer;
>  	bool cs = false;
> -	int status = 0;
>  	u32 val = 0;
>  	bool cs_change = false;
>
> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  				       dspi->progress, !dspi->irq);
>
>  		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
> -			status = dspi_dma_xfer(dspi);
> +			dspi_dma_xfer(dspi);
>  		} else {
>  			/*
>  			 * Reset completion counter to clear any extra
> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>
>  			dspi_fifo_write(dspi);
>
> -			if (dspi->irq) {
> +			if (dspi->irq)
>  				wait_for_completion(&dspi->xfer_done);
> -			} else {
> -				do {
> -					status = dspi_poll(dspi);
> -				} while (status == -EINPROGRESS);
> -			}
> +			else
> +				dspi_poll(dspi);
>  		}
> -		if (status)
> +		if (READ_ONCE(message->status))

Why need READ_ONCE()? Does any hardware (DMA) set status?

where update message->status at pio mode.

Frank
>  			break;
>
>  		spi_transfer_delay_exec(transfer);
> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			dspi_deassert_cs(spi, &cs);
>  	}
>
> -	if (status || !cs_change) {
> +	dspi->cur_msg = NULL;
> +	if (message->status || !cs_change) {
>  		/* Put DSPI in stop mode */
>  		regmap_update_bits(dspi->regmap, SPI_MCR,
>  				   SPI_MCR_HALT, SPI_MCR_HALT);
> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			;
>  	}
>
> -	message->status = status;
>  	spi_finalize_current_message(ctlr);
>
> -	return status;
> +	return message->status;
>  }
>
>  static int dspi_set_mtf(struct fsl_dspi *dspi)
>
> --
> 2.34.1
>
Re: [PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status
Posted by James Clark 3 months, 2 weeks ago 

On 24/06/2025 5:18 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
>> This will allow us to return a status from the interrupt handler in a
>> later commit and avoids copying it at the end of
>> dspi_transfer_one_message(). For consistency make polling and DMA modes
>> use the same mechanism.
>>
>> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
>> this isn't actually a status that was ever returned to the core layer
>> but some internal state. Wherever that was used we can look at dspi->len
>> instead.
>>
>> No functional changes intended.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>>   1 file changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 744dfc561db2..feb29bb92a77 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>
>>   static void dspi_setup_accel(struct fsl_dspi *dspi);
>>
>> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
>> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>>   {
>>   	struct spi_message *message = dspi->cur_msg;
>>   	struct device *dev = &dspi->pdev->dev;
>> -	int ret = 0;
>>
>>   	/*
>>   	 * dspi->len gets decremented by dspi_pop_tx_pushr in
>> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>>   		message->actual_length += dspi->words_in_flight *
>>   					  dspi->oper_word_size;
>>
>> -		ret = dspi_next_xfer_dma_submit(dspi);
>> -		if (ret) {
>> +		message->status = dspi_next_xfer_dma_submit(dspi);
>> +		if (message->status) {
>>   			dev_err(dev, "DMA transfer failed\n");
>>   			break;
>>   		}
>>   	}
>> -
>> -	return ret;
>>   }
>>
>>   static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>>   				dspi->progress, !dspi->irq);
>>   }
>>
>> -static int dspi_rxtx(struct fsl_dspi *dspi)
>> +static void dspi_rxtx(struct fsl_dspi *dspi)
>>   {
>>   	dspi_fifo_read(dspi);
>>
>>   	if (!dspi->len)
>>   		/* Success! */
>> -		return 0;
>> +		return;
>>
>>   	dspi_fifo_write(dspi);
>> -
>> -	return -EINPROGRESS;
>>   }
>>
>> -static int dspi_poll(struct fsl_dspi *dspi)
>> +static void dspi_poll(struct fsl_dspi *dspi)
>>   {
>>   	int tries = 1000;
>>   	u32 spi_sr;
>>
>> -	do {
>> -		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> -		regmap_write(dspi->regmap, SPI_SR, spi_sr);
>> +	while (dspi->len) {
> 
> Preivous have not checked dspi->len.
> 
> Not sure if it is logical equivalence
> 

It used to be this:

    status = dspi_poll(dspi);
  } while (status == -EINPROGRESS);

Where checking status for -EINPROGRESS is equivalent to checking dspi->len.

>> +		do {
>> +			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> +			regmap_write(dspi->regmap, SPI_SR, spi_sr);
>>
>> -		if (spi_sr & SPI_SR_CMDTCF)
>> -			break;
>> -	} while (--tries);
>> +			if (spi_sr & SPI_SR_CMDTCF)
>> +				break;
>> +		} while (--tries);
>>
>> -	if (!tries)
>> -		return -ETIMEDOUT;
>> +		if (!tries) {
>> +			dspi->cur_msg->status = -ETIMEDOUT;
>> +			return;
>> +		}
>>
>> -	return dspi_rxtx(dspi);
>> +		dspi_rxtx(dspi);
>> +	}
>> +
>> +	dspi->cur_msg->status = 0;
>>   }
>>
>>   static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   	if (!(spi_sr & SPI_SR_CMDTCF))
>>   		return IRQ_NONE;
>>
>> -	if (dspi_rxtx(dspi) == 0)
>> +	dspi_rxtx(dspi);
>> +
>> +	if (!dspi->len) {
>> +		if (dspi->cur_msg)
>> +			WRITE_ONCE(dspi->cur_msg->status, 0);
>>   		complete(&dspi->xfer_done);
>> +	}
>>
>>   	return IRQ_HANDLED;
>>   }
>> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   	struct spi_device *spi = message->spi;
>>   	struct spi_transfer *transfer;
>>   	bool cs = false;
>> -	int status = 0;
>>   	u32 val = 0;
>>   	bool cs_change = false;
>>
>> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   				       dspi->progress, !dspi->irq);
>>
>>   		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
>> -			status = dspi_dma_xfer(dspi);
>> +			dspi_dma_xfer(dspi);
>>   		} else {
>>   			/*
>>   			 * Reset completion counter to clear any extra
>> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>
>>   			dspi_fifo_write(dspi);
>>
>> -			if (dspi->irq) {
>> +			if (dspi->irq)
>>   				wait_for_completion(&dspi->xfer_done);
>> -			} else {
>> -				do {
>> -					status = dspi_poll(dspi);
>> -				} while (status == -EINPROGRESS);
>> -			}
>> +			else
>> +				dspi_poll(dspi);
>>   		}
>> -		if (status)
>> +		if (READ_ONCE(message->status))
> 
> Why need READ_ONCE()? Does any hardware (DMA) set status?
> 
> where update message->status at pio mode.
> 
> Frank

Because message->status is set from dspi_interrupt(), so the compiler 
wouldn't be aware that this variable can be changed from elsewhere. 
Otherwise it could assume that it's never written to in 
dspi_transfer_one_message() and not bother to read it again.

>>   			break;
>>
>>   		spi_transfer_delay_exec(transfer);
>> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   			dspi_deassert_cs(spi, &cs);
>>   	}
>>
>> -	if (status || !cs_change) {
>> +	dspi->cur_msg = NULL;
>> +	if (message->status || !cs_change) {
>>   		/* Put DSPI in stop mode */
>>   		regmap_update_bits(dspi->regmap, SPI_MCR,
>>   				   SPI_MCR_HALT, SPI_MCR_HALT);
>> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   			;
>>   	}
>>
>> -	message->status = status;
>>   	spi_finalize_current_message(ctlr);
>>
>> -	return status;
>> +	return message->status;
>>   }
>>
>>   static int dspi_set_mtf(struct fsl_dspi *dspi)
>>
>> --
>> 2.34.1
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.