Received-SPF: pass (google.com: domain of linux-kernel+bounces-56234-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <8bce57857a1fc45f7b80b99f9d0839bf18e7fbd5.camel@gmail.com>
Subject: Re: [PATCH 1/2] spi: axi-spi-engine: remove use of ida for sync id
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:40:31 +0100
In-Reply-To: <20240206-axi-spi-engine-round-2-1-v1-1-ea6eeb60f4fb@baylibre.com>
References: 
	<20240206-axi-spi-engine-round-2-1-v1-0-ea6eeb60f4fb@baylibre.com>
	 <20240206-axi-spi-engine-round-2-1-v1-1-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:
> Profiling has shown that ida_alloc_range() accounts for about 10% of the
> time spent in spi_sync() when using the AXI SPI Engine controller. This
> call is used to create a unique id for each SPI message to match to an
> IRQ when the message is complete.
>=20
> Since the core SPI code serializes messages in a message queue, we can
> only have one message in flight at a time, namely host->cur_msg. This
> means that we can use a fixed value instead of a unique id for each
> message since there can never be more than one message pending at a
> time.
>=20
> This patch removes the use of ida for the sync id and replaces it with a
> constant value. This simplifies the driver and improves performance.
>=20
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

With the removed header:

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

(Christophe suggestion is also pretty good but I would likely do it in a
different patch - maybe we could even annotate the flex array with __counte=
d_by)

- Nuno S=C3=A1

> =C2=A0drivers/spi/spi-axi-spi-engine.c | 27 ++++++---------------------
> =C2=A01 file changed, 6 insertions(+), 21 deletions(-)
>=20
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> engine.c
> index 6b0c72bf3395..9cc602075c17 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -57,6 +57,9 @@
> =C2=A0#define SPI_ENGINE_TRANSFER_WRITE		0x1
> =C2=A0#define SPI_ENGINE_TRANSFER_READ		0x2
> =C2=A0
> +/* Arbitrary sync ID for use by host->cur_msg */
> +#define AXI_SPI_ENGINE_CUR_MSG_SYNC_ID		0x1
> +
> =C2=A0#define SPI_ENGINE_CMD(inst, arg1, arg2) \
> =C2=A0	(((inst) << 12) | ((arg1) << 8) | (arg2))
> =C2=A0
> @@ -98,8 +101,6 @@ struct spi_engine_message_state {
> =C2=A0	unsigned int rx_length;
> =C2=A0	/** @rx_buf: Bytes not yet written to the RX FIFO. */
> =C2=A0	uint8_t *rx_buf;
> -	/** @sync_id: ID to correlate SYNC interrupts with this message. */
> -	u8 sync_id;
> =C2=A0};
> =C2=A0
> =C2=A0struct spi_engine {
> @@ -109,7 +110,6 @@ struct spi_engine {
> =C2=A0	spinlock_t lock;
> =C2=A0
> =C2=A0	void __iomem *base;
> -	struct ida sync_ida;
> =C2=A0	struct timer_list watchdog_timer;
> =C2=A0	struct spi_controller *controller;
> =C2=A0
> @@ -483,9 +483,7 @@ static irqreturn_t spi_engine_irq(int irq, void *devi=
d)
> =C2=A0	}
> =C2=A0
> =C2=A0	if (pending & SPI_ENGINE_INT_SYNC && msg) {
> -		struct spi_engine_message_state *st =3D msg->state;
> -
> -		if (completed_id =3D=3D st->sync_id) {
> +		if (completed_id =3D=3D AXI_SPI_ENGINE_CUR_MSG_SYNC_ID) {
> =C2=A0			if (timer_delete_sync(&spi_engine->watchdog_timer)) {
> =C2=A0				msg->status =3D 0;
> =C2=A0				msg->actual_length =3D msg->frame_length;
> @@ -510,10 +508,8 @@ static int spi_engine_prepare_message(struct
> spi_controller *host,
> =C2=A0				=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct spi_message *msg)
> =C2=A0{
> =C2=A0	struct spi_engine_program p_dry, *p;
> -	struct spi_engine *spi_engine =3D spi_controller_get_devdata(host);
> =C2=A0	struct spi_engine_message_state *st;
> =C2=A0	size_t size;
> -	int ret;
> =C2=A0
> =C2=A0	st =3D kzalloc(sizeof(*st), GFP_KERNEL);
> =C2=A0	if (!st)
> @@ -531,18 +527,10 @@ static int spi_engine_prepare_message(struct
> spi_controller *host,
> =C2=A0		return -ENOMEM;
> =C2=A0	}
> =C2=A0
> -	ret =3D ida_alloc_range(&spi_engine->sync_ida, 0, U8_MAX, GFP_KERNEL);
> -	if (ret < 0) {
> -		kfree(p);
> -		kfree(st);
> -		return ret;
> -	}
> -
> -	st->sync_id =3D ret;
> -
> =C2=A0	spi_engine_compile_message(msg, false, p);
> =C2=A0
> -	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(st-
> >sync_id));
> +	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(
> +						AXI_SPI_ENGINE_CUR_MSG_SYNC_I
> D));
> =C2=A0
> =C2=A0	st->p =3D p;
> =C2=A0	st->cmd_buf =3D p->instructions;
> @@ -555,10 +543,8 @@ static int spi_engine_prepare_message(struct
> spi_controller *host,
> =C2=A0static int spi_engine_unprepare_message(struct spi_controller *host=
,
> =C2=A0					struct spi_message *msg)
> =C2=A0{
> -	struct spi_engine *spi_engine =3D spi_controller_get_devdata(host);
> =C2=A0	struct spi_engine_message_state *st =3D msg->state;
> =C2=A0
> -	ida_free(&spi_engine->sync_ida, st->sync_id);
> =C2=A0	kfree(st->p);
> =C2=A0	kfree(st);
> =C2=A0
> @@ -640,7 +626,6 @@ 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);
> -	ida_init(&spi_engine->sync_ida);
> =C2=A0	timer_setup(&spi_engine->watchdog_timer, spi_engine_timeout,
> TIMER_IRQSAFE);
> =C2=A0	spi_engine->controller =3D host;
> =C2=A0
>=20