Message-ID: <5ad0b5782434eaf4cf565cffb0e4c14b7414ae38.camel@gmail.com>
Date: Thu, 23 May 2024 14:15:35 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Conor Dooley <conor@...nel.org>, 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 Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> 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 the data manipulation is not really a property of the engine. Typically data
going out of the offload engine goes into another "data reorder" block that is pure
HW.

> 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.
> 

Putting this in io-backends is likely a stretch but one thing to add is that the
peripheral is always (I think) kind of the consumer of the resources. Taking the
trigger (PWM) as an example and even when it is directly connected with the offload
block, the peripheral still needs to know about it. Think of sampling frequency...
The period of the trigger signal is strictly connected with the sampling frequency of
the peripheral for example. So I see 2 things:

1) Enabling/Disabling the trigger could be easily done from the peripheral even with
the resource in the spi engine. I think David already has some code in the series
that would make this trivial and so having the property in the spi controller brings
no added complexity.

2) Controlling things like the trigger period/sample_rate. This could be harder to do
over SPI (or making it generic enough) so we would still need to have the same
property on the peripheral (even if not directly connected to it). I kind of agree
with David that having the property both in the peripheral and controller is a bit
weird.

And the DMA block is a complete different story. Sharing that data back with the
peripheral driver (in this case, the IIO subsystem) would be very interesting at the
very least. Note that the DMA block is not really something that is part of the
controller nor the offload block. It is an external block that gets the data coming
out of the offload engine (or the data reorder block). In IIO, we already have a DMA
buffer interface so users of the peripheral can get the data without any intervention
of the driver (on the data). We "just" enable buffering and then everything happens
on HW and userspace can start requesting data. If we are going to attach the DMA in
the controller, I have no idea how we can handle it. Moreover, the offload it's
really just a way of replaying the same spi transfer over and over and that happens
in HW so I'm not sure how we could "integrate" that with dmaengine.

But maybe I'm overlooking things... And thinking more in how this can be done in SW
rather than what makes sense from an HW perspective.


> continuation:
> If offload engines have their own register region in the memory map,


Don't think it has it's own register region... David?

> 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...
> 

But then it would be valid for both peripheral and controller to reference that
phandle right (and get the resources of interest)?

FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
to you:

https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl

- Nuno Sá

Powered by blists - more mailing lists