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