[RFC PATCH v2 0/3] Virtio SPI Linux driver compliant to draft spec V10

[RFC PATCH v2 0/3] Virtio SPI Linux driver compliant to draft spec V10

[Harald Mommer]

This is the 2nd RFC version of a virtio SPI Linux driver which is
intended to be compliant with the proposed virtio SPI draft
specification V10.

The 1st RFC version sent out publicly was compliant to the virtio draft
specification V4.

Changes between 1st and 2nd virtio SPI driver RFC:

- Update to be compliant to virtio SPI draft specification V10.

- Incorporated review comments gotten from the community.

A proposal for a performance enhancement having more than only one SPI
message in flight had to be kept out. The more complicated code would
have caused an unacceptable project risk now.

The virtio SPI driver was smoke tested on qemu using OpenSynergy's
proprietary virtio SPI device doing a SPI backend simulation on top of
next-20240103 and an adapted version on Linux 6.1 with target hardware
providing a physical SPI backend device.

[RFC PATCH v2 1/3] virtio: Add ID for virtio SPI.

[Harald Mommer]

From: Harald Mommer <harald.mommer@opensynergy.com>

Add #define ID VIRTIO_ID_SPI 45 for virtio SPI.

Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
---
 include/uapi/linux/virtio_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 7aa2eb766205..6c12db16faa3 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -68,6 +68,7 @@
 #define VIRTIO_ID_AUDIO_POLICY		39 /* virtio audio policy */
 #define VIRTIO_ID_BT			40 /* virtio bluetooth */
 #define VIRTIO_ID_GPIO			41 /* virtio gpio */
+#define VIRTIO_ID_SPI			45 /* virtio spi */
 
 /*
  * Virtio Transitional IDs
-- 
2.25.1

Re: [RFC PATCH v2 1/3] virtio: Add ID for virtio SPI.

[Viresh Kumar]

On 04-01-24, 14:01, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> Add #define ID VIRTIO_ID_SPI 45 for virtio SPI.
> 
> Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
> ---
>  include/uapi/linux/virtio_ids.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 7aa2eb766205..6c12db16faa3 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -68,6 +68,7 @@
>  #define VIRTIO_ID_AUDIO_POLICY		39 /* virtio audio policy */
>  #define VIRTIO_ID_BT			40 /* virtio bluetooth */
>  #define VIRTIO_ID_GPIO			41 /* virtio gpio */
> +#define VIRTIO_ID_SPI			45 /* virtio spi */

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[RFC PATCH v2 2/3] virtio-spi: Add virtio-spi.h (V10 draft specification).

[Harald Mommer]

From: Harald Mommer <harald.mommer@opensynergy.com>

Add virtio-spi.h header for virtio SPI. The header file is compliant to
the virtio SPI draft specification V10.

Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
---
 include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
 1 file changed, 185 insertions(+)
 create mode 100644 include/uapi/linux/virtio_spi.h

diff --git a/include/uapi/linux/virtio_spi.h b/include/uapi/linux/virtio_spi.h
new file mode 100644
index 000000000000..d56843fcb2ec
--- /dev/null
+++ b/include/uapi/linux/virtio_spi.h
@@ -0,0 +1,185 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (C) 2023 OpenSynergy GmbH
+ */
+#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
+#define _LINUX_VIRTIO_VIRTIO_SPI_H
+
+#include <linux/types.h>
+#include <linux/virtio_types.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+/* Sample data on trailing clock edge */
+#define VIRTIO_SPI_CPHA (1 << 0)
+/* Clock is high when IDLE */
+#define VIRTIO_SPI_CPOL (1 << 1)
+/* Chip Select is active high */
+#define VIRTIO_SPI_CS_HIGH (1 << 2)
+/* Transmit LSB first */
+#define VIRTIO_SPI_MODE_LSB_FIRST (1 << 3)
+/* Loopback mode */
+#define VIRTIO_SPI_MODE_LOOP (1 << 4)
+
+/*
+ * All config fields are read-only for the Virtio SPI driver
+ *
+ * @cs_max_number: maximum number of chipselect the host SPI controller
+ *   supports.
+ * @cs_change_supported: indicates if the host SPI controller supports to toggle
+ * chipselect after each transfer in one message:
+ *   0: unsupported, chipselect will be kept in active state throughout the
+ *      message transaction;
+ *   1: supported.
+ *   Note: Message here contains a sequence of SPI transfers.
+ * @tx_nbits_supported: indicates the supported number of bit for writing:
+ *   bit 0: DUAL (2-bit transfer), 1 for supported
+ *   bit 1: QUAD (4-bit transfer), 1 for supported
+ *   bit 2: OCTAL (8-bit transfer), 1 for supported
+ *   other bits are reserved as 0, 1-bit transfer is always supported.
+ * @rx_nbits_supported: indicates the supported number of bit for reading:
+ *   bit 0: DUAL (2-bit transfer), 1 for supported
+ *   bit 1: QUAD (4-bit transfer), 1 for supported
+ *   bit 2: OCTAL (8-bit transfer), 1 for supported
+ *   other bits are reserved as 0, 1-bit transfer is always supported.
+ * @bits_per_word_mask: mask indicating which values of bits_per_word are
+ *   supported. If not set, no limitation for bits_per_word.
+ * @mode_func_supported: indicates the following features are supported or not:
+ *   bit 0-1: CPHA feature
+ *     0b00: invalid, should support as least one CPHA setting
+ *     0b01: supports CPHA=0 only
+ *     0b10: supports CPHA=1 only
+ *     0b11: supports CPHA=0 and CPHA=1.
+ *   bit 2-3: CPOL feature
+ *     0b00: invalid, should support as least one CPOL setting
+ *     0b01: supports CPOL=0 only
+ *     0b10: supports CPOL=1 only
+ *     0b11: supports CPOL=0 and CPOL=1.
+ *   bit 4: chipselect active high feature, 0 for unsupported and 1 for
+ *     supported, chipselect active low should always be supported.
+ *   bit 5: LSB first feature, 0 for unsupported and 1 for supported,
+ *     MSB first should always be supported.
+ *   bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
+ *     normal mode should always be supported.
+ * @max_freq_hz: the maximum clock rate supported in Hz unit, 0 means no
+ *   limitation for transfer speed.
+ * @max_word_delay_ns: the maximum word delay supported in ns unit,
+ *   0 means word delay feature is unsupported.
+ *   Note: Just as one message contains a sequence of transfers,
+ *         one transfer may contain a sequence of words.
+ * @max_cs_setup_ns: the maximum delay supported after chipselect is asserted,
+ *   in ns unit, 0 means delay is not supported to introduce after chipselect is
+ *   asserted.
+ * @max_cs_hold_ns: the maximum delay supported before chipselect is deasserted,
+ *   in ns unit, 0 means delay is not supported to introduce before chipselect
+ *   is deasserted.
+ * @max_cs_incative_ns: maximum delay supported after chipselect is deasserted,
+ *   in ns unit, 0 means delay is not supported to introduce after chipselect is
+ *   deasserted.
+ */
+struct virtio_spi_config {
+	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
+	__u8 cs_max_number;
+	__u8 cs_change_supported;
+#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL (1 << 0)
+#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD (1 << 1)
+#define VIRTIO_SPI_RX_TX_SUPPORT_OCTAL (1 << 2)
+	__u8 tx_nbits_supported;
+	__u8 rx_nbits_supported;
+	__le32 bits_per_word_mask;
+#define VIRTIO_SPI_MF_SUPPORT_CPHA_0 (1 << 0)
+#define VIRTIO_SPI_MF_SUPPORT_CPHA_1 (1 << 1)
+#define VIRTIO_SPI_MF_SUPPORT_CPOL_0 (1 << 2)
+#define VIRTIO_SPI_MF_SUPPORT_CPOL_1 (1 << 3)
+#define VIRTIO_SPI_MF_SUPPORT_CS_HIGH (1 << 4)
+#define VIRTIO_SPI_MF_SUPPORT_LSB_FIRST (1 << 5)
+#define VIRTIO_SPI_MF_SUPPORT_LOOPBACK (1 << 6)
+	__le32 mode_func_supported;
+	__le32 max_freq_hz;
+	__le32 max_word_delay_ns;
+	__le32 max_cs_setup_ns;
+	__le32 max_cs_hold_ns;
+	__le32 max_cs_inactive_ns;
+};
+
+/*
+ * @chip_select_id: chipselect index the SPI transfer used.
+ *
+ * @bits_per_word: the number of bits in each SPI transfer word.
+ *
+ * @cs_change: whether to deselect device after finishing this transfer
+ *     before starting the next transfer, 0 means cs keep asserted and
+ *     1 means cs deasserted then asserted again.
+ *
+ * @tx_nbits: bus width for write transfer.
+ *     0,1: bus width is 1, also known as SINGLE
+ *     2  : bus width is 2, also known as DUAL
+ *     4  : bus width is 4, also known as QUAD
+ *     8  : bus width is 8, also known as OCTAL
+ *     other values are invalid.
+ *
+ * @rx_nbits: bus width for read transfer.
+ *     0,1: bus width is 1, also known as SINGLE
+ *     2  : bus width is 2, also known as DUAL
+ *     4  : bus width is 4, also known as QUAD
+ *     8  : bus width is 8, also known as OCTAL
+ *     other values are invalid.
+ *
+ * @reserved: for future use.
+ *
+ * @mode: SPI transfer mode.
+ *     bit 0: CPHA, determines the timing (i.e. phase) of the data
+ *         bits relative to the clock pulses.For CPHA=0, the
+ *         "out" side changes the data on the trailing edge of the
+ *         preceding clock cycle, while the "in" side captures the data
+ *         on (or shortly after) the leading edge of the clock cycle.
+ *         For CPHA=1, the "out" side changes the data on the leading
+ *         edge of the current clock cycle, while the "in" side
+ *         captures the data on (or shortly after) the trailing edge of
+ *         the clock cycle.
+ *     bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
+ *         clock which idles at 0, and each cycle consists of a pulse
+ *         of 1. CPOL=1 is a clock which idles at 1, and each cycle
+ *         consists of a pulse of 0.
+ *     bit 2: CS_HIGH, if 1, chip select active high, else active low.
+ *     bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
+ *         first, else LSB first.
+ *     bit 4: LOOP, loopback mode.
+ *
+ * @freq: the transfer speed in Hz.
+ *
+ * @word_delay_ns: delay to be inserted between consecutive words of a
+ *     transfer, in ns unit.
+ *
+ * @cs_setup_ns: delay to be introduced after CS is asserted, in ns
+ *     unit.
+ *
+ * @cs_delay_hold_ns: delay to be introduced before CS is deasserted
+ *     for each transfer, in ns unit.
+ *
+ * @cs_change_delay_inactive_ns: delay to be introduced after CS is
+ *     deasserted and before next asserted, in ns unit.
+ */
+struct spi_transfer_head {
+	__u8 chip_select_id;
+	__u8 bits_per_word;
+	__u8 cs_change;
+	__u8 tx_nbits;
+	__u8 rx_nbits;
+	__u8 reserved[3];
+	__le32 mode;
+	__le32 freq;
+	__le32 word_delay_ns;
+	__le32 cs_setup_ns;
+	__le32 cs_delay_hold_ns;
+	__le32 cs_change_delay_inactive_ns;
+};
+
+struct spi_transfer_result {
+#define VIRTIO_SPI_TRANS_OK 0
+#define VIRTIO_SPI_PARAM_ERR 1
+#define VIRTIO_SPI_TRANS_ERR 2
+	u8 result;
+};
+
+#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_SPI_H */
-- 
2.25.1

Re: [RFC PATCH v2 2/3] virtio-spi: Add virtio-spi.h (V10 draft specification).

[Viresh Kumar]

On 04-01-24, 14:01, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> Add virtio-spi.h header for virtio SPI. The header file is compliant to
> the virtio SPI draft specification V10.
> 
> Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
> ---
>  include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
>  1 file changed, 185 insertions(+)
>  create mode 100644 include/uapi/linux/virtio_spi.h
> 
> diff --git a/include/uapi/linux/virtio_spi.h b/include/uapi/linux/virtio_spi.h
> new file mode 100644
> index 000000000000..d56843fcb2ec
> --- /dev/null
> +++ b/include/uapi/linux/virtio_spi.h
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (C) 2023 OpenSynergy GmbH
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
> +#define _LINUX_VIRTIO_VIRTIO_SPI_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>

Maybe keep them in alphabetical order. Looks good otherwise.

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft specification).

[Harald Mommer]

From: Harald Mommer <harald.mommer@opensynergy.com>

This is the virtio SPI Linux kernel driver compliant to the "PATCH v10"
draft virtio SPI specification.

Signed-off-by: Harald Mommer <Harald.Mommer@opensynergy.com>
---
 drivers/spi/Kconfig      |  11 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-virtio.c | 430 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 442 insertions(+)
 create mode 100644 drivers/spi/spi-virtio.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ddae0fde798e..f4f617c79ad7 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1125,6 +1125,17 @@ config SPI_UNIPHIER
 
 	  If your SoC supports SCSSI, say Y here.
 
+config SPI_VIRTIO
+	tristate "Virtio SPI SPI Controller"
+	depends on VIRTIO
+	help
+	  This enables the Virtio SPI driver.
+
+	  Virtio SPI is an SPI driver for virtual machines using Virtio.
+
+	  If your Linux is a virtual machine using Virtio, say Y here.
+	  If unsure, say N.
+
 config SPI_XCOMM
 	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
 	depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 4ff8d725ba5e..ff2243e44e00 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -146,6 +146,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
 obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
+obj-$(CONFIG_SPI_VIRTIO)		+= spi-virtio.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
 obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
diff --git a/drivers/spi/spi-virtio.c b/drivers/spi/spi-virtio.c
new file mode 100644
index 000000000000..39eb38184793
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,430 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SPI bus driver for the Virtio SPI controller
+ * Copyright (C) 2023 OpenSynergy GmbH
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/version.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+#include <linux/virtio_spi.h>
+
+/* virtio_spi private data structure */
+struct virtio_spi_priv {
+	/* The virtio device we're associated with */
+	struct virtio_device *vdev;
+	/* Pointer to the virtqueue */
+	struct virtqueue *vq;
+	/* Copy of config space mode_func_supported */
+	u32 mode_func_supported;
+	/* Copy of config space max_freq_hz */
+	u32 max_freq_hz;
+};
+
+struct virtio_spi_req {
+	struct completion completion;
+	struct spi_transfer_head transfer_head	____cacheline_aligned;
+	const uint8_t *tx_buf			____cacheline_aligned;
+	uint8_t *rx_buf				____cacheline_aligned;
+	struct spi_transfer_result result	____cacheline_aligned;
+};
+
+static struct spi_board_info board_info = {
+	.modalias = "spi-virtio",
+};
+
+static void virtio_spi_msg_done(struct virtqueue *vq)
+{
+	struct virtio_spi_req *req;
+	unsigned int len;
+
+	while ((req = virtqueue_get_buf(vq, &len)))
+		complete(&req->completion);
+}
+
+static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
+				   struct spi_controller *ctrl,
+				   struct spi_message *msg,
+				   struct spi_transfer *xfer)
+{
+	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
+	struct device *dev = &priv->vdev->dev;
+	struct spi_device *spi = msg->spi;
+	struct spi_transfer_head *th;
+	struct scatterlist sg_out_head, sg_out_payload;
+	struct scatterlist sg_in_result, sg_in_payload;
+	struct scatterlist *sgs[4];
+	unsigned int outcnt = 0u;
+	unsigned int incnt = 0u;
+	int ret;
+
+	th = &spi_req->transfer_head;
+
+	/* Fill struct spi_transfer_head */
+	th->chip_select_id = spi_get_chipselect(spi, 0);
+	th->bits_per_word = spi->bits_per_word;
+	/*
+	 * Got comment: "The virtio spec for cs_change is *not* what the Linux
+	 * cs_change field does, this will not do the right thing."
+	 * TODO: Understand/discuss this, still unclear what may be wrong here
+	 */
+	th->cs_change = xfer->cs_change;
+	th->tx_nbits = xfer->tx_nbits;
+	th->rx_nbits = xfer->rx_nbits;
+	th->reserved[0] = 0;
+	th->reserved[1] = 0;
+	th->reserved[2] = 0;
+
+	BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
+	BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
+	BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
+	BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
+
+	th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
+					    SPI_CPOL | SPI_CPHA));
+	if ((spi->mode & SPI_LOOP) != 0)
+		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
+
+	th->freq = cpu_to_le32(xfer->speed_hz);
+
+	ret = spi_delay_to_ns(&xfer->word_delay, xfer);
+	if (ret < 0) {
+		dev_warn(dev, "Cannot convert word_delay\n");
+		goto msg_done;
+	}
+	th->word_delay_ns = cpu_to_le32((u32)ret);
+
+	ret = spi_delay_to_ns(&xfer->delay, xfer);
+	if (ret < 0) {
+		dev_warn(dev, "Cannot convert delay\n");
+		goto msg_done;
+	}
+	th->cs_setup_ns = cpu_to_le32((u32)ret);
+	th->cs_delay_hold_ns = cpu_to_le32((u32)ret);
+
+	/* This is the "off" time when CS has to be deasserted for a moment */
+	ret = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
+	if (ret < 0) {
+		dev_warn(dev, "Cannot convert cs_change_delay\n");
+		goto msg_done;
+	}
+	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)ret);
+
+	/* Set buffers */
+	spi_req->tx_buf = xfer->tx_buf;
+	spi_req->rx_buf = xfer->rx_buf;
+
+	/* Prepare sending of virtio message */
+	init_completion(&spi_req->completion);
+
+	sg_init_one(&sg_out_head, &spi_req->transfer_head,
+		    sizeof(struct spi_transfer_head));
+	sgs[outcnt] = &sg_out_head;
+	outcnt++;
+
+	if (spi_req->tx_buf) {
+		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
+		sgs[outcnt] = &sg_out_payload;
+		outcnt++;
+	}
+
+	if (spi_req->rx_buf) {
+		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
+		sgs[outcnt + incnt] = &sg_in_payload;
+		incnt++;
+	}
+
+	sg_init_one(&sg_in_result, &spi_req->result,
+		    sizeof(struct spi_transfer_result));
+	sgs[outcnt + incnt] = &sg_in_result;
+	incnt++;
+
+	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
+				GFP_KERNEL);
+
+msg_done:
+	if (ret)
+		msg->status = ret;
+
+	return ret;
+}
+
+static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,
+					   struct spi_message *msg)
+{
+	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
+	struct virtio_spi_req *spi_req;
+	struct spi_transfer *xfer;
+	int ret = 0;
+
+	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
+	if (!spi_req) {
+		ret = -ENOMEM;
+		goto no_mem;
+	}
+
+	/*
+	 * Simple implementation: Process message by message and wait for each
+	 * message to be completed by the device side.
+	 */
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);
+		if (ret)
+			goto msg_done;
+
+		virtqueue_kick(priv->vq);
+
+		wait_for_completion(&spi_req->completion);
+
+		/* Read result from message */
+		ret = (int)spi_req->result.result;
+		if (ret)
+			goto msg_done;
+	}
+
+msg_done:
+	kfree(spi_req);
+no_mem:
+	msg->status = ret;
+	spi_finalize_current_message(ctrl);
+
+	return ret;
+}
+
+static void virtio_spi_read_config(struct virtio_device *vdev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
+	struct virtio_spi_priv *priv = vdev->priv;
+	u8 cs_max_number;
+	u8 tx_nbits_supported;
+	u8 rx_nbits_supported;
+
+	cs_max_number = virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+						     cs_max_number));
+	ctrl->num_chipselect = cs_max_number;
+
+	/* Set the mode bits which are understood by this driver */
+	priv->mode_func_supported =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      mode_func_supported));
+	ctrl->mode_bits = priv->mode_func_supported &
+			  (VIRTIO_SPI_CS_HIGH | VIRTIO_SPI_MODE_LSB_FIRST);
+	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPHA_1) != 0)
+		ctrl->mode_bits |= VIRTIO_SPI_CPHA;
+	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1) != 0)
+		ctrl->mode_bits |= VIRTIO_SPI_CPOL;
+	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST) != 0)
+		ctrl->mode_bits |= SPI_LSB_FIRST;
+	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK) != 0)
+		ctrl->mode_bits |= SPI_LOOP;
+	tx_nbits_supported =
+		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+					     tx_nbits_supported));
+	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
+		ctrl->mode_bits |= SPI_TX_DUAL;
+	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
+		ctrl->mode_bits |= SPI_TX_QUAD;
+	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
+		ctrl->mode_bits |= SPI_TX_OCTAL;
+	rx_nbits_supported =
+		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+					     rx_nbits_supported));
+	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
+		ctrl->mode_bits |= SPI_RX_DUAL;
+	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
+		ctrl->mode_bits |= SPI_RX_QUAD;
+	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
+		ctrl->mode_bits |= SPI_RX_OCTAL;
+
+	ctrl->bits_per_word_mask =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      bits_per_word_mask));
+
+	priv->max_freq_hz =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      max_freq_hz));
+}
+
+static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
+{
+	struct virtqueue *vq;
+
+	vq = virtio_find_single_vq(priv->vdev, virtio_spi_msg_done, "spi-rq");
+	if (IS_ERR(vq))
+		return (int)PTR_ERR(vq);
+	priv->vq = vq;
+	return 0;
+}
+
+/* Function must not be called before virtio_spi_find_vqs() has been run */
+static void virtio_spi_del_vq(struct virtio_device *vdev)
+{
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
+}
+
+static int virtio_spi_validate(struct virtio_device *vdev)
+{
+	/*
+	 * SPI needs always access to the config space.
+	 * Check that the driver can access the config space
+	 */
+	if (!vdev->config->get) {
+		dev_err(&vdev->dev, "%s failure: config access disabled\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+		dev_err(&vdev->dev,
+			"device does not comply with spec version 1.x\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int virtio_spi_probe(struct virtio_device *vdev)
+{
+	struct device_node *np = vdev->dev.parent->of_node;
+	struct virtio_spi_priv *priv;
+	struct spi_controller *ctrl;
+	int err;
+	u32 bus_num;
+	u16 csi;
+
+	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
+	if (!ctrl) {
+		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
+			__func__);
+		err = -ENOMEM;
+		goto err_return;
+	}
+
+	priv = spi_controller_get_devdata(ctrl);
+	priv->vdev = vdev;
+	vdev->priv = priv;
+	dev_set_drvdata(&vdev->dev, ctrl);
+
+	err = of_property_read_u32(np, "spi,bus-num", &bus_num);
+	if (!err && bus_num <= S16_MAX)
+		ctrl->bus_num = (s16)bus_num;
+
+	virtio_spi_read_config(vdev);
+
+	/* Method to transfer a single SPI message */
+	ctrl->transfer_one_message = virtio_spi_transfer_one_message;
+
+	/* Initialize virtqueues */
+	err = virtio_spi_find_vqs(priv);
+	if (err) {
+		dev_err(&vdev->dev, "Cannot setup virtqueues\n");
+		goto err_return;
+	}
+
+	err = spi_register_controller(ctrl);
+	if (err) {
+		dev_err(&vdev->dev, "Cannot register controller\n");
+		goto err_return;
+	}
+
+	board_info.max_speed_hz = priv->max_freq_hz;
+	/* spi_new_device() currently does not use bus_num but better set it */
+	board_info.bus_num = (u16)ctrl->bus_num;
+
+	/* Add chip selects to controller */
+	for (csi = 0; csi < ctrl->num_chipselect; csi++) {
+		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
+		board_info.chip_select = csi;
+		/* TODO: Discuss setting of board_info.mode */
+		if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
+			board_info.mode = SPI_MODE_0;
+		else
+			board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
+		if (!spi_new_device(ctrl, &board_info)) {
+			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
+			err = -ENODEV;
+			goto err_return;
+		}
+	}
+
+	return 0;
+
+err_return:
+	return err;
+}
+
+static void virtio_spi_remove(struct virtio_device *vdev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
+
+	spi_unregister_controller(ctrl);
+	virtio_spi_del_vq(vdev);
+}
+
+static int virtio_spi_freeze(struct virtio_device *vdev)
+{
+	struct device *dev = &vdev->dev;
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	int ret;
+
+	/* Stop the queue running */
+	ret = spi_controller_suspend(ctrl);
+	if (ret) {
+		dev_warn(dev, "cannot suspend controller (%d)\n", ret);
+		return ret;
+	}
+
+	virtio_spi_del_vq(vdev);
+	return 0;
+}
+
+static int virtio_spi_restore(struct virtio_device *vdev)
+{
+	struct device *dev = &vdev->dev;
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	int ret;
+
+	ret = virtio_spi_find_vqs(vdev->priv);
+	if (ret) {
+		dev_err(dev, "problem starting vqueue (%d)\n", ret);
+		return ret;
+	}
+
+	ret = spi_controller_resume(ctrl);
+	if (ret)
+		dev_err(dev, "problem resuming controller (%d)\n", ret);
+
+	return ret;
+}
+
+static struct virtio_device_id virtio_spi_id_table[] = {
+	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static struct virtio_driver virtio_spi_driver = {
+	.driver.name = KBUILD_MODNAME,
+	.driver.owner = THIS_MODULE,
+	.id_table = virtio_spi_id_table,
+	.validate = virtio_spi_validate,
+	.probe = virtio_spi_probe,
+	.remove = virtio_spi_remove,
+	.freeze = pm_sleep_ptr(virtio_spi_freeze),
+	.restore = pm_sleep_ptr(virtio_spi_restore),
+};
+
+module_virtio_driver(virtio_spi_driver);
+MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
+
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Virtio SPI bus driver");
-- 
2.25.1

Re: [RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft specification).

[Viresh Kumar]

Hi Harald,

On 04-01-24, 14:01, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> This is the virtio SPI Linux kernel driver compliant to the "PATCH v10"
> draft virtio SPI specification.

Its okay with the RFC, but later on, please remove the versioning part
from the commit log. All such information can be added to the cover
letter.

> Signed-off-by: Harald Mommer <Harald.Mommer@opensynergy.com>
> ---
>  drivers/spi/Kconfig      |  11 +
>  drivers/spi/Makefile     |   1 +
>  drivers/spi/spi-virtio.c | 430 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 442 insertions(+)
>  create mode 100644 drivers/spi/spi-virtio.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ddae0fde798e..f4f617c79ad7 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -1125,6 +1125,17 @@ config SPI_UNIPHIER
>  
>  	  If your SoC supports SCSSI, say Y here.
>  
> +config SPI_VIRTIO
> +	tristate "Virtio SPI SPI Controller"

s/SPI SPI/SPI/ ?

> +	depends on VIRTIO

Maybe a "depends on SPI_MASTER" as well ? Or "select" ?

> +	help
> +	  This enables the Virtio SPI driver.
> +
> +	  Virtio SPI is an SPI driver for virtual machines using Virtio.
> +
> +	  If your Linux is a virtual machine using Virtio, say Y here.
> +	  If unsure, say N.
> +
>  config SPI_XCOMM
>  	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
>  	depends on I2C
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 4ff8d725ba5e..ff2243e44e00 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -146,6 +146,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
>  obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
>  obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
>  obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
> +obj-$(CONFIG_SPI_VIRTIO)		+= spi-virtio.o
>  obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
>  obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
>  obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
> diff --git a/drivers/spi/spi-virtio.c b/drivers/spi/spi-virtio.c
> new file mode 100644
> index 000000000000..39eb38184793
> --- /dev/null
> +++ b/drivers/spi/spi-virtio.c
> @@ -0,0 +1,430 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SPI bus driver for the Virtio SPI controller
> + * Copyright (C) 2023 OpenSynergy GmbH
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/version.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +#include <linux/virtio_spi.h>

Alphabetical order is preferred normally for headers.

> +
> +/* virtio_spi private data structure */
> +struct virtio_spi_priv {
> +	/* The virtio device we're associated with */
> +	struct virtio_device *vdev;
> +	/* Pointer to the virtqueue */
> +	struct virtqueue *vq;
> +	/* Copy of config space mode_func_supported */
> +	u32 mode_func_supported;
> +	/* Copy of config space max_freq_hz */
> +	u32 max_freq_hz;
> +};
> +
> +struct virtio_spi_req {
> +	struct completion completion;
> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
> +	const uint8_t *tx_buf			____cacheline_aligned;
> +	uint8_t *rx_buf				____cacheline_aligned;
> +	struct spi_transfer_result result	____cacheline_aligned;
> +};
> +
> +static struct spi_board_info board_info = {
> +	.modalias = "spi-virtio",
> +};
> +
> +static void virtio_spi_msg_done(struct virtqueue *vq)
> +{
> +	struct virtio_spi_req *req;
> +	unsigned int len;
> +
> +	while ((req = virtqueue_get_buf(vq, &len)))

Do we really need a while loop here ? Since for now we are
transferring the messages one by one.

> +		complete(&req->completion);
> +}
> +
> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
> +				   struct spi_controller *ctrl,
> +				   struct spi_message *msg,
> +				   struct spi_transfer *xfer)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct device *dev = &priv->vdev->dev;
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer_head *th;
> +	struct scatterlist sg_out_head, sg_out_payload;
> +	struct scatterlist sg_in_result, sg_in_payload;
> +	struct scatterlist *sgs[4];
> +	unsigned int outcnt = 0u;
> +	unsigned int incnt = 0u;
> +	int ret;
> +
> +	th = &spi_req->transfer_head;
> +
> +	/* Fill struct spi_transfer_head */
> +	th->chip_select_id = spi_get_chipselect(spi, 0);
> +	th->bits_per_word = spi->bits_per_word;
> +	/*
> +	 * Got comment: "The virtio spec for cs_change is *not* what the Linux
> +	 * cs_change field does, this will not do the right thing."
> +	 * TODO: Understand/discuss this, still unclear what may be wrong here
> +	 */
> +	th->cs_change = xfer->cs_change;
> +	th->tx_nbits = xfer->tx_nbits;
> +	th->rx_nbits = xfer->rx_nbits;
> +	th->reserved[0] = 0;
> +	th->reserved[1] = 0;
> +	th->reserved[2] = 0;
> +
> +	BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
> +	BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
> +	BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
> +	BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
> +
> +	th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
> +					    SPI_CPOL | SPI_CPHA));
> +	if ((spi->mode & SPI_LOOP) != 0)
> +		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> +	th->freq = cpu_to_le32(xfer->speed_hz);
> +
> +	ret = spi_delay_to_ns(&xfer->word_delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert word_delay\n");
> +		goto msg_done;
> +	}
> +	th->word_delay_ns = cpu_to_le32((u32)ret);
> +
> +	ret = spi_delay_to_ns(&xfer->delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert delay\n");
> +		goto msg_done;
> +	}
> +	th->cs_setup_ns = cpu_to_le32((u32)ret);
> +	th->cs_delay_hold_ns = cpu_to_le32((u32)ret);
> +
> +	/* This is the "off" time when CS has to be deasserted for a moment */
> +	ret = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert cs_change_delay\n");
> +		goto msg_done;
> +	}
> +	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)ret);
> +
> +	/* Set buffers */
> +	spi_req->tx_buf = xfer->tx_buf;
> +	spi_req->rx_buf = xfer->rx_buf;
> +
> +	/* Prepare sending of virtio message */
> +	init_completion(&spi_req->completion);
> +
> +	sg_init_one(&sg_out_head, &spi_req->transfer_head,
> +		    sizeof(struct spi_transfer_head));

sizeof(*th) ?

> +	sgs[outcnt] = &sg_out_head;
> +	outcnt++;
> +
> +	if (spi_req->tx_buf) {
> +		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> +		sgs[outcnt] = &sg_out_payload;
> +		outcnt++;
> +	}
> +
> +	if (spi_req->rx_buf) {
> +		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> +		sgs[outcnt + incnt] = &sg_in_payload;
> +		incnt++;
> +	}
> +
> +	sg_init_one(&sg_in_result, &spi_req->result,
> +		    sizeof(struct spi_transfer_result));
> +	sgs[outcnt + incnt] = &sg_in_result;
> +	incnt++;
> +
> +	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
> +				GFP_KERNEL);
> +
> +msg_done:
> +	if (ret)
> +		msg->status = ret;
> +
> +	return ret;
> +}
> +
> +static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,
> +					   struct spi_message *msg)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct virtio_spi_req *spi_req;
> +	struct spi_transfer *xfer;
> +	int ret = 0;
> +
> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> +	if (!spi_req) {
> +		ret = -ENOMEM;
> +		goto no_mem;
> +	}
> +
> +	/*
> +	 * Simple implementation: Process message by message and wait for each
> +	 * message to be completed by the device side.
> +	 */
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);
> +		if (ret)
> +			goto msg_done;
> +
> +		virtqueue_kick(priv->vq);
> +
> +		wait_for_completion(&spi_req->completion);
> +
> +		/* Read result from message */
> +		ret = (int)spi_req->result.result;
> +		if (ret)
> +			goto msg_done;
> +	}
> +
> +msg_done:
> +	kfree(spi_req);
> +no_mem:
> +	msg->status = ret;
> +	spi_finalize_current_message(ctrl);
> +
> +	return ret;
> +}
> +
> +static void virtio_spi_read_config(struct virtio_device *vdev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
> +	struct virtio_spi_priv *priv = vdev->priv;
> +	u8 cs_max_number;
> +	u8 tx_nbits_supported;
> +	u8 rx_nbits_supported;
> +
> +	cs_max_number = virtio_cread8(vdev, offsetof(struct virtio_spi_config,
> +						     cs_max_number));
> +	ctrl->num_chipselect = cs_max_number;
> +
> +	/* Set the mode bits which are understood by this driver */
> +	priv->mode_func_supported =
> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
> +					      mode_func_supported));
> +	ctrl->mode_bits = priv->mode_func_supported &
> +			  (VIRTIO_SPI_CS_HIGH | VIRTIO_SPI_MODE_LSB_FIRST);
> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPHA_1) != 0)
> +		ctrl->mode_bits |= VIRTIO_SPI_CPHA;
> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1) != 0)
> +		ctrl->mode_bits |= VIRTIO_SPI_CPOL;
> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST) != 0)
> +		ctrl->mode_bits |= SPI_LSB_FIRST;
> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK) != 0)
> +		ctrl->mode_bits |= SPI_LOOP;
> +	tx_nbits_supported =
> +		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
> +					     tx_nbits_supported));
> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
> +		ctrl->mode_bits |= SPI_TX_DUAL;
> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
> +		ctrl->mode_bits |= SPI_TX_QUAD;
> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
> +		ctrl->mode_bits |= SPI_TX_OCTAL;
> +	rx_nbits_supported =
> +		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
> +					     rx_nbits_supported));
> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
> +		ctrl->mode_bits |= SPI_RX_DUAL;
> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
> +		ctrl->mode_bits |= SPI_RX_QUAD;
> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
> +		ctrl->mode_bits |= SPI_RX_OCTAL;
> +
> +	ctrl->bits_per_word_mask =
> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
> +					      bits_per_word_mask));
> +
> +	priv->max_freq_hz =
> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
> +					      max_freq_hz));
> +}
> +
> +static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
> +{
> +	struct virtqueue *vq;
> +
> +	vq = virtio_find_single_vq(priv->vdev, virtio_spi_msg_done, "spi-rq");
> +	if (IS_ERR(vq))
> +		return (int)PTR_ERR(vq);
> +	priv->vq = vq;
> +	return 0;
> +}
> +
> +/* Function must not be called before virtio_spi_find_vqs() has been run */
> +static void virtio_spi_del_vq(struct virtio_device *vdev)
> +{
> +	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_spi_validate(struct virtio_device *vdev)
> +{
> +	/*
> +	 * SPI needs always access to the config space.
> +	 * Check that the driver can access the config space
> +	 */
> +	if (!vdev->config->get) {
> +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +		dev_err(&vdev->dev,
> +			"device does not comply with spec version 1.x\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> +	struct device_node *np = vdev->dev.parent->of_node;
> +	struct virtio_spi_priv *priv;
> +	struct spi_controller *ctrl;
> +	int err;
> +	u32 bus_num;
> +	u16 csi;
> +
> +	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
> +	if (!ctrl) {
> +		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
> +			__func__);

I thought you agreed to drop it earlier ?

> +		err = -ENOMEM;
> +		goto err_return;
> +	}
> +
> +	priv = spi_controller_get_devdata(ctrl);
> +	priv->vdev = vdev;
> +	vdev->priv = priv;
> +	dev_set_drvdata(&vdev->dev, ctrl);
> +
> +	err = of_property_read_u32(np, "spi,bus-num", &bus_num);
> +	if (!err && bus_num <= S16_MAX)
> +		ctrl->bus_num = (s16)bus_num;
> +
> +	virtio_spi_read_config(vdev);
> +
> +	/* Method to transfer a single SPI message */
> +	ctrl->transfer_one_message = virtio_spi_transfer_one_message;
> +
> +	/* Initialize virtqueues */
> +	err = virtio_spi_find_vqs(priv);
> +	if (err) {
> +		dev_err(&vdev->dev, "Cannot setup virtqueues\n");
> +		goto err_return;
> +	}
> +
> +	err = spi_register_controller(ctrl);
> +	if (err) {

Remove virtqueues here ?

> +		dev_err(&vdev->dev, "Cannot register controller\n");
> +		goto err_return;
> +	}
> +
> +	board_info.max_speed_hz = priv->max_freq_hz;
> +	/* spi_new_device() currently does not use bus_num but better set it */
> +	board_info.bus_num = (u16)ctrl->bus_num;
> +
> +	/* Add chip selects to controller */
> +	for (csi = 0; csi < ctrl->num_chipselect; csi++) {
> +		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
> +		board_info.chip_select = csi;
> +		/* TODO: Discuss setting of board_info.mode */
> +		if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
> +			board_info.mode = SPI_MODE_0;
> +		else
> +			board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
> +		if (!spi_new_device(ctrl, &board_info)) {
> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
> +			err = -ENODEV;

Remove controller and virtqueues here ?

> +			goto err_return;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_return:
> +	return err;
> +}

-- 
viresh

Re: [RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft specification).

[Harald Mommer]

Hello,

On 29.01.24 08:06, Viresh Kumar wrote:
> Hi Harald,
>
> On 04-01-24, 14:01, Harald Mommer wrote:
>> From: Harald Mommer<harald.mommer@opensynergy.com>
>>
>> This is the virtio SPI Linux kernel driver compliant to the "PATCH v10"
>> draft virtio SPI specification.
> Its okay with the RFC, but later on, please remove the versioning part
> from the commit log. All such information can be added to the cover
> letter.


For the future. This driver needs to remain RFC at least until the SPI 
specification is accepted anyway.

>> Signed-off-by: Harald Mommer<Harald.Mommer@opensynergy.com>
>> ---
>>   drivers/spi/Kconfig      |  11 +
>>   drivers/spi/Makefile     |   1 +
>>   drivers/spi/spi-virtio.c | 430 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 442 insertions(+)
>>   create mode 100644 drivers/spi/spi-virtio.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index ddae0fde798e..f4f617c79ad7 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -1125,6 +1125,17 @@ config SPI_UNIPHIER
>>   
>>   	  If your SoC supports SCSSI, say Y here.
>>   
>> +config SPI_VIRTIO
>> +	tristate "Virtio SPI SPI Controller"
> s/SPI SPI/SPI/ ?


Will fix.


>> +	depends on VIRTIO
> Maybe a "depends on SPI_MASTER" as well ? Or "select" ?


This depends clearly on SPI_MASTER so something has to happen here.

>> +	help
>> +	  This enables the Virtio SPI driver.
>> +
>> +	  Virtio SPI is an SPI driver for virtual machines using Virtio.
>> +
>> +	  If your Linux is a virtual machine using Virtio, say Y here.
>> +	  If unsure, say N.
>> +
>>   config SPI_XCOMM
>>   	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
>>   	depends on I2C
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index 4ff8d725ba5e..ff2243e44e00 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -146,6 +146,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
>>   obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
>>   obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
>>   obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
>> +obj-$(CONFIG_SPI_VIRTIO)		+= spi-virtio.o
>>   obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
>>   obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
>>   obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
>> diff --git a/drivers/spi/spi-virtio.c b/drivers/spi/spi-virtio.c
>> new file mode 100644
>> index 000000000000..39eb38184793
>> --- /dev/null
>> +++ b/drivers/spi/spi-virtio.c
>> @@ -0,0 +1,430 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * SPI bus driver for the Virtio SPI controller
>> + * Copyright (C) 2023 OpenSynergy GmbH
>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/stddef.h>
>> +#include <linux/virtio.h>
>> +#include <linux/virtio_ring.h>
>> +#include <linux/version.h>
>> +#include <linux/of.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/virtio_spi.h>
> Alphabetical order is preferred normally for headers.


Did not know, can do. As long as the compiler does not overrule me, of 
course.

>> +
>> +/* virtio_spi private data structure */
>> +struct virtio_spi_priv {
>> +	/* The virtio device we're associated with */
>> +	struct virtio_device *vdev;
>> +	/* Pointer to the virtqueue */
>> +	struct virtqueue *vq;
>> +	/* Copy of config space mode_func_supported */
>> +	u32 mode_func_supported;
>> +	/* Copy of config space max_freq_hz */
>> +	u32 max_freq_hz;
>> +};
>> +
>> +struct virtio_spi_req {
>> +	struct completion completion;
>> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
>> +	const uint8_t *tx_buf			____cacheline_aligned;
>> +	uint8_t *rx_buf				____cacheline_aligned;
>> +	struct spi_transfer_result result	____cacheline_aligned;
>> +};
>> +
>> +static struct spi_board_info board_info = {
>> +	.modalias = "spi-virtio",
>> +};
>> +
>> +static void virtio_spi_msg_done(struct virtqueue *vq)
>> +{
>> +	struct virtio_spi_req *req;
>> +	unsigned int len;
>> +
>> +	while ((req = virtqueue_get_buf(vq, &len)))
> Do we really need a while loop here ? Since for now we are
> transferring the messages one by one.


Not strictly needed here yet as there is still this restriction 
transmitting messages. But we all know that this will not remain that 
way for too long, there is also no bug here so I prefer to keep it as it 
was done in virtio_i2c_msg_done().


>> +		complete(&req->completion);
>> +}
>> +
>> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
>> +				   struct spi_controller *ctrl,
>> +				   struct spi_message *msg,
>> +				   struct spi_transfer *xfer)
>> +{
>> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
>> +	struct device *dev = &priv->vdev->dev;
>> +	struct spi_device *spi = msg->spi;
>> +	struct spi_transfer_head *th;
>> +	struct scatterlist sg_out_head, sg_out_payload;
>> +	struct scatterlist sg_in_result, sg_in_payload;
>> +	struct scatterlist *sgs[4];
>> +	unsigned int outcnt = 0u;
>> +	unsigned int incnt = 0u;
>> +	int ret;
>> +
>> +	th = &spi_req->transfer_head;
>> +
>> +	/* Fill struct spi_transfer_head */
>> +	th->chip_select_id = spi_get_chipselect(spi, 0);
>> +	th->bits_per_word = spi->bits_per_word;
>> +	/*
>> +	 * Got comment: "The virtio spec for cs_change is *not* what the Linux
>> +	 * cs_change field does, this will not do the right thing."
>> +	 * TODO: Understand/discuss this, still unclear what may be wrong here
>> +	 */
>> +	th->cs_change = xfer->cs_change;
>> +	th->tx_nbits = xfer->tx_nbits;
>> +	th->rx_nbits = xfer->rx_nbits;
>> +	th->reserved[0] = 0;
>> +	th->reserved[1] = 0;
>> +	th->reserved[2] = 0;
>> +
>> +	BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
>> +	BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
>> +	BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
>> +	BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
>> +
>> +	th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
>> +					    SPI_CPOL | SPI_CPHA));
>> +	if ((spi->mode & SPI_LOOP) != 0)
>> +		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
>> +
>> +	th->freq = cpu_to_le32(xfer->speed_hz);
>> +
>> +	ret = spi_delay_to_ns(&xfer->word_delay, xfer);
>> +	if (ret < 0) {
>> +		dev_warn(dev, "Cannot convert word_delay\n");
>> +		goto msg_done;
>> +	}
>> +	th->word_delay_ns = cpu_to_le32((u32)ret);
>> +
>> +	ret = spi_delay_to_ns(&xfer->delay, xfer);
>> +	if (ret < 0) {
>> +		dev_warn(dev, "Cannot convert delay\n");
>> +		goto msg_done;
>> +	}
>> +	th->cs_setup_ns = cpu_to_le32((u32)ret);
>> +	th->cs_delay_hold_ns = cpu_to_le32((u32)ret);
>> +
>> +	/* This is the "off" time when CS has to be deasserted for a moment */
>> +	ret = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
>> +	if (ret < 0) {
>> +		dev_warn(dev, "Cannot convert cs_change_delay\n");
>> +		goto msg_done;
>> +	}
>> +	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)ret);
>> +
>> +	/* Set buffers */
>> +	spi_req->tx_buf = xfer->tx_buf;
>> +	spi_req->rx_buf = xfer->rx_buf;
>> +
>> +	/* Prepare sending of virtio message */
>> +	init_completion(&spi_req->completion);
>> +
>> +	sg_init_one(&sg_out_head, &spi_req->transfer_head,
>> +		    sizeof(struct spi_transfer_head));
> sizeof(*th) ?


Yes. But then in the form sg_init_one(&sg_out_head, th, sizeof(*th));


>> +	sgs[outcnt] = &sg_out_head;
>> +	outcnt++;
>> +
>> +	if (spi_req->tx_buf) {
>> +		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
>> +		sgs[outcnt] = &sg_out_payload;
>> +		outcnt++;
>> +	}
>> +
>> +	if (spi_req->rx_buf) {
>> +		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
>> +		sgs[outcnt + incnt] = &sg_in_payload;
>> +		incnt++;
>> +	}
>> +
>> +	sg_init_one(&sg_in_result, &spi_req->result,
>> +		    sizeof(struct spi_transfer_result));
>> +	sgs[outcnt + incnt] = &sg_in_result;
>> +	incnt++;
>> +
>> +	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
>> +				GFP_KERNEL);
>> +
>> +msg_done:
>> +	if (ret)
>> +		msg->status = ret;
>> +
>> +	return ret;
>> +}
>> +
>> +static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,
>> +					   struct spi_message *msg)
>> +{
>> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
>> +	struct virtio_spi_req *spi_req;
>> +	struct spi_transfer *xfer;
>> +	int ret = 0;
>> +
>> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
>> +	if (!spi_req) {
>> +		ret = -ENOMEM;
>> +		goto no_mem;
>> +	}
>> +
>> +	/*
>> +	 * Simple implementation: Process message by message and wait for each
>> +	 * message to be completed by the device side.
>> +	 */
>> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>> +		ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);
>> +		if (ret)
>> +			goto msg_done;
>> +
>> +		virtqueue_kick(priv->vq);
>> +
>> +		wait_for_completion(&spi_req->completion);
>> +
>> +		/* Read result from message */
>> +		ret = (int)spi_req->result.result;
>> +		if (ret)
>> +			goto msg_done;
>> +	}
>> +
>> +msg_done:
>> +	kfree(spi_req);
>> +no_mem:
>> +	msg->status = ret;
>> +	spi_finalize_current_message(ctrl);
>> +
>> +	return ret;
>> +}
>> +
>> +static void virtio_spi_read_config(struct virtio_device *vdev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
>> +	struct virtio_spi_priv *priv = vdev->priv;
>> +	u8 cs_max_number;
>> +	u8 tx_nbits_supported;
>> +	u8 rx_nbits_supported;
>> +
>> +	cs_max_number = virtio_cread8(vdev, offsetof(struct virtio_spi_config,
>> +						     cs_max_number));
>> +	ctrl->num_chipselect = cs_max_number;
>> +
>> +	/* Set the mode bits which are understood by this driver */
>> +	priv->mode_func_supported =
>> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
>> +					      mode_func_supported));
>> +	ctrl->mode_bits = priv->mode_func_supported &
>> +			  (VIRTIO_SPI_CS_HIGH | VIRTIO_SPI_MODE_LSB_FIRST);
>> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPHA_1) != 0)
>> +		ctrl->mode_bits |= VIRTIO_SPI_CPHA;
>> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1) != 0)
>> +		ctrl->mode_bits |= VIRTIO_SPI_CPOL;
>> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST) != 0)
>> +		ctrl->mode_bits |= SPI_LSB_FIRST;
>> +	if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK) != 0)
>> +		ctrl->mode_bits |= SPI_LOOP;
>> +	tx_nbits_supported =
>> +		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
>> +					     tx_nbits_supported));
>> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
>> +		ctrl->mode_bits |= SPI_TX_DUAL;
>> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
>> +		ctrl->mode_bits |= SPI_TX_QUAD;
>> +	if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
>> +		ctrl->mode_bits |= SPI_TX_OCTAL;
>> +	rx_nbits_supported =
>> +		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
>> +					     rx_nbits_supported));
>> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
>> +		ctrl->mode_bits |= SPI_RX_DUAL;
>> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
>> +		ctrl->mode_bits |= SPI_RX_QUAD;
>> +	if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
>> +		ctrl->mode_bits |= SPI_RX_OCTAL;
>> +
>> +	ctrl->bits_per_word_mask =
>> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
>> +					      bits_per_word_mask));
>> +
>> +	priv->max_freq_hz =
>> +		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
>> +					      max_freq_hz));
>> +}
>> +
>> +static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
>> +{
>> +	struct virtqueue *vq;
>> +
>> +	vq = virtio_find_single_vq(priv->vdev, virtio_spi_msg_done, "spi-rq");
>> +	if (IS_ERR(vq))
>> +		return (int)PTR_ERR(vq);
>> +	priv->vq = vq;
>> +	return 0;
>> +}
>> +
>> +/* Function must not be called before virtio_spi_find_vqs() has been run */
>> +static void virtio_spi_del_vq(struct virtio_device *vdev)
>> +{
>> +	virtio_reset_device(vdev);
>> +	vdev->config->del_vqs(vdev);
>> +}
>> +
>> +static int virtio_spi_validate(struct virtio_device *vdev)
>> +{
>> +	/*
>> +	 * SPI needs always access to the config space.
>> +	 * Check that the driver can access the config space
>> +	 */
>> +	if (!vdev->config->get) {
>> +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
>> +			__func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>> +		dev_err(&vdev->dev,
>> +			"device does not comply with spec version 1.x\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +	struct device_node *np = vdev->dev.parent->of_node;
>> +	struct virtio_spi_priv *priv;
>> +	struct spi_controller *ctrl;
>> +	int err;
>> +	u32 bus_num;
>> +	u16 csi;
>> +
>> +	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
>> +	if (!ctrl) {
>> +		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
>> +			__func__);
> I thought you agreed to drop it earlier ?


Tried to do this. But I've to maintain a driver version which is not 
based on latest, our internal master from which the public version is 
derived. So there is code which still has to free resources, you just 
don't see it. Was not happy when I realized that I have not yet 
devm_spi_alloc_host() everywhere where I need it.

In this code there is

#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 2, 0)
     ctrl = spi_alloc_master(&vdev->dev, sizeof(*priv));
#else
     ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
#endif

....

err_return:
#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 2, 0)
     spi_controller_put(ctrl);
#endif
     return err;

Either using goto minimizing the number of #if in the code or getting an 
unreadable #if mess in my "master" implementation which needs to be 
compliant also to 6.1 and even 4.14.


>> +		err = -ENOMEM;
>> +		goto err_return;
>> +	}
>> +
>> +	priv = spi_controller_get_devdata(ctrl);
>> +	priv->vdev = vdev;
>> +	vdev->priv = priv;
>> +	dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> +	err = of_property_read_u32(np, "spi,bus-num", &bus_num);
>> +	if (!err && bus_num <= S16_MAX)
>> +		ctrl->bus_num = (s16)bus_num;
>> +
>> +	virtio_spi_read_config(vdev);
>> +
>> +	/* Method to transfer a single SPI message */
>> +	ctrl->transfer_one_message = virtio_spi_transfer_one_message;
>> +
>> +	/* Initialize virtqueues */
>> +	err = virtio_spi_find_vqs(priv);
>> +	if (err) {
>> +		dev_err(&vdev->dev, "Cannot setup virtqueues\n");
>> +		goto err_return;
>> +	}
>> +
>> +	err = spi_register_controller(ctrl);
>> +	if (err) {
> Remove virtqueues here ?


Indeed missing. I guess

   vdev->config->del_vqs(vdev);

is still my friend here.And not only here but also below, so new label 
"err_return_del_vq:" to do it centrally at the end of the function.

And what is also to be improved here is the spi_register_controller() as 
I should better use devm_spi_register_controller() to get the resource 
de-allocation managed. Another #if in my "master" implementation but 
I've to live with it.

>> +		dev_err(&vdev->dev, "Cannot register controller\n");
>> +		goto err_return;
>> +	}
>> +
>> +	board_info.max_speed_hz = priv->max_freq_hz;
>> +	/* spi_new_device() currently does not use bus_num but better set it */
>> +	board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> +	/* Add chip selects to controller */
>> +	for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> +		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> +		board_info.chip_select = csi;
>> +		/* TODO: Discuss setting of board_info.mode */
>> +		if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
>> +			board_info.mode = SPI_MODE_0;
>> +		else
>> +			board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
>> +		if (!spi_new_device(ctrl, &board_info)) {
>> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> +			err = -ENODEV;
> Remove controller and virtqueues here ?


"vdev->config->del_vqs(vdev);" is missing => goto err_return_del_vq;

Not sure about controller. In my internal code also for older kernels 
I've a

#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 2, 0)
             spi_unregister_controller(ctrl);
#endif

and at the end of the function a

   spi_controller_put(ctrl);

but nothing of this here as reading the comments

devm_spi_alloc_host() => __devm_spi_alloc_controller()

" * Allocate an SPI controller and automatically release a reference on it
  * when @dev is unbound from its driver.  Drivers are thus relieved from
  * having to call spi_controller_put()."

no spi_controller_put() with devm.

Using devm_spi_register_controller() instead of the bare 
spi_register_controller() above it should not be necessary to do 
anything with the controller.

>> +			goto err_return;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err_return:
>> +	return err;
>> +}

Re: [RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft specification).

[Haixu Cui]

On 1/4/2024 9:01 PM, Harald Mommer wrote:
> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
> +				   struct spi_controller *ctrl,
> +				   struct spi_message *msg,
> +				   struct spi_transfer *xfer)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct device *dev = &priv->vdev->dev;
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer_head *th;
> +	struct scatterlist sg_out_head, sg_out_payload;
> +	struct scatterlist sg_in_result, sg_in_payload;
> +	struct scatterlist *sgs[4];
> +	unsigned int outcnt = 0u;
> +	unsigned int incnt = 0u;
> +	int ret;
> +
> +	th = &spi_req->transfer_head;
> +
> +	/* Fill struct spi_transfer_head */
> +	th->chip_select_id = spi_get_chipselect(spi, 0);
> +	th->bits_per_word = spi->bits_per_word;
> +	/*
> +	 * Got comment: "The virtio spec for cs_change is*not*  what the Linux
> +	 * cs_change field does, this will not do the right thing."
> +	 * TODO: Understand/discuss this, still unclear what may be wrong here
> +	 */
> +	th->cs_change = xfer->cs_change;
> +	th->tx_nbits = xfer->tx_nbits;
> +	th->rx_nbits = xfer->rx_nbits;
> +	th->reserved[0] = 0;
> +	th->reserved[1] = 0;
> +	th->reserved[2] = 0;
> +
> +	BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
> +	BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
> +	BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
> +	BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
> +
> +	th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
> +					    SPI_CPOL | SPI_CPHA));
> +	if ((spi->mode & SPI_LOOP) != 0)
> +		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> +	th->freq = cpu_to_le32(xfer->speed_hz);
> +
> +	ret = spi_delay_to_ns(&xfer->word_delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert word_delay\n");
> +		goto msg_done;
> +	}
> +	th->word_delay_ns = cpu_to_le32((u32)ret);
> +
> +	ret = spi_delay_to_ns(&xfer->delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert delay\n");
> +		goto msg_done;
> +	}
> +	th->cs_setup_ns = cpu_to_le32((u32)ret);
> +	th->cs_delay_hold_ns = cpu_to_le32((u32)ret);
> +
> +	/* This is the "off" time when CS has to be deasserted for a moment */
> +	ret = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> +	if (ret < 0) {
> +		dev_warn(dev, "Cannot convert cs_change_delay\n");
> +		goto msg_done;
> +	}
> +	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)ret);

Hi Harald,

     spi_device structure has three cs timing members, which will also 
affect the cs timing.

     struct spi_device {
         ...
         struct spi_delay        cs_setup;
         struct spi_delay        cs_hold;
         struct spi_delay        cs_inactive;
         ...
     }

     These three values should also be passed to the backend, for the 
controller to control cs lines.

     spi_device->cs_setup is the delay before cs asserted, 
spi_device->cs_hold with spi_transfer->delay work before cs deasserted, 
and spi_device->cs_inactive with spi_transfer->cs_change_delay take 
effect at the stage after cs deasserted.

Here is the diagram
       .   .      .    .    .   .   .   .   .   .
Delay + A +      + B  +    + C + D + E + F + A +
       .   .      .    .    .   .   .   .   .   .
    ___.   .      .    .    .   .   .___.___.   .
CS#   |___.______.____.____.___.___|   .   |___._____________
       .   .      .    .    .   .   .   .   .   .
       .   .      .    .    .   .   .   .   .   .
SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______


NOTE: 1st transfer has two words, the delay betweent these two words are 
'B' in the diagram.

A => struct spi_device -> cs_setup
B => max{struct spi_transfer -> word_delay,
          struct spi_device -> word_delay}
     Note: spi_device and spi_transfer both have word_delay, Linux
          choose the bigger one, refer to _spi_xfer_word_delay_update
          function
C => struct spi_transfer -> delay
D => struct spi_device -> cs_hold
E => struct spi_device -> cs_inactive
F => struct spi_transfer -> cs_change_delay

So the corresponding relationship:
A <===> cs_setup_ns (after CS asserted)
B <===> word_delay_ns (no matter with CS)
C+D <===> cs_delay_hold_ns (before CS deasserted)
E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two 
values also recommend in Linux driver to be added up)

Best Regards
Haixu


> +
> +	/* Set buffers */
> +	spi_req->tx_buf = xfer->tx_buf;
> +	spi_req->rx_buf = xfer->rx_buf;
> +
> +	/* Prepare sending of virtio message */
> +	init_completion(&spi_req->completion);
> +
> +	sg_init_one(&sg_out_head, &spi_req->transfer_head,
> +		    sizeof(struct spi_transfer_head));
> +	sgs[outcnt] = &sg_out_head;
> +	outcnt++;
> +
> +	if (spi_req->tx_buf) {
> +		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> +		sgs[outcnt] = &sg_out_payload;
> +		outcnt++;
> +	}
> +
> +	if (spi_req->rx_buf) {
> +		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> +		sgs[outcnt + incnt] = &sg_in_payload;
> +		incnt++;
> +	}
> +
> +	sg_init_one(&sg_in_result, &spi_req->result,
> +		    sizeof(struct spi_transfer_result));
> +	sgs[outcnt + incnt] = &sg_in_result;
> +	incnt++;
> +
> +	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
> +				GFP_KERNEL);
> +
> +msg_done:
> +	if (ret)
> +		msg->status = ret;
> +
> +	return ret;
> +}

Re: [virtio-dev] Re: [RFC PATCH v2 3/3] SPI: Add virtio SPI driver (V10 draft specification).

[Harald Mommer]

Hello Haixu,

Thanks. This was a hard one. I knew that I did the delay settingsmost 
probably somewhat wrong but I had no idea that I did it so wrong.

Reworked, made a new function for this, currently testing. Added the 
link to the place where your E-Mail is stored

https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com/

as a comment to the source code and hope this will survive the reviews. 
Another option would be to add the diagram + the explanations below as 
quoted here as comment to the code. Whether 30+ lines of comments would 
survive the reviews I don't know.

Just to say nothing in the code making live hard for people trying to 
understand the code is something I would like to avoid.

Regards
Harald


On 30.01.24 04:21, Haixu Cui wrote:
> .   .      .    .    .   .   .   .   .   .
> Delay + A +      + B  +    + C + D + E + F + A +
>       .   .      .    .    .   .   .   .   .   .
>    ___.   .      .    .    .   .   .___.___.   .
> CS#   |___.______.____.____.___.___|   .   |___._____________
>       .   .      .    .    .   .   .   .   .   .
>       .   .      .    .    .   .   .   .   .   .
> SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______
>
>
> NOTE: 1st transfer has two words, the delay betweent these two words 
> are 'B' in the diagram.
>
> A => struct spi_device -> cs_setup
> B => max{struct spi_transfer -> word_delay,
>          struct spi_device -> word_delay}
>     Note: spi_device and spi_transfer both have word_delay, Linux
>          choose the bigger one, refer to _spi_xfer_word_delay_update
>          function
> C => struct spi_transfer -> delay
> D => struct spi_device -> cs_hold
> E => struct spi_device -> cs_inactive
> F => struct spi_transfer -> cs_change_delay
>
> So the corresponding relationship:
> A <===> cs_setup_ns (after CS asserted)
> B <===> word_delay_ns (no matter with CS)
> C+D <===> cs_delay_hold_ns (before CS deasserted)
> E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two 
> values also recommend in Linux driver to be added up)