| 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
| ||
|
Message-ID: <7e40f4a5df7d6a594659ef1a7705be7d53368466.camel@gmail.com>
Date: Fri, 12 Jan 2024 14:19:59 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: David Lechner <dlechner@...libre.com>, Mark Brown <broonie@...nel.org>,
Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
<conor+dt@...nel.org>, Michael Hennerich <michael.hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>, Frank Rowand
<frowand.list@...il.com>, Thierry Reding <thierry.reding@...il.com>, Uwe
Kleine-König <u.kleine-koenig@...gutronix.de>, Jonathan
Corbet <corbet@....net>, linux-spi@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/13] iio: offload: add new PWM triggered DMA buffer
driver
On Fri, 2024-01-12 at 12:51 +0000, Jonathan Cameron wrote:
> On Thu, 11 Jan 2024 15:59:02 +0100
> Nuno Sá <noname.nuno@...il.com> wrote:
>
> > On Wed, 2024-01-10 at 13:49 -0600, David Lechner wrote:
> > > This adds a new driver for handling SPI offloading using a PWM as the
> > > trigger and DMA for the received data. This will be used by ADCs in
> > > conjunction with SPI controllers with offloading support to be able
> > > to sample at high rates without CPU intervention.
> > >
> > > Signed-off-by: David Lechner <dlechner@...libre.com>
> > > ---
> > > drivers/iio/Kconfig | 1 +
> > > drivers/iio/Makefile | 1 +
> > > .../iio/buffer/industrialio-hw-triggered-buffer.c | 1 +
> > > drivers/iio/offload/Kconfig | 21 ++
> > > drivers/iio/offload/Makefile | 2 +
> > > drivers/iio/offload/iio-pwm-triggered-dma-buffer.c | 212
> > > +++++++++++++++++++++
> > > 6 files changed, 238 insertions(+)
> > >
> > > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> > > index 52eb46ef84c1..56738282d82f 100644
> > > --- a/drivers/iio/Kconfig
> > > +++ b/drivers/iio/Kconfig
> > > @@ -90,6 +90,7 @@ source "drivers/iio/imu/Kconfig"
> > > source "drivers/iio/light/Kconfig"
> > > source "drivers/iio/magnetometer/Kconfig"
> > > source "drivers/iio/multiplexer/Kconfig"
> > > +source "drivers/iio/offload/Kconfig"
> > > source "drivers/iio/orientation/Kconfig"
> > > source "drivers/iio/test/Kconfig"
> > > if IIO_TRIGGER
> > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > index 9622347a1c1b..20acf5e1a4a7 100644
> > > --- a/drivers/iio/Makefile
> > > +++ b/drivers/iio/Makefile
> > > @@ -34,6 +34,7 @@ obj-y += imu/
> > > obj-y += light/
> > > obj-y += magnetometer/
> > > obj-y += multiplexer/
> > > +obj-y += offload/
> > > obj-y += orientation/
> > > obj-y += position/
> > > obj-y += potentiometer/
> > > diff --git a/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > index 7a8a71066b0e..a2fae6059616 100644
> > > --- a/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > +++ b/drivers/iio/buffer/industrialio-hw-triggered-buffer.c
> > > @@ -86,6 +86,7 @@ static int iio_hw_trigger_buffer_probe(struct
> > > auxiliary_device *adev,
> > > }
> > >
> > > static const struct auxiliary_device_id iio_hw_trigger_buffer_id_table[] = {
> > > + { .name = "pwm-triggered-dma-buffer.triggered-buffer" },
> > > { }
> > > };
> > > MODULE_DEVICE_TABLE(auxiliary, iio_hw_trigger_buffer_id_table);
> > > diff --git a/drivers/iio/offload/Kconfig b/drivers/iio/offload/Kconfig
> > > new file mode 100644
> > > index 000000000000..760c0cfe0e9c
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/Kconfig
> > > @@ -0,0 +1,21 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# SPI offload handlers for Industrial I/O
> > > +#
> > > +# When adding new entries keep the list in alphabetical order
> > > +
> > > +menu "SPI offload handlers"
> > > +
> > > +config IIO_PWM_TRIGGERED_DMA_BUFFER
> > > + tristate "PWM trigger and DMA buffer connected to SPI offload"
> > > + select AUXILIARY_BUS
> > > + select IIO_BUFFER_DMAENGINE
> > > + help
> > > + Provides a periodic hardware trigger via a PWM connected to the
> > > + trigger input of a SPI offload and a hardware buffer implemented
> > > + via DMA connected to the data output stream the a SPI offload.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called "iio-pwm-triggered-dma-buffer".
> > > +
> > > +endmenu
> > > diff --git a/drivers/iio/offload/Makefile b/drivers/iio/offload/Makefile
> > > new file mode 100644
> > > index 000000000000..7300ce82f066
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/Makefile
> > > @@ -0,0 +1,2 @@
> > > +
> > > +obj-$(CONFIG_IIO_PWM_TRIGGERED_DMA_BUFFER) := iio-pwm-triggered-dma-buffer.o
> > > diff --git a/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > b/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > new file mode 100644
> > > index 000000000000..970ea82316f6
> > > --- /dev/null
> > > +++ b/drivers/iio/offload/iio-pwm-triggered-dma-buffer.c
> > > @@ -0,0 +1,212 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Platform driver for a PWM trigger and DMA buffer connected to a SPI
> > > + * controller offload instance implementing the iio-hw-triggered-buffer
> > > + * interface.
> > > + *
> > > + * Copyright (C) 2023 Analog Devices, Inc.
> > > + * Copyright (C) 2023 BayLibre, SAS
> > > + */
> > > +
> > > +#include <linux/auxiliary_bus.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/hw_triggered_buffer_impl.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/buffer-dmaengine.h>
> > > +#include <linux/sysfs.h>
> > > +
> > > +struct iio_pwm_triggered_dma_buffer {
> > > + struct iio_hw_triggered_buffer_device hw;
> > > + struct pwm_device *pwm;
> > > +};
> > > +
> > > +static const struct iio_trigger_ops iio_pwm_triggered_dma_buffer_ops;
> > > +
> > > +static int iio_pwm_triggered_dma_buffer_set_state(struct iio_trigger *trig,
> > > bool state)
> > > +{
> > > + struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > +
> > > + if (state)
> > > + return pwm_enable(st->pwm);
> > > +
> > > + pwm_disable(st->pwm);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int iio_pwm_triggered_dma_buffer_validate_device(struct iio_trigger
> > > *trig,
> > > + struct iio_dev
> > > *indio_dev)
> > > +{
> > > + /* Don't allow assigning trigger via sysfs. */
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops iio_pwm_triggered_dma_buffer_ops = {
> > > + .set_trigger_state = iio_pwm_triggered_dma_buffer_set_state,
> > > + .validate_device = iio_pwm_triggered_dma_buffer_validate_device,
> > > +};
> > > +
> > > +static u32 axi_spi_engine_offload_pwm_trigger_get_rate(struct iio_trigger
> > > *trig)
> > > +{
> > > + struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > + u64 period_ns = pwm_get_period(st->pwm);
> > > +
> > > + if (period_ns)
> > > + return DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, period_ns);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +axi_spi_engine_offload_set_samp_freq(struct iio_pwm_triggered_dma_buffer *st,
> > > + u32 requested_hz)
> > > +{
> > > + int period_ns;
> > > +
> > > + if (requested_hz == 0)
> > > + return -EINVAL;
> > > +
> > > + period_ns = DIV_ROUND_UP(NSEC_PER_SEC, requested_hz);
> > > +
> > > + /*
> > > + * FIXME: We really just need a clock, not a PWM. The current duty
> > > cycle
> > > + * value is a hack to work around the edge vs. level offload trigger
> > > + * issue in the ADI AXI SPI Engine firmware.
> > > + */
> > > + return pwm_config(st->pwm, 10, period_ns);
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_show(struct device *dev,
> > > + struct device_attribute *attr, char
> > > *buf)
> > > +{
> > > + struct iio_trigger *trig = to_iio_trigger(dev);
> > > +
> > > + return sysfs_emit(buf, "%u\n",
> > > + axi_spi_engine_offload_pwm_trigger_get_rate(trig));
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t len)
> > > +{
> > > + struct iio_trigger *trig = to_iio_trigger(dev);
> > > + struct iio_pwm_triggered_dma_buffer *st =
> > > iio_trigger_get_drvdata(trig);
> > > + int ret;
> > > + u32 val;
> > > +
> > > + ret = kstrtou32(buf, 10, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = axi_spi_engine_offload_set_samp_freq(st, val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return len;
> > > +}
> > > +
> >
> > This is also something that made me wonder... In the end of the day, the
> > frequency at which we want to sample the data depends on the converter device.
> > This completely detached interface makes it more easy for the user to screw up
> > things, right?
> It's easy to do that anyway :) If you think of a normal SPI attached device that
> has it's own internal clocking then it's usually easy to set the device to grab
> data faster than than the spi bus can drain it. We drop samples.
> Here it might make sense to add some bounds I guess as it's all in hardware
> - either have the ADC provide them or push it to DT,
Indeed... DT would likely be simpler
>
> >
> > Things might even become more complicated in usecases where we have an
> > additional pwm (on top of the data fetch trigger one) for triggering conversions
> > in the converter and we might need to properly control the phase between those
> > two signals. So, how would we handle it in the current form? We have one pwm
> > belonging to the offload engine and one belonging to the converter but they need
> > to cope together...
>
> Groan loudly?
> If someone wants careful sync between a trigger for the ADC and the read back
> triggering
> they should do it in hardware. Sure we can do it if the pwm is sophisticated enough
> but the mess of layering etc needed to make it work is just nasty.
>
> I guess you make a custom trigger that bakes in your requirements.
>
Yeah, we have yet another FPGA soft core implementing a pwm controller (also to be
upstreamed soon) which can output multiple signals at different phases, frequency,
etc...
- Nuno Sá
Powered by blists - more mailing lists