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

[David Lechner <dlechner@xxxxxxxxxxxx>]

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 know, I did said that I did not liked for spi consumers to have to explicitly
> call something like spi_offload_get() but I guess I have been proved wrong :).
> We can also try to be smart about it as an explicit get is only needed (likely)
> in the above scenario (or maybe we can even do it directly in the spi core
> during spi_probe()). Or maybe it's not worth it to play smart and just let
> consumers do it (that's the typical pattern anyways).
> 
> Having said the above, I still think your current proposal for triggers and
> getting DMA streams is valid for the above usecase.
> 
> FWIW, I'm also trying to understand with the HW guys why the hell can't we just
> use the spi_engine controller for everything. But the whole discussion is
> already showing us that we may need more flexibility.
> 
> Thanks!
> - Nuno Sá