linux-kernel - Re: [PATCH RFC v2 6/8] spi: axi-spi-engine: add offload support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 21 May 2024 09:28:54 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: 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 6/8] spi: axi-spi-engine: add offload support

On Tue, May 21, 2024 at 7:27 AM Nuno Sá <noname.nuno@...il.com> wrote:
>
> 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 hardware
> > 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 will
> > fail.
> >
> > Signed-off-by: David Lechner <dlechner@...libre.com>
> > ---
> >

..

> > +
> > +static int spi_engine_offload_map_channel(struct spi_device *spi,
> > +                                       unsigned int id,
> > +                                       unsigned int channel)
> > +{
> > +     struct spi_controller *host = spi->controller;
> > +     struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > +     struct spi_engine_offload *priv;
> > +
> > +     if (channel >= SPI_ENGINE_MAX_NUM_OFFLOADS)
> > +             return -EINVAL;
> > +
> > +     priv = &spi_engine->offload_priv[channel];
> > +
> > +     if (priv->spi)
> > +             return -EBUSY;
>
> I wonder if we need to be this strict? Is there any problem by having two
> devices requesting the same offload engine? I would expect that having multiple
> peripherals trying to actually use it at the same time (with the prepare()
> callback) to be problematic but if they play along it could actually work,
> right? In reality that may never be a realistic usecase so this is likely fine.
>

I guess not. But to keep it simple for now, yeah, let's wait until we
have an actual use case.

..

> > +
> > +static const struct spi_controller_offload_ops spi_engine_offload_ops = {
> > +     .map_channel = spi_engine_offload_map_channel,
> > +     .prepare = spi_engine_offload_prepare,
> > +     .unprepare = spi_engine_offload_unprepare,
> > +     .hw_trigger_enable = spi_engine_offload_enable,
> > +     .hw_trigger_disable = spi_engine_offload_disable,
>
> 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...

It isn't acting as the trigger, just configuring the offload instance
for exclusive use by a hardware trigger.

> Maybe these callbacks could be used to enable/disable the
> actual trigger of the offload engine (in our current cases, the PWM)? So this
> would make it easy to move the trigger DT property where it belongs. The DMA one
> (given it's tight relation with IIO DMA buffers) is another (way more difficult)
> story I think.
>

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?