On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > 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. > > This patch adds the basic infrastructure to support SPI offloading. It > introduces new callbacks that are to be implemented by controllers with > offload capabilities. > > On SPI device probe, the standard spi-offloads devicetree property is > parsed and passed to the controller driver to reserve the resources > requested by the peripheral via the map_channel() callback. > > The peripheral driver can then use spi_offload_prepare() to load a SPI > message into the offload hardware. > > If the controller supports it, this message can then be passed to the > SPI message queue as if it was a normal message. Future patches will > will also implement a way to use a hardware trigger to start the message > transfers rather than going through the message queue. > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > --- > > 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. > Given my reply to the cover you can start calling me names already :). But even with that function back I think we need a more explicit provider/consumer logic. > 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. > > In the previous review, Mark suggested that these functions should not > be separate from the spi_[un]optimize() functions. I understand the > reasoning behind that. However, it seems like there are two different > kinds of things going on here. Currently, spi_optimize() only performs > operations on the message data structures and doesn't poke any hardware. > This makes it free to be use by any peripheral without worrying about > tying up any hardware resources while the message is "optimized". On the > other hand, spi_offload_prepare() is poking hardware, so we need to be > more careful about how it is used. And in these cases, we need a way to > specify exactly which hardware resources it should use, which it is > currently doing with the extra ID parameter. > --- > drivers/spi/spi.c | 123 > ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 57 ++++++++++++++++++++++ > 2 files changed, 180 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index d4da5464dbd0..d01b2e5c8c44 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -2477,6 +2477,51 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, > struct spi_device *spi, > of_spi_parse_dt_cs_delay(nc, &spi->cs_hold, "spi-cs-hold-delay-ns"); > of_spi_parse_dt_cs_delay(nc, &spi->cs_inactive, "spi-cs-inactive- > delay-ns"); > > + /* Offloads */ > + rc = of_count_phandle_with_args(nc, "spi-offloads", "#spi-offload- > cells"); > + if (rc > 0) { > + int num_offload = rc; > + > + if (!ctlr->offload_ops) { > + dev_err(&ctlr->dev, "SPI controller doesn't support > offloading\n"); > + return -EINVAL; > + } > + > + for (idx = 0; idx < num_offload; idx++) { > + struct of_phandle_args args; > + const char *offload_name = NULL; > + > + rc = of_parse_phandle_with_args(nc, "spi-offloads", > + "#spi-offload-cells", > + idx, &args); > + if (rc) { > + dev_err(&spi->dev, "Failed to parse offload > phandle %d: %d\n", > + idx, rc); > + return rc; > + } > + > + if (args.np != ctlr->dev.of_node) { > + dev_err(&spi->dev, "Offload phandle %d is not > for this SPI controller\n", > + idx); > + of_node_put(args.np); > + return -EINVAL; > + } > + > + of_property_read_string_index(nc, "spi-offload- > names", > + idx, &offload_name); > + > + rc = ctlr->offload_ops->map_channel(spi, > offload_name, > + args.args, > + args.args_count); In here, I would expect for the mapping to return something the core could then directly pass into the other operations. And hence saving controllers to always have to do a lookup in all the operations. It seems we may need a struct spi_offload * object that can be attached to a given spi_device and that can be directly passed and used by the specific offload operations. - Nuno Sá