Message-ID: <9762d3f3d3a2e5fbe5e5041cbdc928a9ab24e40b.camel@gmail.com>
Date: Thu, 24 Oct 2024 16:04:14 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Mark Brown <broonie@...nel.org>, 
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sá <nuno.sa@...log.com>, Uwe
 Kleine-König <ukleinek@...nel.org>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Lars-Peter Clausen
	 <lars@...afoo.de>, David Jander <david@...tonic.nl>, Martin Sperl
	 <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH RFC v4 03/15] spi: offload: add support for hardware
 triggers

On Wed, 2024-10-23 at 15:59 -0500, David Lechner 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@...libre.com>
> ---
> 
> 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;
> +
> +	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);

This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I would
expect is for methods of the trigger to get a struct spi_offload_trigger opaque
pointer. Then we provide a get_private kind of API for the private data. I guess you
want to avoid that but IMO it makes for neater API instead of getting void pointers.

Another thing is, can the above actually happen? We have the
spi_offload_triggers_lock grabbed and we got a match so the trigger should not be
able to go away (should block on the same lock).

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

try_module_get() would also make sense...

...

> 
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_alloc(struct device *dev,
> +				struct spi_offload_trigger_info *info)
> +{
> +	struct spi_offload_trigger *trigger;
> +	int ret;
> +
> +	trigger = kzalloc(sizeof(*trigger), GFP_KERNEL);
> +	if (!trigger)
> +		return ERR_PTR(-ENOMEM);
> +
> +	device_initialize(&trigger->dev);

Do we really need the full struct device and the overhead of adding it to the driver
core? AFAICT, we're using it only for refcouting so we could use a plain kref for
that matter. It would make things simpler. Or do you envision an future usecase as
this might matter? Like allowing userspace to set some controls on the trigger (I
would expect to be done through consumers though)?

> +	trigger->dev.parent = info->parent;
> +	trigger->dev.fwnode = info->fwnode;
> +	trigger->dev.of_node = of_node_get(to_of_node(trigger->dev.fwnode));
> +	trigger->dev.of_node_reused = true;
> +	trigger->dev.release = spi_offload_trigger_dev_release;
> +
> +	mutex_init(&trigger->lock);
> +	trigger->ops = info->ops;
> +
> +	ret = devm_add_action_or_reset(dev, spi_offload_trigger_put, trigger);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = dev_set_name(&trigger->dev, "%s-%d", info->name, info->id);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return trigger;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_alloc);
> +
> +static void spi_offload_trigger_unregister(void *data)
> +{
> +	struct spi_offload_trigger *trigger = data;
> +
> +	scoped_guard(mutex, &spi_offload_triggers_lock)
> +		list_del(&trigger->list);
> +
> +	guard(mutex)(&trigger->lock);
> +	trigger->priv = NULL;

nit: I guess this is a good as anything else but *ops could also be a good fit to
nullify :)

> +	device_del(&trigger->dev);
> +}
> +
> +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);
> +	if (ret)
> +		return ret;
> 

return devm_add_action_or_reset()?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);
> diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h
> index 92a557533b83..561cc1fb6f35 100644
> --- a/include/linux/spi/spi-offload.h
> +++ b/include/linux/spi/spi-offload.h
> @@ -22,6 +22,7 @@
>  MODULE_IMPORT_NS(SPI_OFFLOAD);
>  
>  struct device;
> +struct fwnode_handle;
>  struct spi_device;
>  
>  /* Offload can be triggered by external hardware event. */
> @@ -53,6 +54,43 @@ struct spi_offload {
>  	struct spi_device *spi;
>  	/** @priv: provider driver private data */
>  	void *priv;
> +	/** @ops: callbacks for offload support */
> +	const struct spi_offload_ops *ops;
> +};
> +
> +enum spi_offload_trigger_type {
> +	/* Indication from SPI peripheral that data is read to read. */
> +	SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	/* Trigger comes from a periodic source such as a clock. */
> +	SPI_OFFLOAD_TRIGGER_PERIODIC,
> +};
> +
> +struct spi_offload_trigger_periodic {
> +	u64 frequency_hz;
> +};
> +
> +struct spi_offload_trigger_config {
> +	/** @type: type discriminator for union */
> +	enum spi_offload_trigger_type type;
> +	union {
> +		struct spi_offload_trigger_periodic periodic;
> +	};
> +};
> +
> +/**
> + * struct spi_offload_ops - callbacks implemented by offload providers
> + */
> +struct spi_offload_ops {
> +	/**
> +	 * @trigger_enable: Optional callback to enable the trigger for the
> +	 * given offload instance.
> +	 */
> +	int (*trigger_enable)(struct spi_offload *offload);
> +	/**
> +	 * @trigger_disable: Optional callback to disable the trigger for the
> +	 * given offload instance.
> +	 */
> +	void (*trigger_disable)(struct spi_offload *offload);
>  };
>  
>  struct spi_offload *devm_spi_offload_alloc(struct device *dev,
> @@ -61,4 +99,44 @@ struct spi_offload *devm_spi_offload_alloc(struct device *dev,
>  struct spi_offload *devm_spi_offload_get(struct device *dev, struct spi_device
> *spi,
>  					 const struct spi_offload_config *config);
>  
> +struct spi_offload_trigger
> +*devm_spi_offload_trigger_get(struct device *dev,
> +			      struct spi_offload *offload,
> +			      enum spi_offload_trigger_type type);
> +int spi_offload_trigger_validate(struct spi_offload_trigger *trigger,
> +				 struct spi_offload_trigger_config *config);
> +int spi_offload_trigger_enable(struct spi_offload *offload,
> +			       struct spi_offload_trigger *trigger,
> +			       struct spi_offload_trigger_config *config);
> +void spi_offload_trigger_disable(struct spi_offload *offload,
> +				 struct spi_offload_trigger *trigger);
> +
> +/* Trigger providers */
> +
> +struct spi_offload_trigger;
> +
> +struct spi_offload_trigger_ops {
> +	bool (*match)(void *priv, enum spi_offload_trigger_type type, u64 *args,
> u32 nargs);
> +	int (*request)(void *priv, enum spi_offload_trigger_type type, u64 *args,
> u32 nargs);
> +	void (*release)(void *priv);
> +	int (*validate)(void *priv, struct spi_offload_trigger_config *config);
> +	int (*enable)(void *priv, struct spi_offload_trigger_config *config);
> +	void (*disable)(void *priv);
> +};

Yeah, as I said... not a fan of the void *priv thing

- Nuno Sá

Powered by blists - more mailing lists