Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support

[David Lechner <dlechner@xxxxxxxxxxxx>]

On Tue, May 21, 2024 at 7:27 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Fri, 2024-05-10 at 19:44 -0500, David Lechner wrote:
> > This implements SPI offload support for the AXI SPI Engine. Currently,
> > the hardware only supports triggering offload transfers with a hardware
> > trigger so attempting to use an offload message in the regular SPI
> > message queue will fail. Also, only allows streaming rx data to an
> > external sink, so attempts to use a rx_buf in the offload message will
> > fail.
> >
> > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > ---
> >

...

> > +
> > +static int spi_engine_offload_map_channel(struct spi_device *spi,
> > +                                       unsigned int id,
> > +                                       unsigned int channel)
> > +{
> > +     struct spi_controller *host = spi->controller;
> > +     struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > +     struct spi_engine_offload *priv;
> > +
> > +     if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
> > +             return -EINVAL;
> > +
> > +     priv = &spi_engine->offload_priv[channel];
> > +
> > +     if (priv->spi)
> > +             return -EBUSY;
>
> I wonder if we need to be this strict? Is there any problem by having two
> devices requesting the same offload engine? I would expect that having multiple
> peripherals trying to actually use it at the same time (with the prepare()
> callback) to be problematic but if they play along it could actually work,
> right? In reality that may never be a realistic usecase so this is likely fine.
>

I guess not. But to keep it simple for now, yeah, let's wait until we
have an actual use case.

...

> > +
> > +static const struct spi_controller_offload_ops spi_engine_offload_ops = {
> > +     .map_channel = spi_engine_offload_map_channel,
> > +     .prepare = spi_engine_offload_prepare,
> > +     .unprepare = spi_engine_offload_unprepare,
> > +     .hw_trigger_enable = spi_engine_offload_enable,
> > +     .hw_trigger_disable = spi_engine_offload_disable,
>
> I guess this is what you and Conor are already somehow discussing but I would
> expect this to be the actual offload trigger to play a spi transfer. As it
> stands, it looks weird (or confusing) to have the enable/disable of the engine
> to act as a trigger...

It isn't acting as the trigger, just configuring the offload instance
for exclusive use by a hardware trigger.

> Maybe these callbacks could be used to enable/disable the
> actual trigger of the offload engine (in our current cases, the PWM)? So this
> would make it easy to move the trigger DT property where it belongs. The DMA one
> (given it's tight relation with IIO DMA buffers) is another (way more difficult)
> story I think.
>

One issue I have with making the actual hardware trigger part of the
SPI controller is that in some cases, the peripheral could actually be
the trigger. For example, in the case of a self-clocked ADC where
there is just a RDY signal from the ADC when sample data is ready to
be read. In this case would the peripheral have to register a trigger
enable callback with the controller so that the controller can
communicate with the peripheral to enable and disable sampling mode,
and therefore the trigger?