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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Tue, 2024-05-21 at 09:28 -0500, David Lechner wrote:
> 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.
> 

Agreed.

> ...
> 
> > > +
> > > +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?

In those cases, the peripheral would not bother in doing any spi hardware triggering
enable/disable (this assumes that we would need explicit interfaces for offload
enable/disable). In DT, the engine would also have no trigger has it's not required
so the controller even conditionally register these callbacks.

As for the DMA, I have no clue how we can have it associated with the controller
given how we want the data to be exported to userspace. Maybe dma-buf could be used
but it won't be easy. TBH, I'm not sure if it's that bad having the DMA associated
with the peripheral since it's purpose is to transfer peripheral DATA (not like a spi
controller DMA used for the actual SPI transfers).

- Nuno Sá