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

["Nuno Sá" <noname.nuno@gmail.com>]

Hi David,

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> There is a recap at the end of this cover letter for those not familiar
> with the previous discussions. For those that are, we'll get right to
> the changes since the last version.
> 
> In RFC v2, most of the discussion was around the DT bindings, so that
> is what has mostly changed since then. I think we mostly settled on
> what properties are needed and where they should go. There are probably
> still some details to work out (see PATCH 5/9 for more discussion) but
> I think we have the big-picture stuff figured out.
> 
> Here is the actual devicetree used for testing to show how it all
> comes together:
> 
> 	trigger_clk: adc-trigger-clock {
> 		compatible = "pwm-clock";
> 		#clock-cells = <0>;
> 		#trigger-source-cells = <0>;
> 		pwms = <&adc_trigger 0 10000>;
> 	};
> 
> 	...
> 
> 	axi_spi_engine_0: spi@44a00000 {
> 		compatible = "adi,axi-spi-engine-1.00.a";
> 		reg = <0x44a00000 0x1000>;
> 		interrupt-parent = <&intc>;
> 		interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> 		clocks = <&clkc 15>, <&spi_clk>;
> 		clock-names = "s_axi_aclk", "spi_clk";
> 
> 		/* offload-specific properties */
> 		#spi-offload-cells = <1>;
> 		dmas = <&rx_dma 0>;
> 		dma-names = "offload0-rx";
> 		trigger-sources = <&trigger_clk>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		ad7986: adc@0 {
> 			compatible = "adi,ad7986";
> 			reg = <0>;
> 			spi-max-frequency = <111111111>; /* 9 ns period */
> 			adi,spi-mode = "single";
> 			avdd-supply = <&eval_u12>;
> 			dvdd-supply = <&eval_u12>;
> 			vio-supply = <&eval_u3>;
> 			bvdd-supply = <&eval_u10>;
> 			ref-supply = <&eval_u5>;
> 			turbo-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>;
> 
> 			spi-offloads = <&axi_spi_engine_0 0>;
> 		};
> 	};
> 
> A working branch complete with extra hacks can be found at [1].
> 
> Also, I took a detour looking into what it would take to get Martin
> Sperl's Raspberry Pi DMA offload proof-of-concept [2] updated to work
> with this. This way we could have a second user to help guide the
> design process. Given all of the SPI hardware quirks on that platform
> and the unsolved technical issues, like how to get accurate time delays
> and how to work around the 32-bit DMA word limitation, it would be more
> work than I have time for (at least without someone sponsoring the work).
> 
> [1]: https://github.com/dlech/linux/tree/axi-spi-engine-offload-v3
> [2]:
> https://github.com/msperl/spi-bcm2835/blob/refactor_dmachain_for_prepared_messages/spi-bcm2835dma.c
> 
> ---
> Changes in v3:
> - See individual patches for more detailed changes.
> - Reworked DT bindings to have things physically connected to the SPI
>   controller be properties of the SPI controller and use more
>   conventional provider/consumer properties.
> - Added more SPI APIs for peripheral drivers to use to get auxillary
>   offload resources, like triggers.
> - Link to v2:
> https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com
> 
> ---
> 
> As a recap, here is the background and end goal of this series:
> 
> The AXI SPI Engine is a SPI controller that has the ability to record a
> series of SPI transactions and then play them back using a hardware
> trigger. This allows operations to be performed, repeating many times,
> without any CPU intervention. This is needed for achieving high data
> rates (millions of samples per second) from ADCs and DACs that are
> connected via a SPI bus.
> 
> The offload hardware interface consists of a trigger input and a data
> output for the RX data. These are connected to other hardware external
> to the SPI controller.
> 
> To record one or more transactions, commands and TX data are written
> to memories in the controller (RX buffer is not used since RX data gets
> streamed to an external sink). This sequence of transactions can then be
> played back when the trigger input is asserted.
> 
> This series includes core SPI support along with the first SPI
> controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use
> them. This enables capturing analog data at 2 million samples per
> second.
> 
> The hardware setup looks like this:
> 
> +-------------------------------+   +------------------+
> >                               |   |                  |
> >  SOC/FPGA                     |   |  AD7944 ADC      |
> >  +---------------------+      |   |                  |
> >  | AXI SPI Engine      |      |   |                  |
> >  |             SPI Bus ============ SPI Bus          |
> >  |                     |      |   |                  |
> >  |  +---------------+  |      |   |                  |
> >  |  | Offload 0     |  |      |   +------------------+
> >  |  |   RX DATA OUT > > > >   |
> >  |  |    TRIGGER IN < < <  v  |
> >  |  +---------------+  | ^ v  |
> >  +---------------------+ ^ v  |
> >  | AXI PWM             | ^ v  |
> >  |                 CH0 > ^ v  |
> >  +---------------------+   v  |
> >  | AXI DMA             |   v  |
> >  |                 CH0 < < <  |
> >  +---------------------+      |
> >                               |
> +-------------------------------+
> 
> To: Mark Brown <broonie@kernel.org>
> To: Jonathan Cameron <jic23@kernel.org>
> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Nuno Sá <nuno.sa@analog.com>
> Cc: Michael Hennerich <Michael.Hennerich@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: David Jander <david@protonic.nl>
> Cc: Martin Sperl <kernel@martin.sperl.org>
> Cc:  <linux-spi@vger.kernel.org>
> Cc:  <devicetree@vger.kernel.org>
> Cc:  <linux-kernel@vger.kernel.org>
> Cc:  <linux-iio@vger.kernel.org>
> 
> ---
> 

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.

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á