Re: [PATCH v6 05/17] spi: add offload TX/RX streaming APIs

[Nuno Sá]

On Wed, 2024-12-11 at 14:54 -0600, David Lechner wrote:
> Most configuration of SPI offloads is handled opaquely using the offload
> pointer that is passed to the various offload functions. However, there
> are some offload features that need to be controlled on a per transfer
> basis.
> 
> This patch adds a flag field to struct spi_transfer 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 offload struct for providers to
> indicate which flags are supported. This allows for generic checking of
> offload capabilities during __spi_validate() so that each offload
> provider doesn't have to implement their own validation.
> 
> As a first users of this streaming capability, getter functions are
> added to get a DMA channel that is directly connected to the offload.
> Peripheral drivers will use this to get a DMA channel and configure it
> to suit their needs.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---


Still not sure about releasing the channel. But I guess this might be also a
problem today with "plain" IIO DMA buffering. So...

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

> v6 changes:
> * Update for header file split.
> * Fix wrong kernel-doc comments.
> 
> v5 change:
> * Remove incorrect comment about caller needing to release DMA channels.
> 
> v4 changes:
> * DMA API's now automatically release DMA channels instead of leaving
>   it up to the caller.
> 
> v3 changes:
> * Added spi_offload_{tx,rx}_stream_get_dma_chan() functions.
> 
> v2 changes:
> * This is also split out from "spi: add core support for controllers with
>   offload capabilities".
> * In the previous version, we were using (void *)-1 as a sentinel value
>   that could be assigned, e.g. to rx_buf. But this was naive since there
>   is core code that would try to dereference this pointer. So instead,
>   we've added a new flags field to the spi_transfer structure for this
>   sort of thing. This also has the advantage of being able to be used in
>   the future for other arbitrary features.
> ---
>  drivers/spi/spi-offload.c            | 70
> ++++++++++++++++++++++++++++++++++++
>  drivers/spi/spi.c                    | 10 ++++++
>  include/linux/spi/offload/consumer.h |  5 +++
>  include/linux/spi/offload/types.h    | 19 ++++++++++
>  include/linux/spi/spi.h              |  3 ++
>  5 files changed, 107 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index
> 43582e50e279c4b1b958765fec556aaa91180e55..df5e963d5ee29d37833559595536a460c530
> bc81 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
> +#include <linux/dmaengine.h>
>  #include <linux/export.h>
>  #include <linux/kref.h>
>  #include <linux/list.h>
> @@ -332,6 +333,75 @@ void spi_offload_trigger_disable(struct spi_offload
> *offload,
>  }
>  EXPORT_SYMBOL_GPL(spi_offload_trigger_disable);
>  
> +static void spi_offload_release_dma_chan(void *chan)
> +{
> +	dma_release_channel(chan);
> +}
> +
> +/**
> + * devm_spi_offload_tx_stream_request_dma_chan - Get the DMA channel info for
> the TX stream
> + * @dev: Device for devm purposes.
> + * @offload: Offload instance
> + *
> + * This is the DMA channel that will provide data to transfers that use the
> + * %SPI_OFFLOAD_XFER_TX_STREAM offload flag.
> + *
> + * Return: Pointer to DMA channel info, or negative error code
> + */
> +struct dma_chan
> +*devm_spi_offload_tx_stream_request_dma_chan(struct device *dev,
> +					     struct spi_offload *offload)
> +{
> +	struct dma_chan *chan;
> +	int ret;
> +
> +	if (!offload->ops || !offload->ops->tx_stream_request_dma_chan)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	chan = offload->ops->tx_stream_request_dma_chan(offload);
> +	if (IS_ERR(chan))
> +		return chan;
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_release_dma_chan,
> chan);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return chan;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_tx_stream_request_dma_chan);
> +
> +/**
> + * devm_spi_offload_rx_stream_request_dma_chan - Get the DMA channel info for
> the RX stream
> + * @dev: Device for devm purposes.
> + * @offload: Offload instance
> + *
> + * This is the DMA channel that will receive data from transfers that use the
> + * %SPI_OFFLOAD_XFER_RX_STREAM offload flag.
> + *
> + * Return: Pointer to DMA channel info, or negative error code
> + */
> +struct dma_chan
> +*devm_spi_offload_rx_stream_request_dma_chan(struct device *dev,
> +					     struct spi_offload *offload)
> +{
> +	struct dma_chan *chan;
> +	int ret;
> +
> +	if (!offload->ops || !offload->ops->rx_stream_request_dma_chan)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	chan = offload->ops->rx_stream_request_dma_chan(offload);
> +	if (IS_ERR(chan))
> +		return chan;
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_release_dma_chan,
> chan);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return chan;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_rx_stream_request_dma_chan);
> +
>  /* Triggers providers */
>  
>  static void spi_offload_trigger_unregister(void *data)
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index
> ff1add2ecb91f18cf82e6f1e9595584c11adf9d8..4a871db9ee636aba64c866ebdd8bb1dbf82e
> 0f42 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -31,6 +31,7 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/sched/rt.h>
>  #include <linux/slab.h>
> +#include <linux/spi/offload/types.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi-mem.h>
>  #include <uapi/linux/sched/types.h>
> @@ -4163,6 +4164,15 @@ static int __spi_validate(struct spi_device *spi,
> struct spi_message *message)
>  
>  		if (_spi_xfer_word_delay_update(xfer, spi))
>  			return -EINVAL;
> +
> +		/* make sure controller supports required offload features */
> +		if (xfer->offload_flags) {
> +			if (!message->offload)
> +				return -EINVAL;
> +
> +			if (xfer->offload_flags & ~message->offload-
> >xfer_flags)
> +				return -EINVAL;
> +		}
>  	}
>  
>  	message->status = -EINPROGRESS;
> diff --git a/include/linux/spi/offload/consumer.h
> b/include/linux/spi/offload/consumer.h
> index
> 5a0ec5303d600728959594bcdbd0cb2baeba7c77..cd7d5daa21e69b61c16eba6c10c855345a4f
> 3297 100644
> --- a/include/linux/spi/offload/consumer.h
> +++ b/include/linux/spi/offload/consumer.h
> @@ -31,4 +31,9 @@ int spi_offload_trigger_enable(struct spi_offload *offload,
>  void spi_offload_trigger_disable(struct spi_offload *offload,
>  				 struct spi_offload_trigger *trigger);
>  
> +struct dma_chan *devm_spi_offload_tx_stream_request_dma_chan(struct device
> *dev,
> +							     struct
> spi_offload *offload);
> +struct dma_chan *devm_spi_offload_rx_stream_request_dma_chan(struct device
> *dev,
> +							     struct
> spi_offload *offload);
> +
>  #endif /* __LINUX_SPI_OFFLOAD_CONSUMER_H */
> diff --git a/include/linux/spi/offload/types.h
> b/include/linux/spi/offload/types.h
> index
> 7476f2073b02ee0f9edd3ae75e587b075746fa92..86d0e8cb9495bb43e177378b2041067de8ea
> 8786 100644
> --- a/include/linux/spi/offload/types.h
> +++ b/include/linux/spi/offload/types.h
> @@ -11,6 +11,11 @@
>  
>  struct device;
>  
> +/* 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)
> +
>  /* Offload can be triggered by external hardware event. */
>  #define SPI_OFFLOAD_CAP_TRIGGER			BIT(0)
>  /* Offload can record and then play back TX data when triggered. */
> @@ -40,6 +45,8 @@ struct spi_offload {
>  	void *priv;
>  	/** @ops: callbacks for offload support */
>  	const struct spi_offload_ops *ops;
> +	/** @xfer_flags: %SPI_OFFLOAD_XFER_* flags supported by provider */
> +	u32 xfer_flags;
>  };
>  
>  enum spi_offload_trigger_type {
> @@ -75,6 +82,18 @@ struct spi_offload_ops {
>  	 * given offload instance.
>  	 */
>  	void (*trigger_disable)(struct spi_offload *offload);
> +	/**
> +	 * @tx_stream_request_dma_chan: Optional callback for controllers
> that
> +	 * have an offload where the TX data stream is connected directly to
> a
> +	 * DMA channel.
> +	 */
> +	struct dma_chan *(*tx_stream_request_dma_chan)(struct spi_offload
> *offload);
> +	/**
> +	 * @rx_stream_request_dma_chan: Optional callback for controllers
> that
> +	 * have an offload where the RX data stream is connected directly to
> a
> +	 * DMA channel.
> +	 */
> +	struct dma_chan *(*rx_stream_request_dma_chan)(struct spi_offload
> *offload);
>  };
>  
>  #endif /* __LINUX_SPI_OFFLOAD_TYPES_H */
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index
> 98bdc8c16c20521c0a94e5f72f5e71c4f6d7d11e..4c087009cf974595f23036b1b7a030a45913
> 420c 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -1093,6 +1093,9 @@ struct spi_transfer {
>  
>  	u32		effective_speed_hz;
>  
> +	/* Use %SPI_OFFLOAD_XFER_* from spi-offload.h */
> +	unsigned int	offload_flags;
> +
>  	unsigned int	ptp_sts_word_pre;
>  	unsigned int	ptp_sts_word_post;
>  
>