Received-SPF: pass (google.com: domain of linux-kernel+bounces-193696-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <10991373cb9603803df63d8236c475807f6dde68.camel@gmail.com>
Subject: Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add
 spi-offloads property
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Conor Dooley <conor@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>, Mark Brown <broonie@kernel.org>, 
 Jonathan Cameron
	 <jic23@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski
	 <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Nuno
 =?ISO-8859-1?Q?S=E1?=
	 <nuno.sa@analog.com>, Michael Hennerich <Michael.Hennerich@analog.com>, 
 Lars-Peter Clausen
	 <lars@metafoo.de>, David Jander <david@protonic.nl>, Martin Sperl
	 <kernel@martin.sperl.org>, linux-spi@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-iio@vger.kernel.org
Date: Wed, 29 May 2024 10:07:37 +0200
In-Reply-To: <20240526-peculiar-panama-badda4f02336@spud>
References: <20240513-headsman-hacking-d51fcc811695@spud>
	 <CAMknhBE5XJzhdJ=PQUXiubw_CiCLcn1jihiscnQZUzDWMASPKw@mail.gmail.com>
	 <20240514-aspire-ascension-449556da3615@spud>
	 <CAMknhBFFpEGcMoLo5gsC11Syv+CwUM0mnq1yDMUzL1uutUtB+Q@mail.gmail.com>
	 <20240516-rudder-reburial-dcf300504c0a@spud>
	 <CAMknhBF_s0btus4yqPe-T=F3z7Asi9KkRGsGr7FHDFi=k4EQjw@mail.gmail.com>
	 <20240519-abreast-haziness-096a57ef57d3@spud>
	 <CAMknhBHvEse2FyDoBXR1PvymGpSGq8dotKfm+8XH+0+k+xKtQw@mail.gmail.com>
	 <20240522-gullible-ibuprofen-cf9111c25f6f@spud>
	 <5ad0b5782434eaf4cf565cffb0e4c14b7414ae38.camel@gmail.com>
	 <20240526-peculiar-panama-badda4f02336@spud>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 (3.52.1-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno S=C3=A1 wrote:
> > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
>=20
> > >=20
> > > 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.
> > >=20
> >=20
> > I think the data manipulation is not really a property of the engine. T=
ypically
> > data
> > going out of the offload engine goes into another "data reorder" block =
that is
> > pure
> > HW.
> >=20
> > > I think you're right something like that is a stretch to say that tha=
t
> > > 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 ther=
e
> > > seem like something that should/could be modelled as one, with its ow=
n
> > > 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.
> > >=20
> >=20
> > 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.
>=20
> 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 vid=
eo
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 thi=
s point.
Or maybe there's already a future usecase that I'm not aware about (maybe t=
he CRC
stuff David mentioned).

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

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

>=20
> > 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 samplin=
g
> > frequency of
> > the peripheral for example. So I see 2 things:
> >=20
> > 1) Enabling/Disabling the trigger could be easily done from the periphe=
ral 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 cont=
roller
> > brings
> > no added complexity.
> >=20
> > 2) Controlling things like the trigger period/sample_rate. This could b=
e harder
> > to do
> > over SPI (or making it generic enough) so we would still need to have t=
he same
> > property on the peripheral (even if not directly connected to it). I ki=
nd of
> > agree
> > with David that having the property both in the peripheral and controll=
er is a
> > bit
> > weird.
>=20
> 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.
>=20

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

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 of=
fset
(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 {
> }
>=20
> pclk {
> 	compatible =3D pwm-clock
> 	pwms =3D <&pwm 0 x>
> }
>=20
> spi {
> 	compatible =3D spi-engine
> 	clocks =3D <&clks SPI>, <&pwm>
> 	clock-names =3D "bus", "offload"
> }
>=20
> 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.
>=20
> > 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 inter=
esting at
> > the
> > very least. Note that the DMA block is not really something that is par=
t 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 alrea=
dy have a
> > DMA
> > buffer interface so users of the peripheral can get the data without an=
y
> > intervention
> > of the driver (on the data). We "just" enable buffering and then everyt=
hing
> > happens
> > on HW and userspace can start requesting data. If we are going to attac=
h the DMA
> > in
> > the controller, I have no idea how we can handle it. Moreover, the offl=
oad 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.
> >=20
> > But maybe I'm overlooking things... And thinking more in how this can b=
e done in
> > SW
> > rather than what makes sense from an HW perspective.
>=20
> 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 someth=
ing that
only fits a couple usecases.

>=20
> 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 rol=
e 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 comple=
x to share
buffers (using dma-buf or something else) from the spi controller back to c=
onsumers
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.
>=20

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 connecti=
ng to high
speed controllers. Note that in some cases they also manipulate or even cre=
ate data
but since they fit in IIO, having things like the DMA property in the hdl b=
inding was
fairly straight.

Maybe having an offload dedicated API (through spi) to get/share a DMA hand=
le would
be acceptable. Then we could add support to "import" it in the IIO core. Th=
en 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 som=
ething
worth looking at.

- Nuno S=C3=A1
> >=20