Date: Sun, 26 May 2024 18:35:07 +0100
From: Conor Dooley <conor@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: David Lechner <dlechner@...libre.com>, 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 Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:

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

Could you explain you think why making some additional processing done to
the data an io-backend is a stretch? Where else can this RTL be
represented? hint: it's not part of the ADC, just like how if you have
some custom RTL that does video processing that is not part of the
camera!

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

Can you explain what you mean by "same property on the peripheral"? I
would expect a peripheral to state its trigger period (just like how it
states the max frequency) and for the trigger period not to appear in
the controller.

I think a way that this could be modelled to reduce some software
complexity is considering that the periodic trigger is a clock, not
a PWM, provided you are only interested in the period. That'd give you
an interface that was less concerned about what the provider of the
periodic trigger is. After all, I doubt the ADC cares how you decide to
generate the trigger, as long as the periodicity is correct.
With the examples provided, you'd get something like:

pwm {
}

pclk {
	compatible = pwm-clock
	pwms = <&pwm 0 x>
}

spi {
	compatible = spi-engine
	clocks = <&clks SPI>, <&pwm>
	clock-names = "bus", "offload"
}

The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but
maybe you don't need that or maybe it could be added if needed.

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

I don't think you're overlooking things at all, I'm intentionally being
a bit difficult and ignoring what may be convenient for the adc driver.
This is being presented as a solution to a generic problem (and I think
you're right to do that), but it feels as if the one implementation is
all that's being considered here and I'm trying to ensure that what we
end up with doesn't make limiting assumptions.

Part of me says "sure, hook the DMAs up to the devices, as that's what
happens for other IIO devices" but at the same time I recognise that the
DMA isn't actually hooked up like that and the other IIO devices I see
like that are all actually on the SoC, rather than connected over SPI.
It might be easy to do it this way right now, but be problematic for a
future device or if someone wants to chuck away the ADI provided RTL and
do their own thing for this device. Really it just makes me wonder if
what's needed to describe more complex data pipelines uses an of_graph,
just like how video pipelines are handled, rather than the implementation
of io-backends that don't really seem to model the flow of data.

> > 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)?

Sure. But to assume that, for example, the provider of data processing
services also provided the periodic polling trigger would be incorrect
even if in the only currently existing case it does. Nothing is stopping
someone splitting those into different blocks, after all this is "random"
RTL in an FPGA :)

> 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

Yeah, they're a good reference, thanks.

I had to go AFK in the middle of this, I have a nagging feeling that I
forgot to say something but I cannot recall what.

Cheers,
Conor.

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

Powered by blists - more mailing lists