Message-ID: <20240522-gullible-ibuprofen-cf9111c25f6f@spud>
Date: Wed, 22 May 2024 19:24:57 +0100
From: Conor Dooley <conor@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Mark Brown <broonie@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nuno Sá <nuno.sa@...log.com>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	David Jander <david@...tonic.nl>,
	Martin Sperl <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add
 spi-offloads property

On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@...nel.org> wrote:
> >
> > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@...nel.org> wrote:
> > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >
> > > > > Back to "something beyond the SPI controller":
> > > > >
> > > > > Here are some examples of how I envision this would work.
> > > > >
> > > > > Let's suppose we have a SPI controller that has some sort of offload
> > > > > capability with a configurable trigger source. The trigger can either
> > > > > be an internal software trigger (i.e. writing a register of the SPI
> > > > > controller) or and external trigger (i.e. a input signal from a pin on
> > > > > the SoC). The SPI controller has a lookup table with 8 slots where it
> > > > > can store a series of SPI commands that can be played back by
> > > > > asserting the trigger (this is what provides the "offloading").
> > > > >
> > > > > So this SPI controller would have #spi-offload-cells = <2>; where the
> > > > > first cell would be the index in the lookup table 0 to 7 and the
> > > > > second cell would be the trigger source 0 for software or 1 for
> > > > > hardware.
> > > > >
> > > > > Application 1: a network controller
> > > > >
> > > > > This could use two offloads, one for TX and one for RX. For TX, we use
> > > > > the first slot with a software trigger because the data is coming from
> > > > > Linux. For RX we use the second slot with a hardware trigger since
> > > > > data is coming from the network controller (i.e. a data ready signal
> > > > > that would normally be wired to a gpio for interrupt but wired to the
> > > > > SPI offload trigger input pin instead). So the peripheral bindings
> > > > > would be:
> > > > >
> > > > > #define SOFTWARE_TRIGGER 0
> > > > > #define HARDWARE_TRIGGER 1
> > > > >
> > > > > can@0 {
> > > > >     ...
> > > > >     spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>;
> > > > >     /* maybe we need names too? */
> > > > >     spi-offload-names = "tx", "rx";
> > > > > };
> > > > >
> > > > > In this case, there is nothing extra beyond the SPI controller and the
> > > > > network controller, so no extra bindings beyond this are needed.
> > > > >
> > > > > Application 2: an advanced ADC + FPGA
> > > > >
> > > > > This is basically the same as the ad7944 case seen already with one
> > > > > extra feature. In this case, the sample data also contains a CRC byte
> > > > > for error checking. So instead of SPI RX data going directly to DMA,
> > > > > the FPGA removes the CRC byte from the data stream an only the sample
> > > > > data goes to the DMA buffer. The CRC is checked and if bad, an
> > > > > interrupt is asserted.
> > > > >
> > > > > Since this is an FPGA, most everything is hardwired rather than having
> > > > > any kind of mux selection so #spi-offload-cells = <1>; for this
> > > > > controller.
> > > > >
> > > > > By adding spi-offloads to the peripheral node, it also extends the
> > > > > peripheral binding to include the additional properties needed for the
> > > > > extra features provided by the FPGA. In other words, we are saying
> > > > > this DT node now represents the ADC chip plus everything connected to
> > > > > the offload instance used by the ADC chip.
> > > >
> > > > It seems very strange to me that the dmas and the clock triggers are
> > > > going into the spi device nodes. The description is
> > > > | +  dmas:
> > > > | +    maxItems: 1
> > > > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > > > But as far as I can tell this device is in a package of its own and not
> > > > some IP provided by Analog that an engine on the FPGA can actually do
> > > > DMA to, and the actual connection of the device is "just" SPI.
> > > > The dmas and clock triggers etc appear to be resources belonging to the
> > > > controller that can "assigned" to a particular spi device. If the adc
> > > > gets disconnected from the system, the dmas and clock triggers are still
> > > > connected to the spi controller/offload engine, they don't end up n/c,
> > > > right? (Well maybe they would in the case of a fancy SPI device that
> > > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > > responsible for selecting which of the various resources belonging to
> > > > the controller are to be used by a device.
> > > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > > gonna start screaming at me but w/e, looking at it from the point of
> > > > view of how the hardware is laid out (or at least how it is described
> > > > in your FPGA case above) the dma/clock properties looks like they're
> > > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > > to "the ADC chip plus everything connected to the offload instance
> > > > used by the ADC chip".
> > >
> > > This is the same reasoning that led me to the binding proposed in v1.
> > > Rob suggested that these extras (dmas/clocks) should just be
> > > properties directly of the SPI controller.
> >
> > > But the issue I have with
> > > that is that since this is an FPGA, these properties are not fixed.
> >
> > I don't think whether or not we're talking about an FPGA or an ASIC
> > matters at all here to be honest. In my view the main thing that FPGA
> > impact in terms of bindings is the number of properties required to
> > represent the configurability of a particular IP. Sure, you're gonna
> > have to change the dts around in a way you'll never have to with an
> > ASIC, but the description of individual devices or relations between
> > them doesn't change.
> >
> > > Maybe there are more clocks or no clocks or interrupts or something we
> > > didn't think of yet.
> >
> > This could happen with a new generation of an ASIC and is not specific
> > to an FPGA IP core. Even with FPGA IP, while there may be lots of
> > different configuration parameters, they are known & limited - you can
> > look at the IP's documentation (or failing that, the HDL) to figure out
> > what the points of variability are. It's not particularly difficult to
> > account for there being a configurable number of clocks or interrupts.
> > For "something we didn't think of yet" to be a factor, someone has to
> > think of it and add it to the IP core, and which point we can absolutely
> > change the bindings and software to account for the revised IP.
> >
> > > So it doesn't really seem possible to write a
> > > binding for the SPI controller node to cover all of these cases.
> >
> > I dunno, I don't think your concerns about numbers of clocks (or any
> > other such property) are unique to this particular use-case.
> >
> > > These
> > > extras are included in the FPGA bitstream only for a specific type of
> > > peripheral, not for general use of the SPI controller with any type of
> > > peripheral.
> >
> > I accept that, but what I was trying to get across was that you could
> > configure the FPGA with a bitstream that contains these extra resources
> > and then connect a peripheral device that does not make use of them at
> > all.
> 
> Sure, in this case the peripheral has no spi-offloads property and the
> SPI controller acts as a typical SPI controller.
> 
> > Or you could connect a range of different peripherals that do use
> > them.
> 
> OK, you've got me thinking about something I hadn't really thought about yet.
> 
> > Whether or not the resources are there and how they are connected
> > in the FPGA bitstream is not a property of the device connected to the
> > SPI controller, only whether or not you use them is.
> >
> 
> Even when those things are connected directly to a specific peripheral
> device? Or not connected to the offload?
> 
> Here is another potential ADC trigger case to consider.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < BUSY             |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v  > > > > CNV              |
> |  |  |    TRIGGER IN < < <   ^ |   |                  |
> |  |  +---------------+  |    ^ |   +------------------+
> |  +---------------------+    ^ |
> |  | AXI PWM             |    ^ |
> |  |                 CH0 >  > ^ |
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> This time, the periodic trigger (PWM) is connected to the pin on the
> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> that will go high at the start of the conversion and go low at the end
> of the conversion. The BUSY output of the ADC is wired as the hardware
> trigger input of the offload.
> 
> In this case would we still consider the PWM as part of the SPI
> controller/offload since it can only be used in conjunction with the
> SPI offload? It isn't connected to it at all though.

No, in this case the ADC is a PWM consumer and the offload engine is
not. The ADC is a "trigger" provider and the SPI offload engine is a
trigger consumer.

> Another case could be a self-clocked ADC. Here, the ADC has it's own
> periodic sample trigger on the chip and the RDY output goes high
> whenever a sample is ready to read.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < RDY              |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v    |   |                  |
> |  |  |    TRIGGER IN < < <     |   |                  |
> |  |  +---------------+  |      |   +------------------+
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> If both of these are valid wiring configurations for the same ADC,
> wouldn't it make more sense to describe this in the peripheral node
> rather than having to query the controller to see how the peripheral
> is wired up?

In both of these cases, the offload works in the same way, it gets a
trigger from the ADC and acts upon it. I would expect that in this case
the ADC driver would look for an optional pwm property in the devicetree
and if it is present configure the ADC to use that and if absent do then
do whatever configuration is required for self clocking. I would expect
that both cases would be handled identically by the SPI [offload] engine
side of things, other than inverting the polarity of the trigger. (If I
understand correctly, you want to trigger the offload engine on a
falling edge of BUSY but a rising edge of RDY).


> > In fact, looking at the documentation for the "spi engine" some more, I
> > am even less convinced that the right place for describing the offload is
> > the peripheral as I (finally?) noticed that the registers for the offload
> > engine are mapped directly into the "spi engine"'s memory map, rather than
> > it being a entirely separate IP in the FPGA fabric.
> 
> True, but we don't use these registers, e.g., to configure the
> sampling frequency of a trigger (if it can even be done). That is done
> in a completely separate IP block with it's own registers.

Where is the binding for that IP block? I think describing that is
pretty key. goto continuation;

> > Further, what resources are available depends on what the SPI offload
> > engine IP is. If my employer decides to start shipping some SPI offload
> > IP in our catalogue that can work with ADI's ADCs, the set of offload
> > related properties or their names may well differ.
> 
> If all of these resources were fairly generic, like the periodic
> trigger we've been discussing, then I could see the case for trying to
> accommodate this the same way we do for other common features of SPI
> controllers. But for cases like "Application 2" that I described
> previously, these resources can get very specific to a peripheral
> (like the example given of having a data processing unit that knows
> about the exact data format and CRC algorithm used by a specific ADC).
> These seems like too specific of a thing to say that a SPI controller
> "supports".

To remind myself, "Application 2" featured an offload engine designed
specifically to work with a particular data format that would strip a
CRC byte and check the validity of the data stream.

I think you're right something like that is a stretch to say that that
is a feature of the SPI controller - but I still don't believe that
modelling it as part of the ADC is correct. I don't fully understand the
io-backends and how they work yet, but the features you describe there
seem like something that should/could be modelled as one, with its own
node and compatible etc. Describing custom RTL stuff ain't always
strightforward, but the stuff from Analog is versioned and documented
etc so it shouldn't be quite that hard.

continuation:
If offload engines have their own register region in the memory map,
their own resources (the RTL is gonna need at the very least a clock)
and potentially also provide other services (like acting as an
io-backend type device that pre-processes data) it doesn't sound like
either the controller or peripheral nodes are the right place for these
properties. And uh, spi-offloads gets a phandle once more...

FWIW, I did read these examples but didn't feel it was worth commenting
on them given the above. I'll comment on them if they stay "accurate".

Cheers,
Conor.

> But, OK, let's go with the idea of putting everything related to the
> offloads in the SPI controller node an see where it takes us...
> 
> spi@...0 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* PWM is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle 16-bit words. IIO hardware buffer will be CPU-
>      * endian because data is streamed one word at a time. */
>     spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma";
> 
>     /* pwm properties are only allowed because spi-offload-0-capabilities
>      * contains "pwm-trigger" */
>     pwms = <&pwm 0>;
>     pwm-names = "offload-0-pwm-trigger";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@...0 {
>     compatible = "not-adi,other-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Clock is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle one byte at a time. IIO hardware buffer will be big-
>      * endian because data is streamed one byte at a time. */
>     spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma";
> 
>     /* Clock properties are extended because spi-offload-0-capabilities contains
>      * "clock-trigger" and SPI controller itself has a clock */
>     clocks = <&cpu_clock 0>, <&extra_clock 0>;
>     clock-names = "sclk", "offload-0-pwm-trigger";
> 
>     /* DMA properties are extended because spi-offload-0-capabilities contains
>      * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */
>     dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>;
>     dma-names = "tx", "rx", "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@...0 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 16-bit words. */
>     spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma";
> 
>     /* Do we need a property to say that the sample trigger is connected to
>      * adc@0 so that if adc@1 tries to use it, it will fail? */
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). This only makes sense
>          * when offload is used with BSY signal, otherwise we would have CNV
>          * connected to SPI CS. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> spi@...0 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 32-bit words.
>      * This also has the CRC validation that strips off the CRC byte of the
>      * raw data before passing the sample to DMA. */
>     spi-offload-0-capabilities = "sample-trigger",
>                                  "32-bit-rx-dma-with-ad4630-crc-check";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     interrupt-parent = <&intc>;
>     /* First interrupt is for the SPI controller (always present), second
>      * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check"
>      * of offload 0. */
>     interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>;
>     interrupt-names = "controller", "offload-0-crc-error";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad4630";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). Without offload, we would
>          * have cnv-gpios instead. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> So this is what I came up with of how things could look (combining
> suggestions from Rob in v1 and Conor's suggestions here). I can see
> how we can make this work. But the thing I don't like about it is that
> the peripheral drivers still need to know all of the information about
> the offload capabilities and need to interact with the
> pwms/clocks/interrupts/dmas/etc. So this is just adding layers of
> indirection where all of this stuff has to go through the SPI
> controller driver.
> 
> 
> >
> > > Another idea I had was to perhaps use the recently added IIO backend
> > > framework for the "extras". The idea there is that we are creating a
> > > "composite" IIO device that consists of the ADC chip (frontend) plus
> > > these extra hardware trigger and hardware buffer functions provided by
> > > the FPGA (backend).
> > >
> > > offload_backend: adc0-backend {
> > >     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
> > >     compatible = "adi,pulsar-adc-offload";
> > >     #io-backend-cells = <0>;
> > >     dmas = <&dma 0>;
> > >     dma-names = "rx";
> > >     clocks = <&trigger_clock>;
> > > };
> > >
> > > spi {
> > >     ...
> > >     adc@0 {
> > >         ...
> > >         spi-offloads = <0>;
> > >         io-backends = <&offload_backend>;
> > >     };
> > > };
> > >
> > > While this could be a solution for IIO devices, this wouldn't solve
> > > the issue in general though for SPI offloads used with non-IIO
> > > peripherals.
> >
> > Yeah, I agree that using something specific to IIO isn't really a good
> > solution.
> >
> > Cheers,
> > Conor.
> >
> > > So I don't think it is the right thing to do here. But, I
> > > think this idea of a "composite" device helps explain why we are
> > > pushing for putting the "extras" with the peripheral node rather than
> > > the controller node, at least for the specific case of the AXI SPI
> > > Engine controller.
> >

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists