Received-SPF: pass (google.com: domain of linux-kernel+bounces-186191-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <fbcc8bb3804d0f1f178f695cc16a1aa8dbcda1f1.camel@gmail.com>
Subject: Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>
Cc: 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, 22 May 2024 14:08:12 +0200
In-Reply-To: <CAMknhBH9y=tOhHrhBCoMOSSVgZDRbX90cfzqX62m6wLYsKDhNg@mail.gmail.com>
References: 
	<20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com>
	 <20240510-dlech-mainline-spi-engine-offload-2-v2-6-8707a870c435@baylibre.com>
	 <6c5fd2cef9a6412e63f2534243eda37c321ffcd2.camel@gmail.com>
	 <CAMknhBH9y=tOhHrhBCoMOSSVgZDRbX90cfzqX62m6wLYsKDhNg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-05-21 at 09:28 -0500, David Lechner wrote:
> On Tue, May 21, 2024 at 7:27=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail.c=
om> wrote:
> >=20
> > On Fri, 2024-05-10 at 19:44 -0500, David Lechner wrote:
> > > This implements SPI offload support for the AXI SPI Engine. Currently=
,
> > > the hardware only supports triggering offload transfers with a hardwa=
re
> > > trigger so attempting to use an offload message in the regular SPI
> > > message queue will fail. Also, only allows streaming rx data to an
> > > external sink, so attempts to use a rx_buf in the offload message wil=
l
> > > fail.
> > >=20
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >=20
>=20
> ...
>=20
> > > +
> > > +static int spi_engine_offload_map_channel(struct spi_device *spi,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 unsigned int id,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 unsigned int channel)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 struct spi_controller *host =3D spi->contro=
ller;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 struct spi_engine *spi_engine =3D spi_contr=
oller_get_devdata(host);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 struct spi_engine_offload *priv;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (channel >=3D SPI_ENGINE_MAX_NUM_OFFLOAD=
S)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return -EINVAL;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 priv =3D &spi_engine->offload_priv[channel]=
;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 if (priv->spi)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 return -EBUSY;
> >=20
> > I wonder if we need to be this strict? Is there any problem by having t=
wo
> > devices requesting the same offload engine? I would expect that having =
multiple
> > peripherals trying to actually use it at the same time (with the prepar=
e()
> > callback) to be problematic but if they play along it could actually wo=
rk,
> > right? In reality that may never be a realistic usecase so this is like=
ly fine.
> >=20
>=20
> I guess not. But to keep it simple for now, yeah, let's wait until we
> have an actual use case.
>=20

Agreed.

> ...
>=20
> > > +
> > > +static const struct spi_controller_offload_ops spi_engine_offload_op=
s =3D {
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 .map_channel =3D spi_engine_offload_map_cha=
nnel,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 .prepare =3D spi_engine_offload_prepare,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 .unprepare =3D spi_engine_offload_unprepare=
,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 .hw_trigger_enable =3D spi_engine_offload_e=
nable,
> > > +=C2=A0=C2=A0=C2=A0=C2=A0 .hw_trigger_disable =3D spi_engine_offload_=
disable,
> >=20
> > I guess this is what you and Conor are already somehow discussing but I=
 would
> > expect this to be the actual offload trigger to play a spi transfer. As=
 it
> > stands, it looks weird (or confusing) to have the enable/disable of the=
 engine
> > to act as a trigger...
>=20
> It isn't acting as the trigger, just configuring the offload instance
> for exclusive use by a hardware trigger.
>=20
> > Maybe these callbacks could be used to enable/disable the
> > actual trigger of the offload engine (in our current cases, the PWM)? S=
o this
> > would make it easy to move the trigger DT property where it belongs. Th=
e DMA one
> > (given it's tight relation with IIO DMA buffers) is another (way more d=
ifficult)
> > story I think.
> >=20
>=20
> One issue I have with making the actual hardware trigger part of the
> SPI controller is that in some cases, the peripheral could actually be
> the trigger. For example, in the case of a self-clocked ADC where
> there is just a RDY signal from the ADC when sample data is ready to
> be read. In this case would the peripheral have to register a trigger
> enable callback with the controller so that the controller can
> communicate with the peripheral to enable and disable sampling mode,
> and therefore the trigger?

In those cases, the peripheral would not bother in doing any spi hardware t=
riggering
enable/disable (this assumes that we would need explicit interfaces for off=
load
enable/disable). In DT, the engine would also have no trigger has it's not =
required
so the controller even conditionally register these callbacks.

As for the DMA, I have no clue how we can have it associated with the contr=
oller
given how we want the data to be exported to userspace. Maybe dma-buf could=
 be used
but it won't be easy. TBH, I'm not sure if it's that bad having the DMA ass=
ociated
with the peripheral since it's purpose is to transfer peripheral DATA (not =
like a spi
controller DMA used for the actual SPI transfers).

- Nuno S=C3=A1