Received-SPF: pass (google.com: domain of linux-kernel+bounces-56207-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <07371805d3ec96d803a98b5735a114b3af56365a.camel@gmail.com>
Subject: Re: [PATCH 2/2] spi: axi-spi-engine: move msg finalization out of
 irq handler
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>, linux-spi@vger.kernel.org
Cc: Mark Brown <broonie@kernel.org>, Michael Hennerich
 <michael.hennerich@analog.com>, Nuno =?ISO-8859-1?Q?S=E1?=
 <nuno.sa@analog.com>,  linux-kernel@vger.kernel.org
Date: Wed, 07 Feb 2024 10:33:11 +0100
In-Reply-To: <20240206-axi-spi-engine-round-2-1-v1-2-ea6eeb60f4fb@baylibre.com>
References: 
	<20240206-axi-spi-engine-round-2-1-v1-0-ea6eeb60f4fb@baylibre.com>
	 <20240206-axi-spi-engine-round-2-1-v1-2-ea6eeb60f4fb@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> 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.
>=20
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Nice one...

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

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