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

[Jonathan Cameron <jic23@xxxxxxxxxx>]

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

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

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

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

...

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

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