[RFC,v3,2/9] spi: add basic support for SPI offloading

SPI offloading is a feature that allows the SPI controller to perform
transfers without any CPU intervention. This is useful, e.g. for
high-speed data acquisition.

This patch adds the basic infrastructure to support SPI offloading. It
introduces new callbacks that are to be implemented by controllers with
offload capabilities.

On SPI device probe, the standard spi-offloads devicetree property is
parsed and passed to the controller driver to reserve the resources
requested by the peripheral via the map_channel() callback.

The peripheral driver can then use spi_offload_prepare() to load a SPI
message into the offload hardware.

If the controller supports it, this message can then be passed to the
SPI message queue as if it was a normal message. Future patches will
will also implement a way to use a hardware trigger to start the message
transfers rather than going through the message queue.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v3 changes:
* Minor changes to doc comments.
* Changed to use phandle array for spi-offloads.
* Changed id to string to make use of spi-offload-names.

v2 changes:

This is a rework of "spi: add core support for controllers with offload
capabilities" from v1.

The spi_offload_get() function that Nuno didn't like is gone. Instead,
there is now a mapping callback that uses the new generic devicetree
binding to request resources automatically when a SPI device is probed.

The spi_offload_enable/disable() functions for dealing with hardware
triggers are deferred to a separate patch.

This leaves adding spi_offload_prepare/unprepare() which have been
reworked to be a bit more robust.

In the previous review, Mark suggested that these functions should not
be separate from the spi_[un]optimize() functions. I understand the
reasoning behind that. However, it seems like there are two different
kinds of things going on here. Currently, spi_optimize() only performs
operations on the message data structures and doesn't poke any hardware.
This makes it free to be use by any peripheral without worrying about
tying up any hardware resources while the message is "optimized". On the
other hand, spi_offload_prepare() is poking hardware, so we need to be
more careful about how it is used. And in these cases, we need a way to
specify exactly which hardware resources it should use, which it is
currently doing with the extra ID parameter.
---
 drivers/spi/spi.c       | 123 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |  57 ++++++++++++++++++++++
 2 files changed, 180 insertions(+)

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> SPI offloading is a feature that allows the SPI controller to perform
> transfers without any CPU intervention. This is useful, e.g. for
> high-speed data acquisition.
> 
> This patch adds the basic infrastructure to support SPI offloading. It
> introduces new callbacks that are to be implemented by controllers with
> offload capabilities.
> 
> On SPI device probe, the standard spi-offloads devicetree property is
> parsed and passed to the controller driver to reserve the resources
> requested by the peripheral via the map_channel() callback.
> 
> The peripheral driver can then use spi_offload_prepare() to load a SPI
> message into the offload hardware.
> 
> If the controller supports it, this message can then be passed to the
> SPI message queue as if it was a normal message. Future patches will
> will also implement a way to use a hardware trigger to start the message
> transfers rather than going through the message queue.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v3 changes:
> * Minor changes to doc comments.
> * Changed to use phandle array for spi-offloads.
> * Changed id to string to make use of spi-offload-names.
> 
> v2 changes:
> 
> This is a rework of "spi: add core support for controllers with offload
> capabilities" from v1.
> 
> The spi_offload_get() function that Nuno didn't like is gone. Instead,
> there is now a mapping callback that uses the new generic devicetree
> binding to request resources automatically when a SPI device is probed.
> 

Given my reply to the cover you can start calling me names already :). But even
with that function back I think we need a more explicit provider/consumer logic.

> The spi_offload_enable/disable() functions for dealing with hardware
> triggers are deferred to a separate patch.
> 
> This leaves adding spi_offload_prepare/unprepare() which have been
> reworked to be a bit more robust.
> 
> In the previous review, Mark suggested that these functions should not
> be separate from the spi_[un]optimize() functions. I understand the
> reasoning behind that. However, it seems like there are two different
> kinds of things going on here. Currently, spi_optimize() only performs
> operations on the message data structures and doesn't poke any hardware.
> This makes it free to be use by any peripheral without worrying about
> tying up any hardware resources while the message is "optimized". On the
> other hand, spi_offload_prepare() is poking hardware, so we need to be
> more careful about how it is used. And in these cases, we need a way to
> specify exactly which hardware resources it should use, which it is
> currently doing with the extra ID parameter.
> ---
>  drivers/spi/spi.c       | 123
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |  57 ++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d4da5464dbd0..d01b2e5c8c44 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2477,6 +2477,51 @@ static int of_spi_parse_dt(struct spi_controller *ctlr,
> struct spi_device *spi,
>  	of_spi_parse_dt_cs_delay(nc, &spi->cs_hold, "spi-cs-hold-delay-ns");
>  	of_spi_parse_dt_cs_delay(nc, &spi->cs_inactive, "spi-cs-inactive-
> delay-ns");
>  
> +	/* Offloads */
> +	rc = of_count_phandle_with_args(nc, "spi-offloads", "#spi-offload-
> cells");
> +	if (rc > 0) {
> +		int num_offload = rc;
> +
> +		if (!ctlr->offload_ops) {
> +			dev_err(&ctlr->dev, "SPI controller doesn't support
> offloading\n");
> +			return -EINVAL;
> +		}
> +
> +		for (idx = 0; idx < num_offload; idx++) {
> +			struct of_phandle_args args;
> +			const char *offload_name = NULL;
> +
> +			rc = of_parse_phandle_with_args(nc, "spi-offloads",
> +							"#spi-offload-cells",
> +							idx, &args);
> +			if (rc) {
> +				dev_err(&spi->dev, "Failed to parse offload
> phandle %d: %d\n",
> +					idx, rc);
> +				return rc;
> +			}
> +
> +			if (args.np != ctlr->dev.of_node) {
> +				dev_err(&spi->dev, "Offload phandle %d is not
> for this SPI controller\n",
> +					idx);
> +				of_node_put(args.np);
> +				return -EINVAL;
> +			}
> +
> +			of_property_read_string_index(nc, "spi-offload-
> names",
> +						      idx, &offload_name);
> +
> +			rc = ctlr->offload_ops->map_channel(spi,
> offload_name,
> +							    args.args,
> +							    args.args_count);

In here, I would expect for the mapping to return something the core could then
directly pass into the other operations. And hence saving controllers to always
have to do a lookup in all the operations.

It seems we may need a struct spi_offload * object that can be attached to a
given spi_device and that can be directly passed and used by the specific
offload operations. 

- Nuno Sá

On Mon, 22 Jul 2024 16:57:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> SPI offloading is a feature that allows the SPI controller to perform
> transfers without any CPU intervention. This is useful, e.g. for
> high-speed data acquisition.
> 
> This patch adds the basic infrastructure to support SPI offloading. It
> introduces new callbacks that are to be implemented by controllers with
> offload capabilities.
> 
> On SPI device probe, the standard spi-offloads devicetree property is
> parsed and passed to the controller driver to reserve the resources
> requested by the peripheral via the map_channel() callback.
> 
> The peripheral driver can then use spi_offload_prepare() to load a SPI
> message into the offload hardware.
> 
> If the controller supports it, this message can then be passed to the
> SPI message queue as if it was a normal message. Future patches will
> will also implement a way to use a hardware trigger to start the message
> transfers rather than going through the message queue.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few trivial comments inline.

J

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d4da5464dbd0..d01b2e5c8c44 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2477,6 +2477,51 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  	of_spi_parse_dt_cs_delay(nc, &spi->cs_hold, "spi-cs-hold-delay-ns");
>  	of_spi_parse_dt_cs_delay(nc, &spi->cs_inactive, "spi-cs-inactive-delay-ns");
>  
> +	/* Offloads */

Might be good to factor this out as a little utility function.

> +	rc = of_count_phandle_with_args(nc, "spi-offloads", "#spi-offload-cells");
> +	if (rc > 0) {
> +		int num_offload = rc;
> +
> +		if (!ctlr->offload_ops) {
> +			dev_err(&ctlr->dev, "SPI controller doesn't support offloading\n");
> +			return -EINVAL;
> +		}
> +
> +		for (idx = 0; idx < num_offload; idx++) {
> +			struct of_phandle_args args;
> +			const char *offload_name = NULL;
> +
> +			rc = of_parse_phandle_with_args(nc, "spi-offloads",
> +							"#spi-offload-cells",
> +							idx, &args);
> +			if (rc) {
> +				dev_err(&spi->dev, "Failed to parse offload phandle %d: %d\n",
> +					idx, rc);
> +				return rc;
> +			}
> +
> +			if (args.np != ctlr->dev.of_node) {
> +				dev_err(&spi->dev, "Offload phandle %d is not for this SPI controller\n",
> +					idx);
> +				of_node_put(args.np);
> +				return -EINVAL;
> +			}
> +
> +			of_property_read_string_index(nc, "spi-offload-names",
> +						      idx, &offload_name);

Check for errors?  If not, perhaps a comment on why not.

> +
> +			rc = ctlr->offload_ops->map_channel(spi, offload_name,
> +							    args.args,
> +							    args.args_count);
> +			of_node_put(args.np);
> +			if (rc) {
> +				dev_err(&spi->dev, "Failed to map offload channel %d: %d\n",
> +					value, rc);
> +				return rc;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }

...

> +/**
> + * spi_offload_prepare - prepare offload hardware for a transfer
> + * @spi:	The spi device to use for the transfers.
> + * @id:		Function ID if SPI device uses more than one offload or NULL.
> + * @msg:	The SPI message to use for the offload operation.
> + *
> + * Requests an offload instance with the specified ID and programs it with the
> + * provided message.
> + *
> + * The message must not be pre-optimized (do not call spi_optimize_message() on
> + * the message).
> + *
> + * Calls must be balanced with spi_offload_unprepare().
> + *
> + * Return: 0 on success, else a negative error code.
> + */
> +int spi_offload_prepare(struct spi_device *spi, const char *id,
> +			struct spi_message *msg)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +	int ret;
> +
> +	if (!ctlr->offload_ops)
> +		return -EOPNOTSUPP;
> +
> +	msg->offload = true;
I'd set this later perhaps as...
> +
> +	ret = spi_optimize_message(spi, msg);
> +	if (ret)

It otherwise needs clearing here so it doesn't have side
effects if an error occurs.

> +		return ret;
> +
> +	mutex_lock(&ctlr->io_mutex);
> +	ret = ctlr->offload_ops->prepare(spi, id, msg);
> +	mutex_unlock(&ctlr->io_mutex);
> +
> +	if (ret) {
> +		spi_unoptimize_message(msg);
> +		msg->offload = false;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_offload_prepare);

...

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index d7a16e0adf44..4998b48ea7fd 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -31,6 +31,7 @@ struct spi_transfer;

...

> @@ -1122,6 +1127,7 @@ struct spi_transfer {
>   * @pre_optimized: peripheral driver pre-optimized the message
>   * @optimized: the message is in the optimized state
>   * @prepared: spi_prepare_message was called for the this message
> + * @offload: message is to be used with offload hardware
>   * @status: zero for success, else negative errno
>   * @complete: called to report transaction completions
>   * @context: the argument to complete() when it's called
> @@ -1131,6 +1137,7 @@ struct spi_transfer {
>   * @queue: for use by whichever driver currently owns the message
>   * @state: for use by whichever driver currently owns the message
>   * @opt_state: for use by whichever driver currently owns the message
> + * @offload_state: for use by whichever driver currently owns the message
>   * @resources: for resource management when the SPI message is processed
>   *
>   * A @spi_message is used to execute an atomic sequence of data transfers,
> @@ -1159,6 +1166,8 @@ struct spi_message {
>  
>  	/* spi_prepare_message() was called for this message */
>  	bool			prepared;
> +	/* spi_offload_prepare() was called on this message */
> +	bool			offload;

offloaded? To match with prepared.
>  
>  	/*
>  	 * REVISIT: we might want a flag affecting the behavior of the
> @@ -1191,6 +1200,11 @@ struct spi_message {
>  	 * __spi_optimize_message() and __spi_unoptimize_message().
>  	 */
>  	void			*opt_state;
> +	/*
> +	 * Optional state for use by controller driver between calls to
> +	 * offload_ops->prepare() and offload_ops->unprepare().
> +	 */
> +	void			*offload_state;
>  
>  	/* List of spi_res resources when the SPI message is processed */
>  	struct list_head        resources;
> @@ -1556,6 +1570,49 @@ static inline ssize_t spi_w8r16be(struct spi_device *spi, u8 cmd)
>  
>  /*---------------------------------------------------------------------------*/
>  
> +/*
> + * Offloading support.
> + *
> + * Some SPI controllers support offloading of SPI transfers. Essentially,
> + * this allows the SPI controller to record SPI transfers and then play them
> + * back later in one go via a single trigger.
> + */
> +
> +/**
> + * struct spi_controller_offload_ops - callbacks for offload support
> + *
> + * Drivers for hardware with offload support need to implement all of these
> + * callbacks.
> + */
> +struct spi_controller_offload_ops {
> +	/**
> +	 * @map_channel: Required callback to reserve an offload instance for
> +	 * the given SPI device. If a SPI device requires more than one instance,
> +	 * then @id is used to differentiate between them. Channels must be
> +	 * unmapped in the struct spi_controller::cleanup() callback.

Probably a good idea to talk about possible return values as well.

> +	 */
> +	int (*map_channel)(struct spi_device *spi, const char *id,
> +			   const unsigned int *args, unsigned int num_args);

On 7/27/24 8:15 AM, Jonathan Cameron wrote:
> On Mon, 22 Jul 2024 16:57:09 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 


>> +/**
>> + * spi_offload_prepare - prepare offload hardware for a transfer
>> + * @spi:	The spi device to use for the transfers.
>> + * @id:		Function ID if SPI device uses more than one offload or NULL.
>> + * @msg:	The SPI message to use for the offload operation.
>> + *
>> + * Requests an offload instance with the specified ID and programs it with the
>> + * provided message.
>> + *
>> + * The message must not be pre-optimized (do not call spi_optimize_message() on
>> + * the message).
>> + *
>> + * Calls must be balanced with spi_offload_unprepare().
>> + *
>> + * Return: 0 on success, else a negative error code.
>> + */
>> +int spi_offload_prepare(struct spi_device *spi, const char *id,
>> +			struct spi_message *msg)
>> +{
>> +	struct spi_controller *ctlr = spi->controller;
>> +	int ret;
>> +
>> +	if (!ctlr->offload_ops)
>> +		return -EOPNOTSUPP;
>> +
>> +	msg->offload = true;
> I'd set this later perhaps as...

If we move it, then we would have to create a new function
to call instead of spi_optimize_message() so that the controller
driver can know that this is an offload message and not a
regular message since they will need to be handled differently
during the optimization phase.

>> +
>> +	ret = spi_optimize_message(spi, msg);
>> +	if (ret)
> 
> It otherwise needs clearing here so it doesn't have side
> effects if an error occurs.
> 
>> +		return ret;
>> +
>> +	mutex_lock(&ctlr->io_mutex);
>> +	ret = ctlr->offload_ops->prepare(spi, id, msg);
>> +	mutex_unlock(&ctlr->io_mutex);
>> +
>> +	if (ret) {
>> +		spi_unoptimize_message(msg);
>> +		msg->offload = false;
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_offload_prepare);
>

On Tue, 30 Jul 2024 14:35:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/27/24 8:15 AM, Jonathan Cameron wrote:
> > On Mon, 22 Jul 2024 16:57:09 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> 
> 
> >> +/**
> >> + * spi_offload_prepare - prepare offload hardware for a transfer
> >> + * @spi:	The spi device to use for the transfers.
> >> + * @id:		Function ID if SPI device uses more than one offload or NULL.
> >> + * @msg:	The SPI message to use for the offload operation.
> >> + *
> >> + * Requests an offload instance with the specified ID and programs it with the
> >> + * provided message.
> >> + *
> >> + * The message must not be pre-optimized (do not call spi_optimize_message() on
> >> + * the message).
> >> + *
> >> + * Calls must be balanced with spi_offload_unprepare().
> >> + *
> >> + * Return: 0 on success, else a negative error code.
> >> + */
> >> +int spi_offload_prepare(struct spi_device *spi, const char *id,
> >> +			struct spi_message *msg)
> >> +{
> >> +	struct spi_controller *ctlr = spi->controller;
> >> +	int ret;
> >> +
> >> +	if (!ctlr->offload_ops)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	msg->offload = true;  
> > I'd set this later perhaps as...  
> 
> If we move it, then we would have to create a new function
> to call instead of spi_optimize_message() so that the controller
> driver can know that this is an offload message and not a
> regular message since they will need to be handled differently
> during the optimization phase.
Ah. I'd missed that.
> 
> >> +
> >> +	ret = spi_optimize_message(spi, msg);
> >> +	if (ret)  
> > 
> > It otherwise needs clearing here so it doesn't have side
> > effects if an error occurs.

Then it needs clearing here I think.

> >   
> >> +		return ret;
> >> +
> >> +	mutex_lock(&ctlr->io_mutex);
> >> +	ret = ctlr->offload_ops->prepare(spi, id, msg);
> >> +	mutex_unlock(&ctlr->io_mutex);
> >> +
> >> +	if (ret) {
> >> +		spi_unoptimize_message(msg);
> >> +		msg->offload = false;
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(spi_offload_prepare);  
> >