Re: [PATCH RFC v3 0/9] spi: axi-spi-engine: add offload support

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

On Tue, 2024-07-23 at 08:48 -0500, David Lechner wrote:
> On 7/23/24 2:35 AM, Nuno Sá wrote:
> > Hi David,
> > 
> > 
> > I think there are things that we need to better figure but things are improving
> > IMO :)
> > 
> > I'm only doing a very superficial review since I need to better look at the
> > patches...
> > 
> > But one thing that I do want to mention is a scenario (another funny one...)
> > that I've discussing and that might be a reality. Something like:
> > 
> > +-------------------------------+    +------------------+
> > >                               |    |                  |
> > >  SOC/FPGA                     |    |   ADC            |
> > >                               |    |                  |
> > >       +---------------+       |    |                  |
> > >       |  SPI PS Zynq  |============== SPI Bus         |
> > >       +---------------+       |    |                  |
> > >                               |    |                  |
> > >  +---------------------+      |    |                  |
> > >  | AXI SPI Engine      |      |    |                  |
> > >  |                 v================ DATA Bus         |
> > >  |                 v   |      |    |                  |
> > >  |   +---------------+ |      |    |                  |
> > >  |  | Offload 0     |  |      |    +------------------+
> > >  |  |   RX DATA OUT |  |      |
> > >  |  |    TRIGGER IN |  |      |
> > >  |  +---------------+  |      |
> > >                               |
> > +-------------------------------+
> > 
> > From the above, the spi controller for typical register access/configuration is
> > not the spi_enigine and the offload core is pretty much only used for streaming
> > data. So I think your current approach would not work with this usecase. In your
> > first RFC you had something overly complicated (IMHO) but you already had a
> > concept that maybe it's worth looking at again. I mean having a spi_offload
> > object that could describe it and more importantly have a provider/consumer
> > logic where a spi consumer (or maybe even something else?) can get()/put() an
> > offload object to stream data.
> 
> Although it isn't currently implemented this way in the core SPI code, I think
> the DT bindings proposed in this version of the series would allow for this.
> The offload provider is just the one with the #spi-offload-cells and doesn't
> necessarily have to be the parent of the SPI peripheral.
> 

I get that but the thing is that when the spi device consuming an offload service is
under the same controller providing that service we have guarantees regarding
lifetimes. In case the offload is another controller, those guarantees are not true
anymore and we need to make sure to handle things correctly (like the controller
going away under our feet). That's why a proper provider/consumer interface is needed
and I think that's a significant shift from what we have now?

Out of my head (the minimal interface):

spi_controller_offload_register() - we may need a list of register offloads in the
controller struct.
(devm_)spi_offload_get() - and this one could return a nice struct spi_offload
abstraction to pass to the other APIs
spi_offload_put()

Or for starters we may skip the registering in the core and have it per driver but if
we assume more controlles will have this we might just make it common code already.

Just some ideas...

- Nuno Sá