| Message ID | 20241023-dlech-mainline-spi-engine-offload-2-v4-2-f8125b99f5a1@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
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@baylibre.com> > --- Hi David, Just one minor comment... > > 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 Cool, was not aware of this :) > + > +#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; > + } > + > + return offloads; > +} > +EXPORT_SYMBOL_GPL(devm_spi_offload_alloc); > + > +static void spi_offload_put(void *data) > +{ > + struct spi_offload *offload = data; > + > + offload->spi = NULL; > + put_device(offload->provider_dev); > +} > + > +/** > + * devm_spi_offload_get() - Get an offload instance > + * @dev: Device for devm purposes > + * @spi: SPI device to use for the transfers > + * @config: Offload configuration > + * > + * Peripheral drivers call this function to get an offload instance that meets > + * the requirements specified in @config. If no suitable offload instance is > + * available, -ENODEV is returned. > + * > + * Return: Offload instance or error on failure. > + */ > +struct spi_offload *devm_spi_offload_get(struct device *dev, > + struct spi_device *spi, > + const struct spi_offload_config *config) > +{ > + struct spi_offload *offload; > + int ret; > + > + if (!spi || !config) > + return ERR_PTR(-EINVAL); > + > + if (!spi->controller->get_offload) > + return ERR_PTR(-ENODEV); > + > + offload = spi->controller->get_offload(spi, config); > + if (IS_ERR(offload)) > + return offload; > + > + if (offload->spi) > + return ERR_PTR(-EBUSY); > + > + offload->spi = spi; > + get_device(offload->provider_dev); Isn't this redundant? From what I can tell, we're assuming that the spi controller (of the spi device) is the offload provider. Therefore, getting an extra reference for it does not really seems necessary. The device cannot go away without under the spi_device feet. If that could happen, then we would also need to take care about callback access and things like that. Going this way, it would also be arguable to have a try_module_get(). - Nuno Sá
On 10/24/24 8:27 AM, Nuno Sá wrote: > On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote: >> Add the basic infrastructure to support SPI offload providers and >> consumers. >> ... >> +struct spi_offload *devm_spi_offload_get(struct device *dev, >> + struct spi_device *spi, >> + const struct spi_offload_config *config) >> +{ >> + struct spi_offload *offload; >> + int ret; >> + >> + if (!spi || !config) >> + return ERR_PTR(-EINVAL); >> + >> + if (!spi->controller->get_offload) >> + return ERR_PTR(-ENODEV); >> + >> + offload = spi->controller->get_offload(spi, config); >> + if (IS_ERR(offload)) >> + return offload; >> + >> + if (offload->spi) >> + return ERR_PTR(-EBUSY); >> + >> + offload->spi = spi; >> + get_device(offload->provider_dev); > > Isn't this redundant? From what I can tell, we're assuming that the spi controller > (of the spi device) is the offload provider. Therefore, getting an extra reference > for it does not really seems necessary. The device cannot go away without under the > spi_device feet. If that could happen, then we would also need to take care about > callback access and things like that. Going this way, it would also be arguable to > have a try_module_get(). > > - Nuno Sá > > Yes, you are right that we don't really need to take a reference to the device. This was left over from when I made an implementation that assumed the offload provider could be anything, not just a SPI controller.
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@baylibre.com> > --- > > 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)? Now that I also look at this better, I would not do it like this. I would keep it simple and just allocate one spi_offload object and be done with it. In the future and when we actually support more than one instance you could introduce a devm_spi_offload_alloc_array() variant and I'm still not sure if it's that useful. Anyways this is just personal preference I guess... [1]: https://elixir.bootlin.com/linux/v6.12-rc4/source/drivers/base/devres.c#L35 - Nuno Sá
Oct 25, 2024 14:59:20 Nuno Sá <noname.nuno@gmail.com>: > 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@baylibre.com> >> --- >> >> 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á
On Wed, 23 Oct 2024 15:59:09 -0500 David Lechner <dlechner@baylibre.com> 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@baylibre.com> A few minor additions to what has already been raised. Jonathan > 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> ? If it is needed for later patch bring it in then. > +#include <linux/spi/spi-offload.h> > +#include <linux/spi/spi.h> > +#include <linux/types.h> > > + > +/** > + * devm_spi_offload_get() - Get an offload instance > + * @dev: Device for devm purposes > + * @spi: SPI device to use for the transfers > + * @config: Offload configuration > + * > + * Peripheral drivers call this function to get an offload instance that meets > + * the requirements specified in @config. If no suitable offload instance is > + * available, -ENODEV is returned. > + * > + * Return: Offload instance or error on failure. > + */ > +struct spi_offload *devm_spi_offload_get(struct device *dev, > + struct spi_device *spi, > + const struct spi_offload_config *config) > +{ > + struct spi_offload *offload; > + int ret; > + > + if (!spi || !config) > + return ERR_PTR(-EINVAL); > + > + if (!spi->controller->get_offload) > + return ERR_PTR(-ENODEV); > + > + offload = spi->controller->get_offload(spi, config); Why let this return an offload that is already in use? Maybe make that a problem for the spi controller Seems odd to pass it spi then set it later. I.e. have this return ERR_PTR(-EBUSY); > + if (IS_ERR(offload)) > + return offload; > + > + if (offload->spi) > + return ERR_PTR(-EBUSY); > + > + offload->spi = spi; > + get_device(offload->provider_dev); > + > + ret = devm_add_action_or_reset(dev, spi_offload_put, offload); > + if (ret) > + return ERR_PTR(ret); > + > + return offload; > +} > +EXPORT_SYMBOL_GPL(devm_spi_offload_get);
On Wed, Oct 23, 2024 at 03:59:09PM -0500, David Lechner wrote: > +struct spi_offload *devm_spi_offload_alloc(struct device *dev, > + size_t num_offloads, > + size_t priv_size) > + privs = (void *)(offloads + num_offloads); Casting to or from void * is generally suspicious. > + void *priv = privs + i * priv_size; Can we have some brackets here for clarity please?
On 10/26/24 10:05 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:09 -0500 > David Lechner <dlechner@baylibre.com> wrote: > ... >> +struct spi_offload *devm_spi_offload_get(struct device *dev, >> + struct spi_device *spi, >> + const struct spi_offload_config *config) >> +{ >> + struct spi_offload *offload; >> + int ret; >> + >> + if (!spi || !config) >> + return ERR_PTR(-EINVAL); >> + >> + if (!spi->controller->get_offload) >> + return ERR_PTR(-ENODEV); >> + >> + offload = spi->controller->get_offload(spi, config); > > Why let this return an offload that is already in use? > Maybe make that a problem for the spi controller > Seems odd to pass it spi then set it later. > > I.e. have this return ERR_PTR(-EBUSY); I would expect that to effectively be handled by the if (IS_ERR(offload)) below. Only the controller can know which offloads are already in use, so the callback should return the appropriate -EBUSY in that case. > > >> + if (IS_ERR(offload)) >> + return offload; >> + >> + if (offload->spi) >> + return ERR_PTR(-EBUSY); >> + >> + offload->spi = spi; >> + get_device(offload->provider_dev); >> + >> + ret = devm_add_action_or_reset(dev, spi_offload_put, offload); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + return offload; >> +} >> +EXPORT_SYMBOL_GPL(devm_spi_offload_get);
On 11/11/24 11:14 AM, David Lechner wrote: > On 10/26/24 10:05 AM, Jonathan Cameron wrote: >> On Wed, 23 Oct 2024 15:59:09 -0500 >> David Lechner <dlechner@baylibre.com> wrote: >> > > ... > >>> +struct spi_offload *devm_spi_offload_get(struct device *dev, >>> + struct spi_device *spi, >>> + const struct spi_offload_config *config) >>> +{ >>> + struct spi_offload *offload; >>> + int ret; >>> + >>> + if (!spi || !config) >>> + return ERR_PTR(-EINVAL); >>> + >>> + if (!spi->controller->get_offload) >>> + return ERR_PTR(-ENODEV); >>> + >>> + offload = spi->controller->get_offload(spi, config); >> >> Why let this return an offload that is already in use? >> Maybe make that a problem for the spi controller >> Seems odd to pass it spi then set it later. >> >> I.e. have this return ERR_PTR(-EBUSY); > > I would expect that to effectively be handled by the > if (IS_ERR(offload)) below. Only the controller can > know which offloads are already in use, so the callback > should return the appropriate -EBUSY in that case. Just realized I said exactly what you said! Will fix this. > >> >> >>> + if (IS_ERR(offload)) >>> + return offload; >>> + >>> + if (offload->spi) >>> + return ERR_PTR(-EBUSY); >>> + >>> + offload->spi = spi; >>> + get_device(offload->provider_dev); >>> + >>> + ret = devm_add_action_or_reset(dev, spi_offload_put, offload); >>> + if (ret) >>> + return ERR_PTR(ret); >>> + >>> + return offload; >>> +} >>> +EXPORT_SYMBOL_GPL(devm_spi_offload_get); >
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; + } + + return offloads; +} +EXPORT_SYMBOL_GPL(devm_spi_offload_alloc); + +static void spi_offload_put(void *data) +{ + struct spi_offload *offload = data; + + offload->spi = NULL; + put_device(offload->provider_dev); +} + +/** + * devm_spi_offload_get() - Get an offload instance + * @dev: Device for devm purposes + * @spi: SPI device to use for the transfers + * @config: Offload configuration + * + * Peripheral drivers call this function to get an offload instance that meets + * the requirements specified in @config. If no suitable offload instance is + * available, -ENODEV is returned. + * + * Return: Offload instance or error on failure. + */ +struct spi_offload *devm_spi_offload_get(struct device *dev, + struct spi_device *spi, + const struct spi_offload_config *config) +{ + struct spi_offload *offload; + int ret; + + if (!spi || !config) + return ERR_PTR(-EINVAL); + + if (!spi->controller->get_offload) + return ERR_PTR(-ENODEV); + + offload = spi->controller->get_offload(spi, config); + if (IS_ERR(offload)) + return offload; + + if (offload->spi) + return ERR_PTR(-EBUSY); + + offload->spi = spi; + get_device(offload->provider_dev); + + ret = devm_add_action_or_reset(dev, spi_offload_put, offload); + if (ret) + return ERR_PTR(ret); + + return offload; +} +EXPORT_SYMBOL_GPL(devm_spi_offload_get); diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h new file mode 100644 index 000000000000..92a557533b83 --- /dev/null +++ b/include/linux/spi/spi-offload.h @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 Analog Devices Inc. + * Copyright (C) 2024 BayLibre, SAS + */ + +/* + * SPI Offloading support. + * + * Some SPI controllers support offloading of SPI transfers. Essentially, this + * is the ability for a SPI controller to perform SPI transfers with minimal + * or even no CPU intervention, e.g. via a specialized SPI controller with a + * hardware trigger or via a conventional SPI controller using a non-Linux MCU + * processor core to offload the work. + */ + +#ifndef __LINUX_SPI_OFFLOAD_H +#define __LINUX_SPI_OFFLOAD_H + +#include <linux/types.h> + +MODULE_IMPORT_NS(SPI_OFFLOAD); + +struct device; +struct spi_device; + +/* Offload can be triggered by external hardware event. */ +#define SPI_OFFLOAD_CAP_TRIGGER BIT(0) +/* Offload can record and then play back TX data when triggered. */ +#define SPI_OFFLOAD_CAP_TX_STATIC_DATA BIT(1) +/* Offload can get TX data from an external stream source. */ +#define SPI_OFFLOAD_CAP_TX_STREAM_DMA BIT(2) +/* Offload can send RX data to an external stream sink. */ +#define SPI_OFFLOAD_CAP_RX_STREAM_DMA BIT(3) + +/** + * struct spi_offload_config - offload configuration + * + * This is used to request an offload with specific configuration. + */ +struct spi_offload_config { + /** @capability_flags: required capabilities. See %SPI_OFFLOAD_CAP_* */ + u32 capability_flags; +}; + +/** + * struct spi_offload - offload instance + */ +struct spi_offload { + /** @provider_dev: for get/put reference counting */ + struct device *provider_dev; + /** @spi: SPI device that is currently using the offload */ + struct spi_device *spi; + /** @priv: provider driver private data */ + void *priv; +}; + +struct spi_offload *devm_spi_offload_alloc(struct device *dev, + size_t num_offloads, + size_t priv_size); +struct spi_offload *devm_spi_offload_get(struct device *dev, struct spi_device *spi, + const struct spi_offload_config *config); + +#endif /* __LINUX_SPI_OFFLOAD_H */ diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 8497f4747e24..c230d6a209ee 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -31,6 +31,9 @@ struct spi_transfer; struct spi_controller_mem_ops; struct spi_controller_mem_caps; struct spi_message; +struct spi_controller_offload_ops; +struct spi_offload; +struct spi_offload_config; /* * INTERFACES between SPI master-side drivers and SPI slave protocol handlers, @@ -496,6 +499,9 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @mem_ops: optimized/dedicated operations for interactions with SPI memory. * This field is optional and should only be implemented if the * controller has native support for memory like operations. + * @get_offload: callback for controllers with offload support to get matching + * offload instance. Implementations should return -ENODEV if no match is + * found. * @mem_caps: controller capabilities for the handling of memory operations. * @unprepare_message: undo any work done by prepare_message(). * @target_abort: abort the ongoing transfer request on an SPI target controller @@ -740,6 +746,9 @@ struct spi_controller { const struct spi_controller_mem_ops *mem_ops; const struct spi_controller_mem_caps *mem_caps; + struct spi_offload *(*get_offload)(struct spi_device *spi, + const struct spi_offload_config *config); + /* GPIO chip select */ struct gpio_desc **cs_gpiods; bool use_gpio_descriptors; @@ -1108,6 +1117,7 @@ struct spi_transfer { * @state: for use by whichever driver currently owns the message * @opt_state: for use by whichever driver currently owns the message * @resources: for resource management when the SPI message is processed + * @offload: (optional) offload instance used by this message * * A @spi_message is used to execute an atomic sequence of data transfers, * each represented by a struct spi_transfer. The sequence is "atomic" @@ -1168,6 +1178,12 @@ struct spi_message { */ void *opt_state; + /* + * Optional offload instance used by this message. This must be set + * by the peripheral driver before calling spi_optimize_message(). + */ + struct spi_offload *offload; + /* List of spi_res resources when the SPI message is processed */ struct list_head resources; };
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@baylibre.com> --- 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(+)