Received-SPF: pass (google.com: domain of linux-kernel+bounces-72854-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <42bc659ddda05f3d4ece7e4a65e4548da17d17ad.camel@gmail.com>
Subject: Re: [PATCH v2 1/5] spi: add spi_optimize_message() APIs
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>, Mark Brown <broonie@kernel.org>
Cc: Martin Sperl <kernel@martin.sperl.org>, David Jander
 <david@protonic.nl>,  Jonathan Cameron <jic23@kernel.org>, Michael
 Hennerich <michael.hennerich@analog.com>, Nuno =?ISO-8859-1?Q?S=E1?=
 <nuno.sa@analog.com>, Alain Volmat <alain.volmat@foss.st.com>, Maxime
 Coquelin <mcoquelin.stm32@gmail.com>, Alexandre Torgue
 <alexandre.torgue@foss.st.com>,  linux-spi@vger.kernel.org,
 linux-kernel@vger.kernel.org,  linux-stm32@st-md-mailman.stormreply.com,
 linux-arm-kernel@lists.infradead.org,  linux-iio@vger.kernel.org
Date: Tue, 20 Feb 2024 11:50:40 +0100
In-Reply-To: <20240219-mainline-spi-precook-message-v2-1-4a762c6701b9@baylibre.com>
References: 
	<20240219-mainline-spi-precook-message-v2-0-4a762c6701b9@baylibre.com>
	 <20240219-mainline-spi-precook-message-v2-1-4a762c6701b9@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-02-19 at 16:33 -0600, David Lechner wrote:
> This adds a new spi_optimize_message() function that can be used to
> optimize SPI messages that are used more than once. Peripheral drivers
> that use the same message multiple times can use this API to perform SPI
> message validation and controller-specific optimizations once and then
> reuse the message while avoiding the overhead of revalidating the
> message on each spi_(a)sync() call.
>=20
> Internally, the SPI core will also call this function for each message
> if the peripheral driver did not explicitly call it. This is done to so
> that controller drivers don't have to have multiple code paths for
> optimized and non-optimized messages.
>=20
> A hook is provided for controller drivers to perform controller-specific
> optimizations.
>=20
> Suggested-by: Martin Sperl <kernel@martin.sperl.org>
> Link:
> https://lore.kernel.org/linux-spi/39DEC004-10A1-47EF-9D77-276188D2580C@ma=
rtin.sperl.org/
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

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

>=20
> v2 changes:
> - Removed pre_optimized parameter from __spi_optimize_message()
> - Added comment explaining purpose of pre_optimized flag
> - Fixed missing doc comment for @pre_optimized
> - Removed kernel doc inclusion (/** -> /*) from static members
> - Removed unrelated comment about calling spi_finalize_current_message()
>=20
> =C2=A0drivers/spi/spi.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 151 +++++++=
+++++++++++++++++++++++++++++++++++++++--
> =C2=A0include/linux/spi/spi.h |=C2=A0 20 +++++++
> =C2=A02 files changed, 167 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index c2b10e2c75f0..f68d92b57543 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2106,6 +2106,41 @@ struct spi_message *spi_get_next_queued_message(st=
ruct
> spi_controller *ctlr)
> =C2=A0}
> =C2=A0EXPORT_SYMBOL_GPL(spi_get_next_queued_message);
> =C2=A0
> +/*
> + * __spi_unoptimize_message - shared implementation of spi_unoptimize_me=
ssage()
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 and spi_maybe_unoptimize_message()
> + * @msg: the message to unoptimize
> + *
> + * Peripheral drivers should use spi_unoptimize_message() and callers in=
side
> + * core should use spi_maybe_unoptimize_message() rather than calling th=
is
> + * function directly.
> + *
> + * It is not valid to call this on a message that is not currently optim=
ized.
> + */
> +static void __spi_unoptimize_message(struct spi_message *msg)
> +{
> +	struct spi_controller *ctlr =3D msg->spi->controller;
> +
> +	if (ctlr->unoptimize_message)
> +		ctlr->unoptimize_message(msg);
> +
> +	msg->optimized =3D false;
> +	msg->opt_state =3D NULL;
> +}
> +
> +/*
> + * spi_maybe_unoptimize_message - unoptimize msg not managed by a periph=
eral
> + * @msg: the message to unoptimize
> + *
> + * This function is used to unoptimize a message if and only if it was
> + * optimized by the core (via spi_maybe_optimize_message()).
> + */
> +static void spi_maybe_unoptimize_message(struct spi_message *msg)
> +{
> +	if (!msg->pre_optimized && msg->optimized)
> +		__spi_unoptimize_message(msg);
> +}
> +
> =C2=A0/**
> =C2=A0 * spi_finalize_current_message() - the current message is complete
> =C2=A0 * @ctlr: the controller to return the message to
> @@ -2153,6 +2188,8 @@ void spi_finalize_current_message(struct spi_contro=
ller
> *ctlr)
> =C2=A0
> =C2=A0	mesg->prepared =3D false;
> =C2=A0
> +	spi_maybe_unoptimize_message(mesg);
> +
> =C2=A0	WRITE_ONCE(ctlr->cur_msg_incomplete, false);
> =C2=A0	smp_mb(); /* See __spi_pump_transfer_message()... */
> =C2=A0	if (READ_ONCE(ctlr->cur_msg_need_completion))
> @@ -4194,6 +4231,110 @@ static int __spi_validate(struct spi_device *spi,=
 struct
> spi_message *message)
> =C2=A0	return 0;
> =C2=A0}
> =C2=A0
> +/*
> + * __spi_optimize_message - shared implementation for spi_optimize_messa=
ge()
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 and spi_maybe_optimize_message()
> + * @spi: the device that will be used for the message
> + * @msg: the message to optimize
> + *
> + * Peripheral drivers will call spi_optimize_message() and the spi core =
will
> + * call spi_maybe_optimize_message() instead of calling this directly.
> + *
> + * It is not valid to call this on a message that has already been optim=
ized.
> + *
> + * Return: zero on success, else a negative error code
> + */
> +static int __spi_optimize_message(struct spi_device *spi,
> +				=C2=A0 struct spi_message *msg)
> +{
> +	struct spi_controller *ctlr =3D spi->controller;
> +	int ret;
> +
> +	ret =3D __spi_validate(spi, msg);
> +	if (ret)
> +		return ret;
> +
> +	if (ctlr->optimize_message) {
> +		ret =3D ctlr->optimize_message(msg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	msg->optimized =3D true;
> +
> +	return 0;
> +}
> +
> +/*
> + * spi_maybe_optimize_message - optimize message if it isn't already pre=
-optimized
> + * @spi: the device that will be used for the message
> + * @msg: the message to optimize
> + * Return: zero on success, else a negative error code
> + */
> +static int spi_maybe_optimize_message(struct spi_device *spi,
> +				=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct spi_message *msg)
> +{
> +	if (msg->pre_optimized)
> +		return 0;
> +
> +	return __spi_optimize_message(spi, msg);
> +}
> +
> +/**
> + * spi_optimize_message - do any one-time validation and setup for a SPI=
 message
> + * @spi: the device that will be used for the message
> + * @msg: the message to optimize
> + *
> + * Peripheral drivers that reuse the same message repeatedly may call th=
is to
> + * perform as much message prep as possible once, rather than repeating =
it each
> + * time a message transfer is performed to improve throughput and reduce=
 CPU
> + * usage.
> + *
> + * Once a message has been optimized, it cannot be modified with the exc=
eption
> + * of updating the contents of any xfer->tx_buf (the pointer can't be ch=
anged,
> + * only the data in the memory it points to).
> + *
> + * Calls to this function must be balanced with calls to spi_unoptimize_=
message()
> + * to avoid leaking resources.
> + *
> + * Context: can sleep
> + * Return: zero on success, else a negative error code
> + */
> +int spi_optimize_message(struct spi_device *spi, struct spi_message *msg=
)
> +{
> +	int ret;
> +
> +	ret =3D __spi_optimize_message(spi, msg);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * This flag indicates that the peripheral driver called
> spi_optimize_message()
> +	 * and therefore we shouldn't unoptimize message automatically when
> finalizing
> +	 * the message but rather wait until spi_unoptimize_message() is called
> +	 * by the peripheral driver.
> +	 */
> +	msg->pre_optimized =3D true;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_optimize_message);
> +
> +/**
> + * spi_unoptimize_message - releases any resources allocated by
> spi_optimize_message()
> + * @msg: the message to unoptimize
> + *
> + * Calls to this function must be balanced with calls to spi_optimize_me=
ssage().
> + *
> + * Context: can sleep
> + */
> +void spi_unoptimize_message(struct spi_message *msg)
> +{
> +	__spi_unoptimize_message(msg);
> +	msg->pre_optimized =3D false;
> +}
> +EXPORT_SYMBOL_GPL(spi_unoptimize_message);
> +
> =C2=A0static int __spi_async(struct spi_device *spi, struct spi_message *=
message)
> =C2=A0{
> =C2=A0	struct spi_controller *ctlr =3D spi->controller;
> @@ -4258,8 +4399,8 @@ int spi_async(struct spi_device *spi, struct spi_me=
ssage
> *message)
> =C2=A0	int ret;
> =C2=A0	unsigned long flags;
> =C2=A0
> -	ret =3D __spi_validate(spi, message);
> -	if (ret !=3D 0)
> +	ret =3D spi_maybe_optimize_message(spi, message);
> +	if (ret)
> =C2=A0		return ret;
> =C2=A0
> =C2=A0	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
> @@ -4271,6 +4412,8 @@ int spi_async(struct spi_device *spi, struct spi_me=
ssage
> *message)
> =C2=A0
> =C2=A0	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
> =C2=A0
> +	spi_maybe_unoptimize_message(message);
> +
> =C2=A0	return ret;
> =C2=A0}
> =C2=A0EXPORT_SYMBOL_GPL(spi_async);
> @@ -4331,8 +4474,8 @@ static int __spi_sync(struct spi_device *spi, struc=
t
> spi_message *message)
> =C2=A0		return -ESHUTDOWN;
> =C2=A0	}
> =C2=A0
> -	status =3D __spi_validate(spi, message);
> -	if (status !=3D 0)
> +	status =3D spi_maybe_optimize_message(spi, message);
> +	if (status)
> =C2=A0		return status;
> =C2=A0
> =C2=A0	SPI_STATISTICS_INCREMENT_FIELD(ctlr->pcpu_statistics, spi_sync);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 2b8e2746769a..ddfb66dd4caf 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -475,6 +475,8 @@ extern struct spi_device *spi_new_ancillary_device(st=
ruct
> spi_device *spi, u8 ch
> =C2=A0 *
> =C2=A0 * @set_cs: set the logic level of the chip select line.=C2=A0 May =
be called
> =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 from inter=
rupt context.
> + * @optimize_message: optimize the message for reuse
> + * @unoptimize_message: release resources allocated by optimize_message
> =C2=A0 * @prepare_message: set up the controller to transfer a single mes=
sage,
> =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for example doing DMA mapping=
=C2=A0 Called from threaded
> =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 context.
> @@ -715,6 +717,8 @@ struct spi_controller {
> =C2=A0	struct completion=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 xfer_completion;
> =C2=A0	size_t				max_dma_len;
> =C2=A0
> +	int (*optimize_message)(struct spi_message *msg);
> +	int (*unoptimize_message)(struct spi_message *msg);
> =C2=A0	int (*prepare_transfer_hardware)(struct spi_controller *ctlr);
> =C2=A0	int (*transfer_one_message)(struct spi_controller *ctlr,
> =C2=A0				=C2=A0=C2=A0=C2=A0 struct spi_message *mesg);
> @@ -1111,6 +1115,8 @@ struct spi_transfer {
> =C2=A0 * @spi: SPI device to which the transaction is queued
> =C2=A0 * @is_dma_mapped: if true, the caller provided both DMA and CPU vi=
rtual
> =C2=A0 *	addresses for each transfer buffer
> + * @pre_optimized: peripheral driver pre-optimized the message
> + * @optimized: the message is in the optimized state
> =C2=A0 * @prepared: spi_prepare_message was called for the this message
> =C2=A0 * @status: zero for success, else negative errno
> =C2=A0 * @complete: called to report transaction completions
> @@ -1120,6 +1126,7 @@ struct spi_transfer {
> =C2=A0 *	successful segments
> =C2=A0 * @queue: for use by whichever driver currently owns the message
> =C2=A0 * @state: for use by whichever driver currently owns the message
> + * @opt_state: for use by whichever driver currently owns the message
> =C2=A0 * @resources: for resource management when the SPI message is proc=
essed
> =C2=A0 *
> =C2=A0 * A @spi_message is used to execute an atomic sequence of data tra=
nsfers,
> @@ -1143,6 +1150,11 @@ struct spi_message {
> =C2=A0
> =C2=A0	unsigned		is_dma_mapped:1;
> =C2=A0
> +	/* spi_optimize_message() was called for this message */
> +	bool			pre_optimized;
> +	/* __spi_optimize_message() was called for this message */
> +	bool			optimized;
> +
> =C2=A0	/* spi_prepare_message() was called for this message */
> =C2=A0	bool			prepared;
> =C2=A0
> @@ -1172,6 +1184,11 @@ struct spi_message {
> =C2=A0	 */
> =C2=A0	struct list_head	queue;
> =C2=A0	void			*state;
> +	/*
> +	 * Optional state for use by controller driver between calls to
> +	 * __spi_optimize_message() and __spi_unoptimize_message().
> +	 */
> +	void			*opt_state;
> =C2=A0
> =C2=A0	/* List of spi_res resources when the SPI message is processed */
> =C2=A0	struct list_head=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resourc=
es;
> @@ -1255,6 +1272,9 @@ static inline void spi_message_free(struct spi_mess=
age *m)
> =C2=A0	kfree(m);
> =C2=A0}
> =C2=A0
> +extern int spi_optimize_message(struct spi_device *spi, struct spi_messa=
ge *msg);
> +extern void spi_unoptimize_message(struct spi_message *msg);
> +
> =C2=A0extern int spi_setup(struct spi_device *spi);
> =C2=A0extern int spi_async(struct spi_device *spi, struct spi_message *me=
ssage);
> =C2=A0extern int spi_slave_abort(struct spi_device *spi);
>=20