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

[Jonathan Cameron <jic23@kernel.org>]

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);