Re: [PATCH 2/2] spi: axi-spi-engine: move msg finalization out of irq handler

[Nuno Sá]

On Tue, 2024-02-06 at 14:31 -0600, David Lechner wrote:
> As a general principal, it is best to do as little as possible in an
> interrupt handler. This patch reworks the AXI SPI Engine driver to move
> timer_delete_sync() and spi_finalize_current_message() out of the
> interrupt handler. Instead, spi_finalize_current_message() is moved to
> the transfer_one_message function (similar to nearly all other SPI
> controllers). A completion is now used to wait for the sync interrupt
> that indicates that the message is complete. The watchdog timer is no
> longer needed since we can use the wait_for_completion_timeout()
> function to wait for the message to complete with the same effect.
> 
> As a bonus, these changes also improve throughput of the SPI controller.
> For example, this was tested on a ZynqMP with a 80MHz SCLK reading 4
> byte samples from an ADC. The max measured throughput increased from
> 26k to 28k samples per second.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---

Nice one...

Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>

>  drivers/spi/spi-axi-spi-engine.c | 40 +++++++++++++++------------------------
> -
>  1 file changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> engine.c
> index 9cc602075c17..3c96aa9232b3 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/completion.h>
>  #include <linux/fpga/adi-axi-common.h>
>  #include <linux/idr.h>
>  #include <linux/interrupt.h>
> @@ -14,7 +15,6 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/spi/spi.h>
> -#include <linux/timer.h>
>  
>  #define SPI_ENGINE_REG_RESET			0x40
>  
> @@ -110,9 +110,7 @@ struct spi_engine {
>  	spinlock_t lock;
>  
>  	void __iomem *base;
> -	struct timer_list watchdog_timer;
> -	struct spi_controller *controller;
> -
> +	struct completion msg_complete;
>  	unsigned int int_enable;
>  };
>  
> @@ -484,11 +482,9 @@ static irqreturn_t spi_engine_irq(int irq, void *devid)
>  
>  	if (pending & SPI_ENGINE_INT_SYNC && msg) {
>  		if (completed_id == AXI_SPI_ENGINE_CUR_MSG_SYNC_ID) {
> -			if (timer_delete_sync(&spi_engine->watchdog_timer)) {
> -				msg->status = 0;
> -				msg->actual_length = msg->frame_length;
> -				spi_finalize_current_message(host);
> -			}
> +			msg->status = 0;
> +			msg->actual_length = msg->frame_length;
> +			complete(&spi_engine->msg_complete);
>  			disable_int |= SPI_ENGINE_INT_SYNC;
>  		}
>  	}
> @@ -559,7 +555,7 @@ static int spi_engine_transfer_one_message(struct
> spi_controller *host,
>  	unsigned int int_enable = 0;
>  	unsigned long flags;
>  
> -	mod_timer(&spi_engine->watchdog_timer, jiffies +
> msecs_to_jiffies(5000));
> +	reinit_completion(&spi_engine->msg_complete);
>  
>  	spin_lock_irqsave(&spi_engine->lock, flags);
>  
> @@ -581,21 +577,16 @@ static int spi_engine_transfer_one_message(struct
> spi_controller *host,
>  	spi_engine->int_enable = int_enable;
>  	spin_unlock_irqrestore(&spi_engine->lock, flags);
>  
> -	return 0;
> -}
> -
> -static void spi_engine_timeout(struct timer_list *timer)
> -{
> -	struct spi_engine *spi_engine = from_timer(spi_engine, timer,
> watchdog_timer);
> -	struct spi_controller *host = spi_engine->controller;
> -
> -	if (WARN_ON(!host->cur_msg))
> -		return;
> +	if (!wait_for_completion_timeout(&spi_engine->msg_complete,
> +					 msecs_to_jiffies(5000))) {
> +		dev_err(&host->dev,
> +			"Timeout occurred while waiting for transfer to
> complete. Hardware is probably broken.\n");
> +		msg->status = -ETIMEDOUT;
> +	}
>  
> -	dev_err(&host->dev,
> -		"Timeout occurred while waiting for transfer to complete.
> Hardware is probably broken.\n");
> -	host->cur_msg->status = -ETIMEDOUT;
>  	spi_finalize_current_message(host);
> +
> +	return msg->status;
>  }
>  
>  static void spi_engine_release_hw(void *p)
> @@ -626,8 +617,7 @@ static int spi_engine_probe(struct platform_device *pdev)
>  	spi_engine = spi_controller_get_devdata(host);
>  
>  	spin_lock_init(&spi_engine->lock);
> -	timer_setup(&spi_engine->watchdog_timer, spi_engine_timeout,
> TIMER_IRQSAFE);
> -	spi_engine->controller = host;
> +	init_completion(&spi_engine->msg_complete);
>  
>  	spi_engine->clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
>  	if (IS_ERR(spi_engine->clk))
>