| Message ID | 20241023-dlech-mainline-spi-engine-offload-2-v4-0-f8125b99f5a1@baylibre.com |
|---|---|
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Thu, 2024-10-24 at 10:02 -0500, David Lechner wrote: > On 10/24/24 9:04 AM, Nuno Sá wrote: > > On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: > > > Extend SPI offloading to support hardware triggers. > > > > > > This allows an arbitrary hardware trigger to be used to start a SPI > > > transfer that was previously set up with spi_optimize_message(). > > > > > > A new struct spi_offload_trigger is introduced that can be used to > > > configure any type of trigger. It has a type discriminator and a union > > > to allow it to be extended in the future. Two trigger types are defined > > > to start with. One is a trigger that indicates that the SPI peripheral > > > is ready to read or write data. The other is a periodic trigger to > > > repeat a SPI message at a fixed rate. > > > > > > There is also a spi_offload_hw_trigger_validate() function that works > > > similar to clk_round_rate(). It basically asks the question of if we > > > enabled the hardware trigger what would the actual parameters be. This > > > can be used to test if the requested trigger type is actually supported > > > by the hardware and for periodic triggers, it can be used to find the > > > actual rate that the hardware is capable of. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > > > > In previous versions, we locked the SPI bus when the hardware trigger > > > was enabled, but we found this to be too restrictive. In one use case, > > > to avoid a race condition, we need to enable the SPI offload via a > > > hardware trigger, then write a SPI message to the peripheral to place > > > it into a mode that will generate the trigger. If we did it the other > > > way around, we could miss the first trigger. > > > > > > Another likely use case will be enabling two offloads/triggers at one > > > time on the same device, e.g. a read trigger and a write trigger. So > > > the exclusive bus lock for a single trigger would be too restrictive in > > > this case too. > > > > > > So for now, I'm going with Nuno's suggestion to leave any locking up to > > > the individual controller driver. If we do find we need something more > > > generic in the future, we could add a new spi_bus_lock_exclusive() API > > > that causes spi_bus_lock() to fail instead of waiting and add "locked" > > > versions of trigger enable functions. This would allow a peripheral to > > > claim exclusive use of the bus indefinitely while still being able to > > > do any SPI messaging that it needs. > > > > > > v4 changes: > > > * Added new struct spi_offload_trigger that is a generic struct for any > > > hardware trigger rather than returning a struct clk. > > > * Added new spi_offload_hw_trigger_validate() function. > > > * Dropped extra locking since it was too restrictive. > > > > > > v3 changes: > > > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > > > * added spi_offload_hw_trigger_get_clk() function > > > * fixed missing EXPORT_SYMBOL_GPL > > > > > > v2 changes: > > > * This is split out from "spi: add core support for controllers with > > > offload capabilities". > > > * Added locking for offload trigger to claim exclusive use of the SPI > > > bus. > > > --- > > > drivers/spi/spi-offload.c | 266 ++++++++++++++++++++++++++++++++++++++++ > > > include/linux/spi/spi-offload.h | 78 ++++++++++++ > > > 2 files changed, 344 insertions(+) > > > > > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > > > index c344cbf50bdb..2a1f9587f27a 100644 > > > --- a/drivers/spi/spi-offload.c > > > +++ b/drivers/spi/spi-offload.c > > > @@ -9,12 +9,26 @@ > > > #include <linux/cleanup.h> > > > #include <linux/device.h> > > > #include <linux/export.h> > > > +#include <linux/list.h> > > > #include <linux/mutex.h> > > > +#include <linux/of.h> > > > #include <linux/property.h> > > > #include <linux/spi/spi-offload.h> > > > #include <linux/spi/spi.h> > > > #include <linux/types.h> > > > > > > +struct spi_offload_trigger { > > > + struct list_head list; > > > + struct device dev; > > > + /* synchronizes calling ops and driver registration */ > > > + struct mutex lock; > > > + const struct spi_offload_trigger_ops *ops; > > > + void *priv; > > > +}; > > > + > > > +static LIST_HEAD(spi_offload_triggers); > > > +static DEFINE_MUTEX(spi_offload_triggers_lock); > > > + > > > /** > > > * devm_spi_offload_alloc() - Allocate offload instances > > > * @dev: Device for devm purposes > > > @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device > > > *dev, > > > return offload; > > > } > > > EXPORT_SYMBOL_GPL(devm_spi_offload_get); > > > + > > > +static void spi_offload_trigger_release(void *data) > > > +{ > > > + struct spi_offload_trigger *trigger = data; > > > + > > > + guard(mutex)(&trigger->lock); > > > + if (trigger->priv && trigger->ops->release) > > > + trigger->ops->release(trigger->priv); > > > + > > > + put_device(&trigger->dev); > > > +} > > > + > > > +struct spi_offload_trigger > > > +*devm_spi_offload_trigger_get(struct device *dev, > > > + struct spi_offload *offload, > > > + enum spi_offload_trigger_type type) > > > +{ > > > + struct spi_offload_trigger *trigger; > > > + struct fwnode_reference_args args; > > > + bool match = false; > > > + int ret; > > > + > > > + ret = fwnode_property_get_reference_args(dev_fwnode(offload- > > > > provider_dev), > > > + "trigger-sources", > > > + "#trigger-source-cells", 0, > > > 0, > > > + &args); > > > + if (ret) > > > + return ERR_PTR(ret); > > > + > > > + struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = > > > args.fwnode; > > > + > > > + guard(mutex)(&spi_offload_triggers_lock); > > > + > > > + list_for_each_entry(trigger, &spi_offload_triggers, list) { > > > + if (trigger->dev.fwnode != args.fwnode) > > > + continue; > > > + > > > + match = trigger->ops->match(trigger->priv, type, args.args, > > > args.nargs); > > > + if (match) > > > + break; > > > + } > > > + > > > + if (!match) > > > + return ERR_PTR(-EPROBE_DEFER); > > > + > > > + guard(mutex)(&trigger->lock); > > > + > > > + if (!trigger->priv) > > > + return ERR_PTR(-ENODEV); > > > > This is a bit odd tbh. Not a real deal breaker for me but the typical pattern I > > would > > expect is for methods of the trigger to get a struct spi_offload_trigger opaque > > pointer. Then we provide a get_private kind of API for the private data. I guess > > you > > want to avoid that but IMO it makes for neater API instead of getting void > > pointers. > > I was just trying to save a step of an extra call to get *priv > in each callback implementation, but yeah, no problem to change > it to something more "normal" looking. Yeah, I figured that but I guess any of these paths are fastpaths anyways... > > > > > Another thing is, can the above actually happen? We have the > > spi_offload_triggers_lock grabbed and we got a match so the trigger should not be > > able to go away (should block on the same lock). > > The problem is that it could have gone away before we took the lock. > > It could happen like this: > > * Trigger driver registers trigger - sets *priv. > * SPI peripheral driver gets reference to trigger. > * Trigger driver unregisters trigger - removes *priv. > * SPI peripheral tries to call trigger function. > Ah I see... we're using scoped_guard() in the unregister path. - Nuno Sá >
On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: > Most configuration of SPI offloads is handled opaquely using the offload > pointer that is passed to the various offload functions. However, there > are some offload features that need to be controlled on a per transfer > basis. > > This patch adds a flag field to struct spi_transfer to allow specifying > such features. The first feature to be added is the ability to stream > data to/from a hardware sink/source rather than using a tx or rx buffer. > Additional flags can be added in the future as needed. > > A flags field is also added to the offload struct for providers to > indicate which flags are supported. This allows for generic checking of > offload capabilities during __spi_validate() so that each offload > provider doesn't have to implement their own validation. > > As a first users of this streaming capability, getter functions are > added to get a DMA channel that is directly connected to the offload. > Peripheral drivers will use this to get a DMA channel and configure it > to suit their needs. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v4 changes: > * DMA API's now automatically release DMA channels instead of leaving > it up to the caller. > > v3 changes: > * Added spi_offload_{tx,rx}_stream_get_dma_chan() functions. > > v2 changes: > * This is also split out from "spi: add core support for controllers with > offload capabilities". > * In the previous version, we were using (void *)-1 as a sentinel value > that could be assigned, e.g. to rx_buf. But this was naive since there > is core code that would try to dereference this pointer. So instead, > we've added a new flags field to the spi_transfer structure for this > sort of thing. This also has the advantage of being able to be used in > the future for other arbitrary features. > --- > drivers/spi/spi-offload.c | 76 +++++++++++++++++++++++++++++++++++++++++ > drivers/spi/spi.c | 10 ++++++ > include/linux/spi/spi-offload.h | 24 +++++++++++++ > include/linux/spi/spi.h | 3 ++ > 4 files changed, 113 insertions(+) > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > index 2a1f9587f27a..dd4cb3c2e985 100644 > --- a/drivers/spi/spi-offload.c > +++ b/drivers/spi/spi-offload.c > @@ -8,6 +8,7 @@ > > #include <linux/cleanup.h> > #include <linux/device.h> > +#include <linux/dmaengine.h> > #include <linux/export.h> > #include <linux/list.h> > #include <linux/mutex.h> > @@ -282,6 +283,81 @@ void spi_offload_trigger_disable(struct spi_offload *offload, > } > EXPORT_SYMBOL_GPL(spi_offload_trigger_disable); > > +static void spi_offload_release_dma_chan(void *chan) > +{ > + dma_release_channel(chan); > +} > + > +/** > + * spi_offload_tx_stream_request_dma_chan_info - Get the DMA channel info for the > TX stream > + * @spi: SPI device > + * @id: Function ID if SPI device uses more than one offload or NULL. > + * > + * This is the DMA channel that will provide data to transfers that use the > + * %SPI_OFFLOAD_XFER_TX_STREAM offload flag. > + * > + * The caller is responsible for calling spi_offload_free_dma_chan_info() on the > + * returned pointer. I guess the above does not make sense now. But I would still document (just to really make it explicit) that the lifetime of the DMA channel is effectively being handed over to the consumer. - Nuno Sá
On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: > Implement 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@baylibre.com> > --- > > v4 changes: > * Adapted to changes in other patches in the series. > * Moved trigger enable/disable to same function as offload > enable/disable. > > v3 changes: > * Added clk and dma_chan getter callbacks. > * Fixed some bugs. > > v2 changes: > > This patch has been reworked to accommodate the changes described in all > of the other patches. > --- > drivers/spi/Kconfig | 1 + > drivers/spi/spi-axi-spi-engine.c | 273 ++++++++++++++++++++++++++++++++++++++- > 2 files changed, 268 insertions(+), 6 deletions(-) > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > index 50d04fa317b7..af3143ec5245 100644 > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -168,6 +168,7 @@ config SPI_AU1550 > config SPI_AXI_SPI_ENGINE > tristate "Analog Devices AXI SPI Engine controller" > depends on HAS_IOMEM > + select SPI_OFFLOAD > help > This enables support for the Analog Devices AXI SPI Engine SPI > controller. > It is part of the SPI Engine framework that is used in some Analog > Devices > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c > index 2d24d762b5bd..1710847d81a1 100644 > --- a/drivers/spi/spi-axi-spi-engine.c > +++ b/drivers/spi/spi-axi-spi-engine.c > @@ -2,11 +2,14 @@ > /* > * SPI-Engine SPI controller driver > * Copyright 2015 Analog Devices Inc. > + * Copyright 2024 BayLibre, SAS > * Author: Lars-Peter Clausen <lars@metafoo.de> > */ > > +#include <linux/bitops.h> > #include <linux/clk.h> > #include <linux/completion.h> > +#include <linux/dmaengine.h> > #include <linux/fpga/adi-axi-common.h> > #include <linux/interrupt.h> > #include <linux/io.h> > @@ -14,8 +17,10 @@ > #include <linux/module.h> > #include <linux/overflow.h> > #include <linux/platform_device.h> > +#include <linux/spi/spi-offload.h> > #include <linux/spi/spi.h> > ... > +#define SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH 0x10 > #define SPI_ENGINE_REG_RESET 0x40 > > #define SPI_ENGINE_REG_INT_ENABLE 0x80 > @@ -23,6 +28,7 @@ > #define SPI_ENGINE_REG_INT_SOURCE 0x88 > > #define SPI_ENGINE_REG_SYNC_ID 0xc0 > +#define SPI_ENGINE_REG_OFFLOAD_SYNC_ID 0xc4 > > #define SPI_ENGINE_REG_CMD_FIFO_ROOM 0xd0 > #define SPI_ENGINE_REG_SDO_FIFO_ROOM 0xd4 > @@ -33,10 +39,24 @@ > #define SPI_ENGINE_REG_SDI_DATA_FIFO 0xe8 > #define SPI_ENGINE_REG_SDI_DATA_FIFO_PEEK 0xec > > +#define SPI_ENGINE_MAX_NUM_OFFLOADS 32 > + > +#define SPI_ENGINE_REG_OFFLOAD_CTRL(x) (0x100 + > SPI_ENGINE_MAX_NUM_OFFLOADS * (x)) > +#define SPI_ENGINE_REG_OFFLOAD_STATUS(x) (0x104 + > SPI_ENGINE_MAX_NUM_OFFLOADS * (x)) > +#define SPI_ENGINE_REG_OFFLOAD_RESET(x) (0x108 + > SPI_ENGINE_MAX_NUM_OFFLOADS * (x)) > +#define SPI_ENGINE_REG_OFFLOAD_CMD_FIFO(x) (0x110 + > SPI_ENGINE_MAX_NUM_OFFLOADS * (x)) > +#define SPI_ENGINE_REG_OFFLOAD_SDO_FIFO(x) (0x114 + > SPI_ENGINE_MAX_NUM_OFFLOADS * (x)) > + > +#define SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_SDO GENMASK(15, 8) > +#define SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_CMD GENMASK(7, 0) > + > #define SPI_ENGINE_INT_CMD_ALMOST_EMPTY BIT(0) > #define SPI_ENGINE_INT_SDO_ALMOST_EMPTY BIT(1) > #define SPI_ENGINE_INT_SDI_ALMOST_FULL BIT(2) > #define SPI_ENGINE_INT_SYNC BIT(3) > +#define SPI_ENGINE_INT_OFFLOAD_SYNC BIT(4) > + > +#define SPI_ENGINE_OFFLOAD_CTRL_ENABLE BIT(0) > > #define SPI_ENGINE_CONFIG_CPHA BIT(0) > #define SPI_ENGINE_CONFIG_CPOL BIT(1) > @@ -78,6 +98,14 @@ > #define SPI_ENGINE_CMD_CS_INV(flags) \ > SPI_ENGINE_CMD(SPI_ENGINE_INST_CS_INV, 0, (flags)) > > +/* default sizes - can be changed when SPI Engine firmware is compiled */ > +#define SPI_ENGINE_OFFLOAD_CMD_FIFO_SIZE 16 > +#define SPI_ENGINE_OFFLOAD_SDO_FIFO_SIZE 16 > + > +#define SPI_ENGINE_OFFLOAD_CAPS (SPI_OFFLOAD_CAP_TRIGGER | \ > + SPI_OFFLOAD_CAP_TX_STATIC_DATA | \ > + SPI_OFFLOAD_CAP_RX_STREAM_DMA) > + > struct spi_engine_program { > unsigned int length; > uint16_t instructions[] __counted_by(length); > @@ -105,6 +133,16 @@ struct spi_engine_message_state { > uint8_t *rx_buf; > }; > > +enum { > + SPI_ENGINE_OFFLOAD_FLAG_PREPARED, > +}; > + > +struct spi_engine_offload { > + struct spi_engine *spi_engine; > + unsigned long flags; > + unsigned int offload_num; > +}; > + > struct spi_engine { > struct clk *clk; > struct clk *ref_clk; > @@ -117,6 +155,11 @@ struct spi_engine { > unsigned int int_enable; > /* shadows hardware CS inversion flag state */ > u8 cs_inv; > + > + unsigned int offload_ctrl_mem_size; > + unsigned int offload_sdo_mem_size; > + struct spi_offload *offloads; > + unsigned int num_offloads; > }; > > static void spi_engine_program_add_cmd(struct spi_engine_program *p, > @@ -164,7 +207,7 @@ static void spi_engine_gen_xfer(struct spi_engine_program *p, > bool dry, > > if (xfer->tx_buf) > flags |= SPI_ENGINE_TRANSFER_WRITE; > - if (xfer->rx_buf) > + if (xfer->rx_buf || (xfer->offload_flags & > SPI_OFFLOAD_XFER_RX_STREAM)) > flags |= SPI_ENGINE_TRANSFER_READ; > > spi_engine_program_add_cmd(p, dry, > @@ -220,16 +263,24 @@ static void spi_engine_gen_cs(struct spi_engine_program *p, > bool dry, > * > * NB: This is separate from spi_engine_compile_message() because the latter > * is called twice and would otherwise result in double-evaluation. > + * > + * Returns 0 on success, -EINVAL on failure. > */ > -static void spi_engine_precompile_message(struct spi_message *msg) > +static int spi_engine_precompile_message(struct spi_message *msg) > { > unsigned int clk_div, max_hz = msg->spi->controller->max_speed_hz; > struct spi_transfer *xfer; > > list_for_each_entry(xfer, &msg->transfers, transfer_list) { > + /* If we have an offload transfer, we can't rx to buffer */ > + if (msg->offload && xfer->rx_buf) > + return -EINVAL; > + > clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz); > xfer->effective_speed_hz = max_hz / min(clk_div, 256U); > } > + > + return 0; > } > > static void spi_engine_compile_message(struct spi_message *msg, bool dry, > @@ -544,11 +595,94 @@ static irqreturn_t spi_engine_irq(int irq, void *devid) > return IRQ_HANDLED; > } > > +static int spi_engine_offload_prepare(struct spi_message *msg) > +{ > + struct spi_controller *host = msg->spi->controller; > + struct spi_engine *spi_engine = spi_controller_get_devdata(host); > + struct spi_engine_program *p = msg->opt_state; > + struct spi_engine_offload *priv = msg->offload->priv; > + struct spi_transfer *xfer; > + void __iomem *cmd_addr; > + void __iomem *sdo_addr; > + size_t tx_word_count = 0; > + unsigned int i; > + > + if (p->length > spi_engine->offload_ctrl_mem_size) > + return -EINVAL; > + > + /* count total number of tx words in message */ > + list_for_each_entry(xfer, &msg->transfers, transfer_list) { > + if (!xfer->tx_buf) > + continue; > + > + if (xfer->bits_per_word <= 8) > + tx_word_count += xfer->len; > + else if (xfer->bits_per_word <= 16) > + tx_word_count += xfer->len / 2; > + else > + tx_word_count += xfer->len / 4; > + } > + > + if (tx_word_count > spi_engine->offload_sdo_mem_size) > + return -EINVAL; > + > + if (test_and_set_bit_lock(SPI_ENGINE_OFFLOAD_FLAG_PREPARED, &priv->flags)) > + return -EBUSY; > + This is odd. Any special reason for using this with aquire - release semantics? Can optimize() and unoptimize() run concurrently? Because if they can this does not give us mutual exclusion and we really need to do what we're doing with kind of stuff :) - Nuno Sá
I still need to look better at this but I do have one though already :) On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: > Add a new devm_iio_dmaengine_buffer_setup_ext2() function to handle > cases where the DMA channel is managed by the caller rather than being > requested and released by the iio_dmaengine module. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v4 changes: > * This replaces "iio: buffer-dmaengine: generalize requesting DMA channel" > --- > drivers/iio/buffer/industrialio-buffer-dmaengine.c | 107 +++++++++++++++------ > include/linux/iio/buffer-dmaengine.h | 5 + > 2 files changed, 81 insertions(+), 31 deletions(-) > > diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c > b/drivers/iio/buffer/industrialio-buffer-dmaengine.c > index 054af21dfa65..602cb2e147a6 100644 > --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c > +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c > @@ -33,6 +33,7 @@ struct dmaengine_buffer { > struct iio_dma_buffer_queue queue; > > struct dma_chan *chan; > + bool owns_chan; > struct list_head active; > > size_t align; > @@ -216,28 +217,23 @@ static const struct iio_dev_attr > *iio_dmaengine_buffer_attrs[] = { > * Once done using the buffer iio_dmaengine_buffer_free() should be used to > * release it. > */ > -static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev, > - const char *channel) > +static struct iio_buffer *iio_dmaengine_buffer_alloc(struct dma_chan *chan, > + bool owns_chan) > { > struct dmaengine_buffer *dmaengine_buffer; > unsigned int width, src_width, dest_width; > struct dma_slave_caps caps; > - struct dma_chan *chan; > int ret; > > dmaengine_buffer = kzalloc(sizeof(*dmaengine_buffer), GFP_KERNEL); > - if (!dmaengine_buffer) > - return ERR_PTR(-ENOMEM); > - > - chan = dma_request_chan(dev, channel); > - if (IS_ERR(chan)) { > - ret = PTR_ERR(chan); > - goto err_free; > + if (!dmaengine_buffer) { > + ret = -ENOMEM; > + goto err_release; > } > > ret = dma_get_slave_caps(chan, &caps); > if (ret < 0) > - goto err_release; > + goto err_free; > > /* Needs to be aligned to the maximum of the minimums */ > if (caps.src_addr_widths) > @@ -252,6 +248,7 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct > device *dev, > > INIT_LIST_HEAD(&dmaengine_buffer->active); > dmaengine_buffer->chan = chan; > + dmaengine_buffer->owns_chan = owns_chan; > dmaengine_buffer->align = width; > dmaengine_buffer->max_size = dma_get_max_seg_size(chan->device->dev); > > @@ -263,10 +260,12 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct > device *dev, > > return &dmaengine_buffer->queue.buffer; > > -err_release: > - dma_release_channel(chan); > err_free: > kfree(dmaengine_buffer); > +err_release: > + if (owns_chan) > + dma_release_channel(chan); > + > return ERR_PTR(ret); > } > > @@ -282,12 +281,38 @@ void iio_dmaengine_buffer_free(struct iio_buffer *buffer) > iio_buffer_to_dmaengine_buffer(buffer); > > iio_dma_buffer_exit(&dmaengine_buffer->queue); > - dma_release_channel(dmaengine_buffer->chan); > - > iio_buffer_put(buffer); > + > + if (dmaengine_buffer->owns_chan) > + dma_release_channel(dmaengine_buffer->chan); Not sure if I agree much with this owns_chan flag. The way I see it, we should always handover the lifetime of the DMA channel to the IIO DMA framework. Note that even the device you pass in for both requesting the channel of the spi_offload and for setting up the DMA buffer is the same (and i suspect it will always be) so I would not go with the trouble. And with this assumption we could simplify a bit more the spi implementation. And not even related but I even suspect the current implementation could be problematic. Basically I'm suspecting that the lifetime of the DMA channel should be attached to the lifetime of the iio_buffer. IOW, we should only release the channel in iio_dmaengine_buffer_release() - in which case the current implementation with the spi_offload would also be buggy. But bah, the second point is completely theoretical and likely very hard to reproduce in real life (if reproducible at all - for now it's only something I suspect) - Nuno Sá
On 10/25/24 8:09 AM, Nuno Sá wrote: > On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: >> Implement 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@baylibre.com> >> --- >> ... >> +static int spi_engine_offload_prepare(struct spi_message *msg) >> +{ >> + struct spi_controller *host = msg->spi->controller; >> + struct spi_engine *spi_engine = spi_controller_get_devdata(host); >> + struct spi_engine_program *p = msg->opt_state; >> + struct spi_engine_offload *priv = msg->offload->priv; >> + struct spi_transfer *xfer; >> + void __iomem *cmd_addr; >> + void __iomem *sdo_addr; >> + size_t tx_word_count = 0; >> + unsigned int i; >> + >> + if (p->length > spi_engine->offload_ctrl_mem_size) >> + return -EINVAL; >> + >> + /* count total number of tx words in message */ >> + list_for_each_entry(xfer, &msg->transfers, transfer_list) { >> + if (!xfer->tx_buf) >> + continue; >> + >> + if (xfer->bits_per_word <= 8) >> + tx_word_count += xfer->len; >> + else if (xfer->bits_per_word <= 16) >> + tx_word_count += xfer->len / 2; >> + else >> + tx_word_count += xfer->len / 4; >> + } >> + >> + if (tx_word_count > spi_engine->offload_sdo_mem_size) >> + return -EINVAL; >> + >> + if (test_and_set_bit_lock(SPI_ENGINE_OFFLOAD_FLAG_PREPARED, &priv->flags)) >> + return -EBUSY; >> + > > This is odd. Any special reason for using this with aquire - release semantics? Can > optimize() and unoptimize() run concurrently? Because if they can this does not give > us mutual exclusion and we really need to do what we're doing with kind of stuff :) > > - Nuno Sá > > This looks like another leftover from an in-between revision that didn't get fully cleaned up. I will reconsider what is need here. But to answer the question, strictly speaking, there isn't anything to prevent two concurrent calls spi_optimize_message() with different messages but the same offload instance.
On 10/25/24 8:24 AM, Nuno Sá wrote: > I still need to look better at this but I do have one though already :) > > On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: >> Add a new devm_iio_dmaengine_buffer_setup_ext2() function to handle >> cases where the DMA channel is managed by the caller rather than being >> requested and released by the iio_dmaengine module. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> >> v4 changes: >> * This replaces "iio: buffer-dmaengine: generalize requesting DMA channel" >> --- ... >> @@ -282,12 +281,38 @@ void iio_dmaengine_buffer_free(struct iio_buffer *buffer) >> iio_buffer_to_dmaengine_buffer(buffer); >> >> iio_dma_buffer_exit(&dmaengine_buffer->queue); >> - dma_release_channel(dmaengine_buffer->chan); >> - >> iio_buffer_put(buffer); >> + >> + if (dmaengine_buffer->owns_chan) >> + dma_release_channel(dmaengine_buffer->chan); > > Not sure if I agree much with this owns_chan flag. The way I see it, we should always > handover the lifetime of the DMA channel to the IIO DMA framework. Note that even the > device you pass in for both requesting the channel of the spi_offload and for > setting up the DMA buffer is the same (and i suspect it will always be) so I would > not go with the trouble. And with this assumption we could simplify a bit more the > spi implementation. I tried something like this in v3 but Jonathan didn't seem to like it. https://lore.kernel.org/all/20240727144303.4a8604cb@jic23-huawei/ > > And not even related but I even suspect the current implementation could be > problematic. Basically I'm suspecting that the lifetime of the DMA channel should be > attached to the lifetime of the iio_buffer. IOW, we should only release the channel > in iio_dmaengine_buffer_release() - in which case the current implementation with the > spi_offload would also be buggy. The buffer can outlive the iio device driver that created the buffer? > > But bah, the second point is completely theoretical and likely very hard to reproduce > in real life (if reproducible at all - for now it's only something I suspect) > > - Nuno Sá > >
On Wed, 23 Oct 2024 15:59:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > Extend SPI offloading to support hardware triggers. > > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_optimize_message(). > > A new struct spi_offload_trigger is introduced that can be used to > configure any type of trigger. It has a type discriminator and a union > to allow it to be extended in the future. Two trigger types are defined > to start with. One is a trigger that indicates that the SPI peripheral > is ready to read or write data. The other is a periodic trigger to > repeat a SPI message at a fixed rate. > > There is also a spi_offload_hw_trigger_validate() function that works > similar to clk_round_rate(). It basically asks the question of if we > enabled the hardware trigger what would the actual parameters be. This > can be used to test if the requested trigger type is actually supported > by the hardware and for periodic triggers, it can be used to find the > actual rate that the hardware is capable of. > > Signed-off-by: David Lechner <dlechner@baylibre.com> A few generic comments inline. Jonathan > --- > > In previous versions, we locked the SPI bus when the hardware trigger > was enabled, but we found this to be too restrictive. In one use case, > to avoid a race condition, we need to enable the SPI offload via a > hardware trigger, then write a SPI message to the peripheral to place > it into a mode that will generate the trigger. If we did it the other > way around, we could miss the first trigger. > > Another likely use case will be enabling two offloads/triggers at one > time on the same device, e.g. a read trigger and a write trigger. So > the exclusive bus lock for a single trigger would be too restrictive in > this case too. > > So for now, I'm going with Nuno's suggestion to leave any locking up to > the individual controller driver. If we do find we need something more > generic in the future, we could add a new spi_bus_lock_exclusive() API > that causes spi_bus_lock() to fail instead of waiting and add "locked" > versions of trigger enable functions. This would allow a peripheral to > claim exclusive use of the bus indefinitely while still being able to > do any SPI messaging that it needs. > > v4 changes: > * Added new struct spi_offload_trigger that is a generic struct for any > hardware trigger rather than returning a struct clk. > * Added new spi_offload_hw_trigger_validate() function. > * Dropped extra locking since it was too restrictive. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > * This is split out from "spi: add core support for controllers with > offload capabilities". > * Added locking for offload trigger to claim exclusive use of the SPI > bus. > --- > drivers/spi/spi-offload.c | 266 ++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi-offload.h | 78 ++++++++++++ > 2 files changed, 344 insertions(+) > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > index c344cbf50bdb..2a1f9587f27a 100644 > --- a/drivers/spi/spi-offload.c > +++ b/drivers/spi/spi-offload.c > @@ -9,12 +9,26 @@ > #include <linux/cleanup.h> > #include <linux/device.h> > #include <linux/export.h> > +#include <linux/list.h> > #include <linux/mutex.h> > +#include <linux/of.h> > #include <linux/property.h> > #include <linux/spi/spi-offload.h> > #include <linux/spi/spi.h> > #include <linux/types.h> > > +struct spi_offload_trigger { > + struct list_head list; > + struct device dev; > + /* synchronizes calling ops and driver registration */ > + struct mutex lock; > + const struct spi_offload_trigger_ops *ops; > + void *priv; > +}; > + > +static LIST_HEAD(spi_offload_triggers); > +static DEFINE_MUTEX(spi_offload_triggers_lock); > + > /** > * devm_spi_offload_alloc() - Allocate offload instances > * @dev: Device for devm purposes > @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev, > return offload; > } > EXPORT_SYMBOL_GPL(devm_spi_offload_get); > + > +static void spi_offload_trigger_release(void *data) > +{ > + struct spi_offload_trigger *trigger = data; > + > + guard(mutex)(&trigger->lock); > + if (trigger->priv && trigger->ops->release) > + trigger->ops->release(trigger->priv); > + > + put_device(&trigger->dev); > +} > + > +struct spi_offload_trigger > +*devm_spi_offload_trigger_get(struct device *dev, > + struct spi_offload *offload, > + enum spi_offload_trigger_type type) > +{ > + struct spi_offload_trigger *trigger; > + struct fwnode_reference_args args; > + bool match = false; > + int ret; > + > + ret = fwnode_property_get_reference_args(dev_fwnode(offload->provider_dev), > + "trigger-sources", > + "#trigger-source-cells", 0, 0, > + &args); > + if (ret) > + return ERR_PTR(ret); > + > + struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode; I'm not fond of __free usage like this because code could get added above this line and it wouldn't be obvious that it doesn't release the handle. Annoying though it is, maybe just do manual release of the fwnode. Ora factor out the next chunk to a helper function so you can just put the fwnode after that is called. > + > + guard(mutex)(&spi_offload_triggers_lock); > + > + list_for_each_entry(trigger, &spi_offload_triggers, list) { > + if (trigger->dev.fwnode != args.fwnode) > + continue; > + > + match = trigger->ops->match(trigger->priv, type, args.args, args.nargs); > + if (match) > + break; > + } > + > + if (!match) > + return ERR_PTR(-EPROBE_DEFER); > + > + guard(mutex)(&trigger->lock); > + > + if (!trigger->priv) > + return ERR_PTR(-ENODEV); > + > + if (trigger->ops->request) { > + ret = trigger->ops->request(trigger->priv, type, args.args, args.nargs); > + if (ret) > + return ERR_PTR(ret); > + } > + > + get_device(&trigger->dev); > + > + ret = devm_add_action_or_reset(dev, spi_offload_trigger_release, trigger); > + if (ret) > + return ERR_PTR(ret); > + > + return trigger; > +} > +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_get); > +int devm_spi_offload_trigger_register(struct device *dev, > + struct spi_offload_trigger *trigger, > + void *priv) > +{ > + int ret; > + > + ret = device_add(&trigger->dev); > + if (ret) > + return ret; > + > + trigger->priv = priv; > + > + guard(mutex)(&spi_offload_triggers_lock); > + list_add_tail(&trigger->list, &spi_offload_triggers); > + > + ret = devm_add_action_or_reset(dev, spi_offload_trigger_unregister, trigger); I guess there may be more here later in the series, but if not. return devm_add_... > + if (ret) > + return ret; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_spi_offload_trigger_register);
On Wed, 23 Oct 2024 15:59:12 -0500 David Lechner <dlechner@baylibre.com> wrote: > Add a new binding for using a PWM signal as a trigger for SPI offloads. I don't have a better suggestion for this, but it does smell rather like other bridge binding (iio-hwmon for example) where we have had push back on representing something that doesn't really exist but is just a way to tie two bits of hardware together. Those kind of exist because we snuck them in a long time back when no one was paying attention. So this one may need more explanation and justification and I'd definitely like some DT maintainer review on this at a fairly early stage! (might have happened in earlier reviews but it has been a while so I've forgotten if it did) Jonathan > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v4 changes: new patch in v4 > --- > .../devicetree/bindings/spi/trigger-pwm.yaml | 39 ++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/trigger-pwm.yaml b/Documentation/devicetree/bindings/spi/trigger-pwm.yaml > new file mode 100644 > index 000000000000..987638aa4732 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/trigger-pwm.yaml > @@ -0,0 +1,39 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/spi/trigger-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic SPI offload trigger using PWM > + > +description: Remaps a PWM channel as a trigger source. > + > +maintainers: > + - David Lechner <dlechner@baylibre.com> > + > +$ref: /schemas/spi/trigger-source.yaml# > + > +properties: > + compatible: > + const: trigger-pwm > + > + '#trigger-source-cells': > + const: 0 > + > + pwms: > + maxItems: 1 > + > +required: > + - compatible > + - '#trigger-source-cells' > + - pwms > + > +additionalProperties: false > + > +examples: > + - | > + trigger { > + compatible = "trigger-pwm"; > + #trigger-source-cells = <0>; > + pwms = <&pwm 0 1000000 0>; > + }; >
On Wed, 23 Oct 2024 15:59:20 -0500 David Lechner <dlechner@baylibre.com> wrote: > This adds support for SPI offload to the ad7944 driver. This allows > reading data at the max sample rate of 2.5 MSPS. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > static int ad7944_probe(struct spi_device *spi) > { > const struct ad7944_chip_info *chip_info; > @@ -590,20 +743,75 @@ static int ad7944_probe(struct spi_device *spi) > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &ad7944_iio_info; > > - if (adc->spi_mode == AD7944_SPI_MODE_CHAIN) { > - indio_dev->available_scan_masks = chain_scan_masks; > - indio_dev->channels = chain_chan; > - indio_dev->num_channels = n_chain_dev + 1; > + adc->offload = devm_spi_offload_get(dev, spi, &ad7944_offload_config); > + ret = PTR_ERR_OR_ZERO(adc->offload); > + if (ret && ret != -ENODEV) > + return dev_err_probe(dev, ret, "failed to get offload\n"); > + > + if (ret == -ENODEV) { > + dev_info(dev, "SPI offload not available\n"); Too noisy given this applies whenever this device is connected to a normal SPI bus. Or an old DT is used. > + > >
On Wed, 23 Oct 2024 15:59:22 -0500 David Lechner <dlechner@baylibre.com> wrote: > Add support for SPI offload to the ad4695 driver. SPI offload allows > sampling data at the max sample rate (500kSPS or 1MSPS). > > This is developed and tested against the ADI example FPGA design for > this family of ADCs [1]. > > [1]: http://analogdevicesinc.github.io/hdl/projects/ad469x_fmc/index.html > > Signed-off-by: David Lechner <dlechner@baylibre.com> A few questions inline. In general looks ok, but it's complex code and I'm too snowed under for a very close look at the whole thing for a least a few weeks. Jonathan > --- > drivers/iio/adc/Kconfig | 1 + > drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 440 insertions(+), 31 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 92dfb495a8ce..f76a3f62a9ad 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -53,6 +53,7 @@ config AD4695 > depends on SPI > select REGMAP_SPI > select IIO_BUFFER > + select IIO_BUFFER_DMAENGINE > select IIO_TRIGGERED_BUFFER > help > Say yes here to build support for Analog Devices AD4695 and similar > +static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + struct spi_offload_trigger_config config = { > + .type = SPI_OFFLOAD_TRIGGER_DATA_READY, > + }; > + struct spi_transfer *xfer = &st->buf_read_xfer[0]; > + struct pwm_state state; > + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > + u8 num_slots = 0; > + u8 temp_en = 0; > + unsigned int bit; > + int ret; > + > + iio_for_each_active_channel(indio_dev, bit) { > + if (bit == temp_chan_bit) { > + temp_en = 1; > + continue; > + } > + > + ret = regmap_write(st->regmap, AD4695_REG_AS_SLOT(num_slots), > + FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit)); > + if (ret) > + return ret; > + > + num_slots++; > + } > + > + /* > + * For non-offload, we could discard data to work around this > + * restriction, but with offload, that is not possible. > + */ > + if (num_slots < 2) { > + dev_err(&st->spi->dev, > + "At least two voltage channels must be enabled.\n"); > + return -EINVAL; > + } > + > + ret = regmap_update_bits(st->regmap, AD4695_REG_TEMP_CTRL, > + AD4695_REG_TEMP_CTRL_TEMP_EN, > + FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN, > + temp_en)); > + if (ret) > + return ret; > + > + /* Each BUSY event means just one sample for one channel is ready. */ > + memset(xfer, 0, sizeof(*xfer)); > + xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; > + xfer->bits_per_word = 16; > + xfer->len = 2; > + > + spi_message_init_with_transfers(&st->buf_read_msg, xfer, 1); > + st->buf_read_msg.offload = st->offload; > + > + st->spi->max_speed_hz = st->spi_max_speed_hz; > + ret = spi_optimize_message(st->spi, &st->buf_read_msg); > + st->spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > + if (ret) > + return ret; > + > + /* > + * NB: technically, this is part the SPI offload trigger enable, but it > + * doesn't work to call it from the offload trigger enable callback > + * due to issues with ordering with respect to entering/exiting > + * conversion mode. Give some detail on the operations order. > + */ > + ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE, > + AD4695_REG_GP_MODE_BUSY_GP_EN); > + if (ret) > + goto err_unoptimize_message; > + > + ret = spi_offload_trigger_enable(st->offload, st->offload_trigger, > + &config); > + if (ret) > + goto err_disable_busy_output; > + > + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > + if (ret) > + goto err_offload_trigger_disable; > + > + guard(mutex)(&st->cnv_pwm_lock); > + pwm_get_state(st->cnv_pwm, &state); > + /* > + * PWM subsystem generally rounds down, so requesting 2x minimum high > + * time ensures that we meet the minimum high time in any case. > + */ > + state.duty_cycle = AD4695_T_CNVH_NS * 2; > + ret = pwm_apply_might_sleep(st->cnv_pwm, &state); > + if (ret) > + goto err_offload_exit_conversion_mode; > + > + return 0; > + > +err_offload_exit_conversion_mode: > + /* have to unwind in a different order to avoid triggering offload */ Needs more details here. > + spi_offload_trigger_disable(st->offload, st->offload_trigger); > + ad4695_cnv_manual_trigger(st); > + ad4695_exit_conversion_mode(st); > + goto err_disable_busy_output; > + > +err_offload_trigger_disable: > + spi_offload_trigger_disable(st->offload, st->offload_trigger); > + > +err_disable_busy_output: > + regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE, > + AD4695_REG_GP_MODE_BUSY_GP_EN); > + > +err_unoptimize_message: > + spi_unoptimize_message(&st->buf_read_msg); > + > + return ret; > +} > + > static int ad4695_write_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int val, int val2, long mask) > @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, > default: > return -EINVAL; > } > + case IIO_CHAN_INFO_SAMP_FREQ: { > + struct pwm_state state; > + > + if (val <= 0) > + return -EINVAL; > + > + guard(mutex)(&st->cnv_pwm_lock); > + pwm_get_state(st->cnv_pwm, &state); What limits this to rates the ADC can cope with? > + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); > + return pwm_apply_might_sleep(st->cnv_pwm, &state); > + } > default: > return -EINVAL; > } > static int ad4695_probe(struct spi_device *spi) > { > struct device *dev = &spi->dev; > struct ad4695_state *st; > struct iio_dev *indio_dev; > - struct gpio_desc *cnv_gpio; > bool use_internal_ldo_supply; > bool use_internal_ref_buffer; > int ret; > > - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > - if (IS_ERR(cnv_gpio)) > - return dev_err_probe(dev, PTR_ERR(cnv_gpio), > - "Failed to get CNV GPIO\n"); > - > - /* Driver currently requires CNV pin to be connected to SPI CS */ > - if (cnv_gpio) > - return dev_err_probe(dev, -ENODEV, > - "CNV GPIO is not supported\n"); > - > indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > if (!indio_dev) > return -ENOMEM; > @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) > return -EINVAL; > > /* Registers cannot be read at the max allowable speed */ > + st->spi_max_speed_hz = spi->max_speed_hz; > spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > > + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); Why do you need to put it back in devm? What happens after this but without a driver restart that uses that faster rate? > + if (ret) > + return ret; > + > st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config); > if (IS_ERR(st->regmap)) > return dev_err_probe(dev, PTR_ERR(st->regmap), > @@ -1014,6 +1391,11 @@ static int ad4695_probe(struct spi_device *spi) > return dev_err_probe(dev, PTR_ERR(st->regmap16), > "Failed to initialize regmap16\n"); > > + st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > + if (IS_ERR(st->cnv_gpio)) > + return dev_err_probe(dev, PTR_ERR(st->cnv_gpio), > + "Failed to get CNV GPIO\n"); > + > ret = devm_regulator_bulk_get_enable(dev, > ARRAY_SIZE(ad4695_power_supplies), > ad4695_power_supplies); > @@ -1139,14 +1521,39 @@ static int ad4695_probe(struct spi_device *spi) > indio_dev->info = &ad4695_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = st->iio_chan; > - indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; > > - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > - iio_pollfunc_store_time, > - ad4695_trigger_handler, > - &ad4695_buffer_setup_ops); > - if (ret) > - return ret; > + static const struct spi_offload_config ad4695_offload_config = { > + .capability_flags = SPI_OFFLOAD_CAP_TRIGGER > + | SPI_OFFLOAD_CAP_RX_STREAM_DMA, > + }; > + > + st->offload = devm_spi_offload_get(dev, spi, &ad4695_offload_config); > + ret = PTR_ERR_OR_ZERO(st->offload); > + if (ret && ret != -ENODEV) > + return dev_err_probe(dev, ret, "failed to get SPI offload\n"); > + > + if (ret == -ENODEV) { > + /* If no SPI offload, fall back to low speed usage. */ > + dev_info(dev, "SPI offload not available\n"); As previous. Too noisy for a normal thing that might happen. Maybe if we can't figure it out from anything userspace an see we could add a print on the 'offload is enabled' side of things. > + > + /* Driver currently requires CNV pin to be connected to SPI CS */ > + if (st->cnv_gpio) > + return dev_err_probe(dev, -EINVAL, > + "CNV GPIO is not supported\n"); > + > + indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + iio_pollfunc_store_time, > + ad4695_trigger_handler, > + &ad4695_buffer_setup_ops); > + if (ret) > + return ret; > + } else { > + ret = ad4695_probe_spi_offload(indio_dev, st); > + if (ret) > + return ret; > + }
On 10/26/24 11:00 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:22 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> Add support for SPI offload to the ad4695 driver. SPI offload allows >> sampling data at the max sample rate (500kSPS or 1MSPS). >> >> This is developed and tested against the ADI example FPGA design for >> this family of ADCs [1]. >> >> [1]: http://analogdevicesinc.github.io/hdl/projects/ad469x_fmc/index.html >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> > A few questions inline. In general looks ok, but it's complex code and I'm > too snowed under for a very close look at the whole thing for a least a few weeks. > > Jonathan > >> --- >> drivers/iio/adc/Kconfig | 1 + >> drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++++++++++++++++++++++++++---- >> 2 files changed, 440 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 92dfb495a8ce..f76a3f62a9ad 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -53,6 +53,7 @@ config AD4695 >> depends on SPI >> select REGMAP_SPI >> select IIO_BUFFER >> + select IIO_BUFFER_DMAENGINE >> select IIO_TRIGGERED_BUFFER >> help >> Say yes here to build support for Analog Devices AD4695 and similar > >> +static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev) >> +{ >> + struct ad4695_state *st = iio_priv(indio_dev); >> + struct spi_offload_trigger_config config = { >> + .type = SPI_OFFLOAD_TRIGGER_DATA_READY, >> + }; >> + struct spi_transfer *xfer = &st->buf_read_xfer[0]; >> + struct pwm_state state; >> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; >> + u8 num_slots = 0; >> + u8 temp_en = 0; >> + unsigned int bit; >> + int ret; >> + >> + iio_for_each_active_channel(indio_dev, bit) { >> + if (bit == temp_chan_bit) { >> + temp_en = 1; >> + continue; >> + } >> + >> + ret = regmap_write(st->regmap, AD4695_REG_AS_SLOT(num_slots), >> + FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit)); >> + if (ret) >> + return ret; >> + >> + num_slots++; >> + } >> + >> + /* >> + * For non-offload, we could discard data to work around this >> + * restriction, but with offload, that is not possible. >> + */ >> + if (num_slots < 2) { >> + dev_err(&st->spi->dev, >> + "At least two voltage channels must be enabled.\n"); >> + return -EINVAL; >> + } >> + >> + ret = regmap_update_bits(st->regmap, AD4695_REG_TEMP_CTRL, >> + AD4695_REG_TEMP_CTRL_TEMP_EN, >> + FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN, >> + temp_en)); >> + if (ret) >> + return ret; >> + >> + /* Each BUSY event means just one sample for one channel is ready. */ >> + memset(xfer, 0, sizeof(*xfer)); >> + xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; >> + xfer->bits_per_word = 16; >> + xfer->len = 2; >> + >> + spi_message_init_with_transfers(&st->buf_read_msg, xfer, 1); >> + st->buf_read_msg.offload = st->offload; >> + >> + st->spi->max_speed_hz = st->spi_max_speed_hz; >> + ret = spi_optimize_message(st->spi, &st->buf_read_msg); >> + st->spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; >> + if (ret) >> + return ret; >> + >> + /* >> + * NB: technically, this is part the SPI offload trigger enable, but it >> + * doesn't work to call it from the offload trigger enable callback >> + * due to issues with ordering with respect to entering/exiting >> + * conversion mode. > Give some detail on the operations order. > >> + */ >> + ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE, >> + AD4695_REG_GP_MODE_BUSY_GP_EN); >> + if (ret) >> + goto err_unoptimize_message; >> + >> + ret = spi_offload_trigger_enable(st->offload, st->offload_trigger, >> + &config); >> + if (ret) >> + goto err_disable_busy_output; >> + >> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); >> + if (ret) >> + goto err_offload_trigger_disable; >> + >> + guard(mutex)(&st->cnv_pwm_lock); >> + pwm_get_state(st->cnv_pwm, &state); >> + /* >> + * PWM subsystem generally rounds down, so requesting 2x minimum high >> + * time ensures that we meet the minimum high time in any case. >> + */ >> + state.duty_cycle = AD4695_T_CNVH_NS * 2; >> + ret = pwm_apply_might_sleep(st->cnv_pwm, &state); >> + if (ret) >> + goto err_offload_exit_conversion_mode; >> + >> + return 0; >> + >> +err_offload_exit_conversion_mode: >> + /* have to unwind in a different order to avoid triggering offload */ > > Needs more details here. > >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); >> + ad4695_cnv_manual_trigger(st); >> + ad4695_exit_conversion_mode(st); >> + goto err_disable_busy_output; >> + >> +err_offload_trigger_disable: >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); >> + >> +err_disable_busy_output: >> + regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE, >> + AD4695_REG_GP_MODE_BUSY_GP_EN); >> + >> +err_unoptimize_message: >> + spi_unoptimize_message(&st->buf_read_msg); >> + >> + return ret; >> +} > >> + >> static int ad4695_write_raw(struct iio_dev *indio_dev, >> struct iio_chan_spec const *chan, >> int val, int val2, long mask) >> @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, >> default: >> return -EINVAL; >> } >> + case IIO_CHAN_INFO_SAMP_FREQ: { >> + struct pwm_state state; >> + >> + if (val <= 0) >> + return -EINVAL; >> + >> + guard(mutex)(&st->cnv_pwm_lock); >> + pwm_get_state(st->cnv_pwm, &state); > > What limits this to rates the ADC can cope with? > >> + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); >> + return pwm_apply_might_sleep(st->cnv_pwm, &state); >> + } >> default: >> return -EINVAL; >> } > >> static int ad4695_probe(struct spi_device *spi) >> { >> struct device *dev = &spi->dev; >> struct ad4695_state *st; >> struct iio_dev *indio_dev; >> - struct gpio_desc *cnv_gpio; >> bool use_internal_ldo_supply; >> bool use_internal_ref_buffer; >> int ret; >> >> - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); >> - if (IS_ERR(cnv_gpio)) >> - return dev_err_probe(dev, PTR_ERR(cnv_gpio), >> - "Failed to get CNV GPIO\n"); >> - >> - /* Driver currently requires CNV pin to be connected to SPI CS */ >> - if (cnv_gpio) >> - return dev_err_probe(dev, -ENODEV, >> - "CNV GPIO is not supported\n"); >> - >> indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); >> if (!indio_dev) >> return -ENOMEM; >> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) >> return -EINVAL; >> >> /* Registers cannot be read at the max allowable speed */ >> + st->spi_max_speed_hz = spi->max_speed_hz; >> spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; >> >> + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); > > Why do you need to put it back in devm? What happens after this but without > a driver restart that uses that faster rate? > I should have added a comment here as this was a weird bug to trace. The core SPI framework sets the initial value of spi->max_speed_hz to the minimum of the controller max rate and the max rate specified by the devicetree. The SPI device lives beyond this driver, so if we bind the driver and set spi->max_speed_hz to something other than what the SPI core set it, then the next time we bind the driver, we don't get the the max rate from the SPI core, but rather we changed it to when the driver unbound. So on the second bind, the max rate would be the slow register read rate instead of the actual max allowable rate. So we need to reset spi->max_speed_hz to what it was originally on driver unbind so that everything works as expected on the next bind. (Or we call this a SPI core bug and fix it there instead).
On 10/26/24 11:00 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:22 -0500 > David Lechner <dlechner@baylibre.com> wrote: > ... >> static int ad4695_write_raw(struct iio_dev *indio_dev, >> struct iio_chan_spec const *chan, >> int val, int val2, long mask) >> @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, >> default: >> return -EINVAL; >> } >> + case IIO_CHAN_INFO_SAMP_FREQ: { >> + struct pwm_state state; >> + >> + if (val <= 0) >> + return -EINVAL; >> + >> + guard(mutex)(&st->cnv_pwm_lock); >> + pwm_get_state(st->cnv_pwm, &state); > > What limits this to rates the ADC can cope with? > Nothing at the moment. The "obvious" thing to do would be to limit this to the max rate from the datasheet. But that feels a little too strict to me since maybe the PWM can't get exactly the max rate, but can get the max rate + 1% or so. It seems like we should allow that too. It's not like the ADC is going to not work if we go a few Hz over the datasheet rating. Maybe limit it to max + 10% or something like that?
On 10/26/24 10:18 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:12 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> Add a new binding for using a PWM signal as a trigger for SPI offloads. > > I don't have a better suggestion for this, but it does smell rather like > other bridge binding (iio-hwmon for example) where we have had push back on > representing something that doesn't really exist but is just a way to > tie two bits of hardware together. Those kind of exist because we snuck > them in a long time back when no one was paying attention. > > So this one may need more explanation and justification and I'd definitely > like some DT maintainer review on this at a fairly early stage! > (might have happened in earlier reviews but it has been a while so I've > forgotten if it did) > > Jonathan > We could probably make it work like the leds version of this binding where the trigger-sources property can have phandles to anything, not just a dedicated class of device. It just gets messy to implement because every subsystem needs to have core code modified to be able to handle using a device or one channel/gpio/etc. of a device as a trigger instead of whatever it normally is. > >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> >> v4 changes: new patch in v4 >> --- >> .../devicetree/bindings/spi/trigger-pwm.yaml | 39 ++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/spi/trigger-pwm.yaml b/Documentation/devicetree/bindings/spi/trigger-pwm.yaml >> new file mode 100644 >> index 000000000000..987638aa4732 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/trigger-pwm.yaml >> @@ -0,0 +1,39 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/spi/trigger-pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Generic SPI offload trigger using PWM >> + >> +description: Remaps a PWM channel as a trigger source. >> + >> +maintainers: >> + - David Lechner <dlechner@baylibre.com> >> + >> +$ref: /schemas/spi/trigger-source.yaml# >> + >> +properties: >> + compatible: >> + const: trigger-pwm >> + >> + '#trigger-source-cells': >> + const: 0 >> + >> + pwms: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - '#trigger-source-cells' >> + - pwms >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + trigger { >> + compatible = "trigger-pwm"; >> + #trigger-source-cells = <0>; >> + pwms = <&pwm 0 1000000 0>; >> + }; >> >
On Sat, 26 Oct 2024 19:01:53 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/26/24 11:00 AM, Jonathan Cameron wrote: > > On Wed, 23 Oct 2024 15:59:22 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> Add support for SPI offload to the ad4695 driver. SPI offload allows > >> sampling data at the max sample rate (500kSPS or 1MSPS). > >> > >> This is developed and tested against the ADI example FPGA design for > >> this family of ADCs [1]. > >> > >> [1]: http://analogdevicesinc.github.io/hdl/projects/ad469x_fmc/index.html > >> > >> Signed-off-by: David Lechner <dlechner@baylibre.com> > > A few questions inline. In general looks ok, but it's complex code and I'm > > too snowed under for a very close look at the whole thing for a least a few weeks. > > > > Jonathan > > > >> --- > >> drivers/iio/adc/Kconfig | 1 + > >> drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++++++++++++++++++++++++++---- > >> 2 files changed, 440 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > >> index 92dfb495a8ce..f76a3f62a9ad 100644 > >> --- a/drivers/iio/adc/Kconfig > >> +++ b/drivers/iio/adc/Kconfig > >> @@ -53,6 +53,7 @@ config AD4695 > >> depends on SPI > >> select REGMAP_SPI > >> select IIO_BUFFER > >> + select IIO_BUFFER_DMAENGINE > >> select IIO_TRIGGERED_BUFFER > >> help > >> Say yes here to build support for Analog Devices AD4695 and similar > > > >> +static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev) > >> +{ > >> + struct ad4695_state *st = iio_priv(indio_dev); > >> + struct spi_offload_trigger_config config = { > >> + .type = SPI_OFFLOAD_TRIGGER_DATA_READY, > >> + }; > >> + struct spi_transfer *xfer = &st->buf_read_xfer[0]; > >> + struct pwm_state state; > >> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > >> + u8 num_slots = 0; > >> + u8 temp_en = 0; > >> + unsigned int bit; > >> + int ret; > >> + > >> + iio_for_each_active_channel(indio_dev, bit) { > >> + if (bit == temp_chan_bit) { > >> + temp_en = 1; > >> + continue; > >> + } > >> + > >> + ret = regmap_write(st->regmap, AD4695_REG_AS_SLOT(num_slots), > >> + FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit)); > >> + if (ret) > >> + return ret; > >> + > >> + num_slots++; > >> + } > >> + > >> + /* > >> + * For non-offload, we could discard data to work around this > >> + * restriction, but with offload, that is not possible. > >> + */ > >> + if (num_slots < 2) { > >> + dev_err(&st->spi->dev, > >> + "At least two voltage channels must be enabled.\n"); > >> + return -EINVAL; > >> + } > >> + > >> + ret = regmap_update_bits(st->regmap, AD4695_REG_TEMP_CTRL, > >> + AD4695_REG_TEMP_CTRL_TEMP_EN, > >> + FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN, > >> + temp_en)); > >> + if (ret) > >> + return ret; > >> + > >> + /* Each BUSY event means just one sample for one channel is ready. */ > >> + memset(xfer, 0, sizeof(*xfer)); > >> + xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; > >> + xfer->bits_per_word = 16; > >> + xfer->len = 2; > >> + > >> + spi_message_init_with_transfers(&st->buf_read_msg, xfer, 1); > >> + st->buf_read_msg.offload = st->offload; > >> + > >> + st->spi->max_speed_hz = st->spi_max_speed_hz; > >> + ret = spi_optimize_message(st->spi, &st->buf_read_msg); > >> + st->spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > >> + if (ret) > >> + return ret; > >> + > >> + /* > >> + * NB: technically, this is part the SPI offload trigger enable, but it > >> + * doesn't work to call it from the offload trigger enable callback > >> + * due to issues with ordering with respect to entering/exiting > >> + * conversion mode. > > Give some detail on the operations order. > > > >> + */ > >> + ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE, > >> + AD4695_REG_GP_MODE_BUSY_GP_EN); > >> + if (ret) > >> + goto err_unoptimize_message; > >> + > >> + ret = spi_offload_trigger_enable(st->offload, st->offload_trigger, > >> + &config); > >> + if (ret) > >> + goto err_disable_busy_output; > >> + > >> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > >> + if (ret) > >> + goto err_offload_trigger_disable; > >> + > >> + guard(mutex)(&st->cnv_pwm_lock); > >> + pwm_get_state(st->cnv_pwm, &state); > >> + /* > >> + * PWM subsystem generally rounds down, so requesting 2x minimum high > >> + * time ensures that we meet the minimum high time in any case. > >> + */ > >> + state.duty_cycle = AD4695_T_CNVH_NS * 2; > >> + ret = pwm_apply_might_sleep(st->cnv_pwm, &state); > >> + if (ret) > >> + goto err_offload_exit_conversion_mode; > >> + > >> + return 0; > >> + > >> +err_offload_exit_conversion_mode: > >> + /* have to unwind in a different order to avoid triggering offload */ > > > > Needs more details here. > > > >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); > >> + ad4695_cnv_manual_trigger(st); > >> + ad4695_exit_conversion_mode(st); > >> + goto err_disable_busy_output; > >> + > >> +err_offload_trigger_disable: > >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); > >> + > >> +err_disable_busy_output: > >> + regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE, > >> + AD4695_REG_GP_MODE_BUSY_GP_EN); > >> + > >> +err_unoptimize_message: > >> + spi_unoptimize_message(&st->buf_read_msg); > >> + > >> + return ret; > >> +} > > > >> + > >> static int ad4695_write_raw(struct iio_dev *indio_dev, > >> struct iio_chan_spec const *chan, > >> int val, int val2, long mask) > >> @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, > >> default: > >> return -EINVAL; > >> } > >> + case IIO_CHAN_INFO_SAMP_FREQ: { > >> + struct pwm_state state; > >> + > >> + if (val <= 0) > >> + return -EINVAL; > >> + > >> + guard(mutex)(&st->cnv_pwm_lock); > >> + pwm_get_state(st->cnv_pwm, &state); > > > > What limits this to rates the ADC can cope with? > > > >> + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); > >> + return pwm_apply_might_sleep(st->cnv_pwm, &state); > >> + } > >> default: > >> return -EINVAL; > >> } > > > >> static int ad4695_probe(struct spi_device *spi) > >> { > >> struct device *dev = &spi->dev; > >> struct ad4695_state *st; > >> struct iio_dev *indio_dev; > >> - struct gpio_desc *cnv_gpio; > >> bool use_internal_ldo_supply; > >> bool use_internal_ref_buffer; > >> int ret; > >> > >> - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > >> - if (IS_ERR(cnv_gpio)) > >> - return dev_err_probe(dev, PTR_ERR(cnv_gpio), > >> - "Failed to get CNV GPIO\n"); > >> - > >> - /* Driver currently requires CNV pin to be connected to SPI CS */ > >> - if (cnv_gpio) > >> - return dev_err_probe(dev, -ENODEV, > >> - "CNV GPIO is not supported\n"); > >> - > >> indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > >> if (!indio_dev) > >> return -ENOMEM; > >> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) > >> return -EINVAL; > >> > >> /* Registers cannot be read at the max allowable speed */ > >> + st->spi_max_speed_hz = spi->max_speed_hz; > >> spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > >> > >> + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); > > > > Why do you need to put it back in devm? What happens after this but without > > a driver restart that uses that faster rate? > > > I should have added a comment here as this was a weird bug to trace. > > The core SPI framework sets the initial value of spi->max_speed_hz > to the minimum of the controller max rate and the max rate specified > by the devicetree. > > The SPI device lives beyond this driver, so if we bind the driver > and set spi->max_speed_hz to something other than what the SPI core > set it, then the next time we bind the driver, we don't get the > the max rate from the SPI core, but rather we changed it to when > the driver unbound. > > So on the second bind, the max rate would be the slow register > read rate instead of the actual max allowable rate. > > So we need to reset spi->max_speed_hz to what it was originally > on driver unbind so that everything works as expected on the > next bind. > > (Or we call this a SPI core bug and fix it there instead). Definitely a question to ask. Directly accessing spi_max_speed_hz may be the fundamental issue as I don't think the driver is generally expected to touch that in a dynamic fashion. Should we be instead setting it per transfer for the ones that need it controlled? Jonathan >
On Sat, 26 Oct 2024 19:05:44 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/26/24 11:00 AM, Jonathan Cameron wrote: > > On Wed, 23 Oct 2024 15:59:22 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > ... > > >> static int ad4695_write_raw(struct iio_dev *indio_dev, > >> struct iio_chan_spec const *chan, > >> int val, int val2, long mask) > >> @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, > >> default: > >> return -EINVAL; > >> } > >> + case IIO_CHAN_INFO_SAMP_FREQ: { > >> + struct pwm_state state; > >> + > >> + if (val <= 0) > >> + return -EINVAL; > >> + > >> + guard(mutex)(&st->cnv_pwm_lock); > >> + pwm_get_state(st->cnv_pwm, &state); > > > > What limits this to rates the ADC can cope with? > > > > Nothing at the moment. The "obvious" thing to do would > be to limit this to the max rate from the datasheet. > > But that feels a little too strict to me since maybe the > PWM can't get exactly the max rate, but can get the max > rate + 1% or so. It seems like we should allow that too. > It's not like the ADC is going to not work if we go a > few Hz over the datasheet rating. > > Maybe limit it to max + 10% or something like that? Clamp it at datasheet value. That's what is presumably verified not 10% over. If that needs relaxing in future, the datasheet should be updated to reflect the higher verified value. Jonathan
On 10/27/24 4:12 AM, Jonathan Cameron wrote: > On Sat, 26 Oct 2024 19:01:53 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> On 10/26/24 11:00 AM, Jonathan Cameron wrote: >>> On Wed, 23 Oct 2024 15:59:22 -0500 >>> David Lechner <dlechner@baylibre.com> wrote: >>> ... >>> >>>> static int ad4695_probe(struct spi_device *spi) >>>> { >>>> struct device *dev = &spi->dev; >>>> struct ad4695_state *st; >>>> struct iio_dev *indio_dev; >>>> - struct gpio_desc *cnv_gpio; >>>> bool use_internal_ldo_supply; >>>> bool use_internal_ref_buffer; >>>> int ret; >>>> >>>> - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); >>>> - if (IS_ERR(cnv_gpio)) >>>> - return dev_err_probe(dev, PTR_ERR(cnv_gpio), >>>> - "Failed to get CNV GPIO\n"); >>>> - >>>> - /* Driver currently requires CNV pin to be connected to SPI CS */ >>>> - if (cnv_gpio) >>>> - return dev_err_probe(dev, -ENODEV, >>>> - "CNV GPIO is not supported\n"); >>>> - >>>> indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); >>>> if (!indio_dev) >>>> return -ENOMEM; >>>> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) >>>> return -EINVAL; >>>> >>>> /* Registers cannot be read at the max allowable speed */ >>>> + st->spi_max_speed_hz = spi->max_speed_hz; >>>> spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; >>>> >>>> + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); >>> >>> Why do you need to put it back in devm? What happens after this but without >>> a driver restart that uses that faster rate? >>> >> I should have added a comment here as this was a weird bug to trace. >> >> The core SPI framework sets the initial value of spi->max_speed_hz >> to the minimum of the controller max rate and the max rate specified >> by the devicetree. >> >> The SPI device lives beyond this driver, so if we bind the driver >> and set spi->max_speed_hz to something other than what the SPI core >> set it, then the next time we bind the driver, we don't get the >> the max rate from the SPI core, but rather we changed it to when >> the driver unbound. >> >> So on the second bind, the max rate would be the slow register >> read rate instead of the actual max allowable rate. >> >> So we need to reset spi->max_speed_hz to what it was originally >> on driver unbind so that everything works as expected on the >> next bind. >> >> (Or we call this a SPI core bug and fix it there instead). > Definitely a question to ask. Directly accessing spi_max_speed_hz may > be the fundamental issue as I don't think the driver is generally > expected to touch that in a dynamic fashion. Should we be instead setting it > per transfer for the ones that need it controlled? > > Jonathan > The problem is that we are using regmap and that doesn't have a way to specify the max frequency for register reads that is different from other uses of the SPI bus (i.e. reading sample data). So we could fix it in the generic SPI regmap (not exactly trivial) or we could write our own regmap read/write callbacks in this driver that properly sets the per-transfer max speed.
On Sat, Oct 26, 2024 at 04:18:37PM +0100, Jonathan Cameron wrote: > So this one may need more explanation and justification and I'd definitely > like some DT maintainer review on this at a fairly early stage! > (might have happened in earlier reviews but it has been a while so I've > forgotten if it did) I intend looking at this, but it'll be Wednesday before I do.
On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: > Extend SPI offloading to support hardware triggers. > > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_optimize_message(). > > A new struct spi_offload_trigger is introduced that can be used to > configure any type of trigger. It has a type discriminator and a union > to allow it to be extended in the future. Two trigger types are defined > to start with. One is a trigger that indicates that the SPI peripheral > is ready to read or write data. The other is a periodic trigger to > repeat a SPI message at a fixed rate. > > There is also a spi_offload_hw_trigger_validate() function that works > similar to clk_round_rate(). It basically asks the question of if we > enabled the hardware trigger what would the actual parameters be. This > can be used to test if the requested trigger type is actually supported > by the hardware and for periodic triggers, it can be used to find the > actual rate that the hardware is capable of. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > In previous versions, we locked the SPI bus when the hardware trigger > was enabled, but we found this to be too restrictive. In one use case, > to avoid a race condition, we need to enable the SPI offload via a > hardware trigger, then write a SPI message to the peripheral to place > it into a mode that will generate the trigger. If we did it the other > way around, we could miss the first trigger. > > Another likely use case will be enabling two offloads/triggers at one > time on the same device, e.g. a read trigger and a write trigger. So > the exclusive bus lock for a single trigger would be too restrictive in > this case too. > > So for now, I'm going with Nuno's suggestion to leave any locking up to > the individual controller driver. If we do find we need something more > generic in the future, we could add a new spi_bus_lock_exclusive() API > that causes spi_bus_lock() to fail instead of waiting and add "locked" > versions of trigger enable functions. This would allow a peripheral to > claim exclusive use of the bus indefinitely while still being able to > do any SPI messaging that it needs. > > v4 changes: > * Added new struct spi_offload_trigger that is a generic struct for any > hardware trigger rather than returning a struct clk. > * Added new spi_offload_hw_trigger_validate() function. > * Dropped extra locking since it was too restrictive. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > * This is split out from "spi: add core support for controllers with > offload capabilities". > * Added locking for offload trigger to claim exclusive use of the SPI > bus. > --- > drivers/spi/spi-offload.c | 266 ++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi-offload.h | 78 ++++++++++++ > 2 files changed, 344 insertions(+) > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > index c344cbf50bdb..2a1f9587f27a 100644 > --- a/drivers/spi/spi-offload.c > +++ b/drivers/spi/spi-offload.c > @@ -9,12 +9,26 @@ > #include <linux/cleanup.h> > #include <linux/device.h> > #include <linux/export.h> > +#include <linux/list.h> > #include <linux/mutex.h> > +#include <linux/of.h> > #include <linux/property.h> > #include <linux/spi/spi-offload.h> > #include <linux/spi/spi.h> > #include <linux/types.h> > > +struct spi_offload_trigger { > + struct list_head list; > + struct device dev; > + /* synchronizes calling ops and driver registration */ > + struct mutex lock; > + const struct spi_offload_trigger_ops *ops; > + void *priv; > +}; > + > +static LIST_HEAD(spi_offload_triggers); > +static DEFINE_MUTEX(spi_offload_triggers_lock); > + > /** > * devm_spi_offload_alloc() - Allocate offload instances > * @dev: Device for devm purposes > @@ -102,3 +116,255 @@ struct spi_offload *devm_spi_offload_get(struct device *dev, > return offload; > } > EXPORT_SYMBOL_GPL(devm_spi_offload_get); > + > +static void spi_offload_trigger_release(void *data) > +{ > + struct spi_offload_trigger *trigger = data; > + > + guard(mutex)(&trigger->lock); > + if (trigger->priv && trigger->ops->release) > + trigger->ops->release(trigger->priv); > + > + put_device(&trigger->dev); > +} > + > +struct spi_offload_trigger > +*devm_spi_offload_trigger_get(struct device *dev, > + struct spi_offload *offload, > + enum spi_offload_trigger_type type) > +{ > + struct spi_offload_trigger *trigger; > + struct fwnode_reference_args args; > + bool match = false; > + int ret; > + > + ret = fwnode_property_get_reference_args(dev_fwnode(offload- > >provider_dev), > + "trigger-sources", > + "#trigger-source-cells", 0, 0, > + &args); > + if (ret) > + return ERR_PTR(ret); > + > + struct fwnode_handle *trigger_fwnode __free(fwnode_handle) = args.fwnode; > + > + guard(mutex)(&spi_offload_triggers_lock); > + > + list_for_each_entry(trigger, &spi_offload_triggers, list) { > + if (trigger->dev.fwnode != args.fwnode) > + continue; device_match_fwnode() - Nuno Sá
On Sun, 27 Oct 2024 14:52:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/27/24 4:12 AM, Jonathan Cameron wrote: > > On Sat, 26 Oct 2024 19:01:53 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> On 10/26/24 11:00 AM, Jonathan Cameron wrote: > >>> On Wed, 23 Oct 2024 15:59:22 -0500 > >>> David Lechner <dlechner@baylibre.com> wrote: > >>> > > ... > > >>> > >>>> static int ad4695_probe(struct spi_device *spi) > >>>> { > >>>> struct device *dev = &spi->dev; > >>>> struct ad4695_state *st; > >>>> struct iio_dev *indio_dev; > >>>> - struct gpio_desc *cnv_gpio; > >>>> bool use_internal_ldo_supply; > >>>> bool use_internal_ref_buffer; > >>>> int ret; > >>>> > >>>> - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > >>>> - if (IS_ERR(cnv_gpio)) > >>>> - return dev_err_probe(dev, PTR_ERR(cnv_gpio), > >>>> - "Failed to get CNV GPIO\n"); > >>>> - > >>>> - /* Driver currently requires CNV pin to be connected to SPI CS */ > >>>> - if (cnv_gpio) > >>>> - return dev_err_probe(dev, -ENODEV, > >>>> - "CNV GPIO is not supported\n"); > >>>> - > >>>> indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > >>>> if (!indio_dev) > >>>> return -ENOMEM; > >>>> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) > >>>> return -EINVAL; > >>>> > >>>> /* Registers cannot be read at the max allowable speed */ > >>>> + st->spi_max_speed_hz = spi->max_speed_hz; > >>>> spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > >>>> > >>>> + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); > >>> > >>> Why do you need to put it back in devm? What happens after this but without > >>> a driver restart that uses that faster rate? > >>> > >> I should have added a comment here as this was a weird bug to trace. > >> > >> The core SPI framework sets the initial value of spi->max_speed_hz > >> to the minimum of the controller max rate and the max rate specified > >> by the devicetree. > >> > >> The SPI device lives beyond this driver, so if we bind the driver > >> and set spi->max_speed_hz to something other than what the SPI core > >> set it, then the next time we bind the driver, we don't get the > >> the max rate from the SPI core, but rather we changed it to when > >> the driver unbound. > >> > >> So on the second bind, the max rate would be the slow register > >> read rate instead of the actual max allowable rate. > >> > >> So we need to reset spi->max_speed_hz to what it was originally > >> on driver unbind so that everything works as expected on the > >> next bind. > >> > >> (Or we call this a SPI core bug and fix it there instead). > > Definitely a question to ask. Directly accessing spi_max_speed_hz may > > be the fundamental issue as I don't think the driver is generally > > expected to touch that in a dynamic fashion. Should we be instead setting it > > per transfer for the ones that need it controlled? > > > > Jonathan > > > > The problem is that we are using regmap and that doesn't have > a way to specify the max frequency for register reads that is > different from other uses of the SPI bus (i.e. reading sample > data). So we could fix it in the generic SPI regmap (not exactly > trivial) or we could write our own regmap read/write callbacks > in this driver that properly sets the per-transfer max speed. Custom read / write callbacks seems the best approach at first glance, given this is pretty rare thing to do. > >
Hello David, On Wed, Oct 23, 2024 at 03:59:08PM -0500, David Lechner wrote: > Export the pwm_get_state_hw() function. This is useful in cases where > we want to know what the hardware is actually doing, rather than what > what we requested it should do. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v4 changes: new patch in v4 > > And FYI for Uwe and Jonathan, there are a couple of other series > introducing PWM conversion triggers that could make use of this > so that the sampling_frequency attribute can return the actual rate > rather than the requested rate. > > Already applied: > https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/ > > Under review: > https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35 > --- > drivers/pwm/core.c | 55 +++++++++++++++++++++++++++++++++++++---------------- > include/linux/pwm.h | 1 + > 2 files changed, 40 insertions(+), 16 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 634be56e204b..a214d0165d09 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -718,7 +718,7 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state) > } > EXPORT_SYMBOL_GPL(pwm_apply_atomic); > > -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) > +static int __pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) > { > struct pwm_chip *chip = pwm->chip; > const struct pwm_ops *ops = chip->ops; > @@ -730,29 +730,50 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) > > BUG_ON(WFHWSIZE < ops->sizeof_wfhw); > > - scoped_guard(pwmchip, chip) { > - > - ret = __pwm_read_waveform(chip, pwm, &wfhw); > - if (ret) > - return ret; > + ret = __pwm_read_waveform(chip, pwm, &wfhw); > + if (ret) > + return ret; > > - ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf); > - if (ret) > - return ret; > - } > + ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf); > + if (ret) > + return ret; > > pwm_wf2state(&wf, state); > > } else if (ops->get_state) { > - scoped_guard(pwmchip, chip) > - ret = ops->get_state(chip, pwm, state); > - > + ret = ops->get_state(chip, pwm, state); > trace_pwm_get(pwm, state, ret); > } > > return ret; > } I don't understand why you introduce __pwm_get_state_hw() (a variant of pwm_get_state_hw() that expects the caller to hold the chip lock) when the single caller (apart from plain pwm_get_state_hw()) could just continue to use pwm_get_state_hw(). In principle I'm open to such a patch and wonder if there is already a merge plan for this series. If you send a simpler patch soon with the same objective, I'll make sure it goes into v6.13-rc1 in the assumption that it's to late for the whole series to go in then. Or do you still target 6.13-rc1 for the spi bits? Then it would probably better to let this patch go in with the rest via the spi tree. Best regards Uwe
On 10/29/24 3:05 AM, Uwe Kleine-König wrote: > Hello David, > > On Wed, Oct 23, 2024 at 03:59:08PM -0500, David Lechner wrote: >> Export the pwm_get_state_hw() function. This is useful in cases where >> we want to know what the hardware is actually doing, rather than what >> what we requested it should do. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> >> v4 changes: new patch in v4 >> >> And FYI for Uwe and Jonathan, there are a couple of other series >> introducing PWM conversion triggers that could make use of this >> so that the sampling_frequency attribute can return the actual rate >> rather than the requested rate. >> >> Already applied: >> https://lore.kernel.org/linux-iio/20241015-ad7606_add_iio_backend_support-v5-4-654faf1ae08c@baylibre.com/ >> >> Under review: >> https://lore.kernel.org/linux-iio/aea7f92b-3d12-4ced-b1c8-90bcf1d992d3@baylibre.com/T/#m1377d5acd7e996acd1f59038bdd09f0742d3ac35 >> --- >> drivers/pwm/core.c | 55 +++++++++++++++++++++++++++++++++++++---------------- >> include/linux/pwm.h | 1 + >> 2 files changed, 40 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >> index 634be56e204b..a214d0165d09 100644 >> --- a/drivers/pwm/core.c >> +++ b/drivers/pwm/core.c >> @@ -718,7 +718,7 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state) >> } >> EXPORT_SYMBOL_GPL(pwm_apply_atomic); >> >> -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) >> +static int __pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) >> { >> struct pwm_chip *chip = pwm->chip; >> const struct pwm_ops *ops = chip->ops; >> @@ -730,29 +730,50 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state) >> >> BUG_ON(WFHWSIZE < ops->sizeof_wfhw); >> >> - scoped_guard(pwmchip, chip) { >> - >> - ret = __pwm_read_waveform(chip, pwm, &wfhw); >> - if (ret) >> - return ret; >> + ret = __pwm_read_waveform(chip, pwm, &wfhw); >> + if (ret) >> + return ret; >> >> - ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf); >> - if (ret) >> - return ret; >> - } >> + ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf); >> + if (ret) >> + return ret; >> >> pwm_wf2state(&wf, state); >> >> } else if (ops->get_state) { >> - scoped_guard(pwmchip, chip) >> - ret = ops->get_state(chip, pwm, state); >> - >> + ret = ops->get_state(chip, pwm, state); >> trace_pwm_get(pwm, state, ret); >> } >> >> return ret; >> } > > I don't understand why you introduce __pwm_get_state_hw() (a variant of > pwm_get_state_hw() that expects the caller to hold the chip lock) when the > single caller (apart from plain pwm_get_state_hw()) could just continue > to use pwm_get_state_hw(). Hmm... it seems like I thought there was a good reason for it at the time, but looking at it again, I agree with your assessment. > > In principle I'm open to such a patch and wonder if there is already a > merge plan for this series. If you send a simpler patch soon with the > same objective, I'll make sure it goes into v6.13-rc1 in the assumption > that it's to late for the whole series to go in then. Or do you still > target 6.13-rc1 for the spi bits? Then it would probably better to let > this patch go in with the rest via the spi tree. The SPI offload stuff is not likely to be merged soon. But there is ad7606 + AXI ADC support from Guillaume that was just merged that could make use of this. So I can send this as a stand-alone patch so that it can be made available for that too.
On Sat, Oct 26, 2024 at 04:18:37PM +0100, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:12 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > Add a new binding for using a PWM signal as a trigger for SPI offloads. > > I don't have a better suggestion for this, but it does smell rather like > other bridge binding (iio-hwmon for example) where we have had push back on > representing something that doesn't really exist but is just a way to > tie two bits of hardware together. Those kind of exist because we snuck > them in a long time back when no one was paying attention. I dunno. iio-hwmon to me is a particularly strange one, because it is the exact same device being used in different subsystems. Like that voltage monitoring device with 10000 compatibles that I CCed you and Peter on the other day feels like it should really in your subsytem. A "hwmon" isn't a class of device at all. This however, I think is more like pwm-clock (or clk-pwm, they both exist and are opposites) where the node is used to change the type of device rather than the subsystem using it. > So this one may need more explanation and justification and I'd definitely > like some DT maintainer review on this at a fairly early stage! Ye, /shrug. Maybe the others have dissenting opinions. I'd like to hear from them, but I don't personally have a problem with this.
On 10/31/24 1:16 PM, Conor Dooley wrote: > On Sat, Oct 26, 2024 at 04:18:37PM +0100, Jonathan Cameron wrote: >> On Wed, 23 Oct 2024 15:59:12 -0500 >> David Lechner <dlechner@baylibre.com> wrote: >> >>> Add a new binding for using a PWM signal as a trigger for SPI offloads. >> >> I don't have a better suggestion for this, but it does smell rather like >> other bridge binding (iio-hwmon for example) where we have had push back on >> representing something that doesn't really exist but is just a way to >> tie two bits of hardware together. Those kind of exist because we snuck >> them in a long time back when no one was paying attention. > > I dunno. iio-hwmon to me is a particularly strange one, because it is > the exact same device being used in different subsystems. Like that > voltage monitoring device with 10000 compatibles that I CCed you and > Peter on the other day feels like it should really in your subsytem. A > "hwmon" isn't a class of device at all. > > This however, I think is more like pwm-clock (or clk-pwm, they both > exist and are opposites) where the node is used to change the type of > device rather than the subsystem using it. Yes, this is the key reason for the binding. When I was looking at the trigger bindings in the leds subsystem, I came to the realization that we need some way to get the underlying type of the trigger. In the leds bindings, I don't think this was intentional, but effectively this is done with the linux,default-trigger property. So unless there is a reason why copying the clk-pwm/pwm-clock style bindings is not a good idea, that seems the preferable way to do it to me and I'll stick with that. > >> So this one may need more explanation and justification and I'd definitely >> like some DT maintainer review on this at a fairly early stage! > > Ye, /shrug. Maybe the others have dissenting opinions. I'd like to hear > from them, but I don't personally have a problem with this.
In this revision, I ended up changing quite a bit more that I was expecting. In the DT bindings, I ended up dropping the #spi-offload-cells and spi-offload properties. A couple of reasons for this: 1. Several people commented that it is odd to have a separate provider/ consumer binding for something that already has a parent/child relationship (both on this series and in another unrelated series with io-backends). For now, the only SPI offload provider is the AXI SPI Engine, which is a SPI controller. 2. In a discussion unrelated to this series, but related to the idea of SPI offloads [1], it became apparent that the proposed use for the cells to select triggers and tx/rx streams doesn't actually work for that case. 3. In offline review, it was suggested that assigning specific offloads to specific peripherals may be too restrictive, e.g. if there are controllers that have pools of identical offloads. This idea of pools of generic offloads has also come up in previous discussions on the mailing list. [1]: https://lore.kernel.org/linux-iio/e5310b63-9dc4-43af-9fbe-0cc3b604ab8b@baylibre.com/ So the idea is that if we do end up needing to use DT to assign certain resources (triggers, DMA channels, etc.) to specific peripherals, we would make a mapping attribute in the controller node rather than using phandle cells. But we don't need this yet, so it isn't present in The current patches. And if we ever end up with a SPI offload provider that is not a SPI controller, we can bring back the #spi-offload-cells and spi-offload properties. Regarding the SPI core changes, there are more details on each individual patch, but a lot has changed there due to adding a second ADC consumer that is wired up differently. The AD7944 is as pictured below, but the AD4695 that has been added has the ADC chip itself as the SPI offload trigger source, which I found to not be compatible with many of the assumptions we made in previous revisions. So there isn't much that is still the same as in previous revisions. --- Changes in v4: - Dropped #spi-offload-cells and spi-offload properties from DT bindings. - Made an attempt at a more generic trigger interface instead of using clk framework. This also includes a new driver for a generic PWM trigger. - Addressed IIO review comments. - Added new patches for iio/adc/ad4695 as 2nd user of SPI offload. - Link to v3: https://lore.kernel.org/r/20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com Changes in v3: - Reworked DT bindings to have things physically connected to the SPI controller be properties of the SPI controller and use more conventional provider/consumer properties. - Added more SPI APIs for peripheral drivers to use to get auxillary offload resources, like triggers. - Link to v2: https://lore.kernel.org/r/20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com Individual patches have more details on these changes and earlier revisions too. --- As a recap, here is the background and end goal of this series: The AXI SPI Engine is a SPI controller that has the ability to record a series of SPI transactions and then play them back using a hardware trigger. This allows operations to be performed, repeating many times, without any CPU intervention. This is needed for achieving high data rates (millions of samples per second) from ADCs and DACs that are connected via a SPI bus. The offload hardware interface consists of a trigger input and a data output for the RX data. These are connected to other hardware external to the SPI controller. To record one or more transactions, commands and TX data are written to memories in the controller (RX buffer is not used since RX data gets streamed to an external sink). This sequence of transactions can then be played back when the trigger input is asserted. This series includes core SPI support along with the first SPI controller (AXI SPI Engine) and SPI peripheral (AD7944 ADC) that use them. This enables capturing analog data at 2 million samples per second. The hardware setup looks like this: +-------------------------------+ +------------------+ | | | | | SOC/FPGA | | AD7944 ADC | | +---------------------+ | | | | | AXI SPI Engine | | | | | | SPI Bus ============ SPI Bus | | | | | | | | | +---------------+ | | | | | | | Offload 0 | | | +------------------+ | | | RX DATA OUT > > > > | | | | TRIGGER IN < < < v | | | +---------------+ | ^ v | | +---------------------+ ^ v | | | AXI PWM | ^ v | | | CH0 > ^ v | | +---------------------+ v | | | AXI DMA | v | | | CH0 < < < | | +---------------------+ | | | +-------------------------------+ --- David Lechner (15): pwm: core: export pwm_get_state_hw() spi: add basic support for SPI offloading spi: offload: add support for hardware triggers spi: dt-bindings: add trigger-source.yaml spi: dt-bindings: add PWM SPI offload trigger spi: offload-trigger: add PWM trigger driver spi: add offload TX/RX streaming APIs spi: dt-bindings: axi-spi-engine: add SPI offload properties spi: axi-spi-engine: implement offload support iio: buffer-dmaengine: document iio_dmaengine_buffer_setup_ext iio: buffer-dmaengine: add devm_iio_dmaengine_buffer_setup_ext2() iio: adc: ad7944: don't use storagebits for sizing iio: adc: ad7944: add support for SPI offload dt-bindings: iio: adc: adi,ad4695: add SPI offload properties iio: adc: ad4695: Add support for SPI offload .../devicetree/bindings/iio/adc/adi,ad4695.yaml | 13 +- .../bindings/spi/adi,axi-spi-engine.yaml | 22 + .../devicetree/bindings/spi/trigger-pwm.yaml | 39 ++ .../devicetree/bindings/spi/trigger-source.yaml | 28 ++ drivers/iio/adc/Kconfig | 2 + drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++-- drivers/iio/adc/ad7944.c | 249 ++++++++++- drivers/iio/buffer/industrialio-buffer-dmaengine.c | 104 ++++- drivers/pwm/core.c | 55 ++- drivers/spi/Kconfig | 16 + drivers/spi/Makefile | 4 + drivers/spi/spi-axi-spi-engine.c | 273 +++++++++++- drivers/spi/spi-offload-trigger-pwm.c | 169 ++++++++ drivers/spi/spi-offload.c | 446 +++++++++++++++++++ drivers/spi/spi.c | 10 + include/linux/iio/buffer-dmaengine.h | 5 + include/linux/pwm.h | 1 + include/linux/spi/spi-offload.h | 166 ++++++++ include/linux/spi/spi.h | 19 + 19 files changed, 1995 insertions(+), 96 deletions(-) --- base-commit: 6c4b0dd7d0df3a803766d4954dc064dc57aeda17 change-id: 20240510-dlech-mainline-spi-engine-offload-2-afce3790b5ab Best regards,