Re: [PATCH RFC v4 02/15] spi: add basic support for SPI offloading

[Nuno Sá <noname.nuno@xxxxxxxxx>]

Oct 25, 2024 14:59:20 Nuno Sá <noname.nuno@xxxxxxxxx>:

> On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote:
>> Add the basic infrastructure to support SPI offload providers and
>> consumers.
>>
>> SPI offloading is a feature that allows the SPI controller to perform
>> transfers without any CPU intervention. This is useful, e.g. for
>> high-speed data acquisition.
>>
>> SPI controllers with offload support need to implement the get_offload
>> callback and can use the devm_spi_offload_alloc() to allocate offload
>> instances.
>>
>> SPI peripheral drivers will call devm_spi_offload_get() to get a
>> reference to the matching offload instance. This offload instance can
>> then be attached to a SPI message to request offloading that message.
>>
>> It is expected that SPI controllers with offload support will check for
>> the offload instance in the SPI message in the optimize_message()
>> callback and handle it accordingly.
>>
>> CONFIG_SPI_OFFLOAD is intended to be a select-only option. Both
>> consumer and provider drivers should `select SPI_OFFLOAD` in their
>> Kconfig to ensure that the SPI core is built with offload support.
>>
>> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
>> ---
>>
>> v4 changes:
>> * SPI offload functions moved to a separate file instead of spi.c
>>   (spi.c is already too long).
>> * struct spi_offload and devm_spi_offload_get() are back, similar to
>>   but improved over v1. This avoids having to pass the function ID
>>   string to every function call and re-lookup the offload instance.
>> * offload message prepare/unprepare functions are removed. Instead the
>>   existing optimize/unoptimize functions should be used. Setting
>>   spi_message::offload pointer is used as a flag to differentiate
>>   between an offloaded message and a regular message.
>>
>> v3 changes:
>> * Minor changes to doc comments.
>> * Changed to use phandle array for spi-offloads.
>> * Changed id to string to make use of spi-offload-names.
>>
>> v2 changes:
>> * This is a rework of "spi: add core support for controllers with offload
>>   capabilities" from v1.
>> * The spi_offload_get() function that Nuno didn't like is gone. Instead,
>>   there is now a mapping callback that uses the new generic devicetree
>>   binding to request resources automatically when a SPI device is probed.
>> * The spi_offload_enable/disable() functions for dealing with hardware
>>   triggers are deferred to a separate patch.
>> * This leaves adding spi_offload_prepare/unprepare() which have been
>>   reworked to be a bit more robust.
>> ---
>>  drivers/spi/Kconfig             |   3 ++
>>  drivers/spi/Makefile            |   1 +
>>  drivers/spi/spi-offload.c       | 104 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/spi/spi-offload.h |  64 +++++++++++++++++++++++++
>>  include/linux/spi/spi.h         |  16 +++++++
>>  5 files changed, 188 insertions(+)
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 823797217404..d65074b85f62 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -55,6 +55,9 @@ config SPI_MEM
>>       This extension is meant to simplify interaction with SPI memories
>>       by providing a high-level interface to send memory-like commands.
>>  
>> +config SPI_OFFLOAD
>> +   bool
>> +
>>  comment "SPI Master Controller Drivers"
>>  
>>  config SPI_AIROHA_SNFI
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index a9b1bc259b68..6a470eb475a2 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -10,6 +10,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
>>  obj-$(CONFIG_SPI_MASTER)       += spi.o
>>  obj-$(CONFIG_SPI_MEM)          += spi-mem.o
>>  obj-$(CONFIG_SPI_MUX)          += spi-mux.o
>> +obj-$(CONFIG_SPI_OFFLOAD)      += spi-offload.o
>>  obj-$(CONFIG_SPI_SPIDEV)       += spidev.o
>>  obj-$(CONFIG_SPI_LOOPBACK_TEST)        += spi-loopback-test.o
>>  
>> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
>> new file mode 100644
>> index 000000000000..c344cbf50bdb
>> --- /dev/null
>> +++ b/drivers/spi/spi-offload.c
>> @@ -0,0 +1,104 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2024 Analog Devices Inc.
>> + * Copyright (C) 2024 BayLibre, SAS
>> + */
>> +
>> +#define DEFAULT_SYMBOL_NAMESPACE SPI_OFFLOAD
>> +
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/export.h>
>> +#include <linux/mutex.h>
>> +#include <linux/property.h>
>> +#include <linux/spi/spi-offload.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * devm_spi_offload_alloc() - Allocate offload instances
>> + * @dev: Device for devm purposes
>> + * @num_offloads: Number of offloads to allocate
>> + * @priv_size: Size of private data to allocate for each offload
>> + *
>> + * Offload providers should use this to allocate offload instances.
>> + *
>> + * Return: Pointer to array of offloads or error on failure.
>> + */
>> +struct spi_offload *devm_spi_offload_alloc(struct device *dev,
>> +                      size_t num_offloads,
>> +                      size_t priv_size)
>> +{
>> +   struct spi_offload *offloads;
>> +   void *privs;
>> +   size_t i;
>> +
>> +   offloads = devm_kcalloc(dev, num_offloads, sizeof(*offloads) + priv_size,
>> +               GFP_KERNEL);
>> +   if (!offloads)
>> +       return ERR_PTR(-ENOMEM);
>> +
>> +   privs = (void *)(offloads + num_offloads);
>> +
>> +   for (i = 0; i < num_offloads; i++) {
>> +       struct spi_offload *offload = offloads + i;
>> +       void *priv = privs + i * priv_size;
>> +
>> +       offload->provider_dev = dev;
>> +       offload->priv = priv;
>> +   }
>
> Hmm looking at the spi_engine implementation, got me thinking about this. I think
> like this we might mess up with natural alignments which can be pretty nasty. Maybe
> do something like devres [1] (I guess we do not need the flex array though)?
>

Actually we should use the flex array or something similar to what IIO does.

PS: just trying some email client from my phone so hopefully this is not too messed up.

- Nuno Sá