Re: [PATCH RFC v3 2/9] spi: add basic support for SPI offloading

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

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á