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

[Nuno Sá]

On Tue, 2024-07-23 at 09:30 -0500, David Lechner wrote:
> On 7/23/24 2:53 AM, Nuno Sá wrote:
> > On Mon, 2024-07-22 at 16:57 -0500, David Lechner 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.
> > > 
> > > 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.
> > > ---
> > 
> > How explicitly do we want to be about returning errors? It seems that if the
> > trigger is enabled we can't anything else on the controller/offload_engine so we
> > could very well just hold the controller lock when enabling the trigger and 
> > release it when disabling it. Pretty much the same behavior as spi_bus_lock()...
> 
> The problem I see with using spi_bus_lock() in it's current form is that
> SPI offload triggers could be enabled indefinitely. So any other devices
> on the bus that tried to use the bus and take the lock would essentially
> deadlock waiting for the offload user to release the lock. This is why
> I added the -BUSY return, to avoid this deadlock.
> 

If someone does not disable the trigger at some point that's a bug. If I understood
things correctly, even if someone tries to access the bus will get -EBUSY. But yeah,
arguably it's better to get a valid error rather than blocking/sleeping

> > 
> > ...
> > 
> > > 
> > > +
> > > +/**
> > > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger
> > > + * @spi: SPI device
> > > + * @id: Function ID if SPI device uses more than one offload or NULL.
> > > + *
> > > + * The caller is responsible for calling clk_put() on the returned clock.
> > > + *
> > > + * Return: The clock for the offload trigger, or negative error code
> > > + */
> > > +static inline
> > > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char
> > > *id)
> > > +{
> > > +	struct spi_controller *ctlr = spi->controller;
> > > +
> > > +	if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk)
> > > +		return ERR_PTR(-EOPNOTSUPP);
> > > +
> > > +	return ctlr->offload_ops->hw_trigger_get_clk(spi, id);
> > > +}
> > > 
> > 
> > It would be nice if we could have some kind of spi abstraction...
> 
> After reading your other replies, I think I understand what you mean here.
> 
> Are you thinking some kind of `struct spi_offload_trigger` that could be
> any kind of trigger (clk, gpio, etc.)?
> 

Yeah, something in that direction...

- Nuno Sá