Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> 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!

Maybe we are speaking about two different things... I do agree with the video
processing example you gave but for this case I'm not sure there#s any data
manipulation involved. i mean, there is but nothing controlled by SW at this point.
Or maybe there's already a future usecase that I'm not aware about (maybe the CRC
stuff David mentioned).

I'm more focusing on where the trigger (PWMS in this particular case but could be
something else) and the DMA properties belong. I do agree that, Hardware wise, the
trigger is a property of the offload engine even though intrinsically connected with
the peripheral.

The DMA is also directly connected to the offload but I'm not sure if in this case we
should say it's a property of it? It's an external block that we do not control at
all and the data is to be consumed by users of the peripheral.

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

Just have the same 'pwms' property on both the controller and peripheral...

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

Unfortunately that's not the case. For instance, in the design on the link I gave you
on the last reply we do have an averaging mode where we actually need an offset
(effort for supporting that in PWM is already ongoing) between the offload trigger
and the peripheral conversion signal (so assuming we only care about period will fail
pretty soon :)).

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

Yeah, I know and I do agree we need something generic enough and not something that
only fits a couple usecases.

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

Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
solely for carrying the peripheral data. It is done like this so everything works in
HW and there's no need for SW to deal with the samples at all. I mean, only the
userspace app touches the samples.

TBH, the DMA is the bit that worries me the most as it may be overly complex to share
buffers (using dma-buf or something else) from the spi controller back to consumers
of it (IIO in this case). And I mean sharing in a way that there's no need to touch
the buffers.

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

Yeah, backends is more for devices/soft-cores that extend the functionality of the
device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
speed controllers. Note that in some cases they also manipulate or even create data
but since they fit in IIO, having things like the DMA property in the hdl binding was
fairly straight.

Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
be acceptable. Then we could add support to "import" it in the IIO core. Then it
would be up to the controller to accept or not to share the handle (in some cases the
controller could really want to have the control of the DMA transfers).

Not familiar enough with of_graph so can't argue about it but likely is something
worth looking at.

- Nuno Sá
> >