[PATCH RFC v2 0/8] spi: axi-spi-engine: add offload support

[PATCH RFC v2 0/8] spi: axi-spi-engine: add offload support

[David Lechner]

Continuing the discussion started a few months ago [1]...

It was suggested that we were trying to do too much in one series. So
we split out part of the series into a separate series that adds generic
support for pre-preparing SPI messages [2]. That work has been merged.

[1]: https://lore.kernel.org/linux-spi/20240109-axi-spi-engine-series-3-v1-0-e42c6a986580@baylibre.com/
[2]: https://lore.kernel.org/linux-spi/20240219-mainline-spi-precook-message-v2-0-4a762c6701b9@baylibre.com/

I was hoping to also break down the remaining work into smaller parts by
first only considering more generic offload support that could be used
with the regular SPI message queue and leaving out the hardware trigger
bits. But the hardware I have at hand is not capable of doing that so
this is the best I can do for now. (Happy to take suggestions if someone
knows some other hardware that could be used for this.)

There have been significant changes since the last version so I'll
discuss what changed in each patch instead of having a very lengthy
cover letter.

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

[3]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v2

---

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
piped 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 (8):
      spi: dt-bindings: spi-peripheral-props: add spi-offloads property
      spi: add basic support for SPI offloading
      spi: add support for hardware triggered offload
      spi: add offload xfer flags
      spi: dt-bindings: axi-spi-engine: document spi-offloads
      spi: axi-spi-engine: add offload support
      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    |  58 +++++
 .../bindings/spi/adi,axi-spi-engine.yaml           |  14 ++
 .../bindings/spi/spi-peripheral-props.yaml         |  10 +
 drivers/iio/adc/ad7944.c                           | 147 +++++++++---
 drivers/spi/spi-axi-spi-engine.c                   | 267 ++++++++++++++++++++-
 drivers/spi/spi.c                                  | 202 +++++++++++++++-
 include/linux/spi/spi.h                            |  84 +++++++
 7 files changed, 741 insertions(+), 41 deletions(-)
---
base-commit: 14fde009028126f91c2bd72c404e425cf4f8aec3
change-id: 20240510-dlech-mainline-spi-engine-offload-2-afce3790b5ab

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

This adds a new property to the spi-peripheral-props binding for use
with peripherals connected to controllers that support offloading.

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

This 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.

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

v2 changes:

In v1, instead of generic SPI bindings, there were only controller-
specific bindings, so this is a new patch.

In the previous version I also had an offloads object node that described
what the offload capabilities were but it was suggested that this was
not necessary/overcomplicated. So I've gone to the other extreme and
made it perhaps over-simplified now by requiring all information about
how each offload is used to be encoded in a single u32.

We could of course consider using #spi-offload-cells instead for
allowing encoding multiple parameters for each offload instance if that
would be preferable.

I also considered adding spi-offload-names that could be used as sort
of a compatible string (more of an interface name really) in case some
peripherals may want to support more than 1 specialized type of offload.
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce..32991a2d2264 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -113,6 +113,16 @@ properties:
     minItems: 2
     maxItems: 4
 
+  spi-offloads:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of controller offload instances that are reserved for use by the
+      peripheral device. The semantic meaning of the values of the array
+      elements is defined by the controller. For example, it could be a simple
+      0-based index of the offload instance, or it could be a bitfield where
+      a few bits represent the assigned hardware trigger, a few bits represent
+      the assigned RX stream, etc.
+
   st,spi-midi-ns:
     description: |
       Only for STM32H7, (Master Inter-Data Idleness) minimum time

-- 
2.43.2

[PATCH RFC v2 2/8] spi: add basic support for SPI offloading

[David Lechner]

SPI offloading is a feature that allows the SPI controller to perform
complex transfers without 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>
---

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       | 100 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |  57 +++++++++++++++++++++++++++
 2 files changed, 157 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 289feccca376..54b814cea54c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2477,6 +2477,28 @@ 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_property_count_u32_elems(nc, "spi-offloads");
+	if (rc > 0) {
+		int num_ch = rc;
+
+		if (!ctlr->offload_ops) {
+			dev_err(&ctlr->dev, "SPI controller doesn't support offloading\n");
+			return -EINVAL;
+		}
+
+		for (idx = 0; idx < num_ch; idx++) {
+			of_property_read_u32_index(nc, "spi-offloads", idx, &value);
+
+			rc = ctlr->offload_ops->map_channel(spi, idx, value);
+			if (rc) {
+				dev_err(&ctlr->dev, "Failed to map offload channel %d: %d\n",
+					value, rc);
+				return rc;
+			}
+		}
+	}
+
 	return 0;
 }
 
@@ -3231,6 +3253,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;
 }
 
@@ -4708,6 +4735,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:		Unique identifier for SPI device with more than one offload.
+ * @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, unsigned int 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, unsigned int 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 e8e1e798924f..a8fc16c6bf37 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,
@@ -500,6 +501,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
@@ -745,6 +747,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;
@@ -1114,6 +1119,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
@@ -1123,6 +1129,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,
@@ -1151,6 +1158,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
@@ -1183,6 +1192,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;
@@ -1546,6 +1560,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: 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, unsigned int id,
+			   unsigned int channel);
+	/**
+	 * @prepare: 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, unsigned int id,
+		       struct spi_message *msg);
+	/**
+	 * @unprepare: Callback to release any resources used by prepare().
+	 */
+	void (*unprepare)(struct spi_device *spi, unsigned int id);
+};
+
+extern int spi_offload_prepare(struct spi_device *spi, unsigned int id,
+			       struct spi_message *msg);
+extern void spi_offload_unprepare(struct spi_device *spi, unsigned int id,
+				  struct spi_message *msg);
+
+/*---------------------------------------------------------------------------*/
+
 /*
  * INTERFACE between board init code and SPI infrastructure.
  *

-- 
2.43.2

[PATCH RFC v2 3/8] 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.

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

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.

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.
---
 drivers/spi/spi.c       | 92 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/spi/spi.h | 17 +++++++++
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 54b814cea54c..9ad7b04c26a6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4427,7 +4427,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_enabled)
 		ret = -EBUSY;
 	else
 		ret = __spi_async(spi, message);
@@ -4572,6 +4572,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_enabled) {
+		mutex_unlock(&spi->controller->bus_lock_mutex);
+		return -EBUSY;
+	}
+
 	ret = __spi_sync(spi, message);
 	mutex_unlock(&spi->controller->bus_lock_mutex);
 
@@ -4614,7 +4620,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)
 {
@@ -4622,6 +4628,11 @@ int spi_bus_lock(struct spi_controller *ctlr)
 
 	mutex_lock(&ctlr->bus_lock_mutex);
 
+	if (ctlr->offload_hw_trigger_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);
@@ -4808,6 +4819,83 @@ void spi_offload_unprepare(struct spi_device *spi, unsigned int id,
 }
 EXPORT_SYMBOL_GPL(spi_offload_unprepare);
 
+/**
+ * spi_offload_hw_trigger_enable - enables hardware trigger for offload
+ * @spi:	The spi device to use for the transfers.
+ * @id:		Unique identifier for SPI device with more than one offload.
+ *
+ * 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_disable().
+ *
+ * Context: can sleep
+ * Return: 0 on success, else a negative error code.
+ */
+int spi_offload_hw_trigger_enable(struct spi_device *spi, unsigned int id)
+{
+	struct spi_controller *ctlr = spi->controller;
+	unsigned long flags;
+	int ret;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_enable)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&ctlr->bus_lock_mutex);
+
+	if (ctlr->offload_hw_trigger_enabled) {
+		mutex_unlock(&ctlr->bus_lock_mutex);
+		return -EBUSY;
+	}
+
+	spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags);
+	ctlr->offload_hw_trigger_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_enable(spi, id);
+	mutex_unlock(&ctlr->io_mutex);
+
+	if (ret) {
+		ctlr->offload_hw_trigger_enabled = false;
+		mutex_unlock(&ctlr->bus_lock_mutex);
+		return ret;
+	}
+
+	mutex_unlock(&ctlr->bus_lock_mutex);
+
+	return 0;
+}
+
+/**
+ * spi_offload_hw_trigger_disable - disables hardware trigger for offload
+ * @spi:	The same SPI device passed to spi_offload_hw_trigger_enable()
+ * @id:		The same ID device passed to spi_offload_hw_trigger_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_disable(struct spi_device *spi, unsigned int id)
+{
+	struct spi_controller *ctlr = spi->controller;
+
+	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_disable)
+		return;
+
+	mutex_lock(&ctlr->io_mutex);
+	ctlr->offload_ops->hw_trigger_disable(spi, id);
+	mutex_unlock(&ctlr->io_mutex);
+
+	ctlr->offload_hw_trigger_enabled = false;
+}
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_OF_DYNAMIC)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index a8fc16c6bf37..ec8d875d31ff 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -632,6 +632,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_enabled;
+
 	/*
 	 * Setup mode and clock, etc (SPI driver may call many times).
 	 *
@@ -1594,12 +1597,26 @@ struct spi_controller_offload_ops {
 	 * @unprepare: Callback to release any resources used by prepare().
 	 */
 	void (*unprepare)(struct spi_device *spi, unsigned int id);
+
+	/**
+	 * @hw_trigger_enable: Callback to enable the hardware trigger for the
+	 * given offload instance.
+	 */
+
+	int (*hw_trigger_enable)(struct spi_device *spi, unsigned int id);
+	/**
+	 * @hw_trigger_disable: Callback to disable the hardware trigger for the
+	 * given offload instance.
+	 */
+	void (*hw_trigger_disable)(struct spi_device *spi, unsigned int id);
 };
 
 extern int spi_offload_prepare(struct spi_device *spi, unsigned int id,
 			       struct spi_message *msg);
 extern void spi_offload_unprepare(struct spi_device *spi, unsigned int id,
 				  struct spi_message *msg);
+extern int spi_offload_hw_trigger_enable(struct spi_device *spi, unsigned int id);
+extern void spi_offload_hw_trigger_disable(struct spi_device *spi, unsigned int id);
 
 /*---------------------------------------------------------------------------*/
 

-- 
2.43.2

[PATCH RFC v2 4/8] spi: add offload xfer flags

[David Lechner]

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

This patch adds a flags 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.

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

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       | 10 ++++++++++
 include/linux/spi/spi.h | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9ad7b04c26a6..4d34ccf91fd4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4189,6 +4189,16 @@ 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)
+			    != xfer->offload_flags)
+				return -EINVAL;
+		}
 	}
 
 	message->status = -EINPROGRESS;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index ec8d875d31ff..42c39c17ba5a 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -501,6 +501,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
@@ -751,6 +753,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 */
@@ -991,6 +994,7 @@ struct spi_res {
  * @transfer_list: transfers are sequenced through @spi_message.transfers
  * @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,
@@ -1107,6 +1111,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;
 

-- 
2.43.2

[PATCH RFC v2 5/8] 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>
---

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.
---
 .../devicetree/bindings/spi/adi,axi-spi-engine.yaml        | 14 ++++++++++++++
 1 file changed, 14 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..c0668aab8b2a 100644
--- a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
+++ b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
@@ -41,6 +41,20 @@ properties:
       - const: s_axi_aclk
       - const: spi_clk
 
+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

-- 
2.43.2

[PATCH RFC v2 6/8] spi: axi-spi-engine: add 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>
---

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 | 267 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 264 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index e358ac5b4509..95327df572a0 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -2,6 +2,7 @@
 /*
  * SPI-Engine SPI controller driver
  * Copyright 2015 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
  *  Author: Lars-Peter Clausen <lars@metafoo.de>
  */
 
@@ -16,6 +17,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 +25,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 +36,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)
@@ -74,6 +91,10 @@
 #define SPI_ENGINE_CMD_SYNC(id) \
 	SPI_ENGINE_CMD(SPI_ENGINE_INST_MISC, SPI_ENGINE_MISC_SYNC, (id))
 
+/* 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);
@@ -101,6 +122,12 @@ struct spi_engine_message_state {
 	uint8_t *rx_buf;
 };
 
+struct spi_engine_offload {
+	struct spi_device *spi;
+	unsigned int id;
+	bool prepared;
+};
+
 struct spi_engine {
 	struct clk *clk;
 	struct clk *ref_clk;
@@ -111,6 +138,10 @@ struct spi_engine {
 	struct spi_engine_message_state msg_state;
 	struct completion msg_complete;
 	unsigned int int_enable;
+
+	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,
@@ -154,7 +185,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,
@@ -202,16 +233,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,
@@ -503,8 +542,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);
@@ -539,6 +581,11 @@ 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");
+		return -EOPNOTSUPP;
+	}
+
 	/* reinitialize message state for this transfer */
 	memset(st, 0, sizeof(*st));
 	st->cmd_buf = p->instructions;
@@ -579,6 +626,204 @@ static int spi_engine_transfer_one_message(struct spi_controller *host,
 	return msg->status;
 }
 
+static struct spi_engine_offload *spi_engine_get_offload(struct spi_device *spi,
+							 unsigned int 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 && priv->id == id) {
+			*offload_num = i;
+			return priv;
+		}
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+static int spi_engine_offload_map_channel(struct spi_device *spi,
+					  unsigned int id,
+					  unsigned int channel)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+
+	if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
+		return -EINVAL;
+
+	priv = &spi_engine->offload_priv[channel];
+
+	if (priv->spi)
+		return -EBUSY;
+
+	priv->spi = spi;
+	priv->id = id;
+
+	return 0;
+}
+
+static int spi_engine_offload_prepare(struct spi_device *spi, unsigned int 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(priv->id);
+	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, unsigned int 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_offload_enable(struct spi_device *spi, unsigned int 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_offload_disable(struct spi_device *spi, unsigned int 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 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_enable = spi_engine_offload_enable,
+	.hw_trigger_disable = spi_engine_offload_disable,
+};
+
+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;
+	}
+}
+
 static void spi_engine_release_hw(void *p)
 {
 	struct spi_engine *spi_engine = p;
@@ -630,6 +875,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);
@@ -652,6 +910,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;
 
 	if (host->max_speed_hz == 0)
 		return dev_err_probe(&pdev->dev, -EINVAL, "spi_clk rate is 0");

-- 
2.43.2

[PATCH RFC v2 7/8] 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.

The clock trigger and DMA buffer for rx data are external to the SPI
controller and specific to the application so the are included in the
peripheral bindings.

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

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.

On a whim, I also changed pwms to clocks in the binding. The thinking
being that we only care about the frequency (it determines the sample
rate) so a clock made more sense and might allow more flexibility in
the future. But since the actual hardware we have is a PWM, we would
have to use "pwm-clock" in the devicetree to make the PWM act as a
clock. But it turns out that the pwm-clock driver does not support
setting the frequency, so we would have to do more work to actually
be able to use this binding in practice.
---
 .../devicetree/bindings/iio/adc/adi,ad7944.yaml    | 58 ++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
index d17d184842d3..5ea12aac9d25 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
@@ -129,6 +129,25 @@ properties:
       line goes high while the SDI and CNV lines are high (chain mode),
     maxItems: 1
 
+  # optional SPI offload support for use with AXI SPI Engine
+
+  spi-offloads:
+    maxItems: 1
+
+  dmas:
+    maxItems: 1
+    description: RX DMA Channel for receiving a samples from the SPI offload.
+
+  dma-names:
+    const: rx
+
+  clocks:
+    maxItems: 1
+    description: Clock to trigger the SPI offload for reading a sample.
+
+  clock-names:
+    const: trigger
+
 required:
   - compatible
   - reg
@@ -190,6 +209,23 @@ allOf:
       properties:
         turbo-gpios: false
 
+  # certain properties are for SPI offload use only
+  - if:
+      required:
+        - spi-offloads
+    then:
+      required:
+        - dmas
+        - dma-names
+        - clocks
+        - clock-names
+    else:
+      properties:
+        dmas: false
+        dma-names: false
+        clocks: false
+        clock-names: false
+
 unevaluatedProperties: false
 
 examples:
@@ -211,3 +247,25 @@ examples:
             turbo-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
         };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adc@0 {
+            compatible = "adi,ad7944";
+            reg = <0>;
+            spi-cpha;
+            spi-max-frequency = <111111111>;
+            adi,spi-mode = "single";
+            avdd-supply = <&supply_2_5V>;
+            dvdd-supply = <&supply_2_5V>;
+            vio-supply = <&supply_1_8V>;
+            bvdd-supply = <&supply_5V>;
+            spi-offloads = <0>;
+            dmas = <&dma 0>;
+            dma-names = "rx";
+            clocks = <&trigger_clk>;
+            clock-names = "trigger";
+        };
+    };

-- 
2.43.2

[PATCH RFC v2 8/8] 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>
---

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.

RFC: This isn't very polished yet, just FYI. A few things to sort out:

Rather than making the buffer either triggered buffer or hardware buffer,
I'm considering allowing both, e.g. buffer0 will always be the triggered
buffer and buffer1 will will be the hardware buffer if connected to a SPI
controller with offload support, otherwise buffer1 is absent. But since
multiple buffers haven't been used much so far, more investigation is
needed to see how that would work in practice. If we do that though, then
we would always have the sampling_frequency attribute though even though
it only applies to one buffer.
---
 drivers/iio/adc/ad7944.c | 147 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 111 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index 4602ab5ed2a6..6724d6c92778 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>
 
@@ -65,6 +67,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
@@ -123,6 +127,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),				\
 	},								\
@@ -134,18 +139,12 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0);
 /* fully differential */
 AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1);
 
-static void ad7944_unoptimize_msg(void *msg)
-{
-	spi_unoptimize_message(msg);
-}
-
-static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
-					 const struct iio_chan_spec *chan)
+static void ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
+					  const struct iio_chan_spec *chan)
 {
 	unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
 						   : adc->timing_spec->conv_ns;
 	struct spi_transfer *xfers = adc->xfers;
-	int ret;
 
 	/*
 	 * NB: can get better performance from some SPI controllers if we use
@@ -174,21 +173,14 @@ static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *
 	xfers[2].bits_per_word = chan->scan_type.realbits;
 
 	spi_message_init_with_transfers(&adc->msg, xfers, 3);
-
-	ret = spi_optimize_message(adc->spi, &adc->msg);
-	if (ret)
-		return ret;
-
-	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
 }
 
-static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
-				      const struct iio_chan_spec *chan)
+static void ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
+				       const struct iio_chan_spec *chan)
 {
 	unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
 						   : adc->timing_spec->conv_ns;
 	struct spi_transfer *xfers = adc->xfers;
-	int ret;
 
 	/*
 	 * NB: can get better performance from some SPI controllers if we use
@@ -208,12 +200,6 @@ static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc
 	xfers[1].bits_per_word = chan->scan_type.realbits;
 
 	spi_message_init_with_transfers(&adc->msg, xfers, 2);
-
-	ret = spi_optimize_message(adc->spi, &adc->msg);
-	if (ret)
-		return ret;
-
-	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
 }
 
 static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
@@ -345,6 +331,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;
 	}
@@ -352,6 +362,28 @@ 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_ex_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+
+	return spi_offload_hw_trigger_enable(adc->spi, 0);
+}
+
+static int ad7944_offload_ex_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad7944_adc *adc = iio_priv(indio_dev);
+
+	spi_offload_hw_trigger_disable(adc->spi, 0);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ad7944_offload_ex_buffer_setup_ops = {
+	.postenable = &ad7944_offload_ex_buffer_postenable,
+	.predisable = &ad7944_offload_ex_buffer_predisable,
 };
 
 static irqreturn_t ad7944_trigger_handler(int irq, void *p)
@@ -471,6 +503,18 @@ static void ad7944_ref_disable(void *ref)
 	regulator_disable(ref);
 }
 
+static void ad7944_offload_unprepare(void *p)
+{
+	struct ad7944_adc *adc = p;
+
+	spi_offload_unprepare(adc->spi, 0, &adc->msg);
+}
+
+static void ad7944_unoptimize_msg(void *msg)
+{
+	spi_unoptimize_message(msg);
+}
+
 static int ad7944_probe(struct spi_device *spi)
 {
 	const struct ad7944_chip_info *chip_info;
@@ -603,16 +647,10 @@ static int ad7944_probe(struct spi_device *spi)
 
 	switch (adc->spi_mode) {
 	case AD7944_SPI_MODE_DEFAULT:
-		ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
-		if (ret)
-			return ret;
-
+		ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
 		break;
 	case AD7944_SPI_MODE_SINGLE:
-		ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
-		if (ret)
-			return ret;
-
+		ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
 		break;
 	case AD7944_SPI_MODE_CHAIN:
 		ret = device_property_read_u32(dev, "#daisy-chained-devices",
@@ -649,11 +687,48 @@ 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")) {
+		/* TODO: make this a parameter to ad7944_3wire_cs_mode_init_msg() */
+		/* FIXME: wrong index for 4-wire mode */
+		adc->xfers[2].rx_buf = NULL;
+		adc->xfers[2].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+
+		ret = spi_offload_prepare(adc->spi, 0, &adc->msg);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to prepare offload\n");
+
+		ret = devm_add_action_or_reset(dev, ad7944_offload_unprepare, adc);
+		if (ret)
+			return ret;
+
+		adc->trigger_clk = devm_clk_get_enabled(dev, "trigger");
+		if (IS_ERR(adc->trigger_clk))
+			return dev_err_probe(dev, PTR_ERR(adc->trigger_clk),
+					     "failed to get trigger clk\n");
+
+		ret = devm_iio_dmaengine_buffer_setup(dev, indio_dev, "rx");
+		if (ret)
+			return ret;
+
+		indio_dev->setup_ops = &ad7944_offload_ex_buffer_setup_ops;
+		/* offload can't have soft timestamp */
+		indio_dev->num_channels--;
+	} else {
+		ret = spi_optimize_message(adc->spi, &adc->msg);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
+		if (ret)
+			return ret;
+
+		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);
 }

-- 
2.43.2

Re: [PATCH RFC v2 3/8] spi: add support for hardware triggered offload

[Jonathan Cameron]

Drive by comment inline.

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index a8fc16c6bf37..ec8d875d31ff 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -632,6 +632,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_enabled;
> +
>  	/*
>  	 * Setup mode and clock, etc (SPI driver may call many times).
>  	 *
> @@ -1594,12 +1597,26 @@ struct spi_controller_offload_ops {
>  	 * @unprepare: Callback to release any resources used by prepare().
>  	 */
>  	void (*unprepare)(struct spi_device *spi, unsigned int id);
> +
> +	/**
> +	 * @hw_trigger_enable: Callback to enable the hardware trigger for the
> +	 * given offload instance.
> +	 */
> +
Blank line in odd place..

> +	int (*hw_trigger_enable)(struct spi_device *spi, unsigned int id);
> +	/**
> +	 * @hw_trigger_disable: Callback to disable the hardware trigger for the
> +	 * given offload instance.
> +	 */
> +	void (*hw_trigger_disable)(struct spi_device *spi, unsigned int id);
>  };
>  
>  extern int spi_offload_prepare(struct spi_device *spi, unsigned int id,
>  			       struct spi_message *msg);
>  extern void spi_offload_unprepare(struct spi_device *spi, unsigned int id,
>  				  struct spi_message *msg);
> +extern int spi_offload_hw_trigger_enable(struct spi_device *spi, unsigned int id);
> +extern void spi_offload_hw_trigger_disable(struct spi_device *spi, unsigned int id);
>  
>  /*---------------------------------------------------------------------------*/
>  
> 

Re: [PATCH RFC v2 8/8] iio: adc: ad7944: add support for SPI offload

[Jonathan Cameron]

On Fri, 10 May 2024 19:44:31 -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>
> ---
> 
> 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.
> 
> RFC: This isn't very polished yet, just FYI. A few things to sort out:
> 
> Rather than making the buffer either triggered buffer or hardware buffer,
> I'm considering allowing both, e.g. buffer0 will always be the triggered
> buffer and buffer1 will will be the hardware buffer if connected to a SPI
> controller with offload support, otherwise buffer1 is absent. But since
> multiple buffers haven't been used much so far, more investigation is
> needed to see how that would work in practice. If we do that though, then
> we would always have the sampling_frequency attribute though even though
> it only applies to one buffer.

Why would someone who has this nice IP in the path want the conventional
triggered buffer?  I'm not against the two buffer option, but I'd like to know
the reasoning not to just provide the hardware buffer if this SPI offload
is available.

I can conjecture reasons but would like you to write them out for me :)
This feels like if someone has paid for the expensive hardware they probably
only want the best performance.

Jonathan


> ---
>  drivers/iio/adc/ad7944.c | 147 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 111 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index 4602ab5ed2a6..6724d6c92778 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>
>  
> @@ -65,6 +67,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
> @@ -123,6 +127,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),				\
>  	},								\
> @@ -134,18 +139,12 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0);
>  /* fully differential */
>  AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1);
>  
> -static void ad7944_unoptimize_msg(void *msg)
> -{
> -	spi_unoptimize_message(msg);
> -}
> -
> -static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
> -					 const struct iio_chan_spec *chan)
> +static void ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
> +					  const struct iio_chan_spec *chan)
>  {
>  	unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
>  						   : adc->timing_spec->conv_ns;
>  	struct spi_transfer *xfers = adc->xfers;
> -	int ret;
>  
>  	/*
>  	 * NB: can get better performance from some SPI controllers if we use
> @@ -174,21 +173,14 @@ static int ad7944_3wire_cs_mode_init_msg(struct device *dev, struct ad7944_adc *
>  	xfers[2].bits_per_word = chan->scan_type.realbits;
>  
>  	spi_message_init_with_transfers(&adc->msg, xfers, 3);
> -
> -	ret = spi_optimize_message(adc->spi, &adc->msg);
> -	if (ret)
> -		return ret;
> -
> -	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
>  }
>  
> -static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
> -				      const struct iio_chan_spec *chan)
> +static void ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
> +				       const struct iio_chan_spec *chan)
>  {
>  	unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
>  						   : adc->timing_spec->conv_ns;
>  	struct spi_transfer *xfers = adc->xfers;
> -	int ret;
>  
>  	/*
>  	 * NB: can get better performance from some SPI controllers if we use
> @@ -208,12 +200,6 @@ static int ad7944_4wire_mode_init_msg(struct device *dev, struct ad7944_adc *adc
>  	xfers[1].bits_per_word = chan->scan_type.realbits;
>  
>  	spi_message_init_with_transfers(&adc->msg, xfers, 2);
> -
> -	ret = spi_optimize_message(adc->spi, &adc->msg);
> -	if (ret)
> -		return ret;
> -
> -	return devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
>  }
>  
>  static int ad7944_chain_mode_init_msg(struct device *dev, struct ad7944_adc *adc,
> @@ -345,6 +331,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;
>  	}
> @@ -352,6 +362,28 @@ 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_ex_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +
> +	return spi_offload_hw_trigger_enable(adc->spi, 0);
> +}
> +
> +static int ad7944_offload_ex_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +
> +	spi_offload_hw_trigger_disable(adc->spi, 0);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ad7944_offload_ex_buffer_setup_ops = {
> +	.postenable = &ad7944_offload_ex_buffer_postenable,
> +	.predisable = &ad7944_offload_ex_buffer_predisable,
>  };
>  
>  static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> @@ -471,6 +503,18 @@ static void ad7944_ref_disable(void *ref)
>  	regulator_disable(ref);
>  }
>  
> +static void ad7944_offload_unprepare(void *p)
> +{
> +	struct ad7944_adc *adc = p;
> +
> +	spi_offload_unprepare(adc->spi, 0, &adc->msg);
> +}
> +
> +static void ad7944_unoptimize_msg(void *msg)
> +{
> +	spi_unoptimize_message(msg);
> +}
> +
>  static int ad7944_probe(struct spi_device *spi)
>  {
>  	const struct ad7944_chip_info *chip_info;
> @@ -603,16 +647,10 @@ static int ad7944_probe(struct spi_device *spi)
>  
>  	switch (adc->spi_mode) {
>  	case AD7944_SPI_MODE_DEFAULT:
> -		ret = ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
> -		if (ret)
> -			return ret;
> -
> +		ad7944_4wire_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		break;
>  	case AD7944_SPI_MODE_SINGLE:
> -		ret = ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
> -		if (ret)
> -			return ret;
> -
> +		ad7944_3wire_cs_mode_init_msg(dev, adc, &chip_info->channels[0]);
>  		break;
>  	case AD7944_SPI_MODE_CHAIN:
>  		ret = device_property_read_u32(dev, "#daisy-chained-devices",
> @@ -649,11 +687,48 @@ 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")) {
> +		/* TODO: make this a parameter to ad7944_3wire_cs_mode_init_msg() */
> +		/* FIXME: wrong index for 4-wire mode */
> +		adc->xfers[2].rx_buf = NULL;
> +		adc->xfers[2].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +
> +		ret = spi_offload_prepare(adc->spi, 0, &adc->msg);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to prepare offload\n");
> +
> +		ret = devm_add_action_or_reset(dev, ad7944_offload_unprepare, adc);
> +		if (ret)
> +			return ret;
> +
> +		adc->trigger_clk = devm_clk_get_enabled(dev, "trigger");
> +		if (IS_ERR(adc->trigger_clk))
> +			return dev_err_probe(dev, PTR_ERR(adc->trigger_clk),
> +					     "failed to get trigger clk\n");
> +
> +		ret = devm_iio_dmaengine_buffer_setup(dev, indio_dev, "rx");
> +		if (ret)
> +			return ret;
> +
> +		indio_dev->setup_ops = &ad7944_offload_ex_buffer_setup_ops;
> +		/* offload can't have soft timestamp */
> +		indio_dev->num_channels--;
> +	} else {
> +		ret = spi_optimize_message(adc->spi, &adc->msg);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(dev, ad7944_unoptimize_msg, &adc->msg);
> +		if (ret)
> +			return ret;
> +
> +		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);
>  }
> 

Re: [PATCH RFC v2 8/8] iio: adc: ad7944: add support for SPI offload

[David Lechner]

On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 10 May 2024 19:44:31 -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>
> > ---
> >
> > 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.
> >
> > RFC: This isn't very polished yet, just FYI. A few things to sort out:
> >
> > Rather than making the buffer either triggered buffer or hardware buffer,
> > I'm considering allowing both, e.g. buffer0 will always be the triggered
> > buffer and buffer1 will will be the hardware buffer if connected to a SPI
> > controller with offload support, otherwise buffer1 is absent. But since
> > multiple buffers haven't been used much so far, more investigation is
> > needed to see how that would work in practice. If we do that though, then
> > we would always have the sampling_frequency attribute though even though
> > it only applies to one buffer.
>
> Why would someone who has this nice IP in the path want the conventional
> triggered buffer?  I'm not against the two buffer option, but I'd like to know
> the reasoning not to just provide the hardware buffer if this SPI offload
> is available.
>
> I can conjecture reasons but would like you to write them out for me :)
> This feels like if someone has paid for the expensive hardware they probably
> only want the best performance.
>

For me, it was more of a question of if we need to keep the userspace
interface consistent between both with or without offload support. But
if you are happy with it this way where we have only one or the other,
it is less work for me. :-)

Re: [PATCH RFC v2 8/8] iio: adc: ad7944: add support for SPI offload

[Jonathan Cameron]

On Sat, 11 May 2024 13:41:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 10 May 2024 19:44:31 -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>
> > > ---
> > >
> > > 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.
> > >
> > > RFC: This isn't very polished yet, just FYI. A few things to sort out:
> > >
> > > Rather than making the buffer either triggered buffer or hardware buffer,
> > > I'm considering allowing both, e.g. buffer0 will always be the triggered
> > > buffer and buffer1 will will be the hardware buffer if connected to a SPI
> > > controller with offload support, otherwise buffer1 is absent. But since
> > > multiple buffers haven't been used much so far, more investigation is
> > > needed to see how that would work in practice. If we do that though, then
> > > we would always have the sampling_frequency attribute though even though
> > > it only applies to one buffer.  
> >
> > Why would someone who has this nice IP in the path want the conventional
> > triggered buffer?  I'm not against the two buffer option, but I'd like to know
> > the reasoning not to just provide the hardware buffer if this SPI offload
> > is available.
> >
> > I can conjecture reasons but would like you to write them out for me :)
> > This feels like if someone has paid for the expensive hardware they probably
> > only want the best performance.
> >  
> 
> For me, it was more of a question of if we need to keep the userspace
> interface consistent between both with or without offload support. But
> if you are happy with it this way where we have only one or the other,
> it is less work for me. :-)

So inconsistency in userspace interfaces can occur for many reasons like
whether the interrupt is wired or not, but in this particularly
case I guess we have ABI stability issue because there are boards out there
today and people using the driver without this offload functionality.
I'd not really thought that bit through, so I think you are correct that
we need to maintain the triggered buffer interface and 'add' the new
ABI for the offloaded case.  The multibuffer approach should work for this.
Will be interesting if any problem surface from having two very different
types of buffer on the same device.

Jonathan

Re: [PATCH RFC v2 8/8] iio: adc: ad7944: add support for SPI offload

[David Lechner]

On Sun, May 12, 2024 at 6:52 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 11 May 2024 13:41:09 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Fri, 10 May 2024 19:44:31 -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>
> > > > ---
> > > >
> > > > 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.
> > > >
> > > > RFC: This isn't very polished yet, just FYI. A few things to sort out:
> > > >
> > > > Rather than making the buffer either triggered buffer or hardware buffer,
> > > > I'm considering allowing both, e.g. buffer0 will always be the triggered
> > > > buffer and buffer1 will will be the hardware buffer if connected to a SPI
> > > > controller with offload support, otherwise buffer1 is absent. But since
> > > > multiple buffers haven't been used much so far, more investigation is
> > > > needed to see how that would work in practice. If we do that though, then
> > > > we would always have the sampling_frequency attribute though even though
> > > > it only applies to one buffer.
> > >
> > > Why would someone who has this nice IP in the path want the conventional
> > > triggered buffer?  I'm not against the two buffer option, but I'd like to know
> > > the reasoning not to just provide the hardware buffer if this SPI offload
> > > is available.
> > >
> > > I can conjecture reasons but would like you to write them out for me :)
> > > This feels like if someone has paid for the expensive hardware they probably
> > > only want the best performance.
> > >
> >
> > For me, it was more of a question of if we need to keep the userspace
> > interface consistent between both with or without offload support. But
> > if you are happy with it this way where we have only one or the other,
> > it is less work for me. :-)
>
> So inconsistency in userspace interfaces can occur for many reasons like
> whether the interrupt is wired or not, but in this particularly
> case I guess we have ABI stability issue because there are boards out there
> today and people using the driver without this offload functionality.

FWIW, the ad7944 driver will be landing in 6.10, so no users to speak of yet.

> I'd not really thought that bit through, so I think you are correct that
> we need to maintain the triggered buffer interface and 'add' the new
> ABI for the offloaded case.  The multibuffer approach should work for this.
> Will be interesting if any problem surface from having two very different
> types of buffer on the same device.
>

In this particular case, I think the issues will be:
1. The hardware buffer can't allow the soft timestamp. Otherwise, the
buffer layout is the same (on other chips, we won't always be so
lucky).
2. The hardware trigger (sampling_frequency attribute) will only apply
if there is the SPI offload support.

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> This adds a new property to the spi-peripheral-props binding for use
> with peripherals connected to controllers that support offloading.
> 
> Here, offloading means that the controller has the ability to perform
> complex SPI transactions without CPU intervention in some shape or form.
> 
> This 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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v2 changes:
> 
> In v1, instead of generic SPI bindings, there were only controller-
> specific bindings, so this is a new patch.
> 
> In the previous version I also had an offloads object node that described
> what the offload capabilities were but it was suggested that this was
> not necessary/overcomplicated. So I've gone to the other extreme and
> made it perhaps over-simplified now by requiring all information about
> how each offload is used to be encoded in a single u32.

The property is a u32-array, so I guess, not a single u32?

> We could of course consider using #spi-offload-cells instead for
> allowing encoding multiple parameters for each offload instance if that
> would be preferable.

A -cells property was my gut reaction to what you'd written here and
seems especially appropriate if there's any likelihood of some future
device using some external resources for spi-offloading.
However, -cells properties go in providers, not consumers, so it wouldn't
end up in spi-periph-props.yaml, but rather in the controller binding,
and instead there'd be a cell array type property in here. I think you
know that though and I'm interpreting what's been written rather than
what you meant.

> I also considered adding spi-offload-names that could be used as sort
> of a compatible string (more of an interface name really) in case some
> peripherals may want to support more than 1 specialized type of offload.
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 15938f81fdce..32991a2d2264 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -113,6 +113,16 @@ properties:
>      minItems: 2
>      maxItems: 4
>  
> +  spi-offloads:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Array of controller offload instances that are reserved for use by the
> +      peripheral device. The semantic meaning of the values of the array
> +      elements is defined by the controller. For example, it could be a simple
> +      0-based index of the offload instance, or it could be a bitfield where
> +      a few bits represent the assigned hardware trigger, a few bits represent
> +      the assigned RX stream, etc.
> +
>    st,spi-midi-ns:
>      description: |
>        Only for STM32H7, (Master Inter-Data Idleness) minimum time
> 
> -- 
> 2.43.2
> 

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> > This adds a new property to the spi-peripheral-props binding for use
> > with peripherals connected to controllers that support offloading.
> >
> > Here, offloading means that the controller has the ability to perform
> > complex SPI transactions without CPU intervention in some shape or form.
> >
> > This 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.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >
> > v2 changes:
> >
> > In v1, instead of generic SPI bindings, there were only controller-
> > specific bindings, so this is a new patch.
> >
> > In the previous version I also had an offloads object node that described
> > what the offload capabilities were but it was suggested that this was
> > not necessary/overcomplicated. So I've gone to the other extreme and
> > made it perhaps over-simplified now by requiring all information about
> > how each offload is used to be encoded in a single u32.
>
> The property is a u32-array, so I guess, not a single u32?

It is an array to handle cases where a peripheral might need more than
one offload. But the idea was it put everything about each individual
offload in a single u32. e.g. 0x0101 could be offload 1 with hardware
trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then
a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it
needed to select between both triggers at runtime.

>
> > We could of course consider using #spi-offload-cells instead for
> > allowing encoding multiple parameters for each offload instance if that
> > would be preferable.
>
> A -cells property was my gut reaction to what you'd written here and
> seems especially appropriate if there's any likelihood of some future
> device using some external resources for spi-offloading.
> However, -cells properties go in providers, not consumers, so it wouldn't
> end up in spi-periph-props.yaml, but rather in the controller binding,
> and instead there'd be a cell array type property in here. I think you
> know that though and I'm interpreting what's been written rather than
> what you meant.

Indeed you guess correctly. So the next question is if it should be
the kind of #-cells that implies a phandle like most providers or
without phandles like #address-cells? Asking because I got pushback on
v1 for using a phandle with offloads (although in that case, the
phandle was for the offload instance itself instead for the SPI
controller, so maybe this is different in this case?).

>
> > I also considered adding spi-offload-names that could be used as sort
> > of a compatible string (more of an interface name really) in case some
> > peripherals may want to support more than 1 specialized type of offload.
> > ---
> >  .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 15938f81fdce..32991a2d2264 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -113,6 +113,16 @@ properties:
> >      minItems: 2
> >      maxItems: 4
> >
> > +  spi-offloads:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      Array of controller offload instances that are reserved for use by the
> > +      peripheral device. The semantic meaning of the values of the array
> > +      elements is defined by the controller. For example, it could be a simple
> > +      0-based index of the offload instance, or it could be a bitfield where
> > +      a few bits represent the assigned hardware trigger, a few bits represent
> > +      the assigned RX stream, etc.
> > +
> >    st,spi-midi-ns:
> >      description: |
> >        Only for STM32H7, (Master Inter-Data Idleness) minimum time
> >
> > --
> > 2.43.2
> >

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote:
> On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> > > This adds a new property to the spi-peripheral-props binding for use
> > > with peripherals connected to controllers that support offloading.
> > >
> > > Here, offloading means that the controller has the ability to perform
> > > complex SPI transactions without CPU intervention in some shape or form.
> > >
> > > This 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.
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >
> > > v2 changes:
> > >
> > > In v1, instead of generic SPI bindings, there were only controller-
> > > specific bindings, so this is a new patch.
> > >
> > > In the previous version I also had an offloads object node that described
> > > what the offload capabilities were but it was suggested that this was
> > > not necessary/overcomplicated. So I've gone to the other extreme and
> > > made it perhaps over-simplified now by requiring all information about
> > > how each offload is used to be encoded in a single u32.
> >
> > The property is a u32-array, so I guess, not a single u32?
> 
> It is an array to handle cases where a peripheral might need more than
> one offload. But the idea was it put everything about each individual
> offload in a single u32. e.g. 0x0101 could be offload 1 with hardware
> trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then
> a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it
> needed to select between both triggers at runtime.
> 
> >
> > > We could of course consider using #spi-offload-cells instead for
> > > allowing encoding multiple parameters for each offload instance if that
> > > would be preferable.
> >
> > A -cells property was my gut reaction to what you'd written here and
> > seems especially appropriate if there's any likelihood of some future
> > device using some external resources for spi-offloading.
> > However, -cells properties go in providers, not consumers, so it wouldn't
> > end up in spi-periph-props.yaml, but rather in the controller binding,
> > and instead there'd be a cell array type property in here. I think you
> > know that though and I'm interpreting what's been written rather than
> > what you meant.
> 
> Indeed you guess correctly. So the next question is if it should be
> the kind of #-cells that implies a phandle like most providers or
> without phandles like #address-cells?

I'm trying to understand if the offload could ever refer to something
beyond the controller that you'd need the phandle for. I think it would
be really helpful to see an example dt of a non-trivial example for how
this would work. The example in the ad7944 patch has a stub controller
node & the clocks/dmas in the peripheral node so it is difficult to
reason about the spi-offloads property there.

> Asking because I got pushback on
> v1 for using a phandle with offloads (although in that case, the
> phandle was for the offload instance itself instead for the SPI
> controller, so maybe this is different in this case?).

Do you have a link to this v1 pushback? I had looked at the v1's binding
comments and didn't see that type of property being resisted - although
I did see some resistance to the spi peripheral node containing any of
the information about the offloads it had been assigned and instead
doing that mapping in the controller so that the cs was sufficient. I
don't think that'd work with the scenario you describe above though
where there could be two different triggers per device tho.

Cheers,
Conor.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote:
> > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> > > > This adds a new property to the spi-peripheral-props binding for use
> > > > with peripherals connected to controllers that support offloading.
> > > >
> > > > Here, offloading means that the controller has the ability to perform
> > > > complex SPI transactions without CPU intervention in some shape or form.
> > > >
> > > > This 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.
> > > >
> > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > > ---
> > > >
> > > > v2 changes:
> > > >
> > > > In v1, instead of generic SPI bindings, there were only controller-
> > > > specific bindings, so this is a new patch.
> > > >
> > > > In the previous version I also had an offloads object node that described
> > > > what the offload capabilities were but it was suggested that this was
> > > > not necessary/overcomplicated. So I've gone to the other extreme and
> > > > made it perhaps over-simplified now by requiring all information about
> > > > how each offload is used to be encoded in a single u32.
> > >
> > > The property is a u32-array, so I guess, not a single u32?
> >
> > It is an array to handle cases where a peripheral might need more than
> > one offload. But the idea was it put everything about each individual
> > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware
> > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then
> > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it
> > needed to select between both triggers at runtime.
> >
> > >
> > > > We could of course consider using #spi-offload-cells instead for
> > > > allowing encoding multiple parameters for each offload instance if that
> > > > would be preferable.
> > >
> > > A -cells property was my gut reaction to what you'd written here and
> > > seems especially appropriate if there's any likelihood of some future
> > > device using some external resources for spi-offloading.
> > > However, -cells properties go in providers, not consumers, so it wouldn't
> > > end up in spi-periph-props.yaml, but rather in the controller binding,
> > > and instead there'd be a cell array type property in here. I think you
> > > know that though and I'm interpreting what's been written rather than
> > > what you meant.
> >
> > Indeed you guess correctly. So the next question is if it should be
> > the kind of #-cells that implies a phandle like most providers or
> > without phandles like #address-cells?
>
> I'm trying to understand if the offload could ever refer to something
> beyond the controller that you'd need the phandle for. I think it would
> be really helpful to see an example dt of a non-trivial example for how
> this would work. The example in the ad7944 patch has a stub controller
> node & the clocks/dmas in the peripheral node so it is difficult to
> reason about the spi-offloads property there.

The fully implemented and tested version of the .dts corresponding to
the hardware pictured in the cover letter can be found at [1].

[1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts

To be clear though, the idea that I am proposing here is that if there
is something beyond the SPI controller directly connected to the
offload, then we would add those things in the peripheral node along
with the spi-offloads property that specifies the offload those other
things are connected to.

Tangent on phandle vs. no phandle:

If we add #spi-offload-cells, I would expect that it would always be
in the SPI controller node. And the consumers would always be SPI
peripheral nodes. So having a phandle seems redundant since it would
always point to the controller which is the parent of the peripheral.

example_spi: spi {
    ...
    #spi-offload-cells = <1>;

    adc@0 {
        ...
        /* fine, but not sure phandle is needed since it always the parent */
        spi-offloads = <&example_spi 0>;
    };
};

spi {
    ...
    #spi-offload-cells = <1>;

    adc@0 {
        ...
        /* simpler is better? */
        spi-offloads = <0>;
    };
};

Back to "something beyond the SPI controller":

Here are some examples of how I envision this would work.

Let's suppose we have a SPI controller that has some sort of offload
capability with a configurable trigger source. The trigger can either
be an internal software trigger (i.e. writing a register of the SPI
controller) or and external trigger (i.e. a input signal from a pin on
the SoC). The SPI controller has a lookup table with 8 slots where it
can store a series of SPI commands that can be played back by
asserting the trigger (this is what provides the "offloading").

So this SPI controller would have #spi-offload-cells = <2>; where the
first cell would be the index in the lookup table 0 to 7 and the
second cell would be the trigger source 0 for software or 1 for
hardware.

Application 1: a network controller

This could use two offloads, one for TX and one for RX. For TX, we use
the first slot with a software trigger because the data is coming from
Linux. For RX we use the second slot with a hardware trigger since
data is coming from the network controller (i.e. a data ready signal
that would normally be wired to a gpio for interrupt but wired to the
SPI offload trigger input pin instead). So the peripheral bindings
would be:

#define SOFTWARE_TRIGGER 0
#define HARDWARE_TRIGGER 1

can@0 {
    ...
    spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
    /* maybe we need names too? */
    spi-offload-names = "tx", "rx";
};

In this case, there is nothing extra beyond the SPI controller and the
network controller, so no extra bindings beyond this are needed.

Application 2: an advanced ADC + FPGA

This is basically the same as the ad7944 case seen already with one
extra feature. In this case, the sample data also contains a CRC byte
for error checking. So instead of SPI RX data going directly to DMA,
the FPGA removes the CRC byte from the data stream an only the sample
data goes to the DMA buffer. The CRC is checked and if bad, an
interrupt is asserted.

Since this is an FPGA, most everything is hardwired rather than having
any kind of mux selection so #spi-offload-cells = <1>; for this
controller.

By adding spi-offloads to the peripheral node, it also extends the
peripheral binding to include the additional properties needed for the
extra features provided by the FPGA. In other words, we are saying
this DT node now represents the ADC chip plus everything connected to
the offload instance used by the ADC chip.

adc@0 {
    ...
    spi-offloads = <0>;
    dmas = <&dma 0>; /* channel receiving split out sample data */
    dma-names = "rx";
    interrupts = <&intc 99>; /* interrupt for bad CRC */
    interrupt-names = "crc";
};

>
> > Asking because I got pushback on
> > v1 for using a phandle with offloads (although in that case, the
> > phandle was for the offload instance itself instead for the SPI
> > controller, so maybe this is different in this case?).
>
> Do you have a link to this v1 pushback?

Hmm... maybe that was from some internal review before v1 that I was
remembering and confusing with the resistance of different aspects you
mention below.

> I had looked at the v1's binding
> comments and didn't see that type of property being resisted - although
> I did see some resistance to the spi peripheral node containing any of
> the information about the offloads it had been assigned and instead
> doing that mapping in the controller so that the cs was sufficient. I
> don't think that'd work with the scenario you describe above though
> where there could be two different triggers per device tho.

I think most of the objection was to having an offloads object node
with offload@ subnodes in the SPI controller node along side the
peripheral nodes.

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

Yo,

Sorry for the delay, long reply deserved some time to sit and think
about it.

On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote:
> > > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> > > > > This adds a new property to the spi-peripheral-props binding for use
> > > > > with peripherals connected to controllers that support offloading.
> > > > >
> > > > > Here, offloading means that the controller has the ability to perform
> > > > > complex SPI transactions without CPU intervention in some shape or form.
> > > > >
> > > > > This 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.
> > > > >
> > > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > > > ---
> > > > >
> > > > > v2 changes:
> > > > >
> > > > > In v1, instead of generic SPI bindings, there were only controller-
> > > > > specific bindings, so this is a new patch.
> > > > >
> > > > > In the previous version I also had an offloads object node that described
> > > > > what the offload capabilities were but it was suggested that this was
> > > > > not necessary/overcomplicated. So I've gone to the other extreme and
> > > > > made it perhaps over-simplified now by requiring all information about
> > > > > how each offload is used to be encoded in a single u32.
> > > >
> > > > The property is a u32-array, so I guess, not a single u32?
> > >
> > > It is an array to handle cases where a peripheral might need more than
> > > one offload. But the idea was it put everything about each individual
> > > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware
> > > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then
> > > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it
> > > needed to select between both triggers at runtime.
> > >
> > > >
> > > > > We could of course consider using #spi-offload-cells instead for
> > > > > allowing encoding multiple parameters for each offload instance if that
> > > > > would be preferable.
> > > >
> > > > A -cells property was my gut reaction to what you'd written here and
> > > > seems especially appropriate if there's any likelihood of some future
> > > > device using some external resources for spi-offloading.
> > > > However, -cells properties go in providers, not consumers, so it wouldn't
> > > > end up in spi-periph-props.yaml, but rather in the controller binding,
> > > > and instead there'd be a cell array type property in here. I think you
> > > > know that though and I'm interpreting what's been written rather than
> > > > what you meant.
> > >
> > > Indeed you guess correctly. So the next question is if it should be
> > > the kind of #-cells that implies a phandle like most providers or
> > > without phandles like #address-cells?
> >
> > I'm trying to understand if the offload could ever refer to something
> > beyond the controller that you'd need the phandle for. I think it would
> > be really helpful to see an example dt of a non-trivial example for how
> > this would work. The example in the ad7944 patch has a stub controller
> > node & the clocks/dmas in the peripheral node so it is difficult to
> > reason about the spi-offloads property there.
> 
> The fully implemented and tested version of the .dts corresponding to
> the hardware pictured in the cover letter can be found at [1].
> 
> [1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts

Unfortunately this is a trivial example, so there's not much to be
gained in new information from the example in the bindings :/ Your
examples below are good though, which makes up for that and more.

> To be clear though, the idea that I am proposing here is that if there
> is something beyond the SPI controller directly connected to the
> offload, then we would add those things in the peripheral node along
> with the spi-offloads property that specifies the offload those other
> things are connected to.
> 
> Tangent on phandle vs. no phandle:

Yeah, I think not having a phandle makes sense based on what you've
said.

> Back to "something beyond the SPI controller":
> 
> Here are some examples of how I envision this would work.
> 
> Let's suppose we have a SPI controller that has some sort of offload
> capability with a configurable trigger source. The trigger can either
> be an internal software trigger (i.e. writing a register of the SPI
> controller) or and external trigger (i.e. a input signal from a pin on
> the SoC). The SPI controller has a lookup table with 8 slots where it
> can store a series of SPI commands that can be played back by
> asserting the trigger (this is what provides the "offloading").
> 
> So this SPI controller would have #spi-offload-cells = <2>; where the
> first cell would be the index in the lookup table 0 to 7 and the
> second cell would be the trigger source 0 for software or 1 for
> hardware.
> 
> Application 1: a network controller
> 
> This could use two offloads, one for TX and one for RX. For TX, we use
> the first slot with a software trigger because the data is coming from
> Linux. For RX we use the second slot with a hardware trigger since
> data is coming from the network controller (i.e. a data ready signal
> that would normally be wired to a gpio for interrupt but wired to the
> SPI offload trigger input pin instead). So the peripheral bindings
> would be:
> 
> #define SOFTWARE_TRIGGER 0
> #define HARDWARE_TRIGGER 1
> 
> can@0 {
>     ...
>     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
>     /* maybe we need names too? */
>     spi-offload-names = "tx", "rx";
> };
> 
> In this case, there is nothing extra beyond the SPI controller and the
> network controller, so no extra bindings beyond this are needed.
> 
> Application 2: an advanced ADC + FPGA
> 
> This is basically the same as the ad7944 case seen already with one
> extra feature. In this case, the sample data also contains a CRC byte
> for error checking. So instead of SPI RX data going directly to DMA,
> the FPGA removes the CRC byte from the data stream an only the sample
> data goes to the DMA buffer. The CRC is checked and if bad, an
> interrupt is asserted.
> 
> Since this is an FPGA, most everything is hardwired rather than having
> any kind of mux selection so #spi-offload-cells = <1>; for this
> controller.
> 
> By adding spi-offloads to the peripheral node, it also extends the
> peripheral binding to include the additional properties needed for the
> extra features provided by the FPGA. In other words, we are saying
> this DT node now represents the ADC chip plus everything connected to
> the offload instance used by the ADC chip.

It seems very strange to me that the dmas and the clock triggers are
going into the spi device nodes. The description is 
| +  dmas:
| +    maxItems: 1
| +    description: RX DMA Channel for receiving a samples from the SPI offload.
But as far as I can tell this device is in a package of its own and not
some IP provided by Analog that an engine on the FPGA can actually do
DMA to, and the actual connection of the device is "just" SPI.
The dmas and clock triggers etc appear to be resources belonging to the
controller that can "assigned" to a particular spi device. If the adc
gets disconnected from the system, the dmas and clock triggers are still
connected to the spi controller/offload engine, they don't end up n/c,
right? (Well maybe they would in the case of a fancy SPI device that
provides it's own sampling clock or w/e, but then it'd be a clock
provider of sorts). I'd be expecting the spi-offloads property to be
responsible for selecting which of the various resources belonging to
the controller are to be used by a device.
Maybe it overcomplicates the shit out of things and Rob or Mark are
gonna start screaming at me but w/e, looking at it from the point of
view of how the hardware is laid out (or at least how it is described
in your FPGA case above) the dma/clock properties looks like they're
misplaced. IOW, I don't think that adding the spi-offloads property
should convert a node from representing an ADC in a qfn-20 or w/e
to "the ADC chip plus everything connected to the offload instance
used by the ADC chip".

> adc@0 {
>     ...
>     spi-offloads = <0>;
>     dmas = <&dma 0>; /* channel receiving split out sample data */
>     dma-names = "rx";
>     interrupts = <&intc 99>; /* interrupt for bad CRC */
>     interrupt-names = "crc";
> };
> 
> >
> > > Asking because I got pushback on
> > > v1 for using a phandle with offloads (although in that case, the
> > > phandle was for the offload instance itself instead for the SPI
> > > controller, so maybe this is different in this case?).
> >
> > Do you have a link to this v1 pushback?
> 
> Hmm... maybe that was from some internal review before v1 that I was
> remembering and confusing with the resistance of different aspects you
> mention below.
> 
> > I had looked at the v1's binding
> > comments and didn't see that type of property being resisted - although
> > I did see some resistance to the spi peripheral node containing any of
> > the information about the offloads it had been assigned and instead
> > doing that mapping in the controller so that the cs was sufficient. I
> > don't think that'd work with the scenario you describe above though
> > where there could be two different triggers per device tho.
> 
> I think most of the objection was to having an offloads object node
> with offload@ subnodes in the SPI controller node along side the
> peripheral nodes.

I dunno, that was my reading of Rob's comments at least. I know he had
more than one objection though, so maybe we're just looking at different
portions of it - I did note that you removed the offload@ though.

Cheers,
Conor.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
>
> Yo,
>
> Sorry for the delay, long reply deserved some time to sit and think
> about it.
>
> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote:
> > > > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote:
> > > > > > This adds a new property to the spi-peripheral-props binding for use
> > > > > > with peripherals connected to controllers that support offloading.
> > > > > >
> > > > > > Here, offloading means that the controller has the ability to perform
> > > > > > complex SPI transactions without CPU intervention in some shape or form.
> > > > > >
> > > > > > This 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.
> > > > > >
> > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > > > > ---
> > > > > >
> > > > > > v2 changes:
> > > > > >
> > > > > > In v1, instead of generic SPI bindings, there were only controller-
> > > > > > specific bindings, so this is a new patch.
> > > > > >
> > > > > > In the previous version I also had an offloads object node that described
> > > > > > what the offload capabilities were but it was suggested that this was
> > > > > > not necessary/overcomplicated. So I've gone to the other extreme and
> > > > > > made it perhaps over-simplified now by requiring all information about
> > > > > > how each offload is used to be encoded in a single u32.
> > > > >
> > > > > The property is a u32-array, so I guess, not a single u32?
> > > >
> > > > It is an array to handle cases where a peripheral might need more than
> > > > one offload. But the idea was it put everything about each individual
> > > > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware
> > > > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then
> > > > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it
> > > > needed to select between both triggers at runtime.
> > > >
> > > > >
> > > > > > We could of course consider using #spi-offload-cells instead for
> > > > > > allowing encoding multiple parameters for each offload instance if that
> > > > > > would be preferable.
> > > > >
> > > > > A -cells property was my gut reaction to what you'd written here and
> > > > > seems especially appropriate if there's any likelihood of some future
> > > > > device using some external resources for spi-offloading.
> > > > > However, -cells properties go in providers, not consumers, so it wouldn't
> > > > > end up in spi-periph-props.yaml, but rather in the controller binding,
> > > > > and instead there'd be a cell array type property in here. I think you
> > > > > know that though and I'm interpreting what's been written rather than
> > > > > what you meant.
> > > >
> > > > Indeed you guess correctly. So the next question is if it should be
> > > > the kind of #-cells that implies a phandle like most providers or
> > > > without phandles like #address-cells?
> > >
> > > I'm trying to understand if the offload could ever refer to something
> > > beyond the controller that you'd need the phandle for. I think it would
> > > be really helpful to see an example dt of a non-trivial example for how
> > > this would work. The example in the ad7944 patch has a stub controller
> > > node & the clocks/dmas in the peripheral node so it is difficult to
> > > reason about the spi-offloads property there.
> >
> > The fully implemented and tested version of the .dts corresponding to
> > the hardware pictured in the cover letter can be found at [1].
> >
> > [1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts
>
> Unfortunately this is a trivial example, so there's not much to be
> gained in new information from the example in the bindings :/ Your
> examples below are good though, which makes up for that and more.
>
> > To be clear though, the idea that I am proposing here is that if there
> > is something beyond the SPI controller directly connected to the
> > offload, then we would add those things in the peripheral node along
> > with the spi-offloads property that specifies the offload those other
> > things are connected to.
> >
> > Tangent on phandle vs. no phandle:
>
> Yeah, I think not having a phandle makes sense based on what you've
> said.
>
> > Back to "something beyond the SPI controller":
> >
> > Here are some examples of how I envision this would work.
> >
> > Let's suppose we have a SPI controller that has some sort of offload
> > capability with a configurable trigger source. The trigger can either
> > be an internal software trigger (i.e. writing a register of the SPI
> > controller) or and external trigger (i.e. a input signal from a pin on
> > the SoC). The SPI controller has a lookup table with 8 slots where it
> > can store a series of SPI commands that can be played back by
> > asserting the trigger (this is what provides the "offloading").
> >
> > So this SPI controller would have #spi-offload-cells = <2>; where the
> > first cell would be the index in the lookup table 0 to 7 and the
> > second cell would be the trigger source 0 for software or 1 for
> > hardware.
> >
> > Application 1: a network controller
> >
> > This could use two offloads, one for TX and one for RX. For TX, we use
> > the first slot with a software trigger because the data is coming from
> > Linux. For RX we use the second slot with a hardware trigger since
> > data is coming from the network controller (i.e. a data ready signal
> > that would normally be wired to a gpio for interrupt but wired to the
> > SPI offload trigger input pin instead). So the peripheral bindings
> > would be:
> >
> > #define SOFTWARE_TRIGGER 0
> > #define HARDWARE_TRIGGER 1
> >
> > can@0 {
> >     ...
> >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> >     /* maybe we need names too? */
> >     spi-offload-names = "tx", "rx";
> > };
> >
> > In this case, there is nothing extra beyond the SPI controller and the
> > network controller, so no extra bindings beyond this are needed.
> >
> > Application 2: an advanced ADC + FPGA
> >
> > This is basically the same as the ad7944 case seen already with one
> > extra feature. In this case, the sample data also contains a CRC byte
> > for error checking. So instead of SPI RX data going directly to DMA,
> > the FPGA removes the CRC byte from the data stream an only the sample
> > data goes to the DMA buffer. The CRC is checked and if bad, an
> > interrupt is asserted.
> >
> > Since this is an FPGA, most everything is hardwired rather than having
> > any kind of mux selection so #spi-offload-cells = <1>; for this
> > controller.
> >
> > By adding spi-offloads to the peripheral node, it also extends the
> > peripheral binding to include the additional properties needed for the
> > extra features provided by the FPGA. In other words, we are saying
> > this DT node now represents the ADC chip plus everything connected to
> > the offload instance used by the ADC chip.
>
> It seems very strange to me that the dmas and the clock triggers are
> going into the spi device nodes. The description is
> | +  dmas:
> | +    maxItems: 1
> | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> But as far as I can tell this device is in a package of its own and not
> some IP provided by Analog that an engine on the FPGA can actually do
> DMA to, and the actual connection of the device is "just" SPI.
> The dmas and clock triggers etc appear to be resources belonging to the
> controller that can "assigned" to a particular spi device. If the adc
> gets disconnected from the system, the dmas and clock triggers are still
> connected to the spi controller/offload engine, they don't end up n/c,
> right? (Well maybe they would in the case of a fancy SPI device that
> provides it's own sampling clock or w/e, but then it'd be a clock
> provider of sorts). I'd be expecting the spi-offloads property to be
> responsible for selecting which of the various resources belonging to
> the controller are to be used by a device.
> Maybe it overcomplicates the shit out of things and Rob or Mark are
> gonna start screaming at me but w/e, looking at it from the point of
> view of how the hardware is laid out (or at least how it is described
> in your FPGA case above) the dma/clock properties looks like they're
> misplaced. IOW, I don't think that adding the spi-offloads property
> should convert a node from representing an ADC in a qfn-20 or w/e
> to "the ADC chip plus everything connected to the offload instance
> used by the ADC chip".

This is the same reasoning that led me to the binding proposed in v1.
Rob suggested that these extras (dmas/clocks) should just be
properties directly of the SPI controller. But the issue I have with
that is that since this is an FPGA, these properties are not fixed.
Maybe there are more clocks or no clocks or interrupts or something we
didn't think of yet. So it doesn't really seem possible to write a
binding for the SPI controller node to cover all of these cases. These
extras are included in the FPGA bitstream only for a specific type of
peripheral, not for general use of the SPI controller with any type of
peripheral.

Another idea I had was to perhaps use the recently added IIO backend
framework for the "extras". The idea there is that we are creating a
"composite" IIO device that consists of the ADC chip (frontend) plus
these extra hardware trigger and hardware buffer functions provided by
the FPGA (backend).

offload_backend: adc0-backend {
    /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
    compatible = "adi,pulsar-adc-offload";
    #io-backend-cells = <0>;
    dmas = <&dma 0>;
    dma-names = "rx";
    clocks = <&trigger_clock>;
};

spi {
    ...
    adc@0 {
        ...
        spi-offloads = <0>;
        io-backends = <&offload_backend>;
    };
};

While this could be a solution for IIO devices, this wouldn't solve
the issue in general though for SPI offloads used with non-IIO
peripherals. So I don't think it is the right thing to do here. But, I
think this idea of a "composite" device helps explain why we are
pushing for putting the "extras" with the peripheral node rather than
the controller node, at least for the specific case of the AXI SPI
Engine controller.

>
> > adc@0 {
> >     ...
> >     spi-offloads = <0>;
> >     dmas = <&dma 0>; /* channel receiving split out sample data */
> >     dma-names = "rx";
> >     interrupts = <&intc 99>; /* interrupt for bad CRC */
> >     interrupt-names = "crc";
> > };
> >
> > >
> > > > Asking because I got pushback on
> > > > v1 for using a phandle with offloads (although in that case, the
> > > > phandle was for the offload instance itself instead for the SPI
> > > > controller, so maybe this is different in this case?).
> > >
> > > Do you have a link to this v1 pushback?
> >
> > Hmm... maybe that was from some internal review before v1 that I was
> > remembering and confusing with the resistance of different aspects you
> > mention below.
> >
> > > I had looked at the v1's binding
> > > comments and didn't see that type of property being resisted - although
> > > I did see some resistance to the spi peripheral node containing any of
> > > the information about the offloads it had been assigned and instead
> > > doing that mapping in the controller so that the cs was sufficient. I
> > > don't think that'd work with the scenario you describe above though
> > > where there could be two different triggers per device tho.
> >
> > I think most of the objection was to having an offloads object node
> > with offload@ subnodes in the SPI controller node along side the
> > peripheral nodes.
>
> I dunno, that was my reading of Rob's comments at least. I know he had
> more than one objection though, so maybe we're just looking at different
> portions of it - I did note that you removed the offload@ though.
>
> Cheers,
> Conor.

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:

> > > Back to "something beyond the SPI controller":
> > >
> > > Here are some examples of how I envision this would work.
> > >
> > > Let's suppose we have a SPI controller that has some sort of offload
> > > capability with a configurable trigger source. The trigger can either
> > > be an internal software trigger (i.e. writing a register of the SPI
> > > controller) or and external trigger (i.e. a input signal from a pin on
> > > the SoC). The SPI controller has a lookup table with 8 slots where it
> > > can store a series of SPI commands that can be played back by
> > > asserting the trigger (this is what provides the "offloading").
> > >
> > > So this SPI controller would have #spi-offload-cells = <2>; where the
> > > first cell would be the index in the lookup table 0 to 7 and the
> > > second cell would be the trigger source 0 for software or 1 for
> > > hardware.
> > >
> > > Application 1: a network controller
> > >
> > > This could use two offloads, one for TX and one for RX. For TX, we use
> > > the first slot with a software trigger because the data is coming from
> > > Linux. For RX we use the second slot with a hardware trigger since
> > > data is coming from the network controller (i.e. a data ready signal
> > > that would normally be wired to a gpio for interrupt but wired to the
> > > SPI offload trigger input pin instead). So the peripheral bindings
> > > would be:
> > >
> > > #define SOFTWARE_TRIGGER 0
> > > #define HARDWARE_TRIGGER 1
> > >
> > > can@0 {
> > >     ...
> > >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> > >     /* maybe we need names too? */
> > >     spi-offload-names = "tx", "rx";
> > > };
> > >
> > > In this case, there is nothing extra beyond the SPI controller and the
> > > network controller, so no extra bindings beyond this are needed.
> > >
> > > Application 2: an advanced ADC + FPGA
> > >
> > > This is basically the same as the ad7944 case seen already with one
> > > extra feature. In this case, the sample data also contains a CRC byte
> > > for error checking. So instead of SPI RX data going directly to DMA,
> > > the FPGA removes the CRC byte from the data stream an only the sample
> > > data goes to the DMA buffer. The CRC is checked and if bad, an
> > > interrupt is asserted.
> > >
> > > Since this is an FPGA, most everything is hardwired rather than having
> > > any kind of mux selection so #spi-offload-cells = <1>; for this
> > > controller.
> > >
> > > By adding spi-offloads to the peripheral node, it also extends the
> > > peripheral binding to include the additional properties needed for the
> > > extra features provided by the FPGA. In other words, we are saying
> > > this DT node now represents the ADC chip plus everything connected to
> > > the offload instance used by the ADC chip.
> >
> > It seems very strange to me that the dmas and the clock triggers are
> > going into the spi device nodes. The description is
> > | +  dmas:
> > | +    maxItems: 1
> > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > But as far as I can tell this device is in a package of its own and not
> > some IP provided by Analog that an engine on the FPGA can actually do
> > DMA to, and the actual connection of the device is "just" SPI.
> > The dmas and clock triggers etc appear to be resources belonging to the
> > controller that can "assigned" to a particular spi device. If the adc
> > gets disconnected from the system, the dmas and clock triggers are still
> > connected to the spi controller/offload engine, they don't end up n/c,
> > right? (Well maybe they would in the case of a fancy SPI device that
> > provides it's own sampling clock or w/e, but then it'd be a clock
> > provider of sorts). I'd be expecting the spi-offloads property to be
> > responsible for selecting which of the various resources belonging to
> > the controller are to be used by a device.
> > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > gonna start screaming at me but w/e, looking at it from the point of
> > view of how the hardware is laid out (or at least how it is described
> > in your FPGA case above) the dma/clock properties looks like they're
> > misplaced. IOW, I don't think that adding the spi-offloads property
> > should convert a node from representing an ADC in a qfn-20 or w/e
> > to "the ADC chip plus everything connected to the offload instance
> > used by the ADC chip".
> 
> This is the same reasoning that led me to the binding proposed in v1.
> Rob suggested that these extras (dmas/clocks) should just be
> properties directly of the SPI controller.

> But the issue I have with
> that is that since this is an FPGA, these properties are not fixed.

I don't think whether or not we're talking about an FPGA or an ASIC
matters at all here to be honest. In my view the main thing that FPGA
impact in terms of bindings is the number of properties required to
represent the configurability of a particular IP. Sure, you're gonna
have to change the dts around in a way you'll never have to with an
ASIC, but the description of individual devices or relations between
them doesn't change.

> Maybe there are more clocks or no clocks or interrupts or something we
> didn't think of yet.

This could happen with a new generation of an ASIC and is not specific
to an FPGA IP core. Even with FPGA IP, while there may be lots of
different configuration parameters, they are known & limited - you can
look at the IP's documentation (or failing that, the HDL) to figure out
what the points of variability are. It's not particularly difficult to
account for there being a configurable number of clocks or interrupts.
For "something we didn't think of yet" to be a factor, someone has to
think of it and add it to the IP core, and which point we can absolutely
change the bindings and software to account for the revised IP.

> So it doesn't really seem possible to write a
> binding for the SPI controller node to cover all of these cases.

I dunno, I don't think your concerns about numbers of clocks (or any
other such property) are unique to this particular use-case.

> These
> extras are included in the FPGA bitstream only for a specific type of
> peripheral, not for general use of the SPI controller with any type of
> peripheral.

I accept that, but what I was trying to get across was that you could
configure the FPGA with a bitstream that contains these extra resources
and then connect a peripheral device that does not make use of them at
all. Or you could connect a range of different peripherals that do use
them. Whether or not the resources are there and how they are connected
in the FPGA bitstream is not a property of the device connected to the
SPI controller, only whether or not you use them is.

In fact, looking at the documentation for the "spi engine" some more, I
am even less convinced that the right place for describing the offload is
the peripheral as I (finally?) noticed that the registers for the offload
engine are mapped directly into the "spi engine"'s memory map, rather than
it being a entirely separate IP in the FPGA fabric.

Further, what resources are available depends on what the SPI offload
engine IP is. If my employer decides to start shipping some SPI offload
IP in our catalogue that can work with ADI's ADCs, the set of offload
related properties or their names may well differ.

> Another idea I had was to perhaps use the recently added IIO backend
> framework for the "extras". The idea there is that we are creating a
> "composite" IIO device that consists of the ADC chip (frontend) plus
> these extra hardware trigger and hardware buffer functions provided by
> the FPGA (backend).
> 
> offload_backend: adc0-backend {
>     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
>     compatible = "adi,pulsar-adc-offload";
>     #io-backend-cells = <0>;
>     dmas = <&dma 0>;
>     dma-names = "rx";
>     clocks = <&trigger_clock>;
> };
> 
> spi {
>     ...
>     adc@0 {
>         ...
>         spi-offloads = <0>;
>         io-backends = <&offload_backend>;
>     };
> };
> 
> While this could be a solution for IIO devices, this wouldn't solve
> the issue in general though for SPI offloads used with non-IIO
> peripherals.

Yeah, I agree that using something specific to IIO isn't really a good
solution.

Cheers,
Conor.

> So I don't think it is the right thing to do here. But, I
> think this idea of a "composite" device helps explain why we are
> pushing for putting the "extras" with the peripheral node rather than
> the controller node, at least for the specific case of the AXI SPI
> Engine controller.


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

Re: [PATCH RFC v2 2/8] spi: add basic support for SPI offloading

[Nuno Sá]

On Fri, 2024-05-10 at 19:44 -0500, David Lechner wrote:
> SPI offloading is a feature that allows the SPI controller to perform
> complex transfers without 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>
> ---
> 
> 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.
> 

Yeah, what I disliked the most was adding the platform devices from spi-engine
driver and then the complexity in the IIO trigger part of it. I also didn't like
(as you said) for the peripheral to have to explicitly request an offload but,
IIRC, Mark was actually ok with the spi_offload_get/put() mechanism so let's see
what he has to say. He's main point (I think) was for the controllers to have a
way to know which offload instance is busy or not (but I guess controllers can
keep track of that as well with this approach and using the enable/disable
callbacks or the prepare/unprepare).

THB, I do like this approach (and it is what I had in mind) and it's simple
enough while covering what we know about this feature atm.

- Nuno Sá

Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support

[Nuno Sá]

On Fri, 2024-05-10 at 19:44 -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>
> ---
> 
> 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 | 267
> ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 264 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> engine.c
> index e358ac5b4509..95327df572a0 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -2,6 +2,7 @@
>  /*
>   * SPI-Engine SPI controller driver
>   * Copyright 2015 Analog Devices Inc.
> + * Copyright 2024 BayLibre, SAS
>   *  Author: Lars-Peter Clausen <lars@metafoo.de>
>   */
>  
> @@ -16,6 +17,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 +25,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 +36,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)
> @@ -74,6 +91,10 @@
>  #define SPI_ENGINE_CMD_SYNC(id) \
>  	SPI_ENGINE_CMD(SPI_ENGINE_INST_MISC, SPI_ENGINE_MISC_SYNC, (id))
>  
> +/* 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);
> @@ -101,6 +122,12 @@ struct spi_engine_message_state {
>  	uint8_t *rx_buf;
>  };
>  
> +struct spi_engine_offload {
> +	struct spi_device *spi;
> +	unsigned int id;
> +	bool prepared;
> +};
> +
>  struct spi_engine {
>  	struct clk *clk;
>  	struct clk *ref_clk;
> @@ -111,6 +138,10 @@ struct spi_engine {
>  	struct spi_engine_message_state msg_state;
>  	struct completion msg_complete;
>  	unsigned int int_enable;
> +
> +	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,
> @@ -154,7 +185,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,
> @@ -202,16 +233,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,
> @@ -503,8 +542,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);
> @@ -539,6 +581,11 @@ 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");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	/* reinitialize message state for this transfer */
>  	memset(st, 0, sizeof(*st));
>  	st->cmd_buf = p->instructions;
> @@ -579,6 +626,204 @@ static int spi_engine_transfer_one_message(struct
> spi_controller *host,
>  	return msg->status;
>  }
>  
> +static struct spi_engine_offload *spi_engine_get_offload(struct spi_device
> *spi,
> +							 unsigned int 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 && priv->id == id) {
> +			*offload_num = i;
> +			return priv;
> +		}
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static int spi_engine_offload_map_channel(struct spi_device *spi,
> +					  unsigned int id,
> +					  unsigned int channel)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +
> +	if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
> +		return -EINVAL;
> +
> +	priv = &spi_engine->offload_priv[channel];
> +
> +	if (priv->spi)
> +		return -EBUSY;

I wonder if we need to be this strict? Is there any problem by having two
devices requesting the same offload engine? I would expect that having multiple
peripherals trying to actually use it at the same time (with the prepare()
callback) to be problematic but if they play along it could actually work,
right? In reality that may never be a realistic usecase so this is likely fine.

> +
> +	priv->spi = spi;
> +	priv->id = id;
> +
> +	return 0;
> +}
> +
> +static int spi_engine_offload_prepare(struct spi_device *spi, unsigned int
> 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(priv-
> >id);
> +	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, unsigned int
> 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_offload_enable(struct spi_device *spi, unsigned int 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_offload_disable(struct spi_device *spi, unsigned int
> 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 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_enable = spi_engine_offload_enable,
> +	.hw_trigger_disable = spi_engine_offload_disable,

I guess this is what you and Conor are already somehow discussing but I would
expect this to be the actual offload trigger to play a spi transfer. As it
stands, it looks weird (or confusing) to have the enable/disable of the engine
to act as a trigger... Maybe these callbacks could be used to enable/disable the
actual trigger of the offload engine (in our current cases, the PWM)? So this
would make it easy to move the trigger DT property where it belongs. The DMA one
(given it's tight relation with IIO DMA buffers) is another (way more difficult)
story I think.

- Nuno Sá

Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support

[David Lechner]

On Tue, May 21, 2024 at 7:27 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Fri, 2024-05-10 at 19:44 -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>
> > ---
> >

...

> > +
> > +static int spi_engine_offload_map_channel(struct spi_device *spi,
> > +                                       unsigned int id,
> > +                                       unsigned int channel)
> > +{
> > +     struct spi_controller *host = spi->controller;
> > +     struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > +     struct spi_engine_offload *priv;
> > +
> > +     if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
> > +             return -EINVAL;
> > +
> > +     priv = &spi_engine->offload_priv[channel];
> > +
> > +     if (priv->spi)
> > +             return -EBUSY;
>
> I wonder if we need to be this strict? Is there any problem by having two
> devices requesting the same offload engine? I would expect that having multiple
> peripherals trying to actually use it at the same time (with the prepare()
> callback) to be problematic but if they play along it could actually work,
> right? In reality that may never be a realistic usecase so this is likely fine.
>

I guess not. But to keep it simple for now, yeah, let's wait until we
have an actual use case.

...

> > +
> > +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_enable = spi_engine_offload_enable,
> > +     .hw_trigger_disable = spi_engine_offload_disable,
>
> I guess this is what you and Conor are already somehow discussing but I would
> expect this to be the actual offload trigger to play a spi transfer. As it
> stands, it looks weird (or confusing) to have the enable/disable of the engine
> to act as a trigger...

It isn't acting as the trigger, just configuring the offload instance
for exclusive use by a hardware trigger.

> Maybe these callbacks could be used to enable/disable the
> actual trigger of the offload engine (in our current cases, the PWM)? So this
> would make it easy to move the trigger DT property where it belongs. The DMA one
> (given it's tight relation with IIO DMA buffers) is another (way more difficult)
> story I think.
>

One issue I have with making the actual hardware trigger part of the
SPI controller is that in some cases, the peripheral could actually be
the trigger. For example, in the case of a self-clocked ADC where
there is just a RDY signal from the ADC when sample data is ready to
be read. In this case would the peripheral have to register a trigger
enable callback with the controller so that the controller can
communicate with the peripheral to enable and disable sampling mode,
and therefore the trigger?

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>
> > > > Back to "something beyond the SPI controller":
> > > >
> > > > Here are some examples of how I envision this would work.
> > > >
> > > > Let's suppose we have a SPI controller that has some sort of offload
> > > > capability with a configurable trigger source. The trigger can either
> > > > be an internal software trigger (i.e. writing a register of the SPI
> > > > controller) or and external trigger (i.e. a input signal from a pin on
> > > > the SoC). The SPI controller has a lookup table with 8 slots where it
> > > > can store a series of SPI commands that can be played back by
> > > > asserting the trigger (this is what provides the "offloading").
> > > >
> > > > So this SPI controller would have #spi-offload-cells = <2>; where the
> > > > first cell would be the index in the lookup table 0 to 7 and the
> > > > second cell would be the trigger source 0 for software or 1 for
> > > > hardware.
> > > >
> > > > Application 1: a network controller
> > > >
> > > > This could use two offloads, one for TX and one for RX. For TX, we use
> > > > the first slot with a software trigger because the data is coming from
> > > > Linux. For RX we use the second slot with a hardware trigger since
> > > > data is coming from the network controller (i.e. a data ready signal
> > > > that would normally be wired to a gpio for interrupt but wired to the
> > > > SPI offload trigger input pin instead). So the peripheral bindings
> > > > would be:
> > > >
> > > > #define SOFTWARE_TRIGGER 0
> > > > #define HARDWARE_TRIGGER 1
> > > >
> > > > can@0 {
> > > >     ...
> > > >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> > > >     /* maybe we need names too? */
> > > >     spi-offload-names = "tx", "rx";
> > > > };
> > > >
> > > > In this case, there is nothing extra beyond the SPI controller and the
> > > > network controller, so no extra bindings beyond this are needed.
> > > >
> > > > Application 2: an advanced ADC + FPGA
> > > >
> > > > This is basically the same as the ad7944 case seen already with one
> > > > extra feature. In this case, the sample data also contains a CRC byte
> > > > for error checking. So instead of SPI RX data going directly to DMA,
> > > > the FPGA removes the CRC byte from the data stream an only the sample
> > > > data goes to the DMA buffer. The CRC is checked and if bad, an
> > > > interrupt is asserted.
> > > >
> > > > Since this is an FPGA, most everything is hardwired rather than having
> > > > any kind of mux selection so #spi-offload-cells = <1>; for this
> > > > controller.
> > > >
> > > > By adding spi-offloads to the peripheral node, it also extends the
> > > > peripheral binding to include the additional properties needed for the
> > > > extra features provided by the FPGA. In other words, we are saying
> > > > this DT node now represents the ADC chip plus everything connected to
> > > > the offload instance used by the ADC chip.
> > >
> > > It seems very strange to me that the dmas and the clock triggers are
> > > going into the spi device nodes. The description is
> > > | +  dmas:
> > > | +    maxItems: 1
> > > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > > But as far as I can tell this device is in a package of its own and not
> > > some IP provided by Analog that an engine on the FPGA can actually do
> > > DMA to, and the actual connection of the device is "just" SPI.
> > > The dmas and clock triggers etc appear to be resources belonging to the
> > > controller that can "assigned" to a particular spi device. If the adc
> > > gets disconnected from the system, the dmas and clock triggers are still
> > > connected to the spi controller/offload engine, they don't end up n/c,
> > > right? (Well maybe they would in the case of a fancy SPI device that
> > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > responsible for selecting which of the various resources belonging to
> > > the controller are to be used by a device.
> > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > gonna start screaming at me but w/e, looking at it from the point of
> > > view of how the hardware is laid out (or at least how it is described
> > > in your FPGA case above) the dma/clock properties looks like they're
> > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > to "the ADC chip plus everything connected to the offload instance
> > > used by the ADC chip".
> >
> > This is the same reasoning that led me to the binding proposed in v1.
> > Rob suggested that these extras (dmas/clocks) should just be
> > properties directly of the SPI controller.
>
> > But the issue I have with
> > that is that since this is an FPGA, these properties are not fixed.
>
> I don't think whether or not we're talking about an FPGA or an ASIC
> matters at all here to be honest. In my view the main thing that FPGA
> impact in terms of bindings is the number of properties required to
> represent the configurability of a particular IP. Sure, you're gonna
> have to change the dts around in a way you'll never have to with an
> ASIC, but the description of individual devices or relations between
> them doesn't change.
>
> > Maybe there are more clocks or no clocks or interrupts or something we
> > didn't think of yet.
>
> This could happen with a new generation of an ASIC and is not specific
> to an FPGA IP core. Even with FPGA IP, while there may be lots of
> different configuration parameters, they are known & limited - you can
> look at the IP's documentation (or failing that, the HDL) to figure out
> what the points of variability are. It's not particularly difficult to
> account for there being a configurable number of clocks or interrupts.
> For "something we didn't think of yet" to be a factor, someone has to
> think of it and add it to the IP core, and which point we can absolutely
> change the bindings and software to account for the revised IP.
>
> > So it doesn't really seem possible to write a
> > binding for the SPI controller node to cover all of these cases.
>
> I dunno, I don't think your concerns about numbers of clocks (or any
> other such property) are unique to this particular use-case.
>
> > These
> > extras are included in the FPGA bitstream only for a specific type of
> > peripheral, not for general use of the SPI controller with any type of
> > peripheral.
>
> I accept that, but what I was trying to get across was that you could
> configure the FPGA with a bitstream that contains these extra resources
> and then connect a peripheral device that does not make use of them at
> all.

Sure, in this case the peripheral has no spi-offloads property and the
SPI controller acts as a typical SPI controller.

> Or you could connect a range of different peripherals that do use
> them.

OK, you've got me thinking about something I hadn't really thought about yet.

> Whether or not the resources are there and how they are connected
> in the FPGA bitstream is not a property of the device connected to the
> SPI controller, only whether or not you use them is.
>

Even when those things are connected directly to a specific peripheral
device? Or not connected to the offload?

Here is another potential ADC trigger case to consider.

+-------------------------------+   +------------------+
|                               |   |                  |
|  SOC/FPGA                     |   |  ADC             |
|  +---------------------+      |   |                  |
|  | AXI SPI Engine      |      |   |                  |
|  |             SPI Bus ============ SPI Bus          |
|  |                     |      |   |                  |
|  |  +---------------+  |  < < < < < BUSY             |
|  |  | Offload 0     |  | v    |   |                  |
|  |  |               |  | v  > > > > CNV              |
|  |  |    TRIGGER IN < < <   ^ |   |                  |
|  |  +---------------+  |    ^ |   +------------------+
|  +---------------------+    ^ |
|  | AXI PWM             |    ^ |
|  |                 CH0 >  > ^ |
|  +---------------------+      |
|                               |
+-------------------------------+

This time, the periodic trigger (PWM) is connected to the pin on the
ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
that will go high at the start of the conversion and go low at the end
of the conversion. The BUSY output of the ADC is wired as the hardware
trigger input of the offload.

In this case would we still consider the PWM as part of the SPI
controller/offload since it can only be used in conjunction with the
SPI offload? It isn't connected to it at all though.

Another case could be a self-clocked ADC. Here, the ADC has it's own
periodic sample trigger on the chip and the RDY output goes high
whenever a sample is ready to read.

+-------------------------------+   +------------------+
|                               |   |                  |
|  SOC/FPGA                     |   |  ADC             |
|  +---------------------+      |   |                  |
|  | AXI SPI Engine      |      |   |                  |
|  |             SPI Bus ============ SPI Bus          |
|  |                     |      |   |                  |
|  |  +---------------+  |  < < < < < RDY              |
|  |  | Offload 0     |  | v    |   |                  |
|  |  |               |  | v    |   |                  |
|  |  |    TRIGGER IN < < <     |   |                  |
|  |  +---------------+  |      |   +------------------+
|  +---------------------+      |
|                               |
+-------------------------------+

If both of these are valid wiring configurations for the same ADC,
wouldn't it make more sense to describe this in the peripheral node
rather than having to query the controller to see how the peripheral
is wired up?

> In fact, looking at the documentation for the "spi engine" some more, I
> am even less convinced that the right place for describing the offload is
> the peripheral as I (finally?) noticed that the registers for the offload
> engine are mapped directly into the "spi engine"'s memory map, rather than
> it being a entirely separate IP in the FPGA fabric.

True, but we don't use these registers, e.g., to configure the
sampling frequency of a trigger (if it can even be done). That is done
in a completely separate IP block with it's own registers.

>
> Further, what resources are available depends on what the SPI offload
> engine IP is. If my employer decides to start shipping some SPI offload
> IP in our catalogue that can work with ADI's ADCs, the set of offload
> related properties or their names may well differ.

If all of these resources were fairly generic, like the periodic
trigger we've been discussing, then I could see the case for trying to
accommodate this the same way we do for other common features of SPI
controllers. But for cases like "Application 2" that I described
previously, these resources can get very specific to a peripheral
(like the example given of having a data processing unit that knows
about the exact data format and CRC algorithm used by a specific ADC).
These seems like too specific of a thing to say that a SPI controller
"supports".

But, OK, let's go with the idea of putting everything related to the
offloads in the SPI controller node an see where it takes us...

spi@1000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* PWM is connected to offload hardware trigger. DMA for streaming sample
     * data can only handle 16-bit words. IIO hardware buffer will be CPU-
     * endian because data is streamed one word at a time. */
    spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma";

    /* pwm properties are only allowed because spi-offload-0-capabilities
     * contains "pwm-trigger" */
    pwms = <&pwm 0>;
    pwm-names = "offload-0-pwm-trigger";

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...
    };
};

spi@2000 {
    compatible = "not-adi,other-spi-engine";
    #spi-offload-cells = <1>;
    /* Clock is connected to offload hardware trigger. DMA for streaming sample
     * data can only handle one byte at a time. IIO hardware buffer will be big-
     * endian because data is streamed one byte at a time. */
    spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma";

    /* Clock properties are extended because spi-offload-0-capabilities contains
     * "clock-trigger" and SPI controller itself has a clock */
    clocks = <&cpu_clock 0>, <&extra_clock 0>;
    clock-names = "sclk", "offload-0-pwm-trigger";

    /* DMA properties are extended because spi-offload-0-capabilities contains
     * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */
    dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>;
    dma-names = "tx", "rx", "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...
    };
};

spi@3000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* Sample ready signal (~BSY) from ADC is connected to offload hardware
     * trigger. DMA for streaming sample data can only handle 16-bit words. */
    spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma";

    /* Do we need a property to say that the sample trigger is connected to
     * adc@0 so that if adc@1 tries to use it, it will fail? */

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...

        /* PWM connected to the conversion pin (CNV). This only makes sense
         * when offload is used with BSY signal, otherwise we would have CNV
         * connected to SPI CS. */
        pwms = <&pwm 0>;
        pwm-names = "cnv";
    };
};

spi@4000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* Sample ready signal (~BSY) from ADC is connected to offload hardware
     * trigger. DMA for streaming sample data can only handle 32-bit words.
     * This also has the CRC validation that strips off the CRC byte of the
     * raw data before passing the sample to DMA. */
    spi-offload-0-capabilities = "sample-trigger",
                                 "32-bit-rx-dma-with-ad4630-crc-check";

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    interrupt-parent = <&intc>;
    /* First interrupt is for the SPI controller (always present), second
     * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check"
     * of offload 0. */
    interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>;
    interrupt-names = "controller", "offload-0-crc-error";

    ...

    adc@0 {
        compatible = "adi,ad4630";
        spi-offloads = <0>;
        ...

        /* PWM connected to the conversion pin (CNV). Without offload, we would
         * have cnv-gpios instead. */
        pwms = <&pwm 0>;
        pwm-names = "cnv";
    };
};

So this is what I came up with of how things could look (combining
suggestions from Rob in v1 and Conor's suggestions here). I can see
how we can make this work. But the thing I don't like about it is that
the peripheral drivers still need to know all of the information about
the offload capabilities and need to interact with the
pwms/clocks/interrupts/dmas/etc. So this is just adding layers of
indirection where all of this stuff has to go through the SPI
controller driver.


>
> > Another idea I had was to perhaps use the recently added IIO backend
> > framework for the "extras". The idea there is that we are creating a
> > "composite" IIO device that consists of the ADC chip (frontend) plus
> > these extra hardware trigger and hardware buffer functions provided by
> > the FPGA (backend).
> >
> > offload_backend: adc0-backend {
> >     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
> >     compatible = "adi,pulsar-adc-offload";
> >     #io-backend-cells = <0>;
> >     dmas = <&dma 0>;
> >     dma-names = "rx";
> >     clocks = <&trigger_clock>;
> > };
> >
> > spi {
> >     ...
> >     adc@0 {
> >         ...
> >         spi-offloads = <0>;
> >         io-backends = <&offload_backend>;
> >     };
> > };
> >
> > While this could be a solution for IIO devices, this wouldn't solve
> > the issue in general though for SPI offloads used with non-IIO
> > peripherals.
>
> Yeah, I agree that using something specific to IIO isn't really a good
> solution.
>
> Cheers,
> Conor.
>
> > So I don't think it is the right thing to do here. But, I
> > think this idea of a "composite" device helps explain why we are
> > pushing for putting the "extras" with the peripheral node rather than
> > the controller node, at least for the specific case of the AXI SPI
> > Engine controller.
>

Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support

[Nuno Sá]

On Tue, 2024-05-21 at 09:28 -0500, David Lechner wrote:
> On Tue, May 21, 2024 at 7:27 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Fri, 2024-05-10 at 19:44 -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>
> > > ---
> > > 
> 
> ...
> 
> > > +
> > > +static int spi_engine_offload_map_channel(struct spi_device *spi,
> > > +                                       unsigned int id,
> > > +                                       unsigned int channel)
> > > +{
> > > +     struct spi_controller *host = spi->controller;
> > > +     struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +     struct spi_engine_offload *priv;
> > > +
> > > +     if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
> > > +             return -EINVAL;
> > > +
> > > +     priv = &spi_engine->offload_priv[channel];
> > > +
> > > +     if (priv->spi)
> > > +             return -EBUSY;
> > 
> > I wonder if we need to be this strict? Is there any problem by having two
> > devices requesting the same offload engine? I would expect that having multiple
> > peripherals trying to actually use it at the same time (with the prepare()
> > callback) to be problematic but if they play along it could actually work,
> > right? In reality that may never be a realistic usecase so this is likely fine.
> > 
> 
> I guess not. But to keep it simple for now, yeah, let's wait until we
> have an actual use case.
> 

Agreed.

> ...
> 
> > > +
> > > +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_enable = spi_engine_offload_enable,
> > > +     .hw_trigger_disable = spi_engine_offload_disable,
> > 
> > I guess this is what you and Conor are already somehow discussing but I would
> > expect this to be the actual offload trigger to play a spi transfer. As it
> > stands, it looks weird (or confusing) to have the enable/disable of the engine
> > to act as a trigger...
> 
> It isn't acting as the trigger, just configuring the offload instance
> for exclusive use by a hardware trigger.
> 
> > Maybe these callbacks could be used to enable/disable the
> > actual trigger of the offload engine (in our current cases, the PWM)? So this
> > would make it easy to move the trigger DT property where it belongs. The DMA one
> > (given it's tight relation with IIO DMA buffers) is another (way more difficult)
> > story I think.
> > 
> 
> One issue I have with making the actual hardware trigger part of the
> SPI controller is that in some cases, the peripheral could actually be
> the trigger. For example, in the case of a self-clocked ADC where
> there is just a RDY signal from the ADC when sample data is ready to
> be read. In this case would the peripheral have to register a trigger
> enable callback with the controller so that the controller can
> communicate with the peripheral to enable and disable sampling mode,
> and therefore the trigger?

In those cases, the peripheral would not bother in doing any spi hardware triggering
enable/disable (this assumes that we would need explicit interfaces for offload
enable/disable). In DT, the engine would also have no trigger has it's not required
so the controller even conditionally register these callbacks.

As for the DMA, I have no clue how we can have it associated with the controller
given how we want the data to be exported to userspace. Maybe dma-buf could be used
but it won't be easy. TBH, I'm not sure if it's that bad having the DMA associated
with the peripheral since it's purpose is to transfer peripheral DATA (not like a spi
controller DMA used for the actual SPI transfers).

- Nuno Sá

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >
> > > > > Back to "something beyond the SPI controller":
> > > > >
> > > > > Here are some examples of how I envision this would work.
> > > > >
> > > > > Let's suppose we have a SPI controller that has some sort of offload
> > > > > capability with a configurable trigger source. The trigger can either
> > > > > be an internal software trigger (i.e. writing a register of the SPI
> > > > > controller) or and external trigger (i.e. a input signal from a pin on
> > > > > the SoC). The SPI controller has a lookup table with 8 slots where it
> > > > > can store a series of SPI commands that can be played back by
> > > > > asserting the trigger (this is what provides the "offloading").
> > > > >
> > > > > So this SPI controller would have #spi-offload-cells = <2>; where the
> > > > > first cell would be the index in the lookup table 0 to 7 and the
> > > > > second cell would be the trigger source 0 for software or 1 for
> > > > > hardware.
> > > > >
> > > > > Application 1: a network controller
> > > > >
> > > > > This could use two offloads, one for TX and one for RX. For TX, we use
> > > > > the first slot with a software trigger because the data is coming from
> > > > > Linux. For RX we use the second slot with a hardware trigger since
> > > > > data is coming from the network controller (i.e. a data ready signal
> > > > > that would normally be wired to a gpio for interrupt but wired to the
> > > > > SPI offload trigger input pin instead). So the peripheral bindings
> > > > > would be:
> > > > >
> > > > > #define SOFTWARE_TRIGGER 0
> > > > > #define HARDWARE_TRIGGER 1
> > > > >
> > > > > can@0 {
> > > > >     ...
> > > > >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> > > > >     /* maybe we need names too? */
> > > > >     spi-offload-names = "tx", "rx";
> > > > > };
> > > > >
> > > > > In this case, there is nothing extra beyond the SPI controller and the
> > > > > network controller, so no extra bindings beyond this are needed.
> > > > >
> > > > > Application 2: an advanced ADC + FPGA
> > > > >
> > > > > This is basically the same as the ad7944 case seen already with one
> > > > > extra feature. In this case, the sample data also contains a CRC byte
> > > > > for error checking. So instead of SPI RX data going directly to DMA,
> > > > > the FPGA removes the CRC byte from the data stream an only the sample
> > > > > data goes to the DMA buffer. The CRC is checked and if bad, an
> > > > > interrupt is asserted.
> > > > >
> > > > > Since this is an FPGA, most everything is hardwired rather than having
> > > > > any kind of mux selection so #spi-offload-cells = <1>; for this
> > > > > controller.
> > > > >
> > > > > By adding spi-offloads to the peripheral node, it also extends the
> > > > > peripheral binding to include the additional properties needed for the
> > > > > extra features provided by the FPGA. In other words, we are saying
> > > > > this DT node now represents the ADC chip plus everything connected to
> > > > > the offload instance used by the ADC chip.
> > > >
> > > > It seems very strange to me that the dmas and the clock triggers are
> > > > going into the spi device nodes. The description is
> > > > | +  dmas:
> > > > | +    maxItems: 1
> > > > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > > > But as far as I can tell this device is in a package of its own and not
> > > > some IP provided by Analog that an engine on the FPGA can actually do
> > > > DMA to, and the actual connection of the device is "just" SPI.
> > > > The dmas and clock triggers etc appear to be resources belonging to the
> > > > controller that can "assigned" to a particular spi device. If the adc
> > > > gets disconnected from the system, the dmas and clock triggers are still
> > > > connected to the spi controller/offload engine, they don't end up n/c,
> > > > right? (Well maybe they would in the case of a fancy SPI device that
> > > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > > responsible for selecting which of the various resources belonging to
> > > > the controller are to be used by a device.
> > > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > > gonna start screaming at me but w/e, looking at it from the point of
> > > > view of how the hardware is laid out (or at least how it is described
> > > > in your FPGA case above) the dma/clock properties looks like they're
> > > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > > to "the ADC chip plus everything connected to the offload instance
> > > > used by the ADC chip".
> > >
> > > This is the same reasoning that led me to the binding proposed in v1.
> > > Rob suggested that these extras (dmas/clocks) should just be
> > > properties directly of the SPI controller.
> >
> > > But the issue I have with
> > > that is that since this is an FPGA, these properties are not fixed.
> >
> > I don't think whether or not we're talking about an FPGA or an ASIC
> > matters at all here to be honest. In my view the main thing that FPGA
> > impact in terms of bindings is the number of properties required to
> > represent the configurability of a particular IP. Sure, you're gonna
> > have to change the dts around in a way you'll never have to with an
> > ASIC, but the description of individual devices or relations between
> > them doesn't change.
> >
> > > Maybe there are more clocks or no clocks or interrupts or something we
> > > didn't think of yet.
> >
> > This could happen with a new generation of an ASIC and is not specific
> > to an FPGA IP core. Even with FPGA IP, while there may be lots of
> > different configuration parameters, they are known & limited - you can
> > look at the IP's documentation (or failing that, the HDL) to figure out
> > what the points of variability are. It's not particularly difficult to
> > account for there being a configurable number of clocks or interrupts.
> > For "something we didn't think of yet" to be a factor, someone has to
> > think of it and add it to the IP core, and which point we can absolutely
> > change the bindings and software to account for the revised IP.
> >
> > > So it doesn't really seem possible to write a
> > > binding for the SPI controller node to cover all of these cases.
> >
> > I dunno, I don't think your concerns about numbers of clocks (or any
> > other such property) are unique to this particular use-case.
> >
> > > These
> > > extras are included in the FPGA bitstream only for a specific type of
> > > peripheral, not for general use of the SPI controller with any type of
> > > peripheral.
> >
> > I accept that, but what I was trying to get across was that you could
> > configure the FPGA with a bitstream that contains these extra resources
> > and then connect a peripheral device that does not make use of them at
> > all.
> 
> Sure, in this case the peripheral has no spi-offloads property and the
> SPI controller acts as a typical SPI controller.
> 
> > Or you could connect a range of different peripherals that do use
> > them.
> 
> OK, you've got me thinking about something I hadn't really thought about yet.
> 
> > Whether or not the resources are there and how they are connected
> > in the FPGA bitstream is not a property of the device connected to the
> > SPI controller, only whether or not you use them is.
> >
> 
> Even when those things are connected directly to a specific peripheral
> device? Or not connected to the offload?
> 
> Here is another potential ADC trigger case to consider.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < BUSY             |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v  > > > > CNV              |
> |  |  |    TRIGGER IN < < <   ^ |   |                  |
> |  |  +---------------+  |    ^ |   +------------------+
> |  +---------------------+    ^ |
> |  | AXI PWM             |    ^ |
> |  |                 CH0 >  > ^ |
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> This time, the periodic trigger (PWM) is connected to the pin on the
> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> that will go high at the start of the conversion and go low at the end
> of the conversion. The BUSY output of the ADC is wired as the hardware
> trigger input of the offload.
> 
> In this case would we still consider the PWM as part of the SPI
> controller/offload since it can only be used in conjunction with the
> SPI offload? It isn't connected to it at all though.

No, in this case the ADC is a PWM consumer and the offload engine is
not. The ADC is a "trigger" provider and the SPI offload engine is a
trigger consumer.

> Another case could be a self-clocked ADC. Here, the ADC has it's own
> periodic sample trigger on the chip and the RDY output goes high
> whenever a sample is ready to read.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < RDY              |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v    |   |                  |
> |  |  |    TRIGGER IN < < <     |   |                  |
> |  |  +---------------+  |      |   +------------------+
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> If both of these are valid wiring configurations for the same ADC,
> wouldn't it make more sense to describe this in the peripheral node
> rather than having to query the controller to see how the peripheral
> is wired up?

In both of these cases, the offload works in the same way, it gets a
trigger from the ADC and acts upon it. I would expect that in this case
the ADC driver would look for an optional pwm property in the devicetree
and if it is present configure the ADC to use that and if absent do then
do whatever configuration is required for self clocking. I would expect
that both cases would be handled identically by the SPI [offload] engine
side of things, other than inverting the polarity of the trigger. (If I
understand correctly, you want to trigger the offload engine on a
falling edge of BUSY but a rising edge of RDY).


> > In fact, looking at the documentation for the "spi engine" some more, I
> > am even less convinced that the right place for describing the offload is
> > the peripheral as I (finally?) noticed that the registers for the offload
> > engine are mapped directly into the "spi engine"'s memory map, rather than
> > it being a entirely separate IP in the FPGA fabric.
> 
> True, but we don't use these registers, e.g., to configure the
> sampling frequency of a trigger (if it can even be done). That is done
> in a completely separate IP block with it's own registers.

Where is the binding for that IP block? I think describing that is
pretty key. goto continuation;

> > Further, what resources are available depends on what the SPI offload
> > engine IP is. If my employer decides to start shipping some SPI offload
> > IP in our catalogue that can work with ADI's ADCs, the set of offload
> > related properties or their names may well differ.
> 
> If all of these resources were fairly generic, like the periodic
> trigger we've been discussing, then I could see the case for trying to
> accommodate this the same way we do for other common features of SPI
> controllers. But for cases like "Application 2" that I described
> previously, these resources can get very specific to a peripheral
> (like the example given of having a data processing unit that knows
> about the exact data format and CRC algorithm used by a specific ADC).
> These seems like too specific of a thing to say that a SPI controller
> "supports".

To remind myself, "Application 2" featured an offload engine designed
specifically to work with a particular data format that would strip a
CRC byte and check the validity of the data stream.

I think you're right something like that is a stretch to say that that
is a feature of the SPI controller - but I still don't believe that
modelling it as part of the ADC is correct. I don't fully understand the
io-backends and how they work yet, but the features you describe there
seem like something that should/could be modelled as one, with its own
node and compatible etc. Describing custom RTL stuff ain't always
strightforward, but the stuff from Analog is versioned and documented
etc so it shouldn't be quite that hard.

continuation:
If offload engines have their own register region in the memory map,
their own resources (the RTL is gonna need at the very least a clock)
and potentially also provide other services (like acting as an
io-backend type device that pre-processes data) it doesn't sound like
either the controller or peripheral nodes are the right place for these
properties. And uh, spi-offloads gets a phandle once more...

FWIW, I did read these examples but didn't feel it was worth commenting
on them given the above. I'll comment on them if they stay "accurate".

Cheers,
Conor.

> But, OK, let's go with the idea of putting everything related to the
> offloads in the SPI controller node an see where it takes us...
> 
> spi@1000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* PWM is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle 16-bit words. IIO hardware buffer will be CPU-
>      * endian because data is streamed one word at a time. */
>     spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma";
> 
>     /* pwm properties are only allowed because spi-offload-0-capabilities
>      * contains "pwm-trigger" */
>     pwms = <&pwm 0>;
>     pwm-names = "offload-0-pwm-trigger";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@2000 {
>     compatible = "not-adi,other-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Clock is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle one byte at a time. IIO hardware buffer will be big-
>      * endian because data is streamed one byte at a time. */
>     spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma";
> 
>     /* Clock properties are extended because spi-offload-0-capabilities contains
>      * "clock-trigger" and SPI controller itself has a clock */
>     clocks = <&cpu_clock 0>, <&extra_clock 0>;
>     clock-names = "sclk", "offload-0-pwm-trigger";
> 
>     /* DMA properties are extended because spi-offload-0-capabilities contains
>      * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */
>     dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>;
>     dma-names = "tx", "rx", "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@3000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 16-bit words. */
>     spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma";
> 
>     /* Do we need a property to say that the sample trigger is connected to
>      * adc@0 so that if adc@1 tries to use it, it will fail? */
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). This only makes sense
>          * when offload is used with BSY signal, otherwise we would have CNV
>          * connected to SPI CS. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> spi@4000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 32-bit words.
>      * This also has the CRC validation that strips off the CRC byte of the
>      * raw data before passing the sample to DMA. */
>     spi-offload-0-capabilities = "sample-trigger",
>                                  "32-bit-rx-dma-with-ad4630-crc-check";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     interrupt-parent = <&intc>;
>     /* First interrupt is for the SPI controller (always present), second
>      * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check"
>      * of offload 0. */
>     interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>;
>     interrupt-names = "controller", "offload-0-crc-error";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad4630";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). Without offload, we would
>          * have cnv-gpios instead. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> So this is what I came up with of how things could look (combining
> suggestions from Rob in v1 and Conor's suggestions here). I can see
> how we can make this work. But the thing I don't like about it is that
> the peripheral drivers still need to know all of the information about
> the offload capabilities and need to interact with the
> pwms/clocks/interrupts/dmas/etc. So this is just adding layers of
> indirection where all of this stuff has to go through the SPI
> controller driver.
> 
> 
> >
> > > Another idea I had was to perhaps use the recently added IIO backend
> > > framework for the "extras". The idea there is that we are creating a
> > > "composite" IIO device that consists of the ADC chip (frontend) plus
> > > these extra hardware trigger and hardware buffer functions provided by
> > > the FPGA (backend).
> > >
> > > offload_backend: adc0-backend {
> > >     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
> > >     compatible = "adi,pulsar-adc-offload";
> > >     #io-backend-cells = <0>;
> > >     dmas = <&dma 0>;
> > >     dma-names = "rx";
> > >     clocks = <&trigger_clock>;
> > > };
> > >
> > > spi {
> > >     ...
> > >     adc@0 {
> > >         ...
> > >         spi-offloads = <0>;
> > >         io-backends = <&offload_backend>;
> > >     };
> > > };
> > >
> > > While this could be a solution for IIO devices, this wouldn't solve
> > > the issue in general though for SPI offloads used with non-IIO
> > > peripherals.
> >
> > Yeah, I agree that using something specific to IIO isn't really a good
> > solution.
> >
> > Cheers,
> > Conor.
> >
> > > So I don't think it is the right thing to do here. But, I
> > > think this idea of a "composite" device helps explain why we are
> > > pushing for putting the "extras" with the peripheral node rather than
> > > the controller node, at least for the specific case of the AXI SPI
> > > Engine controller.
> >

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Nuno Sá]

On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> > > 
> > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > 
> > > > > > Back to "something beyond the SPI controller":
> > > > > > 
> > > > > > Here are some examples of how I envision this would work.
> > > > > > 
> > > > > > Let's suppose we have a SPI controller that has some sort of offload
> > > > > > capability with a configurable trigger source. The trigger can either
> > > > > > be an internal software trigger (i.e. writing a register of the SPI
> > > > > > controller) or and external trigger (i.e. a input signal from a pin on
> > > > > > the SoC). The SPI controller has a lookup table with 8 slots where it
> > > > > > can store a series of SPI commands that can be played back by
> > > > > > asserting the trigger (this is what provides the "offloading").
> > > > > > 
> > > > > > So this SPI controller would have #spi-offload-cells = <2>; where the
> > > > > > first cell would be the index in the lookup table 0 to 7 and the
> > > > > > second cell would be the trigger source 0 for software or 1 for
> > > > > > hardware.
> > > > > > 
> > > > > > Application 1: a network controller
> > > > > > 
> > > > > > This could use two offloads, one for TX and one for RX. For TX, we use
> > > > > > the first slot with a software trigger because the data is coming from
> > > > > > Linux. For RX we use the second slot with a hardware trigger since
> > > > > > data is coming from the network controller (i.e. a data ready signal
> > > > > > that would normally be wired to a gpio for interrupt but wired to the
> > > > > > SPI offload trigger input pin instead). So the peripheral bindings
> > > > > > would be:
> > > > > > 
> > > > > > #define SOFTWARE_TRIGGER 0
> > > > > > #define HARDWARE_TRIGGER 1
> > > > > > 
> > > > > > can@0 {
> > > > > >     ...
> > > > > >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> > > > > >     /* maybe we need names too? */
> > > > > >     spi-offload-names = "tx", "rx";
> > > > > > };
> > > > > > 
> > > > > > In this case, there is nothing extra beyond the SPI controller and the
> > > > > > network controller, so no extra bindings beyond this are needed.
> > > > > > 
> > > > > > Application 2: an advanced ADC + FPGA
> > > > > > 
> > > > > > This is basically the same as the ad7944 case seen already with one
> > > > > > extra feature. In this case, the sample data also contains a CRC byte
> > > > > > for error checking. So instead of SPI RX data going directly to DMA,
> > > > > > the FPGA removes the CRC byte from the data stream an only the sample
> > > > > > data goes to the DMA buffer. The CRC is checked and if bad, an
> > > > > > interrupt is asserted.
> > > > > > 
> > > > > > Since this is an FPGA, most everything is hardwired rather than having
> > > > > > any kind of mux selection so #spi-offload-cells = <1>; for this
> > > > > > controller.
> > > > > > 
> > > > > > By adding spi-offloads to the peripheral node, it also extends the
> > > > > > peripheral binding to include the additional properties needed for the
> > > > > > extra features provided by the FPGA. In other words, we are saying
> > > > > > this DT node now represents the ADC chip plus everything connected to
> > > > > > the offload instance used by the ADC chip.
> > > > > 
> > > > > It seems very strange to me that the dmas and the clock triggers are
> > > > > going into the spi device nodes. The description is
> > > > > > +  dmas:
> > > > > > +    maxItems: 1
> > > > > > +    description: RX DMA Channel for receiving a samples from the SPI
> > > > > > offload.
> > > > > But as far as I can tell this device is in a package of its own and not
> > > > > some IP provided by Analog that an engine on the FPGA can actually do
> > > > > DMA to, and the actual connection of the device is "just" SPI.
> > > > > The dmas and clock triggers etc appear to be resources belonging to the
> > > > > controller that can "assigned" to a particular spi device. If the adc
> > > > > gets disconnected from the system, the dmas and clock triggers are still
> > > > > connected to the spi controller/offload engine, they don't end up n/c,
> > > > > right? (Well maybe they would in the case of a fancy SPI device that
> > > > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > > > responsible for selecting which of the various resources belonging to
> > > > > the controller are to be used by a device.
> > > > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > > > gonna start screaming at me but w/e, looking at it from the point of
> > > > > view of how the hardware is laid out (or at least how it is described
> > > > > in your FPGA case above) the dma/clock properties looks like they're
> > > > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > > > to "the ADC chip plus everything connected to the offload instance
> > > > > used by the ADC chip".
> > > > 
> > > > This is the same reasoning that led me to the binding proposed in v1.
> > > > Rob suggested that these extras (dmas/clocks) should just be
> > > > properties directly of the SPI controller.
> > > 
> > > > But the issue I have with
> > > > that is that since this is an FPGA, these properties are not fixed.
> > > 
> > > I don't think whether or not we're talking about an FPGA or an ASIC
> > > matters at all here to be honest. In my view the main thing that FPGA
> > > impact in terms of bindings is the number of properties required to
> > > represent the configurability of a particular IP. Sure, you're gonna
> > > have to change the dts around in a way you'll never have to with an
> > > ASIC, but the description of individual devices or relations between
> > > them doesn't change.
> > > 
> > > > Maybe there are more clocks or no clocks or interrupts or something we
> > > > didn't think of yet.
> > > 
> > > This could happen with a new generation of an ASIC and is not specific
> > > to an FPGA IP core. Even with FPGA IP, while there may be lots of
> > > different configuration parameters, they are known & limited - you can
> > > look at the IP's documentation (or failing that, the HDL) to figure out
> > > what the points of variability are. It's not particularly difficult to
> > > account for there being a configurable number of clocks or interrupts.
> > > For "something we didn't think of yet" to be a factor, someone has to
> > > think of it and add it to the IP core, and which point we can absolutely
> > > change the bindings and software to account for the revised IP.
> > > 
> > > > So it doesn't really seem possible to write a
> > > > binding for the SPI controller node to cover all of these cases.
> > > 
> > > I dunno, I don't think your concerns about numbers of clocks (or any
> > > other such property) are unique to this particular use-case.
> > > 
> > > > These
> > > > extras are included in the FPGA bitstream only for a specific type of
> > > > peripheral, not for general use of the SPI controller with any type of
> > > > peripheral.
> > > 
> > > I accept that, but what I was trying to get across was that you could
> > > configure the FPGA with a bitstream that contains these extra resources
> > > and then connect a peripheral device that does not make use of them at
> > > all.
> > 
> > Sure, in this case the peripheral has no spi-offloads property and the
> > SPI controller acts as a typical SPI controller.
> > 
> > > Or you could connect a range of different peripherals that do use
> > > them.
> > 
> > OK, you've got me thinking about something I hadn't really thought about yet.
> > 
> > > Whether or not the resources are there and how they are connected
> > > in the FPGA bitstream is not a property of the device connected to the
> > > SPI controller, only whether or not you use them is.
> > > 
> > 
> > Even when those things are connected directly to a specific peripheral
> > device? Or not connected to the offload?
> > 
> > Here is another potential ADC trigger case to consider.
> > 
> > +-------------------------------+   +------------------+
> > >                               |   |                  |
> > >  SOC/FPGA                     |   |  ADC             |
> > >  +---------------------+      |   |                  |
> > >  | AXI SPI Engine      |      |   |                  |
> > >  |             SPI Bus ============ SPI Bus          |
> > >  |                     |      |   |                  |
> > >  |  +---------------+  |  < < < < < BUSY             |
> > >  |  | Offload 0     |  | v    |   |                  |
> > >  |  |               |  | v  > > > > CNV              |
> > >  |  |    TRIGGER IN < < <   ^ |   |                  |
> > >  |  +---------------+  |    ^ |   +------------------+
> > >  +---------------------+    ^ |
> > >  | AXI PWM             |    ^ |
> > >  |                 CH0 >  > ^ |
> > >  +---------------------+      |
> > >                               |
> > +-------------------------------+
> > 
> > This time, the periodic trigger (PWM) is connected to the pin on the
> > ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> > that will go high at the start of the conversion and go low at the end
> > of the conversion. The BUSY output of the ADC is wired as the hardware
> > trigger input of the offload.
> > 
> > In this case would we still consider the PWM as part of the SPI
> > controller/offload since it can only be used in conjunction with the
> > SPI offload? It isn't connected to it at all though.
> 
> No, in this case the ADC is a PWM consumer and the offload engine is
> not. The ADC is a "trigger" provider and the SPI offload engine is a
> trigger consumer.
> 
> > Another case could be a self-clocked ADC. Here, the ADC has it's own
> > periodic sample trigger on the chip and the RDY output goes high
> > whenever a sample is ready to read.
> > 
> > +-------------------------------+   +------------------+
> > >                               |   |                  |
> > >  SOC/FPGA                     |   |  ADC             |
> > >  +---------------------+      |   |                  |
> > >  | AXI SPI Engine      |      |   |                  |
> > >  |             SPI Bus ============ SPI Bus          |
> > >  |                     |      |   |                  |
> > >  |  +---------------+  |  < < < < < RDY              |
> > >  |  | Offload 0     |  | v    |   |                  |
> > >  |  |               |  | v    |   |                  |
> > >  |  |    TRIGGER IN < < <     |   |                  |
> > >  |  +---------------+  |      |   +------------------+
> > >  +---------------------+      |
> > >                               |
> > +-------------------------------+
> > 
> > If both of these are valid wiring configurations for the same ADC,
> > wouldn't it make more sense to describe this in the peripheral node
> > rather than having to query the controller to see how the peripheral
> > is wired up?
> 
> In both of these cases, the offload works in the same way, it gets a
> trigger from the ADC and acts upon it. I would expect that in this case
> the ADC driver would look for an optional pwm property in the devicetree
> and if it is present configure the ADC to use that and if absent do then
> do whatever configuration is required for self clocking. I would expect
> that both cases would be handled identically by the SPI [offload] engine
> side of things, other than inverting the polarity of the trigger. (If I
> understand correctly, you want to trigger the offload engine on a
> falling edge of BUSY but a rising edge of RDY).
> 
> 
> > > In fact, looking at the documentation for the "spi engine" some more, I
> > > am even less convinced that the right place for describing the offload is
> > > the peripheral as I (finally?) noticed that the registers for the offload
> > > engine are mapped directly into the "spi engine"'s memory map, rather than
> > > it being a entirely separate IP in the FPGA fabric.
> > 
> > True, but we don't use these registers, e.g., to configure the
> > sampling frequency of a trigger (if it can even be done). That is done
> > in a completely separate IP block with it's own registers.
> 
> Where is the binding for that IP block? I think describing that is
> pretty key. goto continuation;
> 
> > > Further, what resources are available depends on what the SPI offload
> > > engine IP is. If my employer decides to start shipping some SPI offload
> > > IP in our catalogue that can work with ADI's ADCs, the set of offload
> > > related properties or their names may well differ.
> > 
> > If all of these resources were fairly generic, like the periodic
> > trigger we've been discussing, then I could see the case for trying to
> > accommodate this the same way we do for other common features of SPI
> > controllers. But for cases like "Application 2" that I described
> > previously, these resources can get very specific to a peripheral
> > (like the example given of having a data processing unit that knows
> > about the exact data format and CRC algorithm used by a specific ADC).
> > These seems like too specific of a thing to say that a SPI controller
> > "supports".
> 
> To remind myself, "Application 2" featured an offload engine designed
> specifically to work with a particular data format that would strip a
> CRC byte and check the validity of the data stream.
> 

I think the data manipulation is not really a property of the engine. Typically data
going out of the offload engine goes into another "data reorder" block that is pure
HW.

> I think you're right something like that is a stretch to say that that
> is a feature of the SPI controller - but I still don't believe that
> modelling it as part of the ADC is correct. I don't fully understand the
> io-backends and how they work yet, but the features you describe there
> seem like something that should/could be modelled as one, with its own
> node and compatible etc. Describing custom RTL stuff ain't always
> strightforward, but the stuff from Analog is versioned and documented
> etc so it shouldn't be quite that hard.
> 

Putting this in io-backends is likely a stretch but one thing to add is that the
peripheral is always (I think) kind of the consumer of the resources. Taking the
trigger (PWM) as an example and even when it is directly connected with the offload
block, the peripheral still needs to know about it. Think of sampling frequency...
The period of the trigger signal is strictly connected with the sampling frequency of
the peripheral for example. So I see 2 things:

1) Enabling/Disabling the trigger could be easily done from the peripheral even with
the resource in the spi engine. I think David already has some code in the series
that would make this trivial and so having the property in the spi controller brings
no added complexity.

2) Controlling things like the trigger period/sample_rate. This could be harder to do
over SPI (or making it generic enough) so we would still need to have the same
property on the peripheral (even if not directly connected to it). I kind of agree
with David that having the property both in the peripheral and controller is a bit
weird.

And the DMA block is a complete different story. Sharing that data back with the
peripheral driver (in this case, the IIO subsystem) would be very interesting at the
very least. Note that the DMA block is not really something that is part of the
controller nor the offload block. It is an external block that gets the data coming
out of the offload engine (or the data reorder block). In IIO, we already have a DMA
buffer interface so users of the peripheral can get the data without any intervention
of the driver (on the data). We "just" enable buffering and then everything happens
on HW and userspace can start requesting data. If we are going to attach the DMA in
the controller, I have no idea how we can handle it. Moreover, the offload it's
really just a way of replaying the same spi transfer over and over and that happens
in HW so I'm not sure how we could "integrate" that with dmaengine.

But maybe I'm overlooking things... And thinking more in how this can be done in SW
rather than what makes sense from an HW perspective.


> continuation:
> If offload engines have their own register region in the memory map,


Don't think it has it's own register region... David?

> their own resources (the RTL is gonna need at the very least a clock)
> and potentially also provide other services (like acting as an
> io-backend type device that pre-processes data) it doesn't sound like
> either the controller or peripheral nodes are the right place for these
> properties. And uh, spi-offloads gets a phandle once more...
> 

But then it would be valid for both peripheral and controller to reference that
phandle right (and get the resources of interest)?

FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
to you:

https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl

- Nuno Sá

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Mark Brown]

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

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/22/24 1:24 PM, Conor Dooley wrote:
> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
>>>
>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>

...

>> This time, the periodic trigger (PWM) is connected to the pin on the
>> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
>> that will go high at the start of the conversion and go low at the end
>> of the conversion. The BUSY output of the ADC is wired as the hardware
>> trigger input of the offload.
>>
>> In this case would we still consider the PWM as part of the SPI
>> controller/offload since it can only be used in conjunction with the
>> SPI offload? It isn't connected to it at all though.
> 
> No, in this case the ADC is a PWM consumer and the offload engine is
> not. The ADC is a "trigger" provider and the SPI offload engine is a
> trigger consumer.

Makes sense.

...


> 
>>> In fact, looking at the documentation for the "spi engine" some more, I
>>> am even less convinced that the right place for describing the offload is
>>> the peripheral as I (finally?) noticed that the registers for the offload
>>> engine are mapped directly into the "spi engine"'s memory map, rather than
>>> it being a entirely separate IP in the FPGA fabric.
>>
>> True, but we don't use these registers, e.g., to configure the
>> sampling frequency of a trigger (if it can even be done). That is done
>> in a completely separate IP block with it's own registers.
> 
> Where is the binding for that IP block? I think describing that is
> pretty key. goto continuation;

For the real-world case I used to test this series, it is an AXI PWMGEN
[1] that is providing the trigger event source. It has a typical PWM
provider binding with #pwm-cells [2].

Calling this a "trigger" provider to the SPI offload instance just like the
case above where the ADC is directly connected as the offload trigger makes
sense to me.

What I was going for in this patch (slightly revised to use #cells) is that
this trigger provider, whatever it is, is selected by one of the cells of
#spi-offload-cells. It doesn't seem like there should be a special case for
if the trigger provider is a clock or PWM where the SPI controller node
becomes a consumer of the clock or PWM provider rather than just describing
the trigger relationship.

For example, supposing we had an FPGA/HDL that could handle all 3 wiring
configurations we have discussed so far. A) PWM connected directly to SPI
offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
to SPI offload as trigger. So the DT would have:

controller:
#spi-offload-cells = <2>: /* 1st cell = offload instance
                           * 2nd cell = trigger provider */

peripheral (choose one based on actual wiring):
spi-offloads = <0 0>; /* case A */
spi-offloads = <0 1>; /* case B */
spi-offloads = <0 2>; /* case C */


As to what is the actual consumer of the PWM provider in each of these
cases...

* C is easy. There isn't a PWM provider since the ADC is self-clocked.
* B, as discussed elsewhere is fairly straight forward. The ADC node is
  the consumer since the PWM is connected directly to the ADC.
* A is the one we need to figure out. I'm proposing that the PWM consumer
  should be whatever kind of composite device node we come up with that
  also solves the issue described below about where does the CRC checker
  (or whatever) go. I think we are in agreement here at least on the point
  that it doesn't belong in the SPI controller node?

[1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
[2]: https://lore.kernel.org/linux-pwm/20240424125850.4189116-2-tgamblin@baylibre.com/



> 
> I think you're right something like that is a stretch to say that that
> is a feature of the SPI controller - but I still don't believe that
> modelling it as part of the ADC is correct. I don't fully understand the
> io-backends and how they work yet, but the features you describe there
> seem like something that should/could be modelled as one, with its own
> node and compatible etc. Describing custom RTL stuff ain't always
> strightforward, but the stuff from Analog is versioned and documented
> etc so it shouldn't be quite that hard.
> 
> continuation:
> If offload engines have their own register region in the memory map,
> their own resources (the RTL is gonna need at the very least a clock)
> and potentially also provide other services (like acting as an
> io-backend type device that pre-processes data) it doesn't sound like
> either the controller or peripheral nodes are the right place for these
> properties. And uh, spi-offloads gets a phandle once more...
> 

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > 
> > I think you're right something like that is a stretch to say that that
> > is a feature of the SPI controller - but I still don't believe that
> > modelling it as part of the ADC is correct. I don't fully understand the
> > io-backends and how they work yet, but the features you describe there
> > seem like something that should/could be modelled as one, with its own
> > node and compatible etc. Describing custom RTL stuff ain't always
> > strightforward, but the stuff from Analog is versioned and documented
> > etc so it shouldn't be quite that hard.
> > 
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources. Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:

Cherry picking this one thing to reply to cos I'm not sure if it was
understood as I intended. When I talked about io-backends I was not
suggesting that we drop the spi-offload idea, I was suggesting that if
something has a dedicated register region & resources that handles both
offloading and some usecase specific data processing that it should be a
device of its own, acting as a provider of both spi-offloads and
io-backends.
I'll look at the rest of the mail soonTM.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Nuno Sá]

On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > 
> 

...

> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 

What about things like DMA? I'm mentioning it a lot because it's way more complex
having it on the controller (from a SW perspective). But from an HW point of view,
it's always very similar (if not the same) as your case A.

- Nuno Sá
> 

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/23/24 7:15 AM, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
>>>>
>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>>

...

>> To remind myself, "Application 2" featured an offload engine designed
>> specifically to work with a particular data format that would strip a
>> CRC byte and check the validity of the data stream.
>>
> 
> I think the data manipulation is not really a property of the engine. Typically data
> going out of the offload engine goes into another "data reorder" block that is pure
> HW.
> 
>> I think you're right something like that is a stretch to say that that
>> is a feature of the SPI controller - but I still don't believe that
>> modelling it as part of the ADC is correct. I don't fully understand the
>> io-backends and how they work yet, but the features you describe there
>> seem like something that should/could be modelled as one, with its own
>> node and compatible etc. Describing custom RTL stuff ain't always
>> strightforward, but the stuff from Analog is versioned and documented
>> etc so it shouldn't be quite that hard.
>>
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources. Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:
> 
> 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> the resource in the spi engine. I think David already has some code in the series
> that would make this trivial and so having the property in the spi controller brings
> no added complexity.
> 
> 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> over SPI (or making it generic enough) so we would still need to have the same
> property on the peripheral (even if not directly connected to it). I kind of agree
> with David that having the property both in the peripheral and controller is a bit
> weird.
> 
> And the DMA block is a complete different story. Sharing that data back with the
> peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> very least. Note that the DMA block is not really something that is part of the
> controller nor the offload block. It is an external block that gets the data coming
> out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> buffer interface so users of the peripheral can get the data without any intervention
> of the driver (on the data). We "just" enable buffering and then everything happens
> on HW and userspace can start requesting data. If we are going to attach the DMA in
> the controller, I have no idea how we can handle it. Moreover, the offload it's
> really just a way of replaying the same spi transfer over and over and that happens
> in HW so I'm not sure how we could "integrate" that with dmaengine.
> 
> But maybe I'm overlooking things... And thinking more in how this can be done in SW
> rather than what makes sense from an HW perspective.
> 
> 
>> continuation:
>> If offload engines have their own register region in the memory map,
> 
> 
> Don't think it has it's own register region... David?

I think the question here was if the CRC checker IP block (or descrambler shown
in the link below, or whatever) had registers in the offload/SPI controller
to control that extra part or if they had their own dedicated registers. So far,
these have been fixed-purpose, so have no registers at all. But I could see
needing a register, e.g. for turning it on or off. In this case, I think it
does become something like an io-backend. Or would we add this on/off switch
to the AXI SPI Engine registers?

Also, as shown in the link below, the extra bits share a clock domain with the
AXI SPI Engine. So, yes, technically I suppose they could/should? be independent
consumers of the same clock like Conor suggests below. It does seems kind of
goofy if we have to write a driver just to turn on a clock that is already
guaranteed to be on though.

> 
>> their own resources (the RTL is gonna need at the very least a clock)
>> and potentially also provide other services (like acting as an
>> io-backend type device that pre-processes data) it doesn't sound like
>> either the controller or peripheral nodes are the right place for these
>> properties. And uh, spi-offloads gets a phandle once more...
>>
> 
> But then it would be valid for both peripheral and controller to reference that
> phandle right (and get the resources of interest)?
> 
> FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
> to you:
> 
> https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl
> 
> - Nuno Sá
> 
> 

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/23/24 9:57 AM, Nuno Sá wrote:
> On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
>> On 5/22/24 1:24 PM, Conor Dooley wrote:
>>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
>>>>>
>>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
>>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>>>
>>
> 
> ...
> 
>>
>> controller:
>> #spi-offload-cells = <2>: /* 1st cell = offload instance
>>                            * 2nd cell = trigger provider */
>>
> 
> What about things like DMA? I'm mentioning it a lot because it's way more complex
> having it on the controller (from a SW perspective). But from an HW point of view,
> it's always very similar (if not the same) as your case A.
> 

If we had a setup where there was more than one place that, e.g. the
RX stream from the offload could be piped, then I would add a 3rd
cell to describe that. If the hardware is fixed and the RX stream
always goes to a specific DMA channel, then it doesn't seem like we
need to describe that in the SPI controller node because the hardware
is fixed.

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Nuno Sá]

On Thu, 2024-05-23 at 10:09 -0500, David Lechner wrote:
> On 5/23/24 9:57 AM, Nuno Sá wrote:
> > On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
> > > On 5/22/24 1:24 PM, Conor Dooley wrote:
> > > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> > > > > > 
> > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > > > 
> > > 
> > 
> > ...
> > 
> > > 
> > > controller:
> > > #spi-offload-cells = <2>: /* 1st cell = offload instance
> > >                            * 2nd cell = trigger provider */
> > > 
> > 
> > What about things like DMA? I'm mentioning it a lot because it's way more complex
> > having it on the controller (from a SW perspective). But from an HW point of
> > view,
> > it's always very similar (if not the same) as your case A.
> > 
> 
> If we had a setup where there was more than one place that, e.g. the
> RX stream from the offload could be piped, then I would add a 3rd
> cell to describe that. If the hardware is fixed and the RX stream
> always goes to a specific DMA channel, then it doesn't seem like we
> need to describe that in the SPI controller node because the hardware
> is fixed.
> 

Well, yes, still the DMA channel is connected on the controller and not the
peripheral. Same deal as we discussed on the IIO backends stuff. But there, since
it's all IIO it was easy to make the DMA a property of the backend device. That said,
I can surely see having the property in the peripheral.

Another thing that came to mind for the trigger case. What about an additional spi
interface for configuring/setting the trigger rate? Sounds generic and then we would
not really need the trigger on the peripheral right (did not checked the CRC issue
you mentioned so not sure if it's somehow trigger related)?

Hmm but sometimes there's other things than rate/period (like offset) to care about
so maybe not doable... Bah!

- Nuno Sá

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Thu, May 23, 2024 at 10:05:49AM -0500, David Lechner wrote:
> On 5/23/24 7:15 AM, Nuno Sá wrote:
> > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> >> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> >>>>
> >>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> >>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> >>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >>>>
> 
> ...
> 
> >> To remind myself, "Application 2" featured an offload engine designed
> >> specifically to work with a particular data format that would strip a
> >> CRC byte and check the validity of the data stream.
> >>
> > 
> > I think the data manipulation is not really a property of the engine. Typically data
> > going out of the offload engine goes into another "data reorder" block that is pure
> > HW.
> > 
> >> I think you're right something like that is a stretch to say that that
> >> is a feature of the SPI controller - but I still don't believe that
> >> modelling it as part of the ADC is correct. I don't fully understand the
> >> io-backends and how they work yet, but the features you describe there
> >> seem like something that should/could be modelled as one, with its own
> >> node and compatible etc. Describing custom RTL stuff ain't always
> >> strightforward, but the stuff from Analog is versioned and documented
> >> etc so it shouldn't be quite that hard.
> >>
> > 
> > Putting this in io-backends is likely a stretch but one thing to add is that the
> > peripheral is always (I think) kind of the consumer of the resources. Taking the
> > trigger (PWM) as an example and even when it is directly connected with the offload
> > block, the peripheral still needs to know about it. Think of sampling frequency...
> > The period of the trigger signal is strictly connected with the sampling frequency of
> > the peripheral for example. So I see 2 things:
> > 
> > 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> > the resource in the spi engine. I think David already has some code in the series
> > that would make this trivial and so having the property in the spi controller brings
> > no added complexity.
> > 
> > 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> > over SPI (or making it generic enough) so we would still need to have the same
> > property on the peripheral (even if not directly connected to it). I kind of agree
> > with David that having the property both in the peripheral and controller is a bit
> > weird.
> > 
> > And the DMA block is a complete different story. Sharing that data back with the
> > peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> > very least. Note that the DMA block is not really something that is part of the
> > controller nor the offload block. It is an external block that gets the data coming
> > out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> > buffer interface so users of the peripheral can get the data without any intervention
> > of the driver (on the data). We "just" enable buffering and then everything happens
> > on HW and userspace can start requesting data. If we are going to attach the DMA in
> > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > really just a way of replaying the same spi transfer over and over and that happens
> > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > 
> > But maybe I'm overlooking things... And thinking more in how this can be done in SW
> > rather than what makes sense from an HW perspective.
> > 
> > 
> >> continuation:
> >> If offload engines have their own register region in the memory map,
> > 
> > 
> > Don't think it has it's own register region... David?
> 
> I think the question here was if the CRC checker IP block (or descrambler shown
> in the link below, or whatever) had registers in the offload/SPI controller
> to control that extra part or if they had their own dedicated registers.

I don't think there was a question here at all. I was simply stating
that if the offload engine was not just a subordinate feature of the SPI
controller, but also provided additional data processing features then
treating the offload engine as a component of the SPI controller would
not be accurate.

> So far,
> these have been fixed-purpose, so have no registers at all. But I could see
> needing a register, e.g. for turning it on or off. In this case, I think it
> does become something like an io-backend. Or would we add this on/off switch
> to the AXI SPI Engine registers?

Seems to be that the CRC checking is a separate piece of IP though, and
so is not part of the offload engine at all, so my concern was
misplaced. I think whether or not you have registers to control it, it
should be represented in DT. How do you know it is there otherwise?

> Also, as shown in the link below, the extra bits share a clock domain with the
> AXI SPI Engine. So, yes, technically I suppose they could/should? be independent
> consumers of the same clock like Conor suggests below. It does seems kind of
> goofy if we have to write a driver just to turn on a clock that is already
> guaranteed to be on though.

You wouldn't necessarily need to write a driver for it, you could reach
out and turn it on from the backend consumer for example. And, obviously
there may be other ways that the FPGA design is configured where the
clock is not shared with the SPI controller or the offload engine.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
> >>>
> >>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> >>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
> >>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >>>
> 
> ...
> 
> >> This time, the periodic trigger (PWM) is connected to the pin on the
> >> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> >> that will go high at the start of the conversion and go low at the end
> >> of the conversion. The BUSY output of the ADC is wired as the hardware
> >> trigger input of the offload.
> >>
> >> In this case would we still consider the PWM as part of the SPI
> >> controller/offload since it can only be used in conjunction with the
> >> SPI offload? It isn't connected to it at all though.
> > 
> > No, in this case the ADC is a PWM consumer and the offload engine is
> > not. The ADC is a "trigger" provider and the SPI offload engine is a
> > trigger consumer.
> 
> Makes sense.
> 
> ...
> 
> 
> > 
> >>> In fact, looking at the documentation for the "spi engine" some more, I
> >>> am even less convinced that the right place for describing the offload is
> >>> the peripheral as I (finally?) noticed that the registers for the offload
> >>> engine are mapped directly into the "spi engine"'s memory map, rather than
> >>> it being a entirely separate IP in the FPGA fabric.
> >>
> >> True, but we don't use these registers, e.g., to configure the
> >> sampling frequency of a trigger (if it can even be done). That is done
> >> in a completely separate IP block with it's own registers.
> > 
> > Where is the binding for that IP block? I think describing that is
> > pretty key. goto continuation;
> 
> For the real-world case I used to test this series, it is an AXI PWMGEN
> [1] that is providing the trigger event source. It has a typical PWM
> provider binding with #pwm-cells [2].
> 
> Calling this a "trigger" provider to the SPI offload instance just like the
> case above where the ADC is directly connected as the offload trigger makes
> sense to me.
> 
> What I was going for in this patch (slightly revised to use #cells) is that
> this trigger provider, whatever it is, is selected by one of the cells of
> #spi-offload-cells. It doesn't seem like there should be a special case for
> if the trigger provider is a clock or PWM where the SPI controller node
> becomes a consumer of the clock or PWM provider rather than just describing
> the trigger relationship.
> 
> For example, supposing we had an FPGA/HDL that could handle all 3 wiring
> configurations we have discussed so far. A) PWM connected directly to SPI
> offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
> to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
> to SPI offload as trigger. So the DT would have:
> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 
> peripheral (choose one based on actual wiring):
> spi-offloads = <0 0>; /* case A */
> spi-offloads = <0 1>; /* case B */
> spi-offloads = <0 2>; /* case C */
> 
> 
> As to what is the actual consumer of the PWM provider in each of these
> cases...
> 
> * C is easy. There isn't a PWM provider since the ADC is self-clocked.
> * B, as discussed elsewhere is fairly straight forward. The ADC node is
>   the consumer since the PWM is connected directly to the ADC.
> * A is the one we need to figure out. I'm proposing that the PWM consumer
>   should be whatever kind of composite device node we come up with that
>   also solves the issue described below about where does the CRC checker
>   (or whatever) go. I think we are in agreement here at least on the point
>   that it doesn't belong in the SPI controller node?

To be clear, you're saying that we agree that the CRC checker doesnt
belong in the SPI controller node, right?

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:

> > 
> > To remind myself, "Application 2" featured an offload engine designed
> > specifically to work with a particular data format that would strip a
> > CRC byte and check the validity of the data stream.
> > 
> 
> I think the data manipulation is not really a property of the engine. Typically data
> going out of the offload engine goes into another "data reorder" block that is pure
> HW.
> 
> > I think you're right something like that is a stretch to say that that
> > is a feature of the SPI controller - but I still don't believe that
> > modelling it as part of the ADC is correct. I don't fully understand the
> > io-backends and how they work yet, but the features you describe there
> > seem like something that should/could be modelled as one, with its own
> > node and compatible etc. Describing custom RTL stuff ain't always
> > strightforward, but the stuff from Analog is versioned and documented
> > etc so it shouldn't be quite that hard.
> > 
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources.

Could you explain you think why making some additional processing done to
the data an io-backend is a stretch? Where else can this RTL be
represented? hint: it's not part of the ADC, just like how if you have
some custom RTL that does video processing that is not part of the
camera!

> Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:
> 
> 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> the resource in the spi engine. I think David already has some code in the series
> that would make this trivial and so having the property in the spi controller brings
> no added complexity.
> 
> 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> over SPI (or making it generic enough) so we would still need to have the same
> property on the peripheral (even if not directly connected to it). I kind of agree
> with David that having the property both in the peripheral and controller is a bit
> weird.

Can you explain what you mean by "same property on the peripheral"? I
would expect a peripheral to state its trigger period (just like how it
states the max frequency) and for the trigger period not to appear in
the controller.

I think a way that this could be modelled to reduce some software
complexity is considering that the periodic trigger is a clock, not
a PWM, provided you are only interested in the period. That'd give you
an interface that was less concerned about what the provider of the
periodic trigger is. After all, I doubt the ADC cares how you decide to
generate the trigger, as long as the periodicity is correct.
With the examples provided, you'd get something like:

pwm {
}

pclk {
	compatible = pwm-clock
	pwms = <&pwm 0 x>
}

spi {
	compatible = spi-engine
	clocks = <&clks SPI>, <&pwm>
	clock-names = "bus", "offload"
}

The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but
maybe you don't need that or maybe it could be added if needed.

> And the DMA block is a complete different story. Sharing that data back with the
> peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> very least. Note that the DMA block is not really something that is part of the
> controller nor the offload block. It is an external block that gets the data coming
> out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> buffer interface so users of the peripheral can get the data without any intervention
> of the driver (on the data). We "just" enable buffering and then everything happens
> on HW and userspace can start requesting data. If we are going to attach the DMA in
> the controller, I have no idea how we can handle it. Moreover, the offload it's
> really just a way of replaying the same spi transfer over and over and that happens
> in HW so I'm not sure how we could "integrate" that with dmaengine.
> 
> But maybe I'm overlooking things... And thinking more in how this can be done in SW
> rather than what makes sense from an HW perspective.

I don't think you're overlooking things at all, I'm intentionally being
a bit difficult and ignoring what may be convenient for the adc driver.
This is being presented as a solution to a generic problem (and I think
you're right to do that), but it feels as if the one implementation is
all that's being considered here and I'm trying to ensure that what we
end up with doesn't make limiting assumptions.

Part of me says "sure, hook the DMAs up to the devices, as that's what
happens for other IIO devices" but at the same time I recognise that the
DMA isn't actually hooked up like that and the other IIO devices I see
like that are all actually on the SoC, rather than connected over SPI.
It might be easy to do it this way right now, but be problematic for a
future device or if someone wants to chuck away the ADI provided RTL and
do their own thing for this device. Really it just makes me wonder if
what's needed to describe more complex data pipelines uses an of_graph,
just like how video pipelines are handled, rather than the implementation
of io-backends that don't really seem to model the flow of data.

> > continuation:
> > If offload engines have their own register region in the memory map,
> 
> 
> Don't think it has it's own register region... David?
> 
> > their own resources (the RTL is gonna need at the very least a clock)
> > and potentially also provide other services (like acting as an
> > io-backend type device that pre-processes data) it doesn't sound like
> > either the controller or peripheral nodes are the right place for these
> > properties. And uh, spi-offloads gets a phandle once more...
> > 
> 
> But then it would be valid for both peripheral and controller to reference that
> phandle right (and get the resources of interest)?

Sure. But to assume that, for example, the provider of data processing
services also provided the periodic polling trigger would be incorrect
even if in the only currently existing case it does. Nothing is stopping
someone splitting those into different blocks, after all this is "random"
RTL in an FPGA :)

> FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
> to you:
> 
> https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl

Yeah, they're a good reference, thanks.

I had to go AFK in the middle of this, I have a nagging feeling that I
forgot to say something but I cannot recall what.

Cheers,
Conor.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Nuno Sá]

On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> 
> > > 
> > > To remind myself, "Application 2" featured an offload engine designed
> > > specifically to work with a particular data format that would strip a
> > > CRC byte and check the validity of the data stream.
> > > 
> > 
> > I think the data manipulation is not really a property of the engine. Typically
> > data
> > going out of the offload engine goes into another "data reorder" block that is
> > pure
> > HW.
> > 
> > > I think you're right something like that is a stretch to say that that
> > > is a feature of the SPI controller - but I still don't believe that
> > > modelling it as part of the ADC is correct. I don't fully understand the
> > > io-backends and how they work yet, but the features you describe there
> > > seem like something that should/could be modelled as one, with its own
> > > node and compatible etc. Describing custom RTL stuff ain't always
> > > strightforward, but the stuff from Analog is versioned and documented
> > > etc so it shouldn't be quite that hard.
> > > 
> > 
> > Putting this in io-backends is likely a stretch but one thing to add is that the
> > peripheral is always (I think) kind of the consumer of the resources.
> 
> Could you explain you think why making some additional processing done to
> the data an io-backend is a stretch? Where else can this RTL be
> represented? hint: it's not part of the ADC, just like how if you have
> some custom RTL that does video processing that is not part of the
> camera!

Maybe we are speaking about two different things... I do agree with the video
processing example you gave but for this case I'm not sure there#s any data
manipulation involved. i mean, there is but nothing controlled by SW at this point.
Or maybe there's already a future usecase that I'm not aware about (maybe the CRC
stuff David mentioned).

I'm more focusing on where the trigger (PWMS in this particular case but could be
something else) and the DMA properties belong. I do agree that, Hardware wise, the
trigger is a property of the offload engine even though intrinsically connected with
the peripheral.

The DMA is also directly connected to the offload but I'm not sure if in this case we
should say it's a property of it? It's an external block that we do not control at
all and the data is to be consumed by users of the peripheral.

> 
> > Taking the
> > trigger (PWM) as an example and even when it is directly connected with the
> > offload
> > block, the peripheral still needs to know about it. Think of sampling
> > frequency...
> > The period of the trigger signal is strictly connected with the sampling
> > frequency of
> > the peripheral for example. So I see 2 things:
> > 
> > 1) Enabling/Disabling the trigger could be easily done from the peripheral even
> > with
> > the resource in the spi engine. I think David already has some code in the series
> > that would make this trivial and so having the property in the spi controller
> > brings
> > no added complexity.
> > 
> > 2) Controlling things like the trigger period/sample_rate. This could be harder
> > to do
> > over SPI (or making it generic enough) so we would still need to have the same
> > property on the peripheral (even if not directly connected to it). I kind of
> > agree
> > with David that having the property both in the peripheral and controller is a
> > bit
> > weird.
> 
> Can you explain what you mean by "same property on the peripheral"? I
> would expect a peripheral to state its trigger period (just like how it
> states the max frequency) and for the trigger period not to appear in
> the controller.
> 

Just have the same 'pwms' property on both the controller and peripheral...

> I think a way that this could be modelled to reduce some software
> complexity is considering that the periodic trigger is a clock, not
> a PWM, provided you are only interested in the period. That'd give you
> an interface that was less concerned about what the provider of the
> periodic trigger is. After all, I doubt the ADC cares how you decide to
> generate the trigger, as long as the periodicity is correct.
> With the examples provided, you'd get something like:
> 

Unfortunately that's not the case. For instance, in the design on the link I gave you
on the last reply we do have an averaging mode where we actually need an offset
(effort for supporting that in PWM is already ongoing) between the offload trigger
and the peripheral conversion signal (so assuming we only care about period will fail
pretty soon :)).

> pwm {
> }
> 
> pclk {
> 	compatible = pwm-clock
> 	pwms = <&pwm 0 x>
> }
> 
> spi {
> 	compatible = spi-engine
> 	clocks = <&clks SPI>, <&pwm>
> 	clock-names = "bus", "offload"
> }
> 
> The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but
> maybe you don't need that or maybe it could be added if needed.
> 
> > And the DMA block is a complete different story. Sharing that data back with the
> > peripheral driver (in this case, the IIO subsystem) would be very interesting at
> > the
> > very least. Note that the DMA block is not really something that is part of the
> > controller nor the offload block. It is an external block that gets the data
> > coming
> > out of the offload engine (or the data reorder block). In IIO, we already have a
> > DMA
> > buffer interface so users of the peripheral can get the data without any
> > intervention
> > of the driver (on the data). We "just" enable buffering and then everything
> > happens
> > on HW and userspace can start requesting data. If we are going to attach the DMA
> > in
> > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > really just a way of replaying the same spi transfer over and over and that
> > happens
> > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > 
> > But maybe I'm overlooking things... And thinking more in how this can be done in
> > SW
> > rather than what makes sense from an HW perspective.
> 
> I don't think you're overlooking things at all, I'm intentionally being
> a bit difficult and ignoring what may be convenient for the adc driver.
> This is being presented as a solution to a generic problem (and I think
> you're right to do that), but it feels as if the one implementation is
> all that's being considered here and I'm trying to ensure that what we
> end up with doesn't make limiting assumptions.

Yeah, I know and I do agree we need something generic enough and not something that
only fits a couple usecases.

> 
> Part of me says "sure, hook the DMAs up to the devices, as that's what
> happens for other IIO devices" but at the same time I recognise that the
> DMA isn't actually hooked up like that and the other IIO devices I see
> like that are all actually on the SoC, rather than connected over SPI.

Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
solely for carrying the peripheral data. It is done like this so everything works in
HW and there's no need for SW to deal with the samples at all. I mean, only the
userspace app touches the samples.

TBH, the DMA is the bit that worries me the most as it may be overly complex to share
buffers (using dma-buf or something else) from the spi controller back to consumers
of it (IIO in this case). And I mean sharing in a way that there's no need to touch
the buffers.

> It might be easy to do it this way right now, but be problematic for a
> future device or if someone wants to chuck away the ADI provided RTL and
> do their own thing for this device. Really it just makes me wonder if
> what's needed to describe more complex data pipelines uses an of_graph,
> just like how video pipelines are handled, rather than the implementation
> of io-backends that don't really seem to model the flow of data.
> 

Yeah, backends is more for devices/soft-cores that extend the functionality of the
device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
speed controllers. Note that in some cases they also manipulate or even create data
but since they fit in IIO, having things like the DMA property in the hdl binding was
fairly straight.

Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
be acceptable. Then we could add support to "import" it in the IIO core. Then it
would be up to the controller to accept or not to share the handle (in some cases the
controller could really want to have the control of the DMA transfers).

Not familiar enough with of_graph so can't argue about it but likely is something
worth looking at.

- Nuno Sá
> > 

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > 
> > > > 
> > > > To remind myself, "Application 2" featured an offload engine designed
> > > > specifically to work with a particular data format that would strip a
> > > > CRC byte and check the validity of the data stream.
> > > > 
> > > 
> > > I think the data manipulation is not really a property of the engine. Typically
> > > data
> > > going out of the offload engine goes into another "data reorder" block that is
> > > pure
> > > HW.
> > > 
> > > > I think you're right something like that is a stretch to say that that
> > > > is a feature of the SPI controller - but I still don't believe that
> > > > modelling it as part of the ADC is correct. I don't fully understand the
> > > > io-backends and how they work yet, but the features you describe there
> > > > seem like something that should/could be modelled as one, with its own
> > > > node and compatible etc. Describing custom RTL stuff ain't always
> > > > strightforward, but the stuff from Analog is versioned and documented
> > > > etc so it shouldn't be quite that hard.
> > > > 
> > > 
> > > Putting this in io-backends is likely a stretch but one thing to add is that the
> > > peripheral is always (I think) kind of the consumer of the resources.
> > 
> > Could you explain you think why making some additional processing done to
> > the data an io-backend is a stretch? Where else can this RTL be
> > represented? hint: it's not part of the ADC, just like how if you have
> > some custom RTL that does video processing that is not part of the
> > camera!
> 
> Maybe we are speaking about two different things... I do agree with the video
> processing example you gave but for this case I'm not sure there#s any data
> manipulation involved. i mean, there is but nothing controlled by SW at this point.
> Or maybe there's already a future usecase that I'm not aware about (maybe the CRC
> stuff David mentioned).

Yes, this was about the CRC or other additional processing - the quoted
text should really make this clear.

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/26/24 10:45 AM, Conor Dooley wrote:
> On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:


>> * A is the one we need to figure out. I'm proposing that the PWM consumer
>>   should be whatever kind of composite device node we come up with that
>>   also solves the issue described below about where does the CRC checker
>>   (or whatever) go. I think we are in agreement here at least on the point
>>   that it doesn't belong in the SPI controller node?
> 
> To be clear, you're saying that we agree that the CRC checker doesnt
> belong in the SPI controller node, right?

Yes. 

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Wed, May 29, 2024 at 03:10:54PM -0500, David Lechner wrote:
> On 5/26/24 10:45 AM, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> 
> 
> >> * A is the one we need to figure out. I'm proposing that the PWM consumer
> >>   should be whatever kind of composite device node we come up with that
> >>   also solves the issue described below about where does the CRC checker
> >>   (or whatever) go. I think we are in agreement here at least on the point
> >>   that it doesn't belong in the SPI controller node?
> > 
> > To be clear, you're saying that we agree that the CRC checker doesnt
> > belong in the SPI controller node, right?
> 
> Yes. 

Okay, ye. We're on the same page then about that part.


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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:

> >>> In fact, looking at the documentation for the "spi engine" some more, I
> >>> am even less convinced that the right place for describing the offload is
> >>> the peripheral as I (finally?) noticed that the registers for the offload
> >>> engine are mapped directly into the "spi engine"'s memory map, rather than
> >>> it being a entirely separate IP in the FPGA fabric.
> >>
> >> True, but we don't use these registers, e.g., to configure the
> >> sampling frequency of a trigger (if it can even be done). That is done
> >> in a completely separate IP block with it's own registers.
> > 
> > Where is the binding for that IP block? I think describing that is
> > pretty key. goto continuation;
> 
> For the real-world case I used to test this series, it is an AXI PWMGEN
> [1] that is providing the trigger event source. It has a typical PWM
> provider binding with #pwm-cells [2].
> 
> Calling this a "trigger" provider to the SPI offload instance just like the
> case above where the ADC is directly connected as the offload trigger makes
> sense to me.
> 
> What I was going for in this patch (slightly revised to use #cells) is that
> this trigger provider, whatever it is, is selected by one of the cells of
> #spi-offload-cells. It doesn't seem like there should be a special case for
> if the trigger provider is a clock or PWM where the SPI controller node
> becomes a consumer of the clock or PWM provider rather than just describing
> the trigger relationship.
> 
> For example, supposing we had an FPGA/HDL that could handle all 3 wiring
> configurations we have discussed so far. A) PWM connected directly to SPI
> offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
> to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
> to SPI offload as trigger. So the DT would have:
> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 
> peripheral (choose one based on actual wiring):
> spi-offloads = <0 0>; /* case A */
> spi-offloads = <0 1>; /* case B */
> spi-offloads = <0 2>; /* case C */
> 
> 
> As to what is the actual consumer of the PWM provider in each of these
> cases...
> 
> * C is easy. There isn't a PWM provider since the ADC is self-clocked.
> * B, as discussed elsewhere is fairly straight forward. The ADC node is
>   the consumer since the PWM is connected directly to the ADC.
> * A is the one we need to figure out. I'm proposing that the PWM consumer
>   should be whatever kind of composite device node we come up with that
>   also solves the issue described below about where does the CRC checker
>   (or whatever) go. I think we are in agreement here at least on the point
>   that it doesn't belong in the SPI controller node?

I think C and B are correct here. The term "composite device" scares me,
but yeah the PWM gets connected to the device that encompasses the
offload engine. In case B and C, the trigger signals are connected to
the offload engine - what property are we using to describe that?
If this were not a "passive" process and the OS was actually involved
in kicking off an action when the ADC asserted the !BSY signal,
interrupts would make sense, but I'm not sure here.

Maybe interrupts is the right thing to do, because that would also make
sense in the case that we used the busy signal without an offload and
plumbed it into a regular interrupt controller?

gpio0: gpio@20120000 {
	compatible = "microchip,mpfs-gpio";
	reg = <0x0 0x20120000 0x0 0x1000>;
	interrupt-parent = <&plic>;
	interrupt-controller;
	#interrupt-cells = <1>;
	clocks = <&clkcfg CLK_GPIO0>;
	gpio-controller;
	#gpio-cells = <2>;
};

adc {
	interrupt-parent = <&gpio0>;
	interrupts = <0 HIGH>;
};

I suppose then the offload needs to be an interrupt provider too, even
if no irqchip is ever actually created for it. I dunno how tf that's
gonna work out on the software side though? If you have spi-offloads you
interpret the interrupts property differently (IOW you ignore it)?

Or were you thinking of not really hooking that up at all, but instead
just saying checking for whatever x is in spi-offloads = <0 x> and
defining 2 to mean "active low" and 1 to mean "active high" etc?

Once again, had to take a break to make food in the middle of responding
here, so I hope I didn't write something incomprehensible.

Thanks,
Conor.

> 
> [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
> [2]: https://lore.kernel.org/linux-pwm/20240424125850.4189116-2-tgamblin@baylibre.com/


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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Conor Dooley]

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

On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:

> > > Taking the
> > > trigger (PWM) as an example and even when it is directly connected with the
> > > offload
> > > block, the peripheral still needs to know about it. Think of sampling
> > > frequency...
> > > The period of the trigger signal is strictly connected with the sampling
> > > frequency of
> > > the peripheral for example. So I see 2 things:
> > > 
> > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even
> > > with
> > > the resource in the spi engine. I think David already has some code in the series
> > > that would make this trivial and so having the property in the spi controller
> > > brings
> > > no added complexity.
> > > 
> > > 2) Controlling things like the trigger period/sample_rate. This could be harder
> > > to do
> > > over SPI (or making it generic enough) so we would still need to have the same
> > > property on the peripheral (even if not directly connected to it). I kind of
> > > agree
> > > with David that having the property both in the peripheral and controller is a
> > > bit
> > > weird.
> > 
> > Can you explain what you mean by "same property on the peripheral"? I
> > would expect a peripheral to state its trigger period (just like how it
> > states the max frequency) and for the trigger period not to appear in
> > the controller.
> > 
> 
> Just have the same 'pwms' property on both the controller and peripheral...

Yeah, no... Opinion unchanged since my last message.

> > I think a way that this could be modelled to reduce some software
> > complexity is considering that the periodic trigger is a clock, not
> > a PWM, provided you are only interested in the period. That'd give you
> > an interface that was less concerned about what the provider of the
> > periodic trigger is. After all, I doubt the ADC cares how you decide to
> > generate the trigger, as long as the periodicity is correct.
> > With the examples provided, you'd get something like:
> > 
> 
> Unfortunately that's not the case.

Ahh, that sucks. Oh well.

> For instance, in the design on the link I gave you
> on the last reply we do have an averaging mode where we actually need an offset
> (effort for supporting that in PWM is already ongoing) between the offload trigger
> and the peripheral conversion signal (so assuming we only care about period will fail
> pretty soon :)).

> > > And the DMA block is a complete different story. Sharing that data back with the
> > > peripheral driver (in this case, the IIO subsystem) would be very interesting at
> > > the
> > > very least. Note that the DMA block is not really something that is part of the
> > > controller nor the offload block. It is an external block that gets the data
> > > coming
> > > out of the offload engine (or the data reorder block). In IIO, we already have a
> > > DMA
> > > buffer interface so users of the peripheral can get the data without any
> > > intervention
> > > of the driver (on the data). We "just" enable buffering and then everything
> > > happens
> > > on HW and userspace can start requesting data. If we are going to attach the DMA
> > > in
> > > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > > really just a way of replaying the same spi transfer over and over and that
> > > happens
> > > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > > 
> > > But maybe I'm overlooking things... And thinking more in how this can be done in
> > > SW
> > > rather than what makes sense from an HW perspective.
> > 
> > I don't think you're overlooking things at all, I'm intentionally being
> > a bit difficult and ignoring what may be convenient for the adc driver.
> > This is being presented as a solution to a generic problem (and I think
> > you're right to do that), but it feels as if the one implementation is
> > all that's being considered here and I'm trying to ensure that what we
> > end up with doesn't make limiting assumptions.
> 
> Yeah, I know and I do agree we need something generic enough and not something that
> only fits a couple usecases.

If only we had another user... I suppose you lads are the market leader
in these kinds of devices. If I did happen to know if Microchip was
working on anything similar (which I don't, I work on FPGAs not these
kinds of devices) I couldn't even tell you. I suppose I could ask around
and see. Do you know if TI is doing anything along these lines?

> > Part of me says "sure, hook the DMAs up to the devices, as that's what
> > happens for other IIO devices" but at the same time I recognise that the
> > DMA isn't actually hooked up like that and the other IIO devices I see
> > like that are all actually on the SoC, rather than connected over SPI.
> 
> Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
> solely for carrying the peripheral data. It is done like this so everything works in
> HW and there's no need for SW to deal with the samples at all. I mean, only the
> userspace app touches the samples.
> 
> TBH, the DMA is the bit that worries me the most as it may be overly complex to share
> buffers (using dma-buf or something else) from the spi controller back to consumers
> of it (IIO in this case). And I mean sharing in a way that there's no need to touch
> the buffers.

<snip>

> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> be acceptable. Then we could add support to "import" it in the IIO core. Then it
> would be up to the controller to accept or not to share the handle (in some cases the
> controller could really want to have the control of the DMA transfers).

Yeah, that is about what I was thinking. I wasn't expecting the spi code
to grow handing for dmabuf or anything like that, just a way for the
offload consumer to say "yo, can you tell me what dma buffer I can
use?". Unless (until?) there's some controller that wants to manage it,
I think that'd be sufficient?

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

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/29/24 3:07 AM, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:


>> It might be easy to do it this way right now, but be problematic for a
>> future device or if someone wants to chuck away the ADI provided RTL and
>> do their own thing for this device. Really it just makes me wonder if
>> what's needed to describe more complex data pipelines uses an of_graph,
>> just like how video pipelines are handled, rather than the implementation
>> of io-backends that don't really seem to model the flow of data.
>>
> 
> Yeah, backends is more for devices/soft-cores that extend the functionality of the
> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
> speed controllers. Note that in some cases they also manipulate or even create data
> but since they fit in IIO, having things like the DMA property in the hdl binding was
> fairly straight.
> 
> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> be acceptable. Then we could add support to "import" it in the IIO core. Then it
> would be up to the controller to accept or not to share the handle (in some cases the
> controller could really want to have the control of the DMA transfers).

I could see this working for some SPI controllers, but for the AXI SPI Engine
+ DMA currently, the DMA has a fixed word size, so can't be used as a generic
DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
word size, then even if we are reading 16-bit sample data, the DMA is going to
put it in a 32-bit slot. So one could argue that this is still doing some data
manipulation similar to the CRC checker example.

> 
> Not familiar enough with of_graph so can't argue about it but likely is something
> worth looking at.
> 
> - Nuno Sá
>>>

I did try implementing something using graph bindings when I first started
working on this, but it didn't seem to really give us any extra useful
information. It was just describing connections (endpoints) that I thought
we could just implicitly assume. After this discussion though, maybe worth
a second look. I'll have to think about it more.

Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner]

On 5/30/24 2:18 PM, Conor Dooley wrote:

> 
> If only we had another user... I suppose you lads are the market leader
> in these kinds of devices. If I did happen to know if Microchip was
> working on anything similar (which I don't, I work on FPGAs not these
> kinds of devices) I couldn't even tell you. I suppose I could ask around
> and see. Do you know if TI is doing anything along these lines?
> 
I think the most popular use case for the performance improvements made
possible with a SPI offload unrelated to ADCs/DACs is for CAN controllers
(e.g. the discussion from David Jander a few years ago).

I think one of my colleagues was working on one in the last year that we
might still have lying around. But I don't know what we could use as the
SPI controller that would have offload support. 

I suppose we could make something using e.g. the PRU in a BeagleBone, but
that would take lots of engineering resources and we could design it to fit
whatever interface we want, so I'm not sure that really helps much. If we
could find an off-the-shelf SPI controller with offload capabilities that
would be helpful, but I don't know of any.