This implements 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>
---

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/spi-axi-spi-engine.c | 341 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 337 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index cb3fdcbca2be..314f3afb9357 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -2,11 +2,13 @@
 /*
  * SPI-Engine SPI controller driver
  * Copyright 2015 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
  *  Author: Lars-Peter Clausen <lars@metafoo.de>
  */
 
 #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>
@@ -16,6 +18,7 @@
 #include <linux/platform_device.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 +26,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 +37,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)
@@ -77,6 +95,10 @@
 #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
+
 struct spi_engine_program {
 	unsigned int length;
 	uint16_t instructions[] __counted_by(length);
@@ -104,6 +126,12 @@ struct spi_engine_message_state {
 	uint8_t *rx_buf;
 };
 
+struct spi_engine_offload {
+	struct spi_device *spi;
+	const char *id;
+	bool prepared;
+};
+
 struct spi_engine {
 	struct clk *clk;
 	struct clk *ref_clk;
@@ -116,6 +144,10 @@ 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_engine_offload offload_priv[SPI_ENGINE_MAX_NUM_OFFLOADS];
 };
 
 static void spi_engine_program_add_cmd(struct spi_engine_program *p,
@@ -159,7 +191,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,
@@ -211,16 +243,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,
@@ -518,8 +558,11 @@ static irqreturn_t spi_engine_irq(int irq, void *devid)
 static int spi_engine_optimize_message(struct spi_message *msg)
 {
 	struct spi_engine_program p_dry, *p;
+	int ret;
 
-	spi_engine_precompile_message(msg);
+	ret = spi_engine_precompile_message(msg);
+	if (ret)
+		return ret;
 
 	p_dry.length = 0;
 	spi_engine_compile_message(msg, true, &p_dry);
@@ -531,7 +574,7 @@ static int spi_engine_optimize_message(struct spi_message *msg)
 	spi_engine_compile_message(msg, false, p);
 
 	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(
-						AXI_SPI_ENGINE_CUR_MSG_SYNC_ID));
+		msg->offload ? 0 : AXI_SPI_ENGINE_CUR_MSG_SYNC_ID));
 
 	msg->opt_state = p;
 
@@ -577,6 +620,12 @@ static int spi_engine_transfer_one_message(struct spi_controller *host,
 	unsigned int int_enable = 0;
 	unsigned long flags;
 
+	if (msg->offload) {
+		dev_err(&host->dev, "Single transfer offload not supported\n");
+		msg->status = -EOPNOTSUPP;
+		goto out;
+	}
+
 	/* reinitialize message state for this transfer */
 	memset(st, 0, sizeof(*st));
 	st->cmd_buf = p->instructions;
@@ -612,11 +661,279 @@ static int spi_engine_transfer_one_message(struct spi_controller *host,
 		msg->status = -ETIMEDOUT;
 	}
 
+out:
 	spi_finalize_current_message(host);
 
 	return msg->status;
 }
 
+static bool spi_engine_offload_id_eq(const char *id1, const char *id2)
+{
+	if (!id1 && !id2)
+		return true;
+
+	if (!id1 || !id2)
+		return false;
+
+	return strcmp(id1, id2) == 0;
+}
+
+static struct spi_engine_offload *spi_engine_get_offload(struct spi_device *spi,
+							 const char *id,
+							 unsigned int *offload_num)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	int i;
+
+	for (i = 0; i < SPI_ENGINE_MAX_NUM_OFFLOADS; i++) {
+		priv = &spi_engine->offload_priv[i];
+
+		if (priv->spi == spi && spi_engine_offload_id_eq(priv->id, id)) {
+			*offload_num = i;
+			return priv;
+		}
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+static int spi_engine_offload_map_channel(struct spi_device *spi,
+					  const char *id,
+					  const unsigned int *args,
+					  unsigned int num_args)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+
+	if (num_args != 1)
+		return -EINVAL;
+
+	if (args[0] >= SPI_ENGINE_MAX_NUM_OFFLOADS)
+		return -EINVAL;
+
+	priv = &spi_engine->offload_priv[args[0]];
+
+	if (priv->spi)
+		return -EBUSY;
+
+	priv->spi = spi;
+
+	priv->id = kstrdup(id, GFP_KERNEL);
+	if (!priv->id && id)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int spi_engine_offload_prepare(struct spi_device *spi, const char *id,
+				      struct spi_message *msg)
+{
+	struct spi_controller *host = 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;
+	struct spi_transfer *xfer;
+	void __iomem *cmd_addr;
+	void __iomem *sdo_addr;
+	size_t tx_word_count = 0;
+	unsigned int offload_num, i;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
+
+	if (priv->prepared)
+		return -EBUSY;
+
+	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;
+
+	cmd_addr = spi_engine->base + SPI_ENGINE_REG_OFFLOAD_CMD_FIFO(offload_num);
+	sdo_addr = spi_engine->base + SPI_ENGINE_REG_OFFLOAD_SDO_FIFO(offload_num);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (!xfer->tx_buf)
+			continue;
+
+		if (xfer->bits_per_word <= 8) {
+			const u8 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		} else if (xfer->bits_per_word <= 16) {
+			const u16 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len / 2; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		} else {
+			const u32 *buf = xfer->tx_buf;
+
+			for (i = 0; i < xfer->len / 4; i++)
+				writel_relaxed(buf[i], sdo_addr);
+		}
+	}
+
+	for (i = 0; i < p->length; i++)
+		writel_relaxed(p->instructions[i], cmd_addr);
+
+	msg->offload_state = (void *)(intptr_t)offload_num;
+	priv->prepared = true;
+
+	return 0;
+}
+
+static void spi_engine_offload_unprepare(struct spi_device *spi, const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv)) {
+		dev_warn(&spi->dev, "failed match offload in unprepare\n");
+		return;
+	}
+
+	writel_relaxed(1, spi_engine->base + SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
+	writel_relaxed(0, spi_engine->base + SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
+
+	priv->prepared = false;
+}
+
+static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
+					     const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num, reg;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
+
+	reg = readl_relaxed(spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
+	writel_relaxed(reg, spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+
+	return 0;
+}
+
+static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
+					       const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	struct spi_engine_offload *priv;
+	unsigned int offload_num, reg;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv)) {
+		dev_warn(&spi->dev, "failed match offload in disable\n");
+		return;
+	}
+
+	reg = readl_relaxed(spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
+	writel_relaxed(reg, spi_engine->base +
+			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
+}
+
+static struct clk *spi_engine_hw_trigger_get_clk(struct spi_device *spi,
+						 const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine_offload *priv;
+	struct of_phandle_args clkspec;
+	unsigned int offload_num;
+	struct clk *clk;
+	int ret;
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return ERR_CAST(priv);
+
+	ret = of_parse_phandle_with_args(host->dev.of_node, "trigger-sources",
+					 "#trigger-source-cells", offload_num,
+					 &clkspec);
+	if (ret)
+		return ERR_PTR(ret);
+
+	clk = of_clk_get_from_provider(&clkspec);
+
+	of_node_put(clkspec.np);
+
+	return clk;
+}
+
+static struct dma_chan *spi_engine_rx_stream_get_dma_chan(struct spi_device *spi,
+							  const char *id)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine_offload *priv;
+	unsigned int offload_num;
+	char channel_name[16];
+
+	priv = spi_engine_get_offload(spi, id, &offload_num);
+	if (IS_ERR(priv))
+		return ERR_CAST(priv);
+
+	snprintf(channel_name, sizeof(channel_name), "offload%d-rx", offload_num);
+
+	return dma_request_chan(&host->dev, channel_name);
+}
+
+static const struct spi_controller_offload_ops spi_engine_offload_ops = {
+	.map_channel = spi_engine_offload_map_channel,
+	.prepare = spi_engine_offload_prepare,
+	.unprepare = spi_engine_offload_unprepare,
+	.hw_trigger_mode_enable = spi_engine_hw_trigger_mode_enable,
+	.hw_trigger_mode_disable = spi_engine_hw_trigger_mode_disable,
+	.hw_trigger_get_clk = spi_engine_hw_trigger_get_clk,
+	.rx_stream_get_dma_chan = spi_engine_rx_stream_get_dma_chan,
+};
+
+static void spi_engine_cleanup(struct spi_device *spi)
+{
+	struct spi_controller *host = spi->controller;
+	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	int i;
+
+	/* remove spi device to offload mapping */
+	for (i = 0; i < SPI_ENGINE_MAX_NUM_OFFLOADS; i++) {
+		struct spi_engine_offload *priv = &spi_engine->offload_priv[i];
+
+		if (priv->spi == spi) {
+			priv->spi = NULL;
+			kfree(priv->id);
+			priv->id = NULL;
+		}
+	}
+}
+
 static void spi_engine_release_hw(void *p)
 {
 	struct spi_engine *spi_engine = p;
@@ -668,6 +985,19 @@ static int spi_engine_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (ADI_AXI_PCORE_VER_MINOR(version) >= 1) {
+		unsigned int sizes = readl(spi_engine->base +
+				SPI_ENGINE_REG_OFFLOAD_MEM_ADDR_WIDTH);
+
+		spi_engine->offload_ctrl_mem_size = 1 <<
+			FIELD_GET(SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_CMD, sizes);
+		spi_engine->offload_sdo_mem_size = 1 <<
+			FIELD_GET(SPI_ENGINE_SPI_OFFLOAD_MEM_WIDTH_SDO, sizes);
+	} else {
+		spi_engine->offload_ctrl_mem_size = SPI_ENGINE_OFFLOAD_CMD_FIFO_SIZE;
+		spi_engine->offload_sdo_mem_size = SPI_ENGINE_OFFLOAD_SDO_FIFO_SIZE;
+	}
+
 	writel_relaxed(0x00, spi_engine->base + SPI_ENGINE_REG_RESET);
 	writel_relaxed(0xff, spi_engine->base + SPI_ENGINE_REG_INT_PENDING);
 	writel_relaxed(0x00, spi_engine->base + SPI_ENGINE_REG_INT_ENABLE);
@@ -690,6 +1020,9 @@ static int spi_engine_probe(struct platform_device *pdev)
 	host->optimize_message = spi_engine_optimize_message;
 	host->unoptimize_message = spi_engine_unoptimize_message;
 	host->num_chipselect = 8;
+	host->offload_xfer_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	host->offload_ops = &spi_engine_offload_ops;
+	host->cleanup = spi_engine_cleanup;
 
 	/* Some features depend of the IP core version. */
 	if (ADI_AXI_PCORE_VER_MINOR(version) >= 2) {

-- 
2.43.0

On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> This implements 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>
> ---
> 

...


I'm likely missing something but you already have:

priv = &spi_engine->offload_priv[args[0]];

which seems that from FW you already got the offload index you need. Can't we
just save that index in struct spi_device and use that directly in the other
operations? Saving the trouble to save the id string and having to always call 
spi_engine_get_offload()?

> +
> 

...

> +}
> +
> +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
> *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv)) {
> +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
> +		return;
> +	}
> +
> +	writel_relaxed(1, spi_engine->base +
> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> +	writel_relaxed(0, spi_engine->base +
> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> +
> +	priv->prepared = false;
> +}
> +
> +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
> +					     const char *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num, reg;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	reg = readl_relaxed(spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> +	writel_relaxed(reg, spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +
> +	return 0;
> +}
> +
> +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
> +					       const char *id)
> +{
> +	struct spi_controller *host = spi->controller;
> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	struct spi_engine_offload *priv;
> +	unsigned int offload_num, reg;
> +
> +	priv = spi_engine_get_offload(spi, id, &offload_num);
> +	if (IS_ERR(priv)) {
> +		dev_warn(&spi->dev, "failed match offload in disable\n");
> +		return;
> +	}
> +
> +	reg = readl_relaxed(spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> +	writel_relaxed(reg, spi_engine->base +
> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> +}
> +

I would expect for the enable/disable() operations to act on the trigger. In
this case to enable/disable the clock...

- Nuno Sá

On 7/23/24 3:01 AM, Nuno Sá wrote:
> On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
>> This implements 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>
>> ---
>>
> 
> ...
> 
> 
> I'm likely missing something but you already have:
> 
> priv = &spi_engine->offload_priv[args[0]];
> 
> which seems that from FW you already got the offload index you need. Can't we
> just save that index in struct spi_device and use that directly in the other
> operations? Saving the trouble to save the id string and having to always call 
> spi_engine_get_offload()?

Saving the index in the struct spi_device would assume 1. that all SPI
peripherals can only use one SPI offload instance and 2. that all SPI
offload providers have #spi-offload-cells = <1> where the cell is the
index. I don't think either of these are safe assumptions.

> 
>> +
>>
> 
> ...
> 
>> +}
>> +
>> +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
>> *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
>> +		return;
>> +	}
>> +
>> +	writel_relaxed(1, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +	writel_relaxed(0, spi_engine->base +
>> SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
>> +
>> +	priv->prepared = false;
>> +}
>> +
>> +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
>> +					     const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv))
>> +		return PTR_ERR(priv);
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +
>> +	return 0;
>> +}
>> +
>> +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
>> +					       const char *id)
>> +{
>> +	struct spi_controller *host = spi->controller;
>> +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>> +	struct spi_engine_offload *priv;
>> +	unsigned int offload_num, reg;
>> +
>> +	priv = spi_engine_get_offload(spi, id, &offload_num);
>> +	if (IS_ERR(priv)) {
>> +		dev_warn(&spi->dev, "failed match offload in disable\n");
>> +		return;
>> +	}
>> +
>> +	reg = readl_relaxed(spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
>> +	writel_relaxed(reg, spi_engine->base +
>> +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
>> +}
>> +
> 
> I would expect for the enable/disable() operations to act on the trigger. In
> this case to enable/disable the clock...

I'm not opposed to doing that, but things would get more complicated if we
ever added more trigger types. Because then we would need to add some kind
of trigger device abstraction to wrap the enable and disable functions of
the various triggers.

It seems simpler to me to have the peripheral driver do it since it already
needs to get the clock device for other reasons anyway.

But I also got some internal feedback that it might make more sense to add
a trigger abstraction layer, so maybe that is something we should look into
more.

On Tue, 2024-07-23 at 09:19 -0500, David Lechner wrote:
> On 7/23/24 3:01 AM, Nuno Sá wrote:
> > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote:
> > > This implements 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>
> > > ---
> > > 
> > 
> > ...
> > 
> > 
> > I'm likely missing something but you already have:
> > 
> > priv = &spi_engine->offload_priv[args[0]];
> > 
> > which seems that from FW you already got the offload index you need. Can't we
> > just save that index in struct spi_device and use that directly in the other
> > operations? Saving the trouble to save the id string and having to always call 
> > spi_engine_get_offload()?
> 
> Saving the index in the struct spi_device would assume 1. that all SPI
> peripherals can only use one SPI offload instance and 2. that all SPI
> offload providers have #spi-offload-cells = <1> where the cell is the
> index. I don't think either of these are safe assumptions.
> 

Ok, I see what you mean. I guess I just don't like too much of that *id in all over
the place. But we may anyways have to come up with some kind of offload abstraction.

> > 
> > > +
> > > 
> > 
> > ...
> > 
> > > +}
> > > +
> > > +static void spi_engine_offload_unprepare(struct spi_device *spi, const char
> > > *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv)) {
> > > +		dev_warn(&spi->dev, "failed match offload in unprepare\n");
> > > +		return;
> > > +	}
> > > +
> > > +	writel_relaxed(1, spi_engine->base +
> > > SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> > > +	writel_relaxed(0, spi_engine->base +
> > > SPI_ENGINE_REG_OFFLOAD_RESET(offload_num));
> > > +
> > > +	priv->prepared = false;
> > > +}
> > > +
> > > +static int spi_engine_hw_trigger_mode_enable(struct spi_device *spi,
> > > +					     const char *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num, reg;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv))
> > > +		return PTR_ERR(priv);
> > > +
> > > +	reg = readl_relaxed(spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +	reg |= SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> > > +	writel_relaxed(reg, spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void spi_engine_hw_trigger_mode_disable(struct spi_device *spi,
> > > +					       const char *id)
> > > +{
> > > +	struct spi_controller *host = spi->controller;
> > > +	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> > > +	struct spi_engine_offload *priv;
> > > +	unsigned int offload_num, reg;
> > > +
> > > +	priv = spi_engine_get_offload(spi, id, &offload_num);
> > > +	if (IS_ERR(priv)) {
> > > +		dev_warn(&spi->dev, "failed match offload in disable\n");
> > > +		return;
> > > +	}
> > > +
> > > +	reg = readl_relaxed(spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +	reg &= ~SPI_ENGINE_OFFLOAD_CTRL_ENABLE;
> > > +	writel_relaxed(reg, spi_engine->base +
> > > +			    SPI_ENGINE_REG_OFFLOAD_CTRL(offload_num));
> > > +}
> > > +
> > 
> > I would expect for the enable/disable() operations to act on the trigger. In
> > this case to enable/disable the clock...
> 
> I'm not opposed to doing that, but things would get more complicated if we
> ever added more trigger types. Because then we would need to add some kind
> of trigger device abstraction to wrap the enable and disable functions of
> the various triggers.
> 

Yeah, to me is about symmetry... I'm of the opinion that consumers, ideally, would
not have to know about the type of the trigger. Just that we have a trigger and then
have an interface for what can we do with it. The one that needs to know about the
type is the controller driver proving offload capabilities. I guess we can have one
DT cell to specify the type of the trigger.

> It seems simpler to me to have the peripheral driver do it since it already
> needs to get the clock device for other reasons anyway.
> 
> But I also got some internal feedback that it might make more sense to add
> a trigger abstraction layer, so maybe that is something we should look into
> more.

Nice. I admit I did not though too much on an actual implementation so I'm not really
sure how feasible this is without getting overly complicated. But from a conceptual
point of view, it looks the right thing to me.

- Nuno Sá