Re: [PATCH v6 01/17] spi: add basic support for SPI offloading

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Wed, 2024-12-11 at 14:54 -0600, David Lechner wrote:
> Add the basic infrastructure to support SPI offload providers and
> consumers.
> 
> 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.
> 
> SPI controllers with offload support need to implement the get_offload
> and put_offload callbacks and can use the devm_spi_offload_alloc() to
> allocate offload instances.
> 
> SPI peripheral drivers will call devm_spi_offload_get() to get a
> reference to the matching offload instance. This offload instance can
> then be attached to a SPI message to request offloading that message.
> 
> It is expected that SPI controllers with offload support will check for
> the offload instance in the SPI message in the ctlr->optimize_message()
> callback and handle it accordingly.
> 
> CONFIG_SPI_OFFLOAD is intended to be a select-only option. Both
> consumer and provider drivers should `select SPI_OFFLOAD` in their
> Kconfig to ensure that the SPI core is built with offload support.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---

LGTM,

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

> 
> v6 changes:
> * Drop use of PTR_ERR_OR_ZERO().
> * Split header into types.h/provider.h/consumer.h.
> * Remove unused spi_controller_offload_ops forward declaration.
> 
> v5 changes:
> * Don't include linux/property.h (moved to later patch).
> * Only allocate single offload instance instead of array.
> * Allocate *priv separately to avoid alignment issues.
> * Add put_offload() callback instead of assuming devm semantics.
> * Drop struct spi_offload::spi. It was only being used as a flag.
> * Don't get/put struct spi_offload::provider_dev.
> * Add MAINTAINERS entry for me as reviewer for anything related to
>   SPI offload.
> 
> v4 changes:
> * SPI offload functions moved to a separate file instead of spi.c
>   (spi.c is already too long).
> * struct spi_offload and devm_spi_offload_get() are back, similar to
>   but improved over v1. This avoids having to pass the function ID
>   string to every function call and re-lookup the offload instance.
> * offload message prepare/unprepare functions are removed. Instead the
>   existing optimize/unoptimize functions should be used. Setting
>   spi_message::offload pointer is used as a flag to differentiate
>   between an offloaded message and a regular message.
> 
> 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.
> ---
>  MAINTAINERS                          |   6 ++
>  drivers/spi/Kconfig                  |   3 +
>  drivers/spi/Makefile                 |   1 +
>  drivers/spi/spi-offload.c            | 114
> +++++++++++++++++++++++++++++++++++
>  include/linux/spi/offload/consumer.h |  22 +++++++
>  include/linux/spi/offload/provider.h |  19 ++++++
>  include/linux/spi/offload/types.h    |  43 +++++++++++++
>  include/linux/spi/spi.h              |  17 ++++++
>  8 files changed, 225 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index
> bf2dcd4e0261785add520b5eac747ceac523e112..9284a257607a740ab7f6fd960c2bcdc34ead
> 7586 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22129,6 +22129,12 @@
> F:	Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>  F:	drivers/mtd/spi-nor/
>  F:	include/linux/mtd/spi-nor.h
>  
> +SPI OFFLOAD
> +R:	David Lechner <dlechner@xxxxxxxxxxxx>
> +F:	drivers/spi/spi-offload.c
> +F:	include/linux/spi/spi-offload.h
> +K:	spi_offload
> +
>  SPI SUBSYSTEM
>  M:	Mark Brown <broonie@xxxxxxxxxx>
>  L:	linux-spi@xxxxxxxxxxxxxxx
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index
> ea8a310329274bb2701e265cd152a56fb4e0f3a7..02064a4e292815ec0213e2e446b4f90ed885
> 5a52 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -55,6 +55,9 @@ config SPI_MEM
>  	  This extension is meant to simplify interaction with SPI memories
>  	  by providing a high-level interface to send memory-like commands.
>  
> +config SPI_OFFLOAD
> +	bool
> +
>  comment "SPI Master Controller Drivers"
>  
>  config SPI_AIROHA_SNFI
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index
> 9db7554c1864bf9b37dcf59c16eb76f5af03a7e8..bb5fc20df21332232533c2e70c0cc230f6bc
> f27f 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -10,6 +10,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
>  obj-$(CONFIG_SPI_MASTER)		+= spi.o
>  obj-$(CONFIG_SPI_MEM)			+= spi-mem.o
>  obj-$(CONFIG_SPI_MUX)			+= spi-mux.o
> +obj-$(CONFIG_SPI_OFFLOAD)		+= spi-offload.o
>  obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
>  obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
>  
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..3a40ef30debf09c6fd7b2c14526f3e5976e2
> b21f
> --- /dev/null
> +++ b/drivers/spi/spi-offload.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + */
> +
> +/*
> + * SPI Offloading support.
> + *
> + * Some SPI controllers support offloading of SPI transfers. Essentially,
> this
> + * is the ability for a SPI controller to perform SPI transfers with minimal
> + * or even no CPU intervention, e.g. via a specialized SPI controller with a
> + * hardware trigger or via a conventional SPI controller using a non-Linux
> MCU
> + * processor core to offload the work.
> + */
> +
> +#define DEFAULT_SYMBOL_NAMESPACE "SPI_OFFLOAD"
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/offload/consumer.h>
> +#include <linux/spi/offload/provider.h>
> +#include <linux/spi/offload/types.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +
> +struct spi_controller_and_offload {
> +	struct spi_controller *controller;
> +	struct spi_offload *offload;
> +};
> +
> +/**
> + * devm_spi_offload_alloc() - Allocate offload instance
> + * @dev: Device for devm purposes and assigned to &struct
> spi_offload.provider_dev
> + * @priv_size: Size of private data to allocate
> + *
> + * Offload providers should use this to allocate offload instances.
> + *
> + * Return: Pointer to new offload instance or error on failure.
> + */
> +struct spi_offload *devm_spi_offload_alloc(struct device *dev,
> +					   size_t priv_size)
> +{
> +	struct spi_offload *offload;
> +	void *priv;
> +
> +	offload = devm_kzalloc(dev, sizeof(*offload), GFP_KERNEL);
> +	if (!offload)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv = devm_kzalloc(dev, priv_size, GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	offload->provider_dev = dev;
> +	offload->priv = priv;
> +
> +	return offload;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_alloc);
> +
> +static void spi_offload_put(void *data)
> +{
> +	struct spi_controller_and_offload *resource = data;
> +
> +	resource->controller->put_offload(resource->offload);
> +	kfree(resource);
> +}
> +
> +/**
> + * devm_spi_offload_get() - Get an offload instance
> + * @dev: Device for devm purposes
> + * @spi: SPI device to use for the transfers
> + * @config: Offload configuration
> + *
> + * Peripheral drivers call this function to get an offload instance that
> meets
> + * the requirements specified in @config. If no suitable offload instance is
> + * available, -ENODEV is returned.
> + *
> + * Return: Offload instance or error on failure.
> + */
> +struct spi_offload *devm_spi_offload_get(struct device *dev,
> +					 struct spi_device *spi,
> +					 const struct spi_offload_config
> *config)
> +{
> +	struct spi_controller_and_offload *resource;
> +	int ret;
> +
> +	if (!spi || !config)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!spi->controller->get_offload)
> +		return ERR_PTR(-ENODEV);
> +
> +	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> +	if (!resource)
> +		return ERR_PTR(-ENOMEM);
> +
> +	resource->controller = spi->controller;
> +	resource->offload = spi->controller->get_offload(spi, config);
> +	if (IS_ERR(resource->offload)) {
> +		kfree(resource);
> +		return resource->offload;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_put, resource);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return resource->offload;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> diff --git a/include/linux/spi/offload/consumer.h
> b/include/linux/spi/offload/consumer.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..05543dbedf3086fb4befcd149cff3c8c70a8
> 8825
> --- /dev/null
> +++ b/include/linux/spi/offload/consumer.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + */
> +
> +#ifndef __LINUX_SPI_OFFLOAD_CONSUMER_H
> +#define __LINUX_SPI_OFFLOAD_CONSUMER_H
> +
> +#include <linux/module.h>
> +#include <linux/spi/offload/types.h>
> +#include <linux/types.h>
> +
> +MODULE_IMPORT_NS("SPI_OFFLOAD");
> +
> +struct device;
> +struct spi_device;
> +
> +struct spi_offload *devm_spi_offload_get(struct device *dev, struct
> spi_device *spi,
> +					 const struct spi_offload_config
> *config);
> +
> +#endif /* __LINUX_SPI_OFFLOAD_CONSUMER_H */
> diff --git a/include/linux/spi/offload/provider.h
> b/include/linux/spi/offload/provider.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..278c4edfcdb7b1f43870ca99d2ba252bf282
> 0576
> --- /dev/null
> +++ b/include/linux/spi/offload/provider.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + */
> +
> +#ifndef __LINUX_SPI_OFFLOAD_PROVIDER_H
> +#define __LINUX_SPI_OFFLOAD_PROVIDER_H
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +MODULE_IMPORT_NS("SPI_OFFLOAD");
> +
> +struct device;
> +
> +struct spi_offload *devm_spi_offload_alloc(struct device *dev, size_t
> priv_size);
> +
> +#endif /* __LINUX_SPI_OFFLOAD_PROVIDER_H */
> diff --git a/include/linux/spi/offload/types.h
> b/include/linux/spi/offload/types.h
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..a74f8d84541b10062353e81a638f05628b69
> 6394
> --- /dev/null
> +++ b/include/linux/spi/offload/types.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + */
> +
> +#ifndef __LINUX_SPI_OFFLOAD_TYPES_H
> +#define __LINUX_SPI_OFFLOAD_TYPES_H
> +
> +#include <linux/types.h>
> +
> +struct device;
> +
> +/* Offload can be triggered by external hardware event. */
> +#define SPI_OFFLOAD_CAP_TRIGGER			BIT(0)
> +/* Offload can record and then play back TX data when triggered. */
> +#define SPI_OFFLOAD_CAP_TX_STATIC_DATA		BIT(1)
> +/* Offload can get TX data from an external stream source. */
> +#define SPI_OFFLOAD_CAP_TX_STREAM_DMA		BIT(2)
> +/* Offload can send RX data to an external stream sink. */
> +#define SPI_OFFLOAD_CAP_RX_STREAM_DMA		BIT(3)
> +
> +/**
> + * struct spi_offload_config - offload configuration
> + *
> + * This is used to request an offload with specific configuration.
> + */
> +struct spi_offload_config {
> +	/** @capability_flags: required capabilities. See %SPI_OFFLOAD_CAP_*
> */
> +	u32 capability_flags;
> +};
> +
> +/**
> + * struct spi_offload - offload instance
> + */
> +struct spi_offload {
> +	/** @provider_dev: for get/put reference counting */
> +	struct device *provider_dev;
> +	/** @priv: provider driver private data */
> +	void *priv;
> +};
> +
> +#endif /* __LINUX_SPI_OFFLOAD_TYPES_H */
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index
> 8497f4747e24d4ecd85b74f49609ac1c82c73535..98bdc8c16c20521c0a94e5f72f5e71c4f6d7
> d11e 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -31,6 +31,8 @@ struct spi_transfer;
>  struct spi_controller_mem_ops;
>  struct spi_controller_mem_caps;
>  struct spi_message;
> +struct spi_offload;
> +struct spi_offload_config;
>  
>  /*
>   * INTERFACES between SPI master-side drivers and SPI slave protocol
> handlers,
> @@ -496,6 +498,10 @@ extern struct spi_device *spi_new_ancillary_device(struct
> spi_device *spi, u8 ch
>   * @mem_ops: optimized/dedicated operations for interactions with SPI memory.
>   *	     This field is optional and should only be implemented if the
>   *	     controller has native support for memory like operations.
> + * @get_offload: callback for controllers with offload support to get
> matching
> + *	offload instance. Implementations should return -ENODEV if no match
> is
> + *	found.
> + * @put_offload: release the offload instance acquired by @get_offload.
>   * @mem_caps: controller capabilities for the handling of memory operations.
>   * @unprepare_message: undo any work done by prepare_message().
>   * @target_abort: abort the ongoing transfer request on an SPI target
> controller
> @@ -740,6 +746,10 @@ struct spi_controller {
>  	const struct spi_controller_mem_ops *mem_ops;
>  	const struct spi_controller_mem_caps *mem_caps;
>  
> +	struct spi_offload *(*get_offload)(struct spi_device *spi,
> +					   const struct spi_offload_config
> *config);
> +	void (*put_offload)(struct spi_offload *offload);
> +
>  	/* GPIO chip select */
>  	struct gpio_desc	**cs_gpiods;
>  	bool			use_gpio_descriptors;
> @@ -1108,6 +1118,7 @@ struct spi_transfer {
>   * @state: for use by whichever driver currently owns the message
>   * @opt_state: for use by whichever driver currently owns the message
>   * @resources: for resource management when the SPI message is processed
> + * @offload: (optional) offload instance used by this message
>   *
>   * A @spi_message is used to execute an atomic sequence of data transfers,
>   * each represented by a struct spi_transfer.  The sequence is "atomic"
> @@ -1168,6 +1179,12 @@ struct spi_message {
>  	 */
>  	void			*opt_state;
>  
> +	/*
> +	 * Optional offload instance used by this message. This must be set
> +	 * by the peripheral driver before calling spi_optimize_message().
> +	 */
> +	struct spi_offload	*offload;
> +
>  	/* List of spi_res resources when the SPI message is processed */
>  	struct list_head        resources;
>  };
>