Re: [PATCH 01/13] spi: add core support for controllers with offload capabilities

[Mark Brown]

On Mon, Mar 04, 2024 at 05:21:21PM -0600, David Lechner wrote:
> On Wed, Jan 10, 2024 at 3:36 PM Mark Brown <broonie@xxxxxxxxxx> wrote:

> > The bit where messages are initiated by hardware is a step beyond that,
> > I think we need a bit more API for connecting up the triggers and we
> > also need to have something handling what happens with normal operation
> > of the device while these triggers are enabled.  I think it could be
> > useful to split this bit out since there's a lot more to work out there
> > in terms of interfaces.

> > > +/**
> > > + * SPI_OFFLOAD_RX - placeholder for indicating read transfers for offloads
> > > + *
> > > + * Assign xfer->rx_buf to this value for any read transfer passed to
> > > + * spi_offload_prepare(). This will act as a flag to indicate to the offload
> > > + * that it should do something with the data read during this transfer. What
> > > + * that something can be is determined by the specific hardware, e.g. it could
> > > + * be piped to DMA or a DSP, etc.
> > > + */
> > > +#define SPI_OFFLOAD_RX_SENTINEL ((void *)1)

> > This feels like something where there are likely to be multiple options
> > and we need configurability.  I'd also expect to see a similar transmit
> > option.

> Having something similar for TX makes sense. What other sorts of
> options are you envisioning here?

You list two options for something that could be done with the data
above - sending it to DMA or a DSP.  My concern here is that a given
piece of hardware might support more than one option and need to choose
between them.

> > > +int spi_offload_prepare(struct spi_offload *offload, struct spi_device *spi,
> > > +                       struct spi_transfer *xfers, unsigned int num_xfers)

> > I would expect us to just generically prepare a message, then pass a
> > prepared message into the API that enables a trigger.  We would need
> > something that handles the difference between potentially offloading for
> > better performance and having a hardware trigger, I think that might be
> > a case of just not exposing the engine's prepare to client drivers and
> > then having the core track if it needs to do that when enabling a
> > hardware trigger.

> Not exposing the offload prepare to client drivers sounds reasonable.
> I'm not sure I understand the potential need for an offload without a
> hardware trigger though.

Something like pre-cooking the commands to read the interrupt status
registers from a device, send a network transfer, or to download
firmware and settings if you power the device off frequently.  Basically
anything with more than one operation that you might want to run
repeatedly and care about the performance of.

> > I'm not seeing anything in this API that provides a mechanism for
> > configuring what triggers things to start, even in the case where things
> > are triggered by hardware rather than initiated by software I'd expect
> > to see hardware with runtime configurability.  The binding is a bit
> > unclear but it seems to be expecting this to be statically configured in
> > hardware and that there will be a 1:1 mapping between triggers and
> > scripts that can be configured, if nothing else I would expect that
> > there will be hardware with more possible triggers than scripts.

> For the use case of ADCs/DACs we would want a periodic trigger where
> the period of the trigger is runtime configurable (via sysfs). Is this
> the sort of thing you had in mind here? What other sorts of triggers
> do you have in mind?

Well, it could be pretty much any signal - I'd imagine there will be
things that can trigger off GPIOs for example.  Some sort of timer like
you mention does sound plausible too.  I think the API needs to be
general enough to just cope with a very broad range of things in a
possibly system/device specified manner and not have a short,
prescriptive list.

> > I'd also expect some treatement of what happens with the standard SPI
> > API while something is enabled.

> I suppose it makes sense to return -EBUSY from
> spi_sync()/spi_async()/spi_bus_lock() when a hardware trigger is
> enabled.

That sounds reasonable.  If something is software triggered then I'd
expect to integrate with the current queuing mechanism.

Attachment: signature.asc
Description: PGP signature