Re: [PATCH RFC v4 03/15] spi: offload: add support for hardware triggers

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 23 Oct 2024 15:59:10 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Extend SPI offloading to support hardware triggers.
> 
> This allows an arbitrary hardware trigger to be used to start a SPI
> transfer that was previously set up with spi_optimize_message().
> 
> A new struct spi_offload_trigger is introduced that can be used to
> configure any type of trigger. It has a type discriminator and a union
> to allow it to be extended in the future. Two trigger types are defined
> to start with. One is a trigger that indicates that the SPI peripheral
> is ready to read or write data. The other is a periodic trigger to
> repeat a SPI message at a fixed rate.
> 
> There is also a spi_offload_hw_trigger_validate() function that works
> similar to clk_round_rate(). It basically asks the question of if we
> enabled the hardware trigger what would the actual parameters be. This
> can be used to test if the requested trigger type is actually supported
> by the hardware and for periodic triggers, it can be used to find the
> actual rate that the hardware is capable of.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few generic comments inline.

Jonathan

> ---
> 
> In previous versions, we locked the SPI bus when the hardware trigger
> was enabled, but we found this to be too restrictive. In one use case,
> to avoid a race condition, we need to enable the SPI offload via a
> hardware trigger, then write a SPI message to the peripheral to place
> it into a mode that will generate the trigger. If we did it the other
> way around, we could miss the first trigger.
> 
> Another likely use case will be enabling two offloads/triggers at one
> time on the same device, e.g. a read trigger and a write trigger. So
> the exclusive bus lock for a single trigger would be too restrictive in
> this case too.
> 
> So for now, I'm going with Nuno's suggestion to leave any locking up to
> the individual controller driver. If we do find we need something more
> generic in the future, we could add a new spi_bus_lock_exclusive() API
> that causes spi_bus_lock() to fail instead of waiting and add "locked"
> versions of trigger enable functions. This would allow a peripheral to
> claim exclusive use of the bus indefinitely while still being able to
> do any SPI messaging that it needs.
> 
> v4 changes:
> * Added new struct spi_offload_trigger that is a generic struct for any
>   hardware trigger rather than returning a struct clk.
> * Added new spi_offload_hw_trigger_validate() function.
> * Dropped extra locking since it was too restrictive.
> 
> v3 changes:
> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_...
> * added spi_offload_hw_trigger_get_clk() function
> * fixed missing EXPORT_SYMBOL_GPL
> 
> v2 changes:
> * This is split out from "spi: add core support for controllers with
>   offload capabilities".
> * Added locking for offload trigger to claim exclusive use of the SPI
>   bus.
> ---
>  drivers/spi/spi-offload.c       | 266 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-offload.h |  78 ++++++++++++
>  2 files changed, 344 insertions(+)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index c344cbf50bdb..2a1f9587f27a 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -9,12 +9,26 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/spi/spi-offload.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>  
> +struct spi_offload_trigger {
> +	struct list_head list;
> +	struct device dev;
> +	/* synchronizes calling ops and driver registration */
> +	struct mutex lock;
> +	const struct spi_offload_trigger_ops *ops;
> +	void *priv;
> +};
> +
> +static LIST_HEAD(spi_offload_triggers);
> +static DEFINE_MUTEX(spi_offload_triggers_lock);
> +
>  /**
>   * devm_spi_offload_alloc() - Allocate offload instances
>   * @dev: Device for devm purposes
> @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev,
>  	return offload;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_offload_get);
> +
> +static void spi_offload_trigger_release(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	guard(mutex)(&trigger->lock);
> +	if (trigger->priv && trigger->ops->release)
> +		trigger->ops->release(trigger->priv);
> +
> +	put_device(&trigger->dev);
> +}
> +
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_get(struct device *dev,
> +			      struct spi_offload *offload,
> +			      enum spi_offload_trigger_type type)
> +{
> +	struct spi_offload_trigger *trigger;
> +	struct fwnode_reference_args args;
> +	bool match = false;
> +	int ret;
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(offload->provider_dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells", 0, 0,
> +						 &args);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

> +	struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode;
I'm not fond of __free usage like this because code could get added above
this line and it wouldn't be obvious that it doesn't release the handle.

Annoying though it is, maybe just do manual release of the fwnode.  Ora
factor out the next chunk to a helper function so you can just put the fwnode after
that is called.


> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +
> +	list_for_each_entry(trigger, &spi_offload_triggers, list) {
> +		if (trigger->dev.fwnode != args.fwnode)
> +			continue;
> +
> +		match = trigger->ops->match(trigger->priv, type, args.args, args.nargs);
> +		if (match)
> +			break;
> +	}
> +
> +	if (!match)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	guard(mutex)(&trigger->lock);
> +
> +	if (!trigger->priv)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (trigger->ops->request) {
> +		ret = trigger->ops->request(trigger->priv, type, args.args, args.nargs);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	get_device(&trigger->dev);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_release, trigger);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return trigger;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_get);

> +int devm_spi_offload_trigger_register(struct device *dev,
> +				      struct spi_offload_trigger *trigger,
> +				      void *priv)
> +{
> +	int ret;
> +
> +	ret = device_add(&trigger->dev);
> +	if (ret)
> +		return ret;
> +
> +	trigger->priv = priv;
> +
> +	guard(mutex)(&spi_offload_triggers_lock);
> +	list_add_tail(&trigger->list, &spi_offload_triggers);
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_unregister, trigger);
I guess there may be more here later in the series, but if not.

	return devm_add_...

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);