Re: [PATCH 04/13] spi: dt-bindings: adi,axi-spi-engine: add offload bindings

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Rob Herring <robh@kernel.org>, David Lechner <dlechner@baylibre.com>
Cc: "Mark Brown" <broonie@kernel.org>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Michael Hennerich" <michael.hennerich@analog.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Frank Rowand" <frowand.list@gmail.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Jonathan Corbet" <corbet@lwn.net>,
	linux-spi@vger.kernel.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/13] spi: dt-bindings: adi,axi-spi-engine: add offload bindings
Date: Thu, 11 Jan 2024 10:07:38 +0100	[thread overview]
Message-ID: <501bb280c6bd7dcb49929e7dbd242807b828e7de.camel@gmail.com> (raw)
In-Reply-To: <20240110231456.GB2854345-robh@kernel.org>

On Wed, 2024-01-10 at 17:14 -0600, Rob Herring wrote:
> On Wed, Jan 10, 2024 at 01:49:45PM -0600, David Lechner wrote:
> > The ADI AXI SPI Engine driver supports offloading SPI transfers to
> > hardware. This is essentially a feature that allows recording an
> > arbitrary sequence of SPI transfers and then playing them back with
> > no CPU intervention via a hardware trigger.
> > 
> > This adds the bindings for this feature. Each SPI Engine instance
> > can have from 0 to 32 offload instances. Each offload instance has a
> > trigger input and a data stream output. As an example, this could be
> > used with an ADC SPI peripheral. In this case the trigger is connected
> > to a PWM/clock to determine the sampling rate for the ADC and the output
> > stream is connected to a DMA channel to pipe the sample data to memory.
> > 
> > SPI peripherals act as consumers of the offload instances. Typically,
> > one SPI peripheral will be connected to one offload instance. But to
> > make the bindings future-proof, the property is an array.
> 
> Is there some sort of arbitration between multiple offload engines on 
> the same chip select? If not, I don't see how it would work.
> 
> I think this whole thing could be simplified down to just 3 
> SPI controller properties: pwms, dmas, and adi,offload-cs-map. Each 
> property is has entries equal the number of offload engines. The last 
> one maps an offload engine to a SPI chip-select.
> 

I think the whole reason why the offload is being treated as a node + platform
device is to have these properties (or other possible properties depending on
the trigger and data capture being used) in it and so respect the HW
configuration.

While that is conceptually correct I feel that this is bringing a lot of extra
complexity. The end consumer of the offload core (which is a property/feature of
the spi cotroller) are obviously the peripheral devices that in our usecases are
converters and hence IIO devices. So those are the ones consuming all the data.
I saw Mark already giving some pointers and speaking about having a way to
support the triggers (being it pwm, clock, gpio, etc...) directly in the spi
framework and I think that would be nice. For the dmas, I think it would be more
complicated. While we can setup the dma transfer directly in the spi controller,
we would need a mechanism to transfer each block of data (periodically) as soon
as we have it to the peripheral device. In case of IIO, that would then have to
connect to IIO DMA buffers so the data can be consumed in userspace and that
would be the tricky part I believe.

What we have been doing out of tree is to control the trigger and dmas in the
peripheral device even if that does not really directly respect the HW setup (as
these are properties of the offload core). Hence, we can just allocate an IIO
DMA buffer, enable the offload message with the messages we want and data can be
directly transferred to userspace (without any intervention of the peripheral
driver) using the IIO interfaces for it.

To sum it up, I think having the trigger being handled by the spi framework or
even have it as an IIO generic trigger would be simple. To have the dma
transfers in the spi controller would be more complex but anything is possible,
I guess :).

- Nuno Sá
> 
> 