[PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[David Lechner]

There is a recap at the end of this cover letter for those not familiar
with the previous discussions. For those that are, we'll get right to
the changes since the last version.

In RFC v2, most of the discussion was around the DT bindings, so that
is what has mostly changed since then. I think we mostly settled on
what properties are needed and where they should go. There are probably
still some details to work out (see PATCH 5/9 for more discussion) but
I think we have the big-picture stuff figured out.

Here is the actual devicetree used for testing to show how it all
comes together:

	trigger_clk: adc-trigger-clock {
		compatible = "pwm-clock";
		#clock-cells = <0>;
		#trigger-source-cells = <0>;
		pwms = <&adc_trigger 0 10000>;
	};

	...

	axi_spi_engine_0: spi@44a00000 {
		compatible = "adi,axi-spi-engine-1.00.a";
		reg = <0x44a00000 0x1000>;
		interrupt-parent = <&intc>;
		interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
		clocks = <&clkc 15>, <&spi_clk>;
		clock-names = "s_axi_aclk", "spi_clk";

		/* offload-specific properties */
		#spi-offload-cells = <1>;
		dmas = <&rx_dma 0>;
		dma-names = "offload0-rx";
		trigger-sources = <&trigger_clk>;

		#address-cells = <1>;
		#size-cells = <0>;

		ad7986: adc@0 {
			compatible = "adi,ad7986";
			reg = <0>;
			spi-max-frequency = <111111111>; /* 9 ns period */
			adi,spi-mode = "single";
			avdd-supply = <&eval_u12>;
			dvdd-supply = <&eval_u12>;
			vio-supply = <&eval_u3>;
			bvdd-supply = <&eval_u10>;
			ref-supply = <&eval_u5>;
			turbo-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>;

			spi-offloads = <&axi_spi_engine_0 0>;
		};
	};

A working branch complete with extra hacks can be found at [1].

Also, I took a detour looking into what it would take to get Martin
Sperl's Raspberry Pi DMA offload proof-of-concept [2] updated to work
with this. This way we could have a second user to help guide the
design process. Given all of the SPI hardware quirks on that platform
and the unsolved technical issues, like how to get accurate time delays
and how to work around the 32-bit DMA word limitation, it would be more
work than I have time for (at least without someone sponsoring the work).

[1]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v3
[2]: https://github.com/msperl/spi-bcm2835/blob/refactor_dmachain_for_prepared_messages/spi-bcm2835dma.c

---
Changes in v3:
- See individual patches for more detailed changes.
- Reworked DT bindings to have things physically connected to the SPI
  controller be properties of the SPI controller and use more
  conventional provider/consumer properties.
- Added more SPI APIs for peripheral drivers to use to get auxillary
  offload resources, like triggers.
- Link to v2: https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com

---

As a recap, here is the background and end goal of this series:

The AXI SPI Engine is a SPI controller that has the ability to record a
series of SPI transactions and then play them back using a hardware
trigger. This allows operations to be performed, repeating many times,
without any CPU intervention. This is needed for achieving high data
rates (millions of samples per second) from ADCs and DACs that are
connected via a SPI bus.

The offload hardware interface consists of a trigger input and a data
output for the RX data. These are connected to other hardware external
to the SPI controller.

To record one or more transactions, commands and TX data are written
to memories in the controller (RX buffer is not used since RX data gets
streamed to an external sink). This sequence of transactions can then be
played back when the trigger input is asserted.

This series includes core SPI support along with the first SPI
controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use
them. This enables capturing analog data at 2 million samples per
second.

The hardware setup looks like this:

+-------------------------------+   +------------------+
|                               |   |                  |
|  SOC/FPGA                     |   |  AD7944 ADC      |
|  +---------------------+      |   |                  |
|  | AXI SPI Engine      |      |   |                  |
|  |             SPI Bus ============ SPI Bus          |
|  |                     |      |   |                  |
|  |  +---------------+  |      |   |                  |
|  |  | Offload 0     |  |      |   +------------------+
|  |  |   RX DATA OUT > > > >   |
|  |  |    TRIGGER IN < < <  v  |
|  |  +---------------+  | ^ v  |
|  +---------------------+ ^ v  |
|  | AXI PWM             | ^ v  |
|  |                 CH0 > ^ v  |
|  +---------------------+   v  |
|  | AXI DMA             |   v  |
|  |                 CH0 < < <  |
|  +---------------------+      |
|                               |
+-------------------------------+

To: Mark Brown <broonie@kernel.org>
To: Jonathan Cameron <jic23@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Nuno Sá <nuno.sa@analog.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: David Jander <david@protonic.nl>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc:  <linux-spi@vger.kernel.org>
Cc:  <devicetree@vger.kernel.org>
Cc:  <linux-kernel@vger.kernel.org>
Cc:  <linux-iio@vger.kernel.org>

---
David Lechner (9):
      spi: dt-bindings: add spi-offload properties
      spi: add basic support for SPI offloading
      spi: add support for hardware triggered offload
      spi: add offload TX/RX streaming APIs
      spi: dt-bindings: axi-spi-engine: document spi-offloads
      spi: axi-spi-engine: implement offload support
      iio: buffer-dmaengine: generalize requesting DMA channel
      dt-bindings: iio: adc: adi,ad7944: add SPI offload properties
      iio: adc: ad7944: add support for SPI offload

 .../devicetree/bindings/iio/adc/adi,ad7944.yaml    |   3 +
 .../bindings/spi/adi,axi-spi-engine.yaml           |  41 +++
 .../devicetree/bindings/spi/spi-controller.yaml    |   5 +
 .../bindings/spi/spi-peripheral-props.yaml         |  11 +
 drivers/iio/adc/ad7944.c                           | 173 ++++++++++-
 drivers/iio/buffer/industrialio-buffer-dmaengine.c |  39 ++-
 drivers/iio/dac/adi-axi-dac.c                      |   3 +-
 drivers/spi/spi-axi-spi-engine.c                   | 341 ++++++++++++++++++++-
 drivers/spi/spi.c                                  | 226 +++++++++++++-
 include/linux/iio/buffer-dmaengine.h               |  11 +-
 include/linux/spi/spi.h                            | 169 ++++++++++
 11 files changed, 989 insertions(+), 33 deletions(-)
---
base-commit: 7a891f6a5000f7658274b554cf993dd56aa5adbc
change-id: 20240510-dlech-mainline-spi-engine-offload-2-afce3790b5ab

[PATCH RFC v3 1/9] spi: dt-bindings: add spi-offload properties

[David Lechner]

This adds a new provider/consumer property pair to the generic SPI
bindings for use with peripherals connected to controllers that support
offloading.

Here, offloading means that the controller has the ability to perform
SPI transactions without any CPU intervention in some shape or form.

The spi-offloads property will be used to assign controller offload
resources to each peripheral that needs them. What these resources are
will be defined by each specific controller binding by specifying the
value of the #spi-offload-cells property.

SPI peripherals that use multiple offload instances at the same time
for different functions can describe the functions using the
spi-offload-names property, for example, for a SPI flash memory, this
might be "read", "erase" and "write" functions.

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

v3 changes:
* Added #spi-offload-cells property to the controller binding.
* Changed spi-offloads to phandle-array.
* Added spi-offload-names property.

v2 changes:

In v1, instead of generic SPI bindings, there were only controller-
specific bindings, so this is a new patch.
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml     |  5 +++++
 .../devicetree/bindings/spi/spi-peripheral-props.yaml         | 11 +++++++++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 093150c0cb87..0af9cce80be9 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -105,6 +105,11 @@ properties:
     required:
       - compatible
 
+  '#spi-offload-cells':
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Number of cells in a SPI offload specifier.
+
 patternProperties:
   "^.*@[0-9a-f]+$":
     type: object
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 0bb443b8decd..e27577bdae48 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -113,6 +113,17 @@ properties:
     minItems: 2
     maxItems: 4
 
+  spi-offloads:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of SPI offload instances that are used by the peripheral.
+
+  spi-offload-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description:
+      Describes the function of each offload instance. Can be omitted if there
+      is only one item allowed in the spi-offloads array.
+
   st,spi-midi-ns:
     description: |
       Only for STM32H7, (Master Inter-Data Idleness) minimum time

-- 
2.43.0

Re: [PATCH RFC v3 1/9] spi: dt-bindings: add spi-offload properties

[Rob Herring]

On Mon, Jul 22, 2024 at 04:57:08PM -0500, David Lechner wrote:
> This adds a new provider/consumer property pair to the generic SPI
> bindings for use with peripherals connected to controllers that support
> offloading.
> 
> Here, offloading means that the controller has the ability to perform
> SPI transactions without any CPU intervention in some shape or form.
> 
> The spi-offloads property will be used to assign controller offload
> resources to each peripheral that needs them. What these resources are
> will be defined by each specific controller binding by specifying the
> value of the #spi-offload-cells property.
> 
> SPI peripherals that use multiple offload instances at the same time
> for different functions can describe the functions using the
> spi-offload-names property, for example, for a SPI flash memory, this
> might be "read", "erase" and "write" functions.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v3 changes:
> * Added #spi-offload-cells property to the controller binding.
> * Changed spi-offloads to phandle-array.
> * Added spi-offload-names property.
> 
> v2 changes:
> 
> In v1, instead of generic SPI bindings, there were only controller-
> specific bindings, so this is a new patch.
> ---
>  Documentation/devicetree/bindings/spi/spi-controller.yaml     |  5 +++++
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml         | 11 +++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> index 093150c0cb87..0af9cce80be9 100644
> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -105,6 +105,11 @@ properties:
>      required:
>        - compatible
>  
> +  '#spi-offload-cells':
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Number of cells in a SPI offload specifier.
> +

This is the SPI controller/bus schema, but this is likely not part of 
the SPI controller. So needs its own schema.

Some description around what you think would typically be in these cells 
would be good.

Rob

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

[David Lechner]

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(+)

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);
+			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;
 }
 
@@ -3231,6 +3276,11 @@ static int spi_controller_check_ops(struct spi_controller *ctlr)
 		}
 	}
 
+	if (ctlr->offload_ops && !(ctlr->offload_ops->map_channel &&
+				   ctlr->offload_ops->prepare &&
+				   ctlr->offload_ops->unprepare))
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -4751,6 +4801,79 @@ int spi_write_then_read(struct spi_device *spi,
 }
 EXPORT_SYMBOL_GPL(spi_write_then_read);
 
+/**
+ * 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;
+
+	ret = spi_optimize_message(spi, msg);
+	if (ret)
+		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);
+
+/**
+ * spi_offload_unprepare - releases any resources used by spi_offload_prepare()
+ * @spi:	The same SPI device passed to spi_offload_prepare()
+ * @id:		The same ID device passed to spi_offload_prepare()
+ * @msg:	The same SPI message passed to spi_offload_prepare()
+ *
+ * Callers must ensure that the offload is no longer in use before calling this
+ * function, e.g. no in-progress transfers.
+ */
+void spi_offload_unprepare(struct spi_device *spi, const char *id,
+			   struct spi_message *msg)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops)
+		return;
+
+	mutex_lock(&ctlr->io_mutex);
+	ctlr->offload_ops->unprepare(spi, id);
+	mutex_unlock(&ctlr->io_mutex);
+
+	msg->offload = false;
+	msg->offload_state = NULL;
+
+	spi_unoptimize_message(msg);
+}
+EXPORT_SYMBOL_GPL(spi_offload_unprepare);
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_OF_DYNAMIC)
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;
 struct spi_controller_mem_ops;
 struct spi_controller_mem_caps;
 struct spi_message;
+struct spi_controller_offload_ops;
 
 /*
  * INTERFACES between SPI master-side drivers and SPI slave protocol handlers,
@@ -499,6 +500,7 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
  *	     This field is optional and should only be implemented if the
  *	     controller has native support for memory like operations.
  * @mem_caps: controller capabilities for the handling of memory operations.
+ * @offload_ops: operations for controllers with offload support.
  * @unprepare_message: undo any work done by prepare_message().
  * @slave_abort: abort the ongoing transfer request on an SPI slave controller
  * @target_abort: abort the ongoing transfer request on an SPI target controller
@@ -746,6 +748,9 @@ struct spi_controller {
 	const struct spi_controller_mem_ops *mem_ops;
 	const struct spi_controller_mem_caps *mem_caps;
 
+	/* Operations for controllers with offload support. */
+	const struct spi_controller_offload_ops *offload_ops;
+
 	/* GPIO chip select */
 	struct gpio_desc	**cs_gpiods;
 	bool			use_gpio_descriptors;
@@ -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;
 
 	/*
 	 * 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.
+	 */
+	int (*map_channel)(struct spi_device *spi, const char *id,
+			   const unsigned int *args, unsigned int num_args);
+	/**
+	 * @prepare: Required callback to prepare the offload for the given SPI
+	 * message. @msg and any of its members (including any xfer->tx_buf) is
+	 * not guaranteed to be valid beyond the lifetime of this call.
+	 */
+	int (*prepare)(struct spi_device *spi, const char *id,
+		       struct spi_message *msg);
+	/**
+	 * @unprepare: Required callback to release any resources used by prepare().
+	 */
+	void (*unprepare)(struct spi_device *spi, const char *id);
+};
+
+extern int spi_offload_prepare(struct spi_device *spi, const char *id,
+			       struct spi_message *msg);
+extern void spi_offload_unprepare(struct spi_device *spi, const char *id,
+				  struct spi_message *msg);
+
+/*---------------------------------------------------------------------------*/
+
 /*
  * INTERFACE between board init code and SPI infrastructure.
  *

-- 
2.43.0

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

[Nuno Sá]

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á

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

[Jonathan Cameron]

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

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

[David Lechner]

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

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

[Jonathan Cameron]

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

[PATCH RFC v3 3/9] spi: add support for hardware triggered offload

[David Lechner]

This extends the SPI framework to support hardware triggered offloading.
This allows an arbitrary hardware trigger to be used to start a SPI
transfer that was previously set up with spi_offload_prepare().

Since the hardware trigger can happen at any time, this means the SPI
bus must be reserved for exclusive use as long as the hardware trigger
is enabled. Since a hardware trigger could be enabled indefinitely,
we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
otherwise this could cause deadlocks. So we introduce a new flag so that
any attempt to lock or use the bus will fail with -EBUSY as long as the
hardware trigger is enabled.

Peripheral drivers may need to control the trigger source as well. For
this, we introduce a new spi_offload_hw_trigger_get_clk() function that
can be used to get a clock trigger source. This is intended for used
by ADC drivers that will use the clock to control the sample rate.
Additional functions to get other types of trigger sources could be
added in the future.

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

TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
check the return value. All callers will need to be updated first before
this can be merged.

v3 changes:
* renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
* added spi_offload_hw_trigger_get_clk() function
* fixed missing EXPORT_SYMBOL_GPL

v2 changes:

This is split out from "spi: add core support for controllers with
offload capabilities".

Mark suggested that the standard SPI APIs should be aware that the
hardware trigger is enabled. So I've added some locking for this. Nuno
suggested that this might be overly strict though, and that we should
let each individual controller driver decide what to do. For our use
case though, I think we generally are going to have a single peripheral
on the SPI bus, so this seems like a reasonable starting place anyway.
---
 drivers/spi/spi.c       | 94 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/spi/spi.h | 40 +++++++++++++++++++++
 2 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d01b2e5c8c44..7488e71f159f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4495,7 +4495,7 @@ int spi_async(struct spi_device *spi, struct spi_message *message)
 
 	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
 
-	if (ctlr->bus_lock_flag)
+	if (ctlr->bus_lock_flag || ctlr->offload_hw_trigger_mode_enabled)
 		ret = -EBUSY;
 	else
 		ret = __spi_async(spi, message);
@@ -4638,6 +4638,12 @@ int spi_sync(struct spi_device *spi, struct spi_message *message)
 	int ret;
 
 	mutex_lock(&spi->controller->bus_lock_mutex);
+
+	if (spi->controller->offload_hw_trigger_mode_enabled) {
+		mutex_unlock(&spi->controller->bus_lock_mutex);
+		return -EBUSY;
+	}
+
 	ret = __spi_sync(spi, message);
 	mutex_unlock(&spi->controller->bus_lock_mutex);
 
@@ -4680,7 +4686,7 @@ EXPORT_SYMBOL_GPL(spi_sync_locked);
  * exclusive access is over. Data transfer must be done by spi_sync_locked
  * and spi_async_locked calls when the SPI bus lock is held.
  *
- * Return: always zero.
+ * Return: 0 on success, -EBUSY if the bus is reserved by offload hardware.
  */
 int spi_bus_lock(struct spi_controller *ctlr)
 {
@@ -4688,6 +4694,11 @@ int spi_bus_lock(struct spi_controller *ctlr)
 
 	mutex_lock(&ctlr->bus_lock_mutex);
 
+	if (ctlr->offload_hw_trigger_mode_enabled) {
+		mutex_unlock(&ctlr->bus_lock_mutex);
+		return -EBUSY;
+	}
+
 	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
 	ctlr->bus_lock_flag = 1;
 	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
@@ -4874,6 +4885,85 @@ void spi_offload_unprepare(struct spi_device *spi, const char *id,
 }
 EXPORT_SYMBOL_GPL(spi_offload_unprepare);
 
+/**
+ * spi_offload_hw_trigger_mode_enable - enables hardware trigger for offload
+ * @spi:	The spi device to use for the transfers.
+ * @id:		Function ID if SPI device uses more than one offload or NULL.
+ *
+ * There must be a prepared offload instance with the specified ID (i.e.
+ * spi_offload_prepare() was called with the same ID). This will also reserve
+ * the bus for exclusive use by the offload instance until the hardware trigger
+ * is disabled. Any other attempts to send a transfer or lock the bus will fail
+ * with -EBUSY during this time.
+ *
+ * Calls must be balanced with spi_offload_hw_trigger_mode_disable().
+ *
+ * Context: can sleep
+ * Return: 0 on success, else a negative error code.
+ */
+int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *ctlr = spi->controller;
+	unsigned long flags;
+	int ret;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&ctlr->bus_lock_mutex);
+
+	if (ctlr->offload_hw_trigger_mode_enabled) {
+		mutex_unlock(&ctlr->bus_lock_mutex);
+		return -EBUSY;
+	}
+
+	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
+	ctlr->offload_hw_trigger_mode_enabled = true;
+	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
+
+	/* TODO: how to wait for empty message queue? */
+
+	mutex_lock(&ctlr->io_mutex);
+	ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id);
+	mutex_unlock(&ctlr->io_mutex);
+
+	if (ret) {
+		ctlr->offload_hw_trigger_mode_enabled = false;
+		mutex_unlock(&ctlr->bus_lock_mutex);
+		return ret;
+	}
+
+	mutex_unlock(&ctlr->bus_lock_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_offload_hw_trigger_mode_enable);
+
+/**
+ * spi_offload_hw_trigger_mode_disable - disables hardware trigger for offload
+ * @spi:	The same SPI device passed to spi_offload_hw_trigger_mode_enable()
+ * @id:		The same ID device passed to spi_offload_hw_trigger_mode_enable()
+ *
+ * Disables the hardware trigger for the offload instance with the specified ID
+ * and releases the bus for use by other clients.
+ *
+ * Context: can sleep
+ */
+void spi_offload_hw_trigger_mode_disable(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_disable)
+		return;
+
+	mutex_lock(&ctlr->io_mutex);
+	ctlr->offload_ops->hw_trigger_mode_disable(spi, id);
+	mutex_unlock(&ctlr->io_mutex);
+
+	ctlr->offload_hw_trigger_mode_enabled = false;
+}
+EXPORT_SYMBOL_GPL(spi_offload_hw_trigger_mode_disable);
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_OF_DYNAMIC)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4998b48ea7fd..685548883004 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -634,6 +634,9 @@ struct spi_controller {
 	/* Flag indicating that the SPI bus is locked for exclusive use */
 	bool			bus_lock_flag;
 
+	/* Flag indicating the bus is reserved for use by hardware trigger */
+	bool			offload_hw_trigger_mode_enabled;
+
 	/*
 	 * Setup mode and clock, etc (SPI driver may call many times).
 	 *
@@ -1604,12 +1607,49 @@ struct spi_controller_offload_ops {
 	 * @unprepare: Required callback to release any resources used by prepare().
 	 */
 	void (*unprepare)(struct spi_device *spi, const char *id);
+	/**
+	 * @hw_trigger_mode_enable: Optional callback to enable the hardware
+	 * trigger for the given offload instance.
+	 */
+	int (*hw_trigger_mode_enable)(struct spi_device *spi, const char *id);
+	/**
+	 * @hw_trigger_mode_disable: Optional callback to disable the hardware
+	 * trigger for the given offload instance.
+	 */
+	void (*hw_trigger_mode_disable)(struct spi_device *spi, const char *id);
+	/**
+	 * @hw_trigger_get_clk: Optional callback for controllers that have a
+	 * hardware offload trigger that is connected to a clock.
+	 */
+	struct clk *(*hw_trigger_get_clk)(struct spi_device *spi, const char *id);
 };
 
 extern int spi_offload_prepare(struct spi_device *spi, const char *id,
 			       struct spi_message *msg);
 extern void spi_offload_unprepare(struct spi_device *spi, const char *id,
 				  struct spi_message *msg);
+extern int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id);
+extern void spi_offload_hw_trigger_mode_disable(struct spi_device *spi, const char *id);
+
+/**
+ * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
+ * @spi: SPI device
+ * @id: Function ID if SPI device uses more than one offload or NULL.
+ *
+ * The caller is responsible for calling clk_put() on the returned clock.
+ *
+ * Return: The clock for the offload trigger, or negative error code
+ */
+static inline
+struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
+}
 
 /*---------------------------------------------------------------------------*/
 

-- 
2.43.0

Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

[Nuno Sá]

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This extends the SPI framework to support hardware triggered offloading.
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_offload_prepare().
> 
> Since the hardware trigger can happen at any time, this means the SPI
> bus must be reserved for exclusive use as long as the hardware trigger
> is enabled. Since a hardware trigger could be enabled indefinitely,
> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
> otherwise this could cause deadlocks. So we introduce a new flag so that
> any attempt to lock or use the bus will fail with -EBUSY as long as the
> hardware trigger is enabled.
> 
> Peripheral drivers may need to control the trigger source as well. For
> this, we introduce a new spi_offload_hw_trigger_get_clk() function that
> can be used to get a clock trigger source. This is intended for used
> by ADC drivers that will use the clock to control the sample rate.
> Additional functions to get other types of trigger sources could be
> added in the future.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
> check the return value. All callers will need to be updated first before
> this can be merged.
> 
> v3 changes:
> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> * added spi_offload_hw_trigger_get_clk() function
> * fixed missing EXPORT_SYMBOL_GPL
> 
> v2 changes:
> 
> This is split out from "spi: add core support for controllers with
> offload capabilities".
> 
> Mark suggested that the standard SPI APIs should be aware that the
> hardware trigger is enabled. So I've added some locking for this. Nuno
> suggested that this might be overly strict though, and that we should
> let each individual controller driver decide what to do. For our use
> case though, I think we generally are going to have a single peripheral
> on the SPI bus, so this seems like a reasonable starting place anyway.
> ---

How explicitly do we want to be about returning errors? It seems that if the
trigger is enabled we can't anything else on the controller/offload_engine so we
could very well just hold the controller lock when enabling the trigger and 
release it when disabling it. Pretty much the same behavior as spi_bus_lock()...

...

> 
> +
> +/**
> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
> + * @spi: SPI device
> + * @id: Function ID if SPI device uses more than one offload or NULL.
> + *
> + * The caller is responsible for calling clk_put() on the returned clock.
> + *
> + * Return: The clock for the offload trigger, or negative error code
> + */
> +static inline
> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
> *id)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +
> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
> +}
> 

It would be nice if we could have some kind of spi abstraction...

- Nuno Sá

Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

[David Lechner]

On 7/23/24 2:53 AM, Nuno Sá wrote:
> On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
>> This extends the SPI framework to support hardware triggered offloading.
>> This allows an arbitrary hardware trigger to be used to start a SPI
>> transfer that was previously set up with spi_offload_prepare().
>>
>> Since the hardware trigger can happen at any time, this means the SPI
>> bus must be reserved for exclusive use as long as the hardware trigger
>> is enabled. Since a hardware trigger could be enabled indefinitely,
>> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
>> otherwise this could cause deadlocks. So we introduce a new flag so that
>> any attempt to lock or use the bus will fail with -EBUSY as long as the
>> hardware trigger is enabled.
>>
>> Peripheral drivers may need to control the trigger source as well. For
>> this, we introduce a new spi_offload_hw_trigger_get_clk() function that
>> can be used to get a clock trigger source. This is intended for used
>> by ADC drivers that will use the clock to control the sample rate.
>> Additional functions to get other types of trigger sources could be
>> added in the future.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
>> check the return value. All callers will need to be updated first before
>> this can be merged.
>>
>> v3 changes:
>> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
>> * added spi_offload_hw_trigger_get_clk() function
>> * fixed missing EXPORT_SYMBOL_GPL
>>
>> v2 changes:
>>
>> This is split out from "spi: add core support for controllers with
>> offload capabilities".
>>
>> Mark suggested that the standard SPI APIs should be aware that the
>> hardware trigger is enabled. So I've added some locking for this. Nuno
>> suggested that this might be overly strict though, and that we should
>> let each individual controller driver decide what to do. For our use
>> case though, I think we generally are going to have a single peripheral
>> on the SPI bus, so this seems like a reasonable starting place anyway.
>> ---
> 
> How explicitly do we want to be about returning errors? It seems that if the
> trigger is enabled we can't anything else on the controller/offload_engine so we
> could very well just hold the controller lock when enabling the trigger and 
> release it when disabling it. Pretty much the same behavior as spi_bus_lock()...

The problem I see with using spi_bus_lock() in it's current form is that
SPI offload triggers could be enabled indefinitely. So any other devices
on the bus that tried to use the bus and take the lock would essentially
deadlock waiting for the offload user to release the lock. This is why
I added the -BUSY return, to avoid this deadlock.

> 
> ...
> 
>>
>> +
>> +/**
>> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
>> + * @spi: SPI device
>> + * @id: Function ID if SPI device uses more than one offload or NULL.
>> + *
>> + * The caller is responsible for calling clk_put() on the returned clock.
>> + *
>> + * Return: The clock for the offload trigger, or negative error code
>> + */
>> +static inline
>> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
>> *id)
>> +{
>> +	struct spi_controller *ctlr = spi->controller;
>> +
>> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
>> +		return ERR_PTR(-EOPNOTSUPP);
>> +
>> +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
>> +}
>>
> 
> It would be nice if we could have some kind of spi abstraction...

After reading your other replies, I think I understand what you mean here.

Are you thinking some kind of `struct spi_offload_trigger` that could be
any kind of trigger (clk, gpio, etc.)?

Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

[Nuno Sá]

On Tue, 2024-07-23 at 09:30 -0500, David Lechner wrote:
> On 7/23/24 2:53 AM, Nuno Sá wrote:
> > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> > > This extends the SPI framework to support hardware triggered offloading.
> > > This allows an arbitrary hardware trigger to be used to start a SPI
> > > transfer that was previously set up with spi_offload_prepare().
> > > 
> > > Since the hardware trigger can happen at any time, this means the SPI
> > > bus must be reserved for exclusive use as long as the hardware trigger
> > > is enabled. Since a hardware trigger could be enabled indefinitely,
> > > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
> > > otherwise this could cause deadlocks. So we introduce a new flag so that
> > > any attempt to lock or use the bus will fail with -EBUSY as long as the
> > > hardware trigger is enabled.
> > > 
> > > Peripheral drivers may need to control the trigger source as well. For
> > > this, we introduce a new spi_offload_hw_trigger_get_clk() function that
> > > can be used to get a clock trigger source. This is intended for used
> > > by ADC drivers that will use the clock to control the sample rate.
> > > Additional functions to get other types of trigger sources could be
> > > added in the future.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > > 
> > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
> > > check the return value. All callers will need to be updated first before
> > > this can be merged.
> > > 
> > > v3 changes:
> > > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> > > * added spi_offload_hw_trigger_get_clk() function
> > > * fixed missing EXPORT_SYMBOL_GPL
> > > 
> > > v2 changes:
> > > 
> > > This is split out from "spi: add core support for controllers with
> > > offload capabilities".
> > > 
> > > Mark suggested that the standard SPI APIs should be aware that the
> > > hardware trigger is enabled. So I've added some locking for this. Nuno
> > > suggested that this might be overly strict though, and that we should
> > > let each individual controller driver decide what to do. For our use
> > > case though, I think we generally are going to have a single peripheral
> > > on the SPI bus, so this seems like a reasonable starting place anyway.
> > > ---
> > 
> > How explicitly do we want to be about returning errors? It seems that if the
> > trigger is enabled we can't anything else on the controller/offload_engine so we
> > could very well just hold the controller lock when enabling the trigger and 
> > release it when disabling it. Pretty much the same behavior as spi_bus_lock()...
> 
> The problem I see with using spi_bus_lock() in it's current form is that
> SPI offload triggers could be enabled indefinitely. So any other devices
> on the bus that tried to use the bus and take the lock would essentially
> deadlock waiting for the offload user to release the lock. This is why
> I added the -BUSY return, to avoid this deadlock.
> 

If someone does not disable the trigger at some point that's a bug. If I understood
things correctly, even if someone tries to access the bus will get -EBUSY. But yeah,
arguably it's better to get a valid error rather than blocking/sleeping

> > 
> > ...
> > 
> > > 
> > > +
> > > +/**
> > > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
> > > + * @spi: SPI device
> > > + * @id: Function ID if SPI device uses more than one offload or NULL.
> > > + *
> > > + * The caller is responsible for calling clk_put() on the returned clock.
> > > + *
> > > + * Return: The clock for the offload trigger, or negative error code
> > > + */
> > > +static inline
> > > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
> > > *id)
> > > +{
> > > +	struct spi_controller *ctlr = spi->controller;
> > > +
> > > +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
> > > +		return ERR_PTR(-EOPNOTSUPP);
> > > +
> > > +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
> > > +}
> > > 
> > 
> > It would be nice if we could have some kind of spi abstraction...
> 
> After reading your other replies, I think I understand what you mean here.
> 
> Are you thinking some kind of `struct spi_offload_trigger` that could be
> any kind of trigger (clk, gpio, etc.)?
> 

Yeah, something in that direction...

- Nuno Sá

Re: [PATCH RFC v3 3/9] spi: add support for hardware triggered offload

[Jonathan Cameron]

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

> This extends the SPI framework to support hardware triggered offloading.
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_offload_prepare().
> 
> Since the hardware trigger can happen at any time, this means the SPI
> bus must be reserved for exclusive use as long as the hardware trigger
> is enabled. Since a hardware trigger could be enabled indefinitely,
> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions,
> otherwise this could cause deadlocks. So we introduce a new flag so that
> any attempt to lock or use the bus will fail with -EBUSY as long as the
> hardware trigger is enabled.
> 
> Peripheral drivers may need to control the trigger source as well. For
> this, we introduce a new spi_offload_hw_trigger_get_clk() function that
> can be used to get a clock trigger source. This is intended for used
> by ADC drivers that will use the clock to control the sample rate.
> Additional functions to get other types of trigger sources could be
> added in the future.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers
> check the return value. All callers will need to be updated first before
> this can be merged.

If it's going to fail sometimes, probably needs a name that indicates
that. I'm not sure spi_bus_try_lock() is appropriate though.

> 
> v3 changes:
> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> * added spi_offload_hw_trigger_get_clk() function
> * fixed missing EXPORT_SYMBOL_GPL
> 
> v2 changes:
> 
> This is split out from "spi: add core support for controllers with
> offload capabilities".
> 
> Mark suggested that the standard SPI APIs should be aware that the
> hardware trigger is enabled. So I've added some locking for this. Nuno
> suggested that this might be overly strict though, and that we should
> let each individual controller driver decide what to do. For our use
> case though, I think we generally are going to have a single peripheral
> on the SPI bus, so this seems like a reasonable starting place anyway.

...

> +int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id)
> +{
> +	struct spi_controller *ctlr = spi->controller;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&ctlr->bus_lock_mutex);
> +
> +	if (ctlr->offload_hw_trigger_mode_enabled) {
> +		mutex_unlock(&ctlr->bus_lock_mutex);
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
> +	ctlr->offload_hw_trigger_mode_enabled = true;
Why do you need to take the spinlock when setting this to true, but not when
setting it to fast (in the error path below)?

> +	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
> +
> +	/* TODO: how to wait for empty message queue? */
> +
> +	mutex_lock(&ctlr->io_mutex);
> +	ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id);
> +	mutex_unlock(&ctlr->io_mutex);
> +
> +	if (ret) {
> +		ctlr->offload_hw_trigger_mode_enabled = false;
> +		mutex_unlock(&ctlr->bus_lock_mutex);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&ctlr->bus_lock_mutex);
> +
> +	return 0;
> +}
> 

[PATCH RFC v3 4/9] spi: add offload TX/RX streaming APIs

[David Lechner]

Most configuration of SPI offloads is handled opaquely using the
function ID that is passed to the various offload functions. However,
there are some offload features that need to be controlled on a per
transfer basis.

This patch adds a flag field to the spi_transfer structure to allow
specifying such features. The first feature to be added is the ability
to stream data to/from a hardware sink/source rather than using a tx or
rx buffer. Additional flags can be added in the future as needed.

A flags field is also added to the controller struct for controllers to
indicate which flags are supported. This allows for generic checking of
offload capabilities during __spi_validate() so that each controller
doesn't have to provide their own validation.

As a first users of this streaming capability, getter functions are
added to get a DMA channel that is directly connected to the offload.
Peripheral drivers will use this to get a DMA channel and configure it
to suit their needs.

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

v3 changes:
* Added spi_offload_{tx,rx}_stream_get_dma_chan() functions.

v2 changes:

This is also split out from "spi: add core support for controllers with
offload capabilities".

In the previous version, we were using (void *)-1 as a sentinel value
that could be assigned, e.g. to rx_buf. But this was naive since there
is core code that would try to dereference this pointer. So instead,
we've added a new flags field to the spi_transfer structure for this
sort of thing. This also has the advantage of being able to be used in
the future for other arbitrary features.
---
 drivers/spi/spi.c       |  9 +++++++
 include/linux/spi/spi.h | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7488e71f159f..0ee741d911d3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4214,6 +4214,15 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 
 		if (_spi_xfer_word_delay_update(xfer, spi))
 			return -EINVAL;
+
+		/* make sure controller supports required offload features */
+		if (xfer->offload_flags) {
+			if (!message->offload)
+				return -EINVAL;
+
+			if (xfer->offload_flags & ~ctlr->offload_xfer_flags)
+				return -EINVAL;
+		}
 	}
 
 	message->status = -EINPROGRESS;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 685548883004..a4a7449c4259 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -500,6 +500,8 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
  *	     This field is optional and should only be implemented if the
  *	     controller has native support for memory like operations.
  * @mem_caps: controller capabilities for the handling of memory operations.
+ * @offload_xfer_flags: flags supported by this controller for offloading
+ *	transfers. See struct spi_transfer for the list of flags.
  * @offload_ops: operations for controllers with offload support.
  * @unprepare_message: undo any work done by prepare_message().
  * @slave_abort: abort the ongoing transfer request on an SPI slave controller
@@ -752,6 +754,7 @@ struct spi_controller {
 	const struct spi_controller_mem_caps *mem_caps;
 
 	/* Operations for controllers with offload support. */
+	unsigned int offload_xfer_flags;
 	const struct spi_controller_offload_ops *offload_ops;
 
 	/* GPIO chip select */
@@ -995,6 +998,7 @@ struct spi_res {
  * @rx_sg_mapped: If true, the @rx_sg is mapped for DMA
  * @tx_sg: Scatterlist for transmit, currently not for client use
  * @rx_sg: Scatterlist for receive, currently not for client use
+ * @offload_flags: flags for xfers that use special hardware offload features
  * @ptp_sts_word_pre: The word (subject to bits_per_word semantics) offset
  *	within @tx_buf for which the SPI device is requesting that the time
  *	snapshot for this transfer begins. Upon completing the SPI transfer,
@@ -1115,6 +1119,12 @@ struct spi_transfer {
 
 	u32		effective_speed_hz;
 
+	unsigned int	offload_flags;
+/* this is write xfer but TX uses external data stream rather than tx_buf */
+#define SPI_OFFLOAD_XFER_TX_STREAM	BIT(0)
+/* this is read xfer but RX uses external data stream rather than rx_buf */
+#define SPI_OFFLOAD_XFER_RX_STREAM	BIT(1)
+
 	unsigned int	ptp_sts_word_pre;
 	unsigned int	ptp_sts_word_post;
 
@@ -1622,6 +1632,20 @@ struct spi_controller_offload_ops {
 	 * hardware offload trigger that is connected to a clock.
 	 */
 	struct clk *(*hw_trigger_get_clk)(struct spi_device *spi, const char *id);
+	/**
+	 * @tx_stream_get_dma_chan: Optional callback for controllers that have
+	 * an offload where the TX data stream is connected directly to a DMA
+	 * channel.
+	 */
+	struct dma_chan *(*tx_stream_get_dma_chan)(struct spi_device *spi,
+						   const char *id);
+	/**
+	 * @rx_stream_get_dma_chan: Optional callback for controllers that have
+	 * an offload where the RX data stream is connected directly to a DMA
+	 * channel.
+	 */
+	struct dma_chan *(*rx_stream_get_dma_chan)(struct spi_device *spi,
+						   const char *id);
 };
 
 extern int spi_offload_prepare(struct spi_device *spi, const char *id,
@@ -1651,6 +1675,54 @@ struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char *i
 	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
 }
 
+/**
+ * spi_offload_tx_stream_get_dma_chan - Get the DMA channel for the TX stream
+ * @spi: SPI device
+ * @id: Function ID if SPI device uses more than one offload or NULL.
+ *
+ * This is the DMA channel that will provide data to transfers that use the
+ * %SPI_OFFLOAD_XFER_TX_STREAM offload flag.
+ *
+ * The caller is responsible for calling dma_release_channel() on the returned
+ * DMA channel.
+ *
+ * Return: The DMA channel for the TX stream, or negative error code
+ */
+static inline struct dma_chan
+*spi_offload_tx_stream_get_dma_chan(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->tx_stream_get_dma_chan)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	return ctlr->offload_ops->tx_stream_get_dma_chan(spi, id);
+}
+
+/**
+ * spi_offload_rx_stream_get_dma_chan - Get the DMA channel for the RX stream
+ * @spi: SPI device
+ * @id: Function ID if SPI device uses more than one offload or NULL.
+ *
+ * This is the DMA channel that will receive data from transfers that use the
+ * %SPI_OFFLOAD_XFER_RX_STREAM offload flag.
+ *
+ * The caller is responsible for calling dma_release_channel() on the returned
+ * DMA channel.
+ *
+ * Return: The DMA channel for the RX stream, or negative error code
+ */
+static inline struct dma_chan
+*spi_offload_rx_stream_get_dma_chan(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->rx_stream_get_dma_chan)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	return ctlr->offload_ops->rx_stream_get_dma_chan(spi, id);
+}
+
 /*---------------------------------------------------------------------------*/
 
 /*

-- 
2.43.0

[PATCH RFC v3 5/9] spi: dt-bindings: axi-spi-engine: document spi-offloads

[David Lechner]

The AXI SPI Engine has support for hardware offloading capabilities.
There can be up to 32 offload instances per SPI controller, so the
bindings limit the value accordingly.

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

RFC: I have a few questions about this one...

1.  The trigger-source properties are borrowed from the leds bindings.
    Do we want to promote this to a generic binding that can be used by
    any type of device?

2.  Some folks are working on adding DMA to TX stream support to the
    AXI SPI Engine hardware. I assume that the `dmas` property is like
    others where the order/index in the phandle array matters. So this
    would mean that for device that only uses 1 out of the 32 offloads
    and only uses 1 TX DMA channel, we would have to have 32 <0>s for
    each of the possible RX dmas in the array. Any way to do some kind
    of mapping to avoid this?

3.  In v2, we discussed about having some sort of data processing unit
    between the AXI SPI Engine RX stream interface and the DMA channel
    interface on the DMA controller. I haven't included this in the
    bindings yet because we don't have a user yet. But it was suggested
    that we could use the graph bindings for this. So here is what that
    might look like:

    Additional property for the AXI SPI Engine controller bindings:

        out-ports:
            $ref: /schemas/graph.yaml#/properties/ports
            unevaluatedProperties: false
            patternProperties:
            "^port@1?[0-9a-f]$":
                $ref: /schemas/graph.yaml#/properties/port
                unevaluatedProperties: false

    And this would be connected to a device node similar to this:

        ip-block@3000 {
            // Something similar to, but not exactly like
            // http://analogdevicesinc.github.io/hdl/library/util_extract/index.html
            compatible = "adi,crc-check";
            // clock that runs this IP block
            clocks = <&sysclk 15>;
            // interrupt raised on bad CRC
            interrupts = <&intc 99>;
            interrupt-names = "crc";
            // output stream with CRC byte removed piped to DMA
            dmas = <&adc_dma 0>;
            dma-names = "rx";

            port {
                adc_crc_check: endpoint {
                    remote-endpoint: <&offload0_rx>;
                };
            };
        };

    Does this sound reasonable?

v3 changes:
* Added #spi-offload-cells property.
* Added properties for triggers and RX data stream connected to DMA.

v2 changes:

This is basically a new patch. It partially replaces "dt-bindings: iio:
offload: add binding for PWM/DMA triggered buffer".

The controller no longer has an offloads object node and the
spi-offloads property is now a standard SPI peripheral property.
---
 .../bindings/spi/adi,axi-spi-engine.yaml           | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
index d48faa42d025..ec18eabb993a 100644
--- a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
+++ b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
@@ -41,6 +41,42 @@ properties:
       - const: s_axi_aclk
       - const: spi_clk
 
+  '#spi-offload-cells':
+    description: The cell value is the offload instance number.
+    const: 1
+
+  trigger-sources:
+    description:
+      An array of trigger source phandles for offload instances. The index in
+      the array corresponds to the offload instance number.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  dmas:
+    description:
+      DMA channels connected to the output stream interface of an offload instance.
+    minItems: 1
+    maxItems: 32
+
+  dma-names:
+    minItems: 1
+    maxItems: 32
+    items:
+      pattern: "^offload(?:[12]?[0-9]|3[01])-rx$"
+
+patternProperties:
+  "^.*@[0-9a-f]+$":
+    type: object
+    $ref: spi-peripheral-props.yaml
+    additionalProperties: true
+    properties:
+      spi-offloads:
+        description:
+          An array of 1 or more offload instance numbers assigned to this
+          peripheral.
+        items:
+          minimum: 0
+          maximum: 31
+
 required:
   - compatible
   - reg
@@ -59,6 +95,11 @@ examples:
         clocks = <&clkc 15>, <&clkc 15>;
         clock-names = "s_axi_aclk", "spi_clk";
 
+        #spi-offload-cells = <1>;
+        trigger-sources = <&trigger_clock>;
+        dmas = <&dma 0>;
+        dma-names = "offload0-rx";
+
         #address-cells = <1>;
         #size-cells = <0>;
 

-- 
2.43.0

Re: [PATCH RFC v3 5/9] spi: dt-bindings: axi-spi-engine: document spi-offloads

[Rob Herring]

On Mon, Jul 22, 2024 at 04:57:12PM -0500, David Lechner wrote:
> The AXI SPI Engine has support for hardware offloading capabilities.
> There can be up to 32 offload instances per SPI controller, so the
> bindings limit the value accordingly.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> RFC: I have a few questions about this one...
> 
> 1.  The trigger-source properties are borrowed from the leds bindings.
>     Do we want to promote this to a generic binding that can be used by
>     any type of device?

I would make it specific to spi-offload.

> 
> 2.  Some folks are working on adding DMA to TX stream support to the
>     AXI SPI Engine hardware. I assume that the `dmas` property is like
>     others where the order/index in the phandle array matters. So this
>     would mean that for device that only uses 1 out of the 32 offloads
>     and only uses 1 TX DMA channel, we would have to have 32 <0>s for
>     each of the possible RX dmas in the array. Any way to do some kind
>     of mapping to avoid this?

That's why -names exists.

> 
> 3.  In v2, we discussed about having some sort of data processing unit
>     between the AXI SPI Engine RX stream interface and the DMA channel
>     interface on the DMA controller. I haven't included this in the
>     bindings yet because we don't have a user yet. But it was suggested
>     that we could use the graph bindings for this. So here is what that
>     might look like:
> 
>     Additional property for the AXI SPI Engine controller bindings:
> 
>         out-ports:
>             $ref: /schemas/graph.yaml#/properties/ports
>             unevaluatedProperties: false
>             patternProperties:
>             "^port@1?[0-9a-f]$":
>                 $ref: /schemas/graph.yaml#/properties/port
>                 unevaluatedProperties: false
> 
>     And this would be connected to a device node similar to this:
> 
>         ip-block@3000 {
>             // Something similar to, but not exactly like
>             // http://analogdevicesinc.github.io/hdl/library/util_extract/index.html
>             compatible = "adi,crc-check";
>             // clock that runs this IP block
>             clocks = <&sysclk 15>;
>             // interrupt raised on bad CRC
>             interrupts = <&intc 99>;
>             interrupt-names = "crc";
>             // output stream with CRC byte removed piped to DMA
>             dmas = <&adc_dma 0>;
>             dma-names = "rx";
> 
>             port {
>                 adc_crc_check: endpoint {
>                     remote-endpoint: <&offload0_rx>;
>                 };
>             };
>         };
> 
>     Does this sound reasonable?

Shrug.

Unlike the offload which is internal to the controller driver?, isn't 
this specific to the device because it needs to be aware of any 
processing done or not done. Or maybe it wants to configure the 
processing.

OTOH, maybe this isn't any different than offload?

Rob

Re: [PATCH RFC v3 5/9] spi: dt-bindings: axi-spi-engine: document spi-offloads

[David Lechner]

On 7/26/24 7:38 AM, Rob Herring wrote:
> On Mon, Jul 22, 2024 at 04:57:12PM -0500, David Lechner wrote:
>> The AXI SPI Engine has support for hardware offloading capabilities.
>> There can be up to 32 offload instances per SPI controller, so the
>> bindings limit the value accordingly.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> RFC: I have a few questions about this one...
>>
>> 1.  The trigger-source properties are borrowed from the leds bindings.
>>     Do we want to promote this to a generic binding that can be used by
>>     any type of device?
> 
> I would make it specific to spi-offload.

OK

Meanwhile, we are working on some other ADCs (without SPI offload) and
finding that they are using basically the same sorts of triggers. And
on the driver side of things in this series, I'm getting feedback that
we should have some sort of generic trigger device rather than using,
e.g. a clk directly. If we need this same sort of trigger abstraction
for both SPI offloads and IIO device, it does seems like we might want
to consider something like a new trigger subsystem.

> 
>>
>> 2.  Some folks are working on adding DMA to TX stream support to the
>>     AXI SPI Engine hardware. I assume that the `dmas` property is like
>>     others where the order/index in the phandle array matters. So this
>>     would mean that for device that only uses 1 out of the 32 offloads
>>     and only uses 1 TX DMA channel, we would have to have 32 <0>s for
>>     each of the possible RX dmas in the array. Any way to do some kind
>>     of mapping to avoid this?
> 
> That's why -names exists.

OK

> 
>>
>> 3.  In v2, we discussed about having some sort of data processing unit
>>     between the AXI SPI Engine RX stream interface and the DMA channel
>>     interface on the DMA controller. I haven't included this in the
>>     bindings yet because we don't have a user yet. But it was suggested
>>     that we could use the graph bindings for this. So here is what that
>>     might look like:
>>
>>     Additional property for the AXI SPI Engine controller bindings:
>>
>>         out-ports:
>>             $ref: /schemas/graph.yaml#/properties/ports
>>             unevaluatedProperties: false
>>             patternProperties:
>>             "^port@1?[0-9a-f]$":
>>                 $ref: /schemas/graph.yaml#/properties/port
>>                 unevaluatedProperties: false
>>
>>     And this would be connected to a device node similar to this:
>>
>>         ip-block@3000 {
>>             // Something similar to, but not exactly like
>>             // http://analogdevicesinc.github.io/hdl/library/util_extract/index.html
>>             compatible = "adi,crc-check";
>>             // clock that runs this IP block
>>             clocks = <&sysclk 15>;
>>             // interrupt raised on bad CRC
>>             interrupts = <&intc 99>;
>>             interrupt-names = "crc";
>>             // output stream with CRC byte removed piped to DMA
>>             dmas = <&adc_dma 0>;
>>             dma-names = "rx";
>>
>>             port {
>>                 adc_crc_check: endpoint {
>>                     remote-endpoint: <&offload0_rx>;
>>                 };
>>             };
>>         };
>>
>>     Does this sound reasonable?
> 
> Shrug.
> 
> Unlike the offload which is internal to the controller driver?

Correct. And in the case of the AXI SPI Engine, the offload is
part of the controller IP block in hardware as well.

> isn't 
> this specific to the device because it needs to be aware of any 
> processing done or not done. Or maybe it wants to configure the 
> processing.

Yes, the SPI peripheral driver would be the one needing to know
what sort of data processing unit it is connected to so it knows
how to configure the chip and how to interpret the received data
or other signals from the data processing unit.

> 
> OTOH, maybe this isn't any different than offload?

Also true since the SPI peripheral needs to know what kind of
capabilities that the offload itself has.

> 
> Rob

Re: [PATCH RFC v3 5/9] spi: dt-bindings: axi-spi-engine: document spi-offloads

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

On Fri, Jul 26, 2024 at 02:17:00PM -0500, David Lechner wrote:
> On 7/26/24 7:38 AM, Rob Herring wrote:
> > On Mon, Jul 22, 2024 at 04:57:12PM -0500, David Lechner wrote:
> >> The AXI SPI Engine has support for hardware offloading capabilities.
> >> There can be up to 32 offload instances per SPI controller, so the
> >> bindings limit the value accordingly.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---
> >>
> >> RFC: I have a few questions about this one...
> >>
> >> 1.  The trigger-source properties are borrowed from the leds bindings.
> >>     Do we want to promote this to a generic binding that can be used by
> >>     any type of device?
> > 
> > I would make it specific to spi-offload.
> 
> OK
> 
> Meanwhile, we are working on some other ADCs (without SPI offload) and
> finding that they are using basically the same sorts of triggers. And
> on the driver side of things in this series, I'm getting feedback that
> we should have some sort of generic trigger device rather than using,
> e.g. a clk directly. If we need this same sort of trigger abstraction
> for both SPI offloads and IIO device, it does seems like we might want
> to consider something like a new trigger subsystem.

A "device" in the sense that "pwm-clk" is a device I suppose. Are any of
these other things WIP on the lists (that I may have missed while I was
away) or are they still something you're working on internally.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC v3 5/9] spi: dt-bindings: axi-spi-engine: document spi-offloads

[David Lechner]

On Wed, Aug 14, 2024 at 10:58 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Jul 26, 2024 at 02:17:00PM -0500, David Lechner wrote:
> > On 7/26/24 7:38 AM, Rob Herring wrote:
> > > On Mon, Jul 22, 2024 at 04:57:12PM -0500, David Lechner wrote:
> > >> The AXI SPI Engine has support for hardware offloading capabilities.
> > >> There can be up to 32 offload instances per SPI controller, so the
> > >> bindings limit the value accordingly.
> > >>
> > >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> > >> ---
> > >>
> > >> RFC: I have a few questions about this one...
> > >>
> > >> 1.  The trigger-source properties are borrowed from the leds bindings.
> > >>     Do we want to promote this to a generic binding that can be used by
> > >>     any type of device?
> > >
> > > I would make it specific to spi-offload.
> >
> > OK
> >
> > Meanwhile, we are working on some other ADCs (without SPI offload) and
> > finding that they are using basically the same sorts of triggers. And
> > on the driver side of things in this series, I'm getting feedback that
> > we should have some sort of generic trigger device rather than using,
> > e.g. a clk directly. If we need this same sort of trigger abstraction
> > for both SPI offloads and IIO device, it does seems like we might want
> > to consider something like a new trigger subsystem.
>
> A "device" in the sense that "pwm-clk" is a device I suppose. >

In simple cases, yes it could be like "pwm-clk" where a PWM/clock/gpio
is used directly as the trigger. We also have a case where there is a
PWM output combined with a clock output using an AND gate, so more of
a "real" device. And finally, there is actual dedicated hardware, like
this [1] time division duplexing (TDD) controller that, in addition to
it's primary purpose for RF applications, can be used as a general
purpose trigger source - mostly useful for creating burst of a finite
number of pulses.

[1]: http://analogdevicesinc.github.io/hdl/library/axi_tdd/index.html

> Are any of
> these other things WIP on the lists (that I may have missed while I was
> away) or are they still something you're working on internally.

My ideas on actual trigger devices and bindings are still mostly on
paper, but we do have a couple of ADCs on the mailing lists right now
where I think it would make more sense to have a flexible "trigger"
but we have been making due with what is currently available.

ad7525

In this case, we need two coordinated triggers for the CNV and CLK
signals, one that generates a single pulse and one that generates a
burst of 16 or 18 pulses, both repeating periodically. Right now, the
proposed DT bindings only allow specifying a PWM to provide the CNV
signal and a second PWM combined with a clock and an AND gate (same
one mentioned above) to provide the CLK signal because that is the
reference hardware design. But technically if one wanted to use, for
example, the aforementioned TDD controller to create these signals for
CNV and CLK instead, it should work just the same.

[ad7525]: https://lore.kernel.org/linux-iio/20240809-ad7625_r1-v2-0-f85e7ac83150@baylibre.com/

ad4030

This also needs a CNV trigger, but it works slightly differently than
ad7525. For now, the proposed DT bindings just have a cnv-gpios to
describe what is connected to the CNV pin. But for certain
applications, a GPIO is not the best choice. For example, to use the
oversampling feature of the chip, we have to provide a burst of some
power of 2 pulses, up to 16k pulses, with specific timing to trigger
2**N conversions before reading one sample. This can be done by
bit-banging the GPIO but could be done much better/faster by something
like the TDD controller that is specifically designed to create a
burst of a finite number of pulses.

[ad4030]: https://lore.kernel.org/linux-iio/20240627-eblanc-ad4630_v1-v1-0-fdc0610c23b0@baylibre.com/

Having a generic DT binding for these ADC input pins that can be
connected to a wide variety of outputs seems more future proof than
having to modify the bindings each time someone wants to support a new
type of output provider.

[PATCH RFC v3 6/9] spi: axi-spi-engine: implement offload support

[David Lechner]

This implements SPI offload support for the AXI SPI Engine. Currently,
the hardware only supports triggering offload transfers with a hardware
trigger so attempting to use an offload message in the regular SPI
message queue will fail. Also, only allows streaming rx data to an
external sink, so attempts to use a rx_buf in the offload message will
fail.

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

v3 changes:
* Added clk and dma_chan getter callbacks.
* Fixed some bugs.

v2 changes:

This patch has been reworked to accommodate the changes described in all
of the other patches.
---
 drivers/spi/spi-axi-spi-engine.c | 341 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 337 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index cb3fdcbca2be..314f3afb9357 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -2,11 +2,13 @@
 /*
  * SPI-Engine SPI controller driver
  * Copyright 2015 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
  *  Author: Lars-Peter Clausen <lars@metafoo.de>
  */
 
 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/dmaengine.h>
 #include <linux/fpga/adi-axi-common.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -16,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 
+#define SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH	0x10
 #define SPI_ENGINE_REG_RESET			0x40
 
 #define SPI_ENGINE_REG_INT_ENABLE		0x80
@@ -23,6 +26,7 @@
 #define SPI_ENGINE_REG_INT_SOURCE		0x88
 
 #define SPI_ENGINE_REG_SYNC_ID			0xc0
+#define SPI_ENGINE_REG_OFFLOAD_SYNC_ID		0xc4
 
 #define SPI_ENGINE_REG_CMD_FIFO_ROOM		0xd0
 #define SPI_ENGINE_REG_SDO_FIFO_ROOM		0xd4
@@ -33,10 +37,24 @@
 #define SPI_ENGINE_REG_SDI_DATA_FIFO		0xe8
 #define SPI_ENGINE_REG_SDI_DATA_FIFO_PEEK	0xec
 
+#define SPI_ENGINE_MAX_NUM_OFFLOADS		32
+
+#define SPI_ENGINE_REG_OFFLOAD_CTRL(x)		(0x100 + SPI_ENGINE_MAX_NUM_OFFLOADS * (x))
+#define SPI_ENGINE_REG_OFFLOAD_STATUS(x)	(0x104 + SPI_ENGINE_MAX_NUM_OFFLOADS * (x))
+#define SPI_ENGINE_REG_OFFLOAD_RESET(x)		(0x108 + SPI_ENGINE_MAX_NUM_OFFLOADS * (x))
+#define SPI_ENGINE_REG_OFFLOAD_CMD_FIFO(x)	(0x110 + SPI_ENGINE_MAX_NUM_OFFLOADS * (x))
+#define SPI_ENGINE_REG_OFFLOAD_SDO_FIFO(x)	(0x114 + SPI_ENGINE_MAX_NUM_OFFLOADS * (x))
+
+#define SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_SDO	GENMASK(15, 8)
+#define SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_CMD	GENMASK(7, 0)
+
 #define SPI_ENGINE_INT_CMD_ALMOST_EMPTY		BIT(0)
 #define SPI_ENGINE_INT_SDO_ALMOST_EMPTY		BIT(1)
 #define SPI_ENGINE_INT_SDI_ALMOST_FULL		BIT(2)
 #define SPI_ENGINE_INT_SYNC			BIT(3)
+#define SPI_ENGINE_INT_OFFLOAD_SYNC		BIT(4)
+
+#define SPI_ENGINE_OFFLOAD_CTRL_ENABLE		BIT(0)
 
 #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
 #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
@@ -77,6 +95,10 @@
 #define SPI_ENGINE_CMD_CS_INV(flags) \
 	SPI_ENGINE_CMD(SPI_ENGINE_INST_CS_INV, 0, (flags))
 
+/* default sizes - can be changed when SPI Engine firmware is compiled */
+#define SPI_ENGINE_OFFLOAD_CMD_FIFO_SIZE	16
+#define SPI_ENGINE_OFFLOAD_SDO_FIFO_SIZE	16
+
 struct spi_engine_program {
 	unsigned int length;
 	uint16_t instructions[] __counted_by(length);
@@ -104,6 +126,12 @@ struct spi_engine_message_state {
 	uint8_t *rx_buf;
 };
 
+struct spi_engine_offload {
+	struct spi_device *spi;
+	const char *id;
+	bool prepared;
+};
+
 struct spi_engine {
 	struct clk *clk;
 	struct clk *ref_clk;
@@ -116,6 +144,10 @@ struct spi_engine {
 	unsigned int int_enable;
 	/* shadows hardware CS inversion flag state */
 	u8 cs_inv;
+
+	unsigned int offload_ctrl_mem_size;
+	unsigned int offload_sdo_mem_size;
+	struct spi_engine_offload offload_priv[SPI_ENGINE_MAX_NUM_OFFLOADS];
 };
 
 static void spi_engine_program_add_cmd(struct spi_engine_program *p,
@@ -159,7 +191,7 @@ static void spi_engine_gen_xfer(struct spi_engine_program *p, bool dry,
 
 		if (xfer->tx_buf)
 			flags |= SPI_ENGINE_TRANSFER_WRITE;
-		if (xfer->rx_buf)
+		if (xfer->rx_buf || (xfer->offload_flags & SPI_OFFLOAD_XFER_RX_STREAM))
 			flags |= SPI_ENGINE_TRANSFER_READ;
 
 		spi_engine_program_add_cmd(p, dry,
@@ -211,16 +243,24 @@ static void spi_engine_gen_cs(struct spi_engine_program *p, bool dry,
  *
  * NB: This is separate from spi_engine_compile_message() because the latter
  * is called twice and would otherwise result in double-evaluation.
+ *
+ * Returns 0 on success, -EINVAL on failure.
  */
-static void spi_engine_precompile_message(struct spi_message *msg)
+static int spi_engine_precompile_message(struct spi_message *msg)
 {
 	unsigned int clk_div, max_hz = msg->spi->controller->max_speed_hz;
 	struct spi_transfer *xfer;
 
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		/* If we have an offload transfer, we can't rx to buffer */
+		if (msg->offload && xfer->rx_buf)
+			return -EINVAL;
+
 		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
 		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
 	}
+
+	return 0;
 }
 
 static void spi_engine_compile_message(struct spi_message *msg, bool dry,
@@ -518,8 +558,11 @@ static irqreturn_t spi_engine_irq(int irq, void *devid)
 static int spi_engine_optimize_message(struct spi_message *msg)
 {
 	struct spi_engine_program p_dry, *p;
+	int ret;
 
-	spi_engine_precompile_message(msg);
+	ret = spi_engine_precompile_message(msg);
+	if (ret)
+		return ret;
 
 	p_dry.length = 0;
 	spi_engine_compile_message(msg, true, &p_dry);
@@ -531,7 +574,7 @@ static int spi_engine_optimize_message(struct spi_message *msg)
 	spi_engine_compile_message(msg, false, p);
 
 	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(
-						AXI_SPI_ENGINE_CUR_MSG_SYNC_ID));
+		msg->offload ? 0 : AXI_SPI_ENGINE_CUR_MSG_SYNC_ID));
 
 	msg->opt_state = p;
 
@@ -577,6 +620,12 @@ static int spi_engine_transfer_one_message(struct spi_controller *host,
 	unsigned int int_enable = 0;
 	unsigned long flags;
 
+	if (msg->offload) {
+		dev_err(&host->dev, "Single transfer offload not supported\n");
+		msg->status = -EOPNOTSUPP;
+		goto out;
+	}
+
 	/* reinitialize message state for this transfer */
 	memset(st, 0, sizeof(*st));
 	st->cmd_buf = p->instructions;
@@ -612,11 +661,279 @@ static int spi_engine_transfer_one_message(struct spi_controller *host,
 		msg->status = -ETIMEDOUT;
 	}
 
+out:
 	spi_finalize_current_message(host);
 
 	return msg->status;
 }
 
+static bool spi_engine_offload_id_eq(const char *id1, const char *id2)
+{
+	if (!id1 && !id2)
+		return true;
+
+	if (!id1 || !id2)
+		return false;
+
+	return strcmp(id1, id2) == 0;
+}
+
+static struct spi_engine_offload *spi_engine_get_offload(struct spi_device *spi,
+							 const char *id,
+							 unsigned int *offload_num)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	int i;
+
+	for (i = 0; i < SPI_ENGINE_MAX_NUM_OFFLOADS; i++) {
+		priv = &spi_engine->offload_priv[i];
+
+		if (priv->spi == spi && spi_engine_offload_id_eq(priv->id, id)) {
+			*offload_num = i;
+			return priv;
+		}
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+static int spi_engine_offload_map_channel(struct spi_device *spi,
+					  const char *id,
+					  const unsigned int *args,
+					  unsigned int num_args)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+
+	if (num_args != 1)
+		return -EINVAL;
+
+	if (args[0] >= SPI_ENGINE_MAX_NUM_OFFLOADS)
+		return -EINVAL;
+
+	priv = &spi_engine->offload_priv[args[0]];
+
+	if (priv->spi)
+		return -EBUSY;
+
+	priv->spi = spi;
+
+	priv->id = kstrdup(id, GFP_KERNEL);
+	if (!priv->id && id)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int spi_engine_offload_prepare(struct spi_device *spi, const char *id,
+				      struct spi_message *msg)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_program *p = msg->opt_state;
+	struct spi_engine_offload *priv;
+	struct spi_transfer *xfer;
+	void __iomem *cmd_addr;
+	void __iomem *sdo_addr;
+	size_t tx_word_count = 0;
+	unsigned int offload_num, i;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
+
+	if (priv->prepared)
+		return -EBUSY;
+
+	if (p->length > spi_engine->offload_ctrl_mem_size)
+		return -EINVAL;
+
+	/* count total number of tx words in message */
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (!xfer->tx_buf)
+			continue;
+
+		if (xfer->bits_per_word <= 8)
+			tx_word_count += xfer->len;
+		else if (xfer->bits_per_word <= 16)
+			tx_word_count += xfer->len / 2;
+		else
+			tx_word_count += xfer->len / 4;
+	}
+
+	if (tx_word_count > spi_engine->offload_sdo_mem_size)
+		return -EINVAL;
+
+	cmd_addr = spi_engine->base + SPI_ENGINE_REG_OFFLOAD_CMD_FIFO(offload_num);
+	sdo_addr = spi_engine->base + SPI_ENGINE_REG_OFFLOAD_SDO_FIFO(offload_num);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (!xfer->tx_buf)
+			continue;
+
+		if (xfer->bits_per_word <= 8) {
+			const u8 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		} else if (xfer->bits_per_word <= 16) {
+			const u16 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len / 2; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		} else {
+			const u32 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len / 4; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		}
+	}
+
+	for (i = 0; i < p->length; i++)
+		writel_relaxed(p->instructions[i], cmd_addr);
+
+	msg->offload_state = (void *)(intptr_t)offload_num;
+	priv->prepared = true;
+
+	return 0;
+}
+
+static void spi_engine_offload_unprepare(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv)) {
+		dev_warn(&spi->dev, "failed match offload in unprepare\n");
+		return;
+	}
+
+	writel_relaxed(1, spi_engine->base + SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
+	writel_relaxed(0, spi_engine->base + SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
+
+	priv->prepared = false;
+}
+
+static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
+					     const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num, reg;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
+
+	reg = readl_relaxed(spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
+	writel_relaxed(reg, spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+
+	return 0;
+}
+
+static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
+					       const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num, reg;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv)) {
+		dev_warn(&spi->dev, "failed match offload in disable\n");
+		return;
+	}
+
+	reg = readl_relaxed(spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
+	writel_relaxed(reg, spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+}
+
+static struct clk *spi_engine_hw_trigger_get_clk(struct spi_device *spi,
+						 const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine_offload *priv;
+	struct of_phandle_args clkspec;
+	unsigned int offload_num;
+	struct clk *clk;
+	int ret;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return ERR_CAST(priv);
+
+	ret = of_parse_phandle_with_args(host->dev.of_node, "trigger-sources",
+					 "#trigger-source-cells", offload_num,
+					 &clkspec);
+	if (ret)
+		return ERR_PTR(ret);
+
+	clk = of_clk_get_from_provider(&clkspec);
+
+	of_node_put(clkspec.np);
+
+	return clk;
+}
+
+static struct dma_chan *spi_engine_rx_stream_get_dma_chan(struct spi_device *spi,
+							  const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine_offload *priv;
+	unsigned int offload_num;
+	char channel_name[16];
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return ERR_CAST(priv);
+
+	snprintf(channel_name, sizeof(channel_name), "offload%d-rx", offload_num);
+
+	return dma_request_chan(&host->dev, channel_name);
+}
+
+static const struct spi_controller_offload_ops spi_engine_offload_ops = {
+	.map_channel = spi_engine_offload_map_channel,
+	.prepare = spi_engine_offload_prepare,
+	.unprepare = spi_engine_offload_unprepare,
+	.hw_trigger_mode_enable = spi_engine_hw_trigger_mode_enable,
+	.hw_trigger_mode_disable = spi_engine_hw_trigger_mode_disable,
+	.hw_trigger_get_clk = spi_engine_hw_trigger_get_clk,
+	.rx_stream_get_dma_chan = spi_engine_rx_stream_get_dma_chan,
+};
+
+static void spi_engine_cleanup(struct spi_device *spi)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	int i;
+
+	/* remove spi device to offload mapping */
+	for (i = 0; i < SPI_ENGINE_MAX_NUM_OFFLOADS; i++) {
+		struct spi_engine_offload *priv = &spi_engine->offload_priv[i];
+
+		if (priv->spi == spi) {
+			priv->spi = NULL;
+			kfree(priv->id);
+			priv->id = NULL;
+		}
+	}
+}
+
 static void spi_engine_release_hw(void *p)
 {
 	struct spi_engine *spi_engine = p;
@@ -668,6 +985,19 @@ static int spi_engine_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (ADI_AXI_PCORE_VER_MINOR(version) >= 1) {
+		unsigned int sizes = readl(spi_engine->base +
+				SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH);
+
+		spi_engine->offload_ctrl_mem_size = 1 <<
+			FIELD_GET(SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_CMD, sizes);
+		spi_engine->offload_sdo_mem_size = 1 <<
+			FIELD_GET(SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_SDO, sizes);
+	} else {
+		spi_engine->offload_ctrl_mem_size = SPI_ENGINE_OFFLOAD_CMD_FIFO_SIZE;
+		spi_engine->offload_sdo_mem_size = SPI_ENGINE_OFFLOAD_SDO_FIFO_SIZE;
+	}
+
 	writel_relaxed(0x00, spi_engine->base + SPI_ENGINE_REG_RESET);
 	writel_relaxed(0xff, spi_engine->base + SPI_ENGINE_REG_INT_PENDING);
 	writel_relaxed(0x00, spi_engine->base + SPI_ENGINE_REG_INT_ENABLE);
@@ -690,6 +1020,9 @@ static int spi_engine_probe(struct platform_device *pdev)
 	host->optimize_message = spi_engine_optimize_message;
 	host->unoptimize_message = spi_engine_unoptimize_message;
 	host->num_chipselect = 8;
+	host->offload_xfer_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	host->offload_ops = &spi_engine_offload_ops;
+	host->cleanup = spi_engine_cleanup;
 
 	/* Some features depend of the IP core version. */
 	if (ADI_AXI_PCORE_VER_MINOR(version) >= 2) {

-- 
2.43.0

Re: [PATCH RFC v3 6/9] spi: axi-spi-engine: implement offload support

[Nuno Sá]

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This implements SPI offload support for the AXI SPI Engine. Currently,
> the hardware only supports triggering offload transfers with a hardware
> trigger so attempting to use an offload message in the regular SPI
> message queue will fail. Also, only allows streaming rx data to an
> external sink, so attempts to use a rx_buf in the offload message will
> fail.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 

...


I'm likely missing something but you already have:

priv = &spi_engine->offload_priv[args[0]];

which seems that from FW you already got the offload index you need. Can't we
just save that index in struct spi_device and use that directly in the other
operations? Saving the trouble to save the id string and having to always call 
spi_engine_get_offload()?

> +
> 

...

> +}
> +
> +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
> *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv)) {
> +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
> +		return;
> +	}
> +
> +	writel_relaxed(1, spi_engine->base +
> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> +	writel_relaxed(0, spi_engine->base +
> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> +
> +	priv->prepared = false;
> +}
> +
> +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
> +					     const char *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num, reg;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	reg = readl_relaxed(spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> +	writel_relaxed(reg, spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +
> +	return 0;
> +}
> +
> +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
> +					       const char *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num, reg;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv)) {
> +		dev_warn(&spi->dev, "failed match offload in disable\n");
> +		return;
> +	}
> +
> +	reg = readl_relaxed(spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> +	writel_relaxed(reg, spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +}
> +

I would expect for the enable/disable() operations to act on the trigger. In
this case to enable/disable the clock...

- Nuno Sá

Re: [PATCH RFC v3 6/9] spi: axi-spi-engine: implement offload support

[David Lechner]

On 7/23/24 3:01 AM, Nuno Sá wrote:
> On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
>> This implements SPI offload support for the AXI SPI Engine. Currently,
>> the hardware only supports triggering offload transfers with a hardware
>> trigger so attempting to use an offload message in the regular SPI
>> message queue will fail. Also, only allows streaming rx data to an
>> external sink, so attempts to use a rx_buf in the offload message will
>> fail.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
> 
> ...
> 
> 
> I'm likely missing something but you already have:
> 
> priv = &spi_engine->offload_priv[args[0]];
> 
> which seems that from FW you already got the offload index you need. Can't we
> just save that index in struct spi_device and use that directly in the other
> operations? Saving the trouble to save the id string and having to always call 
> spi_engine_get_offload()?

Saving the index in the struct spi_device would assume 1. that all SPI
peripherals can only use one SPI offload instance and 2. that all SPI
offload providers have #spi-offload-cells = <1> where the cell is the
index. I don't think either of these are safe assumptions.

> 
>> +
>>
> 
> ...
> 
>> +}
>> +
>> +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
>> *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
>> +		return;
>> +	}
>> +
>> +	writel_relaxed(1, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +	writel_relaxed(0, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +
>> +	priv->prepared = false;
>> +}
>> +
>> +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
>> +					     const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv))
>> +		return PTR_ERR(priv);
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +
>> +	return 0;
>> +}
>> +
>> +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
>> +					       const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in disable\n");
>> +		return;
>> +	}
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +}
>> +
> 
> I would expect for the enable/disable() operations to act on the trigger. In
> this case to enable/disable the clock...

I'm not opposed to doing that, but things would get more complicated if we
ever added more trigger types. Because then we would need to add some kind
of trigger device abstraction to wrap the enable and disable functions of
the various triggers.

It seems simpler to me to have the peripheral driver do it since it already
needs to get the clock device for other reasons anyway.

But I also got some internal feedback that it might make more sense to add
a trigger abstraction layer, so maybe that is something we should look into
more.

Re: [PATCH RFC v3 6/9] spi: axi-spi-engine: implement offload support

[Nuno Sá]

On Tue, 2024-07-23 at 09:19 -0500, David Lechner wrote:
> On 7/23/24 3:01 AM, Nuno Sá wrote:
> > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> > > This implements SPI offload support for the AXI SPI Engine. Currently,
> > > the hardware only supports triggering offload transfers with a hardware
> > > trigger so attempting to use an offload message in the regular SPI
> > > message queue will fail. Also, only allows streaming rx data to an
> > > external sink, so attempts to use a rx_buf in the offload message will
> > > fail.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > > 
> > 
> > ...
> > 
> > 
> > I'm likely missing something but you already have:
> > 
> > priv = &spi_engine->offload_priv[args[0]];
> > 
> > which seems that from FW you already got the offload index you need. Can't we
> > just save that index in struct spi_device and use that directly in the other
> > operations? Saving the trouble to save the id string and having to always call 
> > spi_engine_get_offload()?
> 
> Saving the index in the struct spi_device would assume 1. that all SPI
> peripherals can only use one SPI offload instance and 2. that all SPI
> offload providers have #spi-offload-cells = <1> where the cell is the
> index. I don't think either of these are safe assumptions.
> 

Ok, I see what you mean. I guess I just don't like too much of that *id in all over
the place. But we may anyways have to come up with some kind of offload abstraction.

> > 
> > > +
> > > 
> > 
> > ...
> > 
> > > +}
> > > +
> > > +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
> > > *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv)) {
> > > +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
> > > +		return;
> > > +	}
> > > +
> > > +	writel_relaxed(1, spi_engine->base +
> > > SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> > > +	writel_relaxed(0, spi_engine->base +
> > > SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> > > +
> > > +	priv->prepared = false;
> > > +}
> > > +
> > > +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
> > > +					     const char *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num, reg;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv))
> > > +		return PTR_ERR(priv);
> > > +
> > > +	reg = readl_relaxed(spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> > > +	writel_relaxed(reg, spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
> > > +					       const char *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num, reg;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv)) {
> > > +		dev_warn(&spi->dev, "failed match offload in disable\n");
> > > +		return;
> > > +	}
> > > +
> > > +	reg = readl_relaxed(spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> > > +	writel_relaxed(reg, spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +}
> > > +
> > 
> > I would expect for the enable/disable() operations to act on the trigger. In
> > this case to enable/disable the clock...
> 
> I'm not opposed to doing that, but things would get more complicated if we
> ever added more trigger types. Because then we would need to add some kind
> of trigger device abstraction to wrap the enable and disable functions of
> the various triggers.
> 

Yeah, to me is about symmetry... I'm of the opinion that consumers, ideally, would
not have to know about the type of the trigger. Just that we have a trigger and then
have an interface for what can we do with it. The one that needs to know about the
type is the controller driver proving offload capabilities. I guess we can have one
DT cell to specify the type of the trigger.

> It seems simpler to me to have the peripheral driver do it since it already
> needs to get the clock device for other reasons anyway.
> 
> But I also got some internal feedback that it might make more sense to add
> a trigger abstraction layer, so maybe that is something we should look into
> more.

Nice. I admit I did not though too much on an actual implementation so I'm not really
sure how feasible this is without getting overly complicated. But from a conceptual
point of view, it looks the right thing to me.

- Nuno Sá

[PATCH RFC v3 7/9] iio: buffer-dmaengine: generalize requesting DMA channel

[David Lechner]

This patch generalizes the iio_dmaengine_buffer_setup_ext() functions
by passing the pointer to the DMA channel as an argument rather than
the channel name. This will allow future callers of the function to
use other methods to get the DMA channel pointer.

This aims to keep it as easy to use as possible by stealing ownership
of the dma_chan pointer from the caller. This way, dma_request_chan()
can be called inline in the function call without any extra error
handling.

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

v3 changes:
* This is a new patch in v3.
---
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 39 +++++++++++++---------
 drivers/iio/dac/adi-axi-dac.c                      |  3 +-
 include/linux/iio/buffer-dmaengine.h               | 11 +++---
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 918f6f8d65b6..44c3b4fe0643 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -163,32 +163,34 @@ static const struct iio_dev_attr *iio_dmaengine_buffer_attrs[] = {
 /**
  * iio_dmaengine_buffer_alloc() - Allocate new buffer which uses DMAengine
  * @dev: Parent device for the buffer
- * @channel: DMA channel name, typically "rx".
+ * @chan: DMA channel.
  *
  * This allocates a new IIO buffer which internally uses the DMAengine framework
  * to perform its transfers. The parent device will be used to request the DMA
  * channel.
  *
+ * This "steals" the @chan pointer, so the caller must not call
+ * dma_release_channel() on it. @chan is also checked for error, so callers
+ * can pass the result of dma_request_chan() directly.
+ *
  * Once done using the buffer iio_dmaengine_buffer_free() should be used to
  * release it.
  */
 static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
-	const char *channel)
+						     struct dma_chan *chan)
 {
 	struct dmaengine_buffer *dmaengine_buffer;
 	unsigned int width, src_width, dest_width;
 	struct dma_slave_caps caps;
-	struct dma_chan *chan;
 	int ret;
 
-	dmaengine_buffer = kzalloc(sizeof(*dmaengine_buffer), GFP_KERNEL);
-	if (!dmaengine_buffer)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(chan))
+		return ERR_CAST(chan);
 
-	chan = dma_request_chan(dev, channel);
-	if (IS_ERR(chan)) {
-		ret = PTR_ERR(chan);
-		goto err_free;
+	dmaengine_buffer = kzalloc(sizeof(*dmaengine_buffer), GFP_KERNEL);
+	if (!dmaengine_buffer) {
+		ret = -ENOMEM;
+		goto err_release;
 	}
 
 	ret = dma_get_slave_caps(chan, &caps);
@@ -221,6 +223,9 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
 
 err_free:
 	kfree(dmaengine_buffer);
+err_release:
+	dma_release_channel(chan);
+
 	return ERR_PTR(ret);
 }
 
@@ -244,13 +249,13 @@ EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_free, IIO_DMAENGINE_BUFFER);
 
 struct iio_buffer *iio_dmaengine_buffer_setup_ext(struct device *dev,
 						  struct iio_dev *indio_dev,
-						  const char *channel,
+						  struct dma_chan *chan,
 						  enum iio_buffer_direction dir)
 {
 	struct iio_buffer *buffer;
 	int ret;
 
-	buffer = iio_dmaengine_buffer_alloc(dev, channel);
+	buffer = iio_dmaengine_buffer_alloc(dev, chan);
 	if (IS_ERR(buffer))
 		return ERR_CAST(buffer);
 
@@ -277,22 +282,26 @@ static void __devm_iio_dmaengine_buffer_free(void *buffer)
  * devm_iio_dmaengine_buffer_setup_ext() - Setup a DMA buffer for an IIO device
  * @dev: Parent device for the buffer
  * @indio_dev: IIO device to which to attach this buffer.
- * @channel: DMA channel name, typically "rx".
+ * @chan: DMA channel.
  * @dir: Direction of buffer (in or out)
  *
  * This allocates a new IIO buffer with devm_iio_dmaengine_buffer_alloc()
  * and attaches it to an IIO device with iio_device_attach_buffer().
  * It also appends the INDIO_BUFFER_HARDWARE mode to the supported modes of the
  * IIO device.
+ *
+ * This "steals" the @chan pointer, so the caller must not call
+ * dma_release_channel() on it. @chan is also checked for error, so callers
+ * can pass the result of dma_request_chan() directly.
  */
 int devm_iio_dmaengine_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
-					const char *channel,
+					struct dma_chan *chan,
 					enum iio_buffer_direction dir)
 {
 	struct iio_buffer *buffer;
 
-	buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, channel, dir);
+	buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, chan, dir);
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
 
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index 880d83a014a1..7e6225920e49 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -124,7 +124,8 @@ static struct iio_buffer *axi_dac_request_buffer(struct iio_backend *back,
 	if (device_property_read_string(st->dev, "dma-names", &dma_name))
 		dma_name = "tx";
 
-	return iio_dmaengine_buffer_setup_ext(st->dev, indio_dev, dma_name,
+	return iio_dmaengine_buffer_setup_ext(st->dev, indio_dev,
+					      dma_request_chan(st->dev, dma_name),
 					      IIO_BUFFER_DIRECTION_OUT);
 }
 
diff --git a/include/linux/iio/buffer-dmaengine.h b/include/linux/iio/buffer-dmaengine.h
index 81d9a19aeb91..a80397c3b198 100644
--- a/include/linux/iio/buffer-dmaengine.h
+++ b/include/linux/iio/buffer-dmaengine.h
@@ -7,6 +7,7 @@
 #ifndef __IIO_DMAENGINE_H__
 #define __IIO_DMAENGINE_H__
 
+#include <linux/dmaengine.h>
 #include <linux/iio/buffer.h>
 
 struct iio_dev;
@@ -15,20 +16,22 @@ struct device;
 void iio_dmaengine_buffer_free(struct iio_buffer *buffer);
 struct iio_buffer *iio_dmaengine_buffer_setup_ext(struct device *dev,
 						  struct iio_dev *indio_dev,
-						  const char *channel,
+						  struct dma_chan *chan,
 						  enum iio_buffer_direction dir);
 
 #define iio_dmaengine_buffer_setup(dev, indio_dev, channel)	\
-	iio_dmaengine_buffer_setup_ext(dev, indio_dev, channel,	\
+	iio_dmaengine_buffer_setup_ext(dev, indio_dev,		\
+				       dma_request_chan(dev, channel),\
 				       IIO_BUFFER_DIRECTION_IN)
 
 int devm_iio_dmaengine_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
-					const char *channel,
+					struct dma_chan *chan,
 					enum iio_buffer_direction dir);
 
 #define devm_iio_dmaengine_buffer_setup(dev, indio_dev, channel)	\
-	devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, channel,	\
+	devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev,		\
+					    dma_request_chan(dev, channel), \
 					    IIO_BUFFER_DIRECTION_IN)
 
 #endif

-- 
2.43.0

Re: [PATCH RFC v3 7/9] iio: buffer-dmaengine: generalize requesting DMA channel

[Jonathan Cameron]

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

> This patch generalizes the iio_dmaengine_buffer_setup_ext() functions
> by passing the pointer to the DMA channel as an argument rather than
> the channel name. This will allow future callers of the function to
> use other methods to get the DMA channel pointer.
> 
> This aims to keep it as easy to use as possible by stealing ownership
> of the dma_chan pointer from the caller. This way, dma_request_chan()
> can be called inline in the function call without any extra error
> handling.

That's odd enough to be a likely source of future bugs. Doesn't seem
necessary to me. Just have the extra handling in the few places it's needed.

Or add a wrapper for this case where you just provide the
channel name as was done before this patch.

> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> 
> v3 changes:
> * This is a new patch in v3.


...

> @@ -277,22 +282,26 @@ static void __devm_iio_dmaengine_buffer_free(void *buffer)
>   * devm_iio_dmaengine_buffer_setup_ext() - Setup a DMA buffer for an IIO device
>   * @dev: Parent device for the buffer
>   * @indio_dev: IIO device to which to attach this buffer.
> - * @channel: DMA channel name, typically "rx".
> + * @chan: DMA channel.
>   * @dir: Direction of buffer (in or out)
>   *
>   * This allocates a new IIO buffer with devm_iio_dmaengine_buffer_alloc()
>   * and attaches it to an IIO device with iio_device_attach_buffer().
>   * It also appends the INDIO_BUFFER_HARDWARE mode to the supported modes of the
>   * IIO device.
> + *
> + * This "steals" the @chan pointer, so the caller must not call
> + * dma_release_channel() on it. @chan is also checked for error, so callers
> + * can pass the result of dma_request_chan() directly.
>   */
>  int devm_iio_dmaengine_buffer_setup_ext(struct device *dev,
>  					struct iio_dev *indio_dev,
> -					const char *channel,
> +					struct dma_chan *chan,
>  					enum iio_buffer_direction dir)
>  {
>  	struct iio_buffer *buffer;
>  
> -	buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, channel, dir);
> +	buffer = iio_dmaengine_buffer_setup_ext(dev, indio_dev, chan, dir);
>  	if (IS_ERR(buffer))
>  		return PTR_ERR(buffer);
>  

[PATCH RFC v3 8/9] dt-bindings: iio: adc: adi,ad7944: add SPI offload properties

[David Lechner]

To enable capturing data at high rates, the AD7944 is frequently used
with the AXI SPI Engine IP core. This patch adds the properties needed
to describe the SPI offload configuration in the device tree.

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

v3 changes:
* Dropped properties that should be SPI controller properties.

v2 changes:

This is a new patch that partially replaces "dt-bindings: iio: offload:
add binding for PWM/DMA triggered buffer".

In the previous review, it was suggested that having a separate binding
and object node for the offload was overcomplicated. So instead this
opts to use the proposed standard spi-offloads property as a flag to
allow the SPI periperhal node require additional properties that are
resources that are physically connected to the SPI offload.
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
index d17d184842d3..6533459add87 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
@@ -129,6 +129,9 @@ properties:
       line goes high while the SDI and CNV lines are high (chain mode),
     maxItems: 1
 
+  spi-offloads:
+    maxItems: 1
+
 required:
   - compatible
   - reg

-- 
2.43.0

[PATCH RFC v3 9/9] iio: adc: ad7944: add support for SPI offload

[David Lechner]

This adds support for SPI offload to the ad7944 driver. This allows
reading data at the max sample rate of 2.5 MSPS.

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

Note: in v2 we discussed if we should make the SPI offload use buffer1
instead of buffer0 as to not break userspace. I'm still on the fence
about if we should do that or not. Mainly because many userspace tools
aren't aware of multiple buffers yet, so would make it harder to
use the driver. And technically, the way it is implemented right now
is not going to change anything for existing users since they won't
be using the offload feature. So the argument could be made that this
isn't really breaking userspace after all.

v3 changes:
* Finished TODOs.
* Adapted to changes in other patches.

v2 changes:

In the previous version, there was a new separate driver for the PWM
trigger and DMA hardware buffer. This was deemed too complex so they
are moved into the ad7944 driver.

It has also been reworked to accommodate for the changes described in
the other patches.
---
 drivers/iio/adc/ad7944.c | 173 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 166 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index 0f36138a7144..43674ff439d2 100644
--- a/drivers/iio/adc/ad7944.c
+++ b/drivers/iio/adc/ad7944.c
@@ -9,6 +9,7 @@
 #include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -21,6 +22,7 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer-dmaengine.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
@@ -54,6 +56,8 @@ struct ad7944_adc {
 	enum ad7944_spi_mode spi_mode;
 	struct spi_transfer xfers[3];
 	struct spi_message msg;
+	struct spi_transfer offload_xfers[3];
+	struct spi_message offload_msg;
 	void *chain_mode_buf;
 	/* Chip-specific timing specifications. */
 	const struct ad7944_timing_spec *timing_spec;
@@ -65,6 +69,8 @@ struct ad7944_adc {
 	bool always_turbo;
 	/* Reference voltage (millivolts). */
 	unsigned int ref_mv;
+	/* Clock that triggers SPI offload. */
+	struct clk *trigger_clk;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -81,6 +87,8 @@ struct ad7944_adc {
 
 /* quite time before CNV rising edge */
 #define T_QUIET_NS	20
+/* minimum CNV high time to trigger conversion */
+#define T_CNVH_NS	20
 
 static const struct ad7944_timing_spec ad7944_timing_spec = {
 	.conv_ns = 420,
@@ -123,6 +131,7 @@ static const struct ad7944_chip_info _name##_chip_info = {		\
 			.scan_type.endianness = IIO_CPU,		\
 			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	\
 					| BIT(IIO_CHAN_INFO_SCALE),	\
+			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 		},							\
 		IIO_CHAN_SOFT_TIMESTAMP(1),				\
 	},								\
@@ -236,6 +245,54 @@ static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc
 	return devm_spi_optimize_message(dev, adc->spi, &adc->msg);
 }
 
+static void ad7944_offload_unprepare(void *p)
+{
+	struct ad7944_adc *adc = p;
+
+	spi_offload_unprepare(adc->spi, NULL, &adc->offload_msg);
+}
+
+/*
+ * Unlike ad7944_3wire_cs_mode_init_msg(), this creates a message that reads
+ * during the conversion phase instead of the acquisition phase when reading
+ * a sample from the ADC. This is needed to be able to read at the maximum
+ * sample rate. It requires the SPI controller to have offload support and a
+ * high enough SCLK rate to read the sample during the conversion phase.
+ */
+static int ad7944_3wire_cs_mode_init_offload_msg(struct device *dev,
+						 struct ad7944_adc *adc,
+						 const struct iio_chan_spec *chan)
+{
+	struct spi_transfer *xfers = adc->offload_xfers;
+	int ret;
+
+	/*
+	 * CS is tied to CNV and we need a low to high transition to start the
+	 * conversion, so place CNV low for t_QUIET to prepare for this.
+	 */
+	xfers[0].delay.value = T_QUIET_NS;
+	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	/* CNV has to be high for a minimum time to trigger conversion. */
+	xfers[1].cs_off = 1;
+	xfers[1].delay.value = T_CNVH_NS;
+	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	/* Then we can read the previous sample during the conversion phase */
+	xfers[2].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	xfers[2].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+	xfers[2].bits_per_word = chan->scan_type.realbits;
+
+	spi_message_init_with_transfers(&adc->offload_msg, xfers,
+					ARRAY_SIZE(adc->offload_xfers));
+
+	ret = spi_offload_prepare(adc->spi, NULL, &adc->offload_msg);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to prepare offload\n");
+
+	return devm_add_action_or_reset(dev, ad7944_offload_unprepare, adc);
+}
+
 /**
  * ad7944_convert_and_acquire - Perform a single conversion and acquisition
  * @adc: The ADC device structure
@@ -323,6 +380,30 @@ static int ad7944_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!adc->trigger_clk)
+			return -EOPNOTSUPP;
+
+		*val = clk_get_rate(adc->trigger_clk);
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7944_write_raw(struct iio_dev *indio_dev,
+			    const struct iio_chan_spec *chan,
+			    int val, int val2, long info)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!adc->trigger_clk)
+			return -EOPNOTSUPP;
+
+		return clk_set_rate(adc->trigger_clk, val);
 	default:
 		return -EINVAL;
 	}
@@ -330,6 +411,48 @@ static int ad7944_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_info ad7944_iio_info = {
 	.read_raw = &ad7944_read_raw,
+	.write_raw = &ad7944_write_raw,
+};
+
+static int ad7944_offload_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	gpiod_set_value_cansleep(adc->turbo, 1);
+
+	ret = clk_prepare_enable(adc->trigger_clk);
+	if (ret)
+		goto err_turbo_off;
+
+	ret = spi_offload_hw_trigger_mode_enable(adc->spi, NULL);
+	if (ret)
+		goto err_clk_off;
+
+	return 0;
+
+err_clk_off:
+	clk_disable_unprepare(adc->trigger_clk);
+err_turbo_off:
+	gpiod_set_value_cansleep(adc->turbo, 0);
+
+	return ret;
+}
+
+static int ad7944_offload_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+
+	spi_offload_hw_trigger_mode_disable(adc->spi, NULL);
+	clk_disable_unprepare(adc->trigger_clk);
+	gpiod_set_value_cansleep(adc->turbo, 0);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ad7944_offload_buffer_setup_ops = {
+	.postenable = &ad7944_offload_buffer_postenable,
+	.predisable = &ad7944_offload_buffer_predisable,
 };
 
 static irqreturn_t ad7944_trigger_handler(int irq, void *p)
@@ -444,6 +567,11 @@ static const char * const ad7944_power_supplies[] = {
 	"avdd",	"dvdd",	"bvdd", "vio"
 };
 
+static void ad7944_put_clk_trigger(void *p)
+{
+	clk_put(p);
+}
+
 static int ad7944_probe(struct spi_device *spi)
 {
 	const struct ad7944_chip_info *chip_info;
@@ -554,13 +682,11 @@ static int ad7944_probe(struct spi_device *spi)
 		ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
 		if (ret)
 			return ret;
-
 		break;
 	case AD7944_SPI_MODE_SINGLE:
 		ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
 		if (ret)
 			return ret;
-
 		break;
 	case AD7944_SPI_MODE_CHAIN:
 		ret = device_property_read_u32(dev, "#daisy-chained-devices",
@@ -597,11 +723,43 @@ static int ad7944_probe(struct spi_device *spi)
 		indio_dev->num_channels = ARRAY_SIZE(chip_info->channels);
 	}
 
-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
-					      iio_pollfunc_store_time,
-					      ad7944_trigger_handler, NULL);
-	if (ret)
-		return ret;
+	if (device_property_present(dev, "spi-offloads")) {
+		if (adc->spi_mode != AD7944_SPI_MODE_SINGLE)
+			return dev_err_probe(dev, -EINVAL,
+					     "offload only supported in single mode\n");
+
+		ret = ad7944_3wire_cs_mode_init_offload_msg(dev, adc,
+							    &chip_info->channels[0]);
+		if (ret)
+			return ret;
+
+		adc->trigger_clk = spi_offload_hw_trigger_get_clk(spi, NULL);
+		if (IS_ERR(adc->trigger_clk))
+			return dev_err_probe(dev, PTR_ERR(adc->trigger_clk),
+					     "failed to get trigger clk\n");
+
+		ret = devm_add_action_or_reset(dev, ad7944_put_clk_trigger,
+					       adc->trigger_clk);
+		if (ret)
+			return ret;
+
+		ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev,
+			spi_offload_rx_stream_get_dma_chan(spi, NULL),
+			IIO_BUFFER_DIRECTION_IN);
+		if (ret)
+			return ret;
+
+		indio_dev->setup_ops = &ad7944_offload_buffer_setup_ops;
+		/* offload can't have soft timestamp */
+		indio_dev->num_channels--;
+	} else {
+		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+						      iio_pollfunc_store_time,
+						      ad7944_trigger_handler,
+						      NULL);
+		if (ret)
+			return ret;
+	}
 
 	return devm_iio_device_register(dev, indio_dev);
 }
@@ -636,3 +794,4 @@ module_spi_driver(ad7944_driver);
 MODULE_AUTHOR("David Lechner <dlechner@baylibre.com>");
 MODULE_DESCRIPTION("Analog Devices AD7944 PulSAR ADC family driver");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);

-- 
2.43.0

Re: [PATCH RFC v3 9/9] iio: adc: ad7944: add support for SPI offload

[Nuno Sá]

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This adds support for SPI offload to the ad7944 driver. This allows
> reading data at the max sample rate of 2.5 MSPS.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 

...

> +static void ad7944_put_clk_trigger(void *p)
> +{
> +	clk_put(p);
> +}
> +

I think this means we may still need to improve the API a bit. This asymmetric
handling is (to me) and indicator that something is not very well from a design
perspective. What I mean is that if you get the clock through spi I would also
expect to put() it through SPI. Now that I think about it that's also true for
the DMA channel handling but in there things are a bit more complicated.

I mean, at least you're making this explicit in the docs so maybe it's
acceptable. But it stills feels strange to me that the place where the resources
are requested and bound too is not the same one responsible for releasing them.

If we go with the provider/consumer approach and having a properly refcounted
spi_offload object I think we may be able to do it from the offload object
context. Maybe not worth it though... Not sure tbh.


- Nuno Sá

Re: [PATCH RFC v3 9/9] iio: adc: ad7944: add support for SPI offload

[Jonathan Cameron]

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

> This adds support for SPI offload to the ad7944 driver. This allows
> reading data at the max sample rate of 2.5 MSPS.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
I think you can easily make the sampling frequence attribute disappear for
cases where we can't provide a real value back for a read.

> ---
> 
> Note: in v2 we discussed if we should make the SPI offload use buffer1
> instead of buffer0 as to not break userspace. I'm still on the fence
> about if we should do that or not. Mainly because many userspace tools
> aren't aware of multiple buffers yet, so would make it harder to
> use the driver. And technically, the way it is implemented right now
> is not going to change anything for existing users since they won't
> be using the offload feature. So the argument could be made that this
> isn't really breaking userspace after all.
> 
> v3 changes:
> * Finished TODOs.
> * Adapted to changes in other patches.
> 
> v2 changes:
> 
> In the previous version, there was a new separate driver for the PWM
> trigger and DMA hardware buffer. This was deemed too complex so they
> are moved into the ad7944 driver.
> 
> It has also been reworked to accommodate for the changes described in
> the other patches.
> ---
>  drivers/iio/adc/ad7944.c | 173 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 166 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index 0f36138a7144..43674ff439d2 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -9,6 +9,7 @@
>  #include <linux/align.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -21,6 +22,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer-dmaengine.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> @@ -54,6 +56,8 @@ struct ad7944_adc {
>  	enum ad7944_spi_mode spi_mode;
>  	struct spi_transfer xfers[3];
>  	struct spi_message msg;
> +	struct spi_transfer offload_xfers[3];
> +	struct spi_message offload_msg;
>  	void *chain_mode_buf;
>  	/* Chip-specific timing specifications. */
>  	const struct ad7944_timing_spec *timing_spec;
> @@ -65,6 +69,8 @@ struct ad7944_adc {
>  	bool always_turbo;
>  	/* Reference voltage (millivolts). */
>  	unsigned int ref_mv;
> +	/* Clock that triggers SPI offload. */
> +	struct clk *trigger_clk;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> @@ -81,6 +87,8 @@ struct ad7944_adc {
>  
>  /* quite time before CNV rising edge */
>  #define T_QUIET_NS	20

I'd prefer these prefixed as AD7944_T_QUIET_NS etc

> +/* minimum CNV high time to trigger conversion */
> +#define T_CNVH_NS	20
>  
>  static const struct ad7944_timing_spec ad7944_timing_spec = {
>  	.conv_ns = 420,
> @@ -123,6 +131,7 @@ static const struct ad7944_chip_info _name##_chip_info = {		\
>  			.scan_type.endianness = IIO_CPU,		\
>  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	\
>  					| BIT(IIO_CHAN_INFO_SCALE),	\
> +			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\

So this gets registered for all cases, but may return -EIOPNOTSUPPORTED?

That's not ideal.  If we can we should hide this file if we aren't going
to be able to read it.
Whilst it may seem overkill that's a separate iio_chan_spec array etc
that is picked based on whether we are using spi offloading or not.

>  		},							\
>  		IIO_CHAN_SOFT_TIMESTAMP(1),				\
>  	},								\
> @@ -236,6 +245,54 @@ static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc
>  	return devm_spi_optimize_message(dev, adc->spi, &adc->msg);
>  }
>  
> +static void ad7944_offload_unprepare(void *p)
> +{
> +	struct ad7944_adc *adc = p;
> +
> +	spi_offload_unprepare(adc->spi, NULL, &adc->offload_msg);
> +}

>  static int ad7944_probe(struct spi_device *spi)
>  {
>  	const struct ad7944_chip_info *chip_info;
> @@ -554,13 +682,11 @@ static int ad7944_probe(struct spi_device *spi)
>  		ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		if (ret)
>  			return ret;	
> -
>  		break;
>  	case AD7944_SPI_MODE_SINGLE:
>  		ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		if (ret)
>  			return ret;
> -
Don't edit unrelated white space in a patch that is doing more important things.
Just noise...
>  		break;

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[Nuno Sá]

Hi David,

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> There is a recap at the end of this cover letter for those not familiar
> with the previous discussions. For those that are, we'll get right to
> the changes since the last version.
> 
> In RFC v2, most of the discussion was around the DT bindings, so that
> is what has mostly changed since then. I think we mostly settled on
> what properties are needed and where they should go. There are probably
> still some details to work out (see PATCH 5/9 for more discussion) but
> I think we have the big-picture stuff figured out.
> 
> Here is the actual devicetree used for testing to show how it all
> comes together:
> 
> 	trigger_clk: adc-trigger-clock {
> 		compatible = "pwm-clock";
> 		#clock-cells = <0>;
> 		#trigger-source-cells = <0>;
> 		pwms = <&adc_trigger 0 10000>;
> 	};
> 
> 	...
> 
> 	axi_spi_engine_0: spi@44a00000 {
> 		compatible = "adi,axi-spi-engine-1.00.a";
> 		reg = <0x44a00000 0x1000>;
> 		interrupt-parent = <&intc>;
> 		interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> 		clocks = <&clkc 15>, <&spi_clk>;
> 		clock-names = "s_axi_aclk", "spi_clk";
> 
> 		/* offload-specific properties */
> 		#spi-offload-cells = <1>;
> 		dmas = <&rx_dma 0>;
> 		dma-names = "offload0-rx";
> 		trigger-sources = <&trigger_clk>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		ad7986: adc@0 {
> 			compatible = "adi,ad7986";
> 			reg = <0>;
> 			spi-max-frequency = <111111111>; /* 9 ns period */
> 			adi,spi-mode = "single";
> 			avdd-supply = <&eval_u12>;
> 			dvdd-supply = <&eval_u12>;
> 			vio-supply = <&eval_u3>;
> 			bvdd-supply = <&eval_u10>;
> 			ref-supply = <&eval_u5>;
> 			turbo-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>;
> 
> 			spi-offloads = <&axi_spi_engine_0 0>;
> 		};
> 	};
> 
> A working branch complete with extra hacks can be found at [1].
> 
> Also, I took a detour looking into what it would take to get Martin
> Sperl's Raspberry Pi DMA offload proof-of-concept [2] updated to work
> with this. This way we could have a second user to help guide the
> design process. Given all of the SPI hardware quirks on that platform
> and the unsolved technical issues, like how to get accurate time delays
> and how to work around the 32-bit DMA word limitation, it would be more
> work than I have time for (at least without someone sponsoring the work).
> 
> [1]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v3
> [2]:
> https://github.com/msperl/spi-bcm2835/blob/refactor_dmachain_for_prepared_messages/spi-bcm2835dma.c
> 
> ---
> Changes in v3:
> - See individual patches for more detailed changes.
> - Reworked DT bindings to have things physically connected to the SPI
>   controller be properties of the SPI controller and use more
>   conventional provider/consumer properties.
> - Added more SPI APIs for peripheral drivers to use to get auxillary
>   offload resources, like triggers.
> - Link to v2:
> https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com
> 
> ---
> 
> As a recap, here is the background and end goal of this series:
> 
> The AXI SPI Engine is a SPI controller that has the ability to record a
> series of SPI transactions and then play them back using a hardware
> trigger. This allows operations to be performed, repeating many times,
> without any CPU intervention. This is needed for achieving high data
> rates (millions of samples per second) from ADCs and DACs that are
> connected via a SPI bus.
> 
> The offload hardware interface consists of a trigger input and a data
> output for the RX data. These are connected to other hardware external
> to the SPI controller.
> 
> To record one or more transactions, commands and TX data are written
> to memories in the controller (RX buffer is not used since RX data gets
> streamed to an external sink). This sequence of transactions can then be
> played back when the trigger input is asserted.
> 
> This series includes core SPI support along with the first SPI
> controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use
> them. This enables capturing analog data at 2 million samples per
> second.
> 
> The hardware setup looks like this:
> 
> +-------------------------------+   +------------------+
> >                               |   |                  |
> >  SOC/FPGA                     |   |  AD7944 ADC      |
> >  +---------------------+      |   |                  |
> >  | AXI SPI Engine      |      |   |                  |
> >  |             SPI Bus ============ SPI Bus          |
> >  |                     |      |   |                  |
> >  |  +---------------+  |      |   |                  |
> >  |  | Offload 0     |  |      |   +------------------+
> >  |  |   RX DATA OUT > > > >   |
> >  |  |    TRIGGER IN < < <  v  |
> >  |  +---------------+  | ^ v  |
> >  +---------------------+ ^ v  |
> >  | AXI PWM             | ^ v  |
> >  |                 CH0 > ^ v  |
> >  +---------------------+   v  |
> >  | AXI DMA             |   v  |
> >  |                 CH0 < < <  |
> >  +---------------------+      |
> >                               |
> +-------------------------------+
> 
> To: Mark Brown <broonie@kernel.org>
> To: Jonathan Cameron <jic23@kernel.org>
> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Nuno Sá <nuno.sa@analog.com>
> Cc: Michael Hennerich <Michael.Hennerich@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: David Jander <david@protonic.nl>
> Cc: Martin Sperl <kernel@martin.sperl.org>
> Cc:  <linux-spi@vger.kernel.org>
> Cc:  <devicetree@vger.kernel.org>
> Cc:  <linux-kernel@vger.kernel.org>
> Cc:  <linux-iio@vger.kernel.org>
> 
> ---
> 

I think there are things that we need to better figure but things are improving
IMO :)

I'm only doing a very superficial review since I need to better look at the
patches...

But one thing that I do want to mention is a scenario (another funny one...)
that I've discussing and that might be a reality. Something like:

+-------------------------------+    +------------------+
|                               |    |                  |
|  SOC/FPGA                     |    |   ADC            |
|                               |    |                  |
|	+---------------+       |    |                  |
|       |  SPI PS Zynq  |============== SPI Bus         |
|	+---------------+	|    |	                |
|                               |    |                  |
|  +---------------------+      |    |                  |
|  | AXI SPI Engine      |      |    |                  |
|  |                 v================ DATA Bus         |
|  |                 v   |      |    |                  |
|  |   +---------------+ |      |    |                  |
|  |  | Offload 0     |  |      |    +------------------+
|  |  |   RX DATA OUT |  |      |
|  |  |    TRIGGER IN |  |      |
|  |  +---------------+  |      |
|                               |
+-------------------------------+

From the above, the spi controller for typical register access/configuration is
not the spi_enigine and the offload core is pretty much only used for streaming
data. So I think your current approach would not work with this usecase. In your
first RFC you had something overly complicated (IMHO) but you already had a
concept that maybe it's worth looking at again. I mean having a spi_offload
object that could describe it and more importantly have a provider/consumer
logic where a spi consumer (or maybe even something else?) can get()/put() an
offload object to stream data.

I know, I did said that I did not liked for spi consumers to have to explicitly
call something like spi_offload_get() but I guess I have been proved wrong :).
We can also try to be smart about it as an explicit get is only needed (likely)
in the above scenario (or maybe we can even do it directly in the spi core
during spi_probe()). Or maybe it's not worth it to play smart and just let
consumers do it (that's the typical pattern anyways).

Having said the above, I still think your current proposal for triggers and
getting DMA streams is valid for the above usecase.

FWIW, I'm also trying to understand with the HW guys why the hell can't we just
use the spi_engine controller for everything. But the whole discussion is
already showing us that we may need more flexibility.

Thanks!
- Nuno Sá

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[David Lechner]

On 7/23/24 2:35 AM, Nuno Sá wrote:
> Hi David,
> 
> 
> I think there are things that we need to better figure but things are improving
> IMO :)
> 
> I'm only doing a very superficial review since I need to better look at the
> patches...
> 
> But one thing that I do want to mention is a scenario (another funny one...)
> that I've discussing and that might be a reality. Something like:
> 
> +-------------------------------+    +------------------+
> |                               |    |                  |
> |  SOC/FPGA                     |    |   ADC            |
> |                               |    |                  |
> |       +---------------+       |    |                  |
> |       |  SPI PS Zynq  |============== SPI Bus         |
> |       +---------------+       |    |                  |
> |                               |    |                  |
> |  +---------------------+      |    |                  |
> |  | AXI SPI Engine      |      |    |                  |
> |  |                 v================ DATA Bus         |
> |  |                 v   |      |    |                  |
> |  |   +---------------+ |      |    |                  |
> |  |  | Offload 0     |  |      |    +------------------+
> |  |  |   RX DATA OUT |  |      |
> |  |  |    TRIGGER IN |  |      |
> |  |  +---------------+  |      |
> |                               |
> +-------------------------------+
> 
> From the above, the spi controller for typical register access/configuration is
> not the spi_enigine and the offload core is pretty much only used for streaming
> data. So I think your current approach would not work with this usecase. In your
> first RFC you had something overly complicated (IMHO) but you already had a
> concept that maybe it's worth looking at again. I mean having a spi_offload
> object that could describe it and more importantly have a provider/consumer
> logic where a spi consumer (or maybe even something else?) can get()/put() an
> offload object to stream data.

Although it isn't currently implemented this way in the core SPI code, I think
the DT bindings proposed in this version of the series would allow for this.
The offload provider is just the one with the #spi-offload-cells and doesn't
necessarily have to be the parent of the SPI peripheral.

> 
> I know, I did said that I did not liked for spi consumers to have to explicitly
> call something like spi_offload_get() but I guess I have been proved wrong :).
> We can also try to be smart about it as an explicit get is only needed (likely)
> in the above scenario (or maybe we can even do it directly in the spi core
> during spi_probe()). Or maybe it's not worth it to play smart and just let
> consumers do it (that's the typical pattern anyways).
> 
> Having said the above, I still think your current proposal for triggers and
> getting DMA streams is valid for the above usecase.
> 
> FWIW, I'm also trying to understand with the HW guys why the hell can't we just
> use the spi_engine controller for everything. But the whole discussion is
> already showing us that we may need more flexibility.
> 
> Thanks!
> - Nuno Sá

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[Nuno Sá]

On Tue, 2024-07-23 at 08:48 -0500, David Lechner wrote:
> On 7/23/24 2:35 AM, Nuno Sá wrote:
> > Hi David,
> > 
> > 
> > I think there are things that we need to better figure but things are improving
> > IMO :)
> > 
> > I'm only doing a very superficial review since I need to better look at the
> > patches...
> > 
> > But one thing that I do want to mention is a scenario (another funny one...)
> > that I've discussing and that might be a reality. Something like:
> > 
> > +-------------------------------+    +------------------+
> > >                               |    |                  |
> > >  SOC/FPGA                     |    |   ADC            |
> > >                               |    |                  |
> > >       +---------------+       |    |                  |
> > >       |  SPI PS Zynq  |============== SPI Bus         |
> > >       +---------------+       |    |                  |
> > >                               |    |                  |
> > >  +---------------------+      |    |                  |
> > >  | AXI SPI Engine      |      |    |                  |
> > >  |                 v================ DATA Bus         |
> > >  |                 v   |      |    |                  |
> > >  |   +---------------+ |      |    |                  |
> > >  |  | Offload 0     |  |      |    +------------------+
> > >  |  |   RX DATA OUT |  |      |
> > >  |  |    TRIGGER IN |  |      |
> > >  |  +---------------+  |      |
> > >                               |
> > +-------------------------------+
> > 
> > From the above, the spi controller for typical register access/configuration is
> > not the spi_enigine and the offload core is pretty much only used for streaming
> > data. So I think your current approach would not work with this usecase. In your
> > first RFC you had something overly complicated (IMHO) but you already had a
> > concept that maybe it's worth looking at again. I mean having a spi_offload
> > object that could describe it and more importantly have a provider/consumer
> > logic where a spi consumer (or maybe even something else?) can get()/put() an
> > offload object to stream data.
> 
> Although it isn't currently implemented this way in the core SPI code, I think
> the DT bindings proposed in this version of the series would allow for this.
> The offload provider is just the one with the #spi-offload-cells and doesn't
> necessarily have to be the parent of the SPI peripheral.
> 

I get that but the thing is that when the spi device consuming an offload service is
under the same controller providing that service we have guarantees regarding
lifetimes. In case the offload is another controller, those guarantees are not true
anymore and we need to make sure to handle things correctly (like the controller
going away under our feet). That's why a proper provider/consumer interface is needed
and I think that's a significant shift from what we have now?

Out of my head (the minimal interface):

spi_controller_offload_register() - we may need a list of register offloads in the
controller struct.
(devm_)spi_offload_get() - and this one could return a nice struct spi_offload
abstraction to pass to the other APIs
spi_offload_put()

Or for starters we may skip the registering in the core and have it per driver but if
we assume more controlles will have this we might just make it common code already.

Just some ideas...

- Nuno Sá

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote:
> There is a recap at the end of this cover letter for those not familiar
> with the previous discussions. For those that are, we'll get right to
> the changes since the last version.
> 
> In RFC v2, most of the discussion was around the DT bindings, so that
> is what has mostly changed since then. I think we mostly settled on
> what properties are needed and where they should go. There are probably
> still some details to work out (see PATCH 5/9 for more discussion) but
> I think we have the big-picture stuff figured out.

Thanks for the updates. I'm on holiday until rc2, so it'll not be until
then that I can really take a look at this. Figured I'd let you know
rather than just ignore you...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Tue, Jul 23, 2024 at 09:58:30AM +0100, Conor Dooley wrote:
> On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote:
> > There is a recap at the end of this cover letter for those not familiar
> > with the previous discussions. For those that are, we'll get right to
> > the changes since the last version.
> > 
> > In RFC v2, most of the discussion was around the DT bindings, so that
> > is what has mostly changed since then. I think we mostly settled on
> > what properties are needed and where they should go. There are probably
> > still some details to work out (see PATCH 5/9 for more discussion) but
> > I think we have the big-picture stuff figured out.
> 
> Thanks for the updates. I'm on holiday until rc2, so it'll not be until
> then that I can really take a look at this. Figured I'd let you know
> rather than just ignore you...

Finally got around to actually looking at the binding patches, but since
Rob got there before me I didn't have all that much to say. Thanks for
looking into the graph idea, and I think I agree that it is worth
excluding that until you're actually going to use the crc checker etc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Mon, Jul 22, 2024 at 04:57:07PM -0500, David Lechner wrote:
> There is a recap at the end of this cover letter for those not familiar
> with the previous discussions. For those that are, we'll get right to
> the changes since the last version.

I didn't reply on this mainly because I don't have anything super
substantial to say that wasn't already covered.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]