Received-SPF: pass (google.com: domain of linux-kernel+bounces-189775-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 26 May 2024 16:42:50 +0100
From: Conor Dooley <conor@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: Nuno =?iso-8859-1?Q?S=E1?= <noname.nuno@gmail.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
Subject: Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add
 spi-offloads property
Message-ID: <20240526-spotting-relapsing-ffb60b535c18@spud>
References: <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>
 <6e929426-25fa-4e91-8790-0774d59b34e0@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="38StDdanHQmO3L2P"
Content-Disposition: inline
In-Reply-To: <6e929426-25fa-4e91-8790-0774d59b34e0@baylibre.com>


--38StDdanHQmO3L2P
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, May 23, 2024 at 10:05:49AM -0500, David Lechner wrote:
> On 5/23/24 7:15 AM, Nuno S=C3=A1 wrote:
> > 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=E2=80=AFAM Conor Dooley <conor@kernel.or=
g> wrote:
> >>>>
> >>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> >>>>> On Thu, May 16, 2024 at 4:32=E2=80=AFPM Conor Dooley <conor@kernel.=
org> wrote:
> >>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner 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
> > 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 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 t=
he
> >> 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.
> >>
> >=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. T=
aking 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 f=
requency...
> > 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
> > 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
> >=20
> >> continuation:
> >> If offload engines have their own register region in the memory map,
> >=20
> >=20
> > Don't think it has it's own register region... David?
>=20
> I think the question here was if the CRC checker IP block (or descrambler=
 shown
> in the link below, or whatever) had registers in the offload/SPI controll=
er
> to control that extra part or if they had their own dedicated registers.

I don't think there was a question here at all. I was simply stating
that if the offload engine was not just a subordinate feature of the SPI
controller, but also provided additional data processing features then
treating the offload engine as a component of the SPI controller would
not be accurate.

> So far,
> these have been fixed-purpose, so have no registers at all. But I could s=
ee
> needing a register, e.g. for turning it on or off. In this case, I think =
it
> does become something like an io-backend. Or would we add this on/off swi=
tch
> to the AXI SPI Engine registers?

Seems to be that the CRC checking is a separate piece of IP though, and
so is not part of the offload engine at all, so my concern was
misplaced. I think whether or not you have registers to control it, it
should be represented in DT. How do you know it is there otherwise?

> Also, as shown in the link below, the extra bits share a clock domain wit=
h the
> AXI SPI Engine. So, yes, technically I suppose they could/should? be inde=
pendent
> consumers of the same clock like Conor suggests below. It does seems kind=
 of
> goofy if we have to write a driver just to turn on a clock that is already
> guaranteed to be on though.

You wouldn't necessarily need to write a driver for it, you could reach
out and turn it on from the backend consumer for example. And, obviously
there may be other ways that the FPGA design is configured where the
clock is not shared with the SPI controller or the offload engine.

--38StDdanHQmO3L2P
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZlNYegAKCRB4tDGHoIJi
0p//AP94S+X1TgXzvOP8jFQtnTOXCYT5iXNNaILC5azI/oPwmAEAhpLXc29r8igQ
eiVFFU2qSsUCwA+W1hiajgJZLdKtDQA=
=zON3
-----END PGP SIGNATURE-----

--38StDdanHQmO3L2P--