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

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

[Harald Mommer]

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

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

- Update from virtio SPI draft specification V4 to V10.

- Incorporate 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.

Changes between 2nd and 3rd virtio SPI driver RFC:

- Order header inclusion alphabetically

- Add Viresh Kumar's "signed-off" to the header files

- Rework virtio_spi_one_transfer()
  - Rework the delays according to Haixu Cui's advise. Delays are now
    handled in a new sub-function virtio_spi_set_delays()
  - Minor change: Re-formulate arguments of sg_init_one()

- Rework virtio_spi_probe()
  - Replace some goto in error paths by return
  - Add spi_unregister_controller() to an error path. Abstained from
    using devm_spi_register_controller() to keep order of
    de-initialization in virtio_spi_remove().
  - Add deletion of vqueue to all error paths taken after the virtqueues
    have been initialized

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-20240213 and an adapted version on Linux 6.5 with target hardware
providing a physical SPI backend device.

[RFC PATCH v3 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>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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.43.0

[RFC PATCH v3 2/3] virtio-spi: Add virtio-spi.h.

[Harald Mommer]

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

Add virtio-spi.h header for virtio SPI.

Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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..d6923f4080b4
--- /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_config.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_types.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.43.0

[RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Harald Mommer]

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

This is the virtio SPI Linux kernel driver.

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

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index bc7021da2fe9..0b5cd4c1f06b 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 Controller"
+	depends on SPI_MASTER && 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..700cb36e815f
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,475 @@
+// 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/of.h>
+#include <linux/spi/spi.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.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);
+}
+
+/*
+ * See also
+ * https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com
+ */
+static int virtio_spi_set_delays(struct spi_transfer_head *th,
+				 struct spi_device *spi,
+				 struct spi_transfer *xfer)
+{
+	int cs_setup;
+	int cs_word_delay_xfer;
+	int cs_word_delay_spi;
+	int delay;
+	int cs_hold;
+	int cs_inactive;
+	int cs_change_delay;
+
+	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
+	if (cs_setup < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_setup\n");
+		return cs_setup;
+	}
+	th->cs_setup_ns = cpu_to_le32((u32)cs_setup);
+
+	cs_word_delay_xfer = spi_delay_to_ns(&xfer->word_delay, xfer);
+	if (cs_word_delay_xfer < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_word_delay_xfer\n");
+		return cs_word_delay_xfer;
+	}
+	cs_word_delay_spi = spi_delay_to_ns(&spi->word_delay, xfer);
+	if (cs_word_delay_spi < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_word_delay_spi\n");
+		return cs_word_delay_spi;
+	}
+	if (cs_word_delay_spi > cs_word_delay_xfer)
+		th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_spi);
+	else
+		th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_xfer);
+
+	delay = spi_delay_to_ns(&xfer->delay, xfer);
+	if (delay < 0) {
+		dev_warn(&spi->dev, "Cannot convert delay\n");
+		return delay;
+	}
+	cs_hold = spi_delay_to_ns(&spi->cs_hold, xfer);
+	if (cs_hold < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_hold\n");
+		return cs_hold;
+	}
+	th->cs_delay_hold_ns = cpu_to_le32((u32)delay + (u32)cs_hold);
+
+	cs_inactive = spi_delay_to_ns(&spi->cs_inactive, xfer);
+	if (cs_inactive < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_inactive\n");
+		return cs_inactive;
+	}
+	cs_change_delay = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
+	if (cs_change_delay < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_change_delay\n");
+		return cs_change_delay;
+	}
+	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)cs_inactive +
+						      (u32)cs_change_delay);
+
+	return 0;
+}
+
+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 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);
+
+	if (virtio_spi_set_delays(th, spi, xfer))
+		goto msg_done;
+
+	/* 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, 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__);
+		return -ENOMEM;
+	}
+
+	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");
+		return err;
+	}
+
+	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);
+			spi_unregister_controller(ctrl);
+			err = -ENODEV;
+			goto err_return;
+		}
+	}
+
+	return 0;
+
+err_return:
+	vdev->config->del_vqs(vdev);
+	return err;
+}
+
+static void virtio_spi_remove(struct virtio_device *vdev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
+
+	/* Order: 1.) unregister controller, 2.) remove virtqueue */
+	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.43.0

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:

> +/*
> + * See also
> + * https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com
> + */
> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *xfer)

Please write actual comments that can be read standalone, the reader has
absolutely no idea why they'd want to follow the link and there's
nothing being referenced by that "also".

> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
> +				   struct spi_controller *ctrl,
> +				   struct spi_message *msg,
> +				   struct spi_transfer *xfer)

> +	/*
> +	 * 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;

> +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;
> +	}

Why not just allocate this once, it's not like it's possible to send
more than one message simultaneously?

> +	/*
> +	 * 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) {

This is processing transfers within a message rather than messages.

> +		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;

It's not clear why this isn't within _spi_transfer_one() and then we
don't just use a transfer_one() callback and factor everything out to
the core?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Harald Mommer]

On 13.02.24 18:49, Mark Brown wrote:
> On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:
>
>> +/*
>> + * See also
>> + *https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com
>> + */
>> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
>> +				 struct spi_device *spi,
>> +				 struct spi_transfer *xfer)
> Please write actual comments that can be read standalone, the reader has
> absolutely no idea why they'd want to follow the link and there's
> nothing being referenced by that "also".
Replace link by Haixu Cui's diagram + explanations.The explanations were 
helpful so I wanted to keep this but the comment may now be too long to 
be accepted. We will see what happens.
>> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
>> +				   struct spi_controller *ctrl,
>> +				   struct spi_message *msg,
>> +				   struct spi_transfer *xfer)
>> +	/*
>> +	 * 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;

I got the comment originally from you, Mark Brown. After some digging 
still unclear what should be wrong and in the meantime I think nothing 
is wrong at all. To point you with the nose on the pending issue I added 
this comment here.

I'll remove the comment because I think there is no problem. Please 
protest if I'm wrong.

>> +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;
>> +	}
> Why not just allocate this once, it's not like it's possible to send
> more than one message simultaneously?
Will be done, struct virtio_spi_req spi_req will become a member of 
struct virtio_spi_priv.
>> +	/*
>> +	 * 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) {
> This is processing transfers within a message rather than messages.
Obviously I did not get the terminology of messages and transfers not 
correct which made the comment wrong. Comment to be corrected (and 
shortened).
>> +		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;
> It's not clear why this isn't within _spi_transfer_one() and then we
> don't just use a transfer_one() callback and factor everything out to
> the core?

Lack of experience. I saw one way of doing the job which missing the 
more simple way. Therefore we have reviews. Using now the alternative 
callback which shortens and simplifies the code.

Applied code changes, have to run some more tests.

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Tue, Feb 27, 2024 at 03:12:55PM +0100, Harald Mommer wrote:
> On 13.02.24 18:49, Mark Brown wrote:
> > On Tue, Feb 13, 2024 at 02:53:50PM +0100, 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)
> > > +	/*
> > > +	 * 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;

> I got the comment originally from you, Mark Brown. After some digging still
> unclear what should be wrong and in the meantime I think nothing is wrong at
> all. To point you with the nose on the pending issue I added this comment
> here.

Without going and checking the spec IIRC cs_change only applies within a
message in the virtio spec but it has effects on the final transfer in
Linux.  

> 
> I'll remove the comment because I think there is no problem. Please protest
> if I'm wrong.
> 
> > > +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;
> > > +	}
> > Why not just allocate this once, it's not like it's possible to send
> > more than one message simultaneously?
> Will be done, struct virtio_spi_req spi_req will become a member of struct
> virtio_spi_priv.
> > > +	/*
> > > +	 * 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) {
> > This is processing transfers within a message rather than messages.
> Obviously I did not get the terminology of messages and transfers not
> correct which made the comment wrong. Comment to be corrected (and
> shortened).
> > > +		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;
> > It's not clear why this isn't within _spi_transfer_one() and then we
> > don't just use a transfer_one() callback and factor everything out to
> > the core?
> 
> Lack of experience. I saw one way of doing the job which missing the more
> simple way. Therefore we have reviews. Using now the alternative callback
> which shortens and simplifies the code.
> 
> Applied code changes, have to run some more tests.
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Haixu Cui]

On 2/13/2024 9:53 PM, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> This is the virtio SPI Linux kernel driver.
> 
> Signed-off-by: Harald Mommer <Harald.Mommer@opensynergy.com>
> ---
>   drivers/spi/Kconfig      |  11 +
>   drivers/spi/Makefile     |   1 +
>   drivers/spi/spi-virtio.c | 475 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 487 insertions(+)
>   create mode 100644 drivers/spi/spi-virtio.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index bc7021da2fe9..0b5cd4c1f06b 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 Controller"
> +	depends on SPI_MASTER && 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..700cb36e815f
> --- /dev/null
> +++ b/drivers/spi/spi-virtio.c
> @@ -0,0 +1,475 @@
> +// 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/of.h>
> +#include <linux/spi/spi.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.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;
> +};
> +

Hello Harald,

     Do you add the "spi-virtio" in spidev_spi_ids in spidev.c when you 
doing the tests? And any other method to expose the spidev interface to 
userspace?

> +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);
> +}
> +
> +/*
> + * See also
> + * https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com
> + */

     For setting delay function, looks good to me. And I will look into 
other parts.

Thanks
> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *xfer)
> +{
> +	int cs_setup;
> +	int cs_word_delay_xfer;
> +	int cs_word_delay_spi;
> +	int delay;
> +	int cs_hold;
> +	int cs_inactive;
> +	int cs_change_delay;
> +
> +	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
> +	if (cs_setup < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_setup\n");
> +		return cs_setup;
> +	}
> +	th->cs_setup_ns = cpu_to_le32((u32)cs_setup);
> +
> +	cs_word_delay_xfer = spi_delay_to_ns(&xfer->word_delay, xfer);
> +	if (cs_word_delay_xfer < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_word_delay_xfer\n");
> +		return cs_word_delay_xfer;
> +	}
> +	cs_word_delay_spi = spi_delay_to_ns(&spi->word_delay, xfer);
> +	if (cs_word_delay_spi < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_word_delay_spi\n");
> +		return cs_word_delay_spi;
> +	}
> +	if (cs_word_delay_spi > cs_word_delay_xfer)
> +		th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_spi);
> +	else
> +		th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_xfer);
> +
> +	delay = spi_delay_to_ns(&xfer->delay, xfer);
> +	if (delay < 0) {
> +		dev_warn(&spi->dev, "Cannot convert delay\n");
> +		return delay;
> +	}
> +	cs_hold = spi_delay_to_ns(&spi->cs_hold, xfer);
> +	if (cs_hold < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_hold\n");
> +		return cs_hold;
> +	}
> +	th->cs_delay_hold_ns = cpu_to_le32((u32)delay + (u32)cs_hold);
> +
> +	cs_inactive = spi_delay_to_ns(&spi->cs_inactive, xfer);
> +	if (cs_inactive < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_inactive\n");
> +		return cs_inactive;
> +	}
> +	cs_change_delay = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> +	if (cs_change_delay < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_change_delay\n");
> +		return cs_change_delay;
> +	}
> +	th->cs_change_delay_inactive_ns = cpu_to_le32((u32)cs_inactive +
> +						      (u32)cs_change_delay);
> +
> +	return 0;
> +}
> +
> +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 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);
> +
> +	if (virtio_spi_set_delays(th, spi, xfer))
> +		goto msg_done;
> +
> +	/* 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, 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__);
> +		return -ENOMEM;
> +	}
> +
> +	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");
> +		return err;
> +	}
> +
> +	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);
> +			spi_unregister_controller(ctrl);
> +			err = -ENODEV;
> +			goto err_return;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_return:
> +	vdev->config->del_vqs(vdev);
> +	return err;
> +}
> +
> +static void virtio_spi_remove(struct virtio_device *vdev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
> +
> +	/* Order: 1.) unregister controller, 2.) remove virtqueue */
> +	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");

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Harald Mommer]

Hello Haixu,

I was on vacation, therefore no immediate answer. I should not forget to 
set an OOO reply before going to vacation.

On 20.02.24 09:30, Haixu Cui wrote:
>
> Hello Harald,
>
>     Do you add the "spi-virtio" in spidev_spi_ids in spidev.c when you 
> doing the tests? And any other method to expose the spidev interface 
> to userspace?
>
>> +static struct spi_board_info board_info = {
>> +    .modalias = "spi-virtio",
>> +};
>> +

I've a udev rule 50-spi-virtio.rules installed which does the job:

# Bind spi-virtio device spiB.C to user mode SPI device /dev/spidevB.C
# Requires spi driver_override sysfs entry (Linux version 4.20+ and later)
#
# See also 
https://www.mail-archive.com/debian-arm@lists.debian.org/msg22090.html
# and Documentation/spi/spidev.rst
#
#ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S%p/subsystem/drivers/spidev/bind"
ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S/bus/spi/drivers/spidev/bind"

There are no other kernel changes.

However for ancient Linux 4.14 there is no udev rule and the board_info 
looks, there

     .modalias = "spidev", /* Must be "spidev" for user mode SPI */

but this is only for old kernels we're still using in some setup and 
this is irrelevant at latest with 5.14.14 where was a documentation 
update of Documentation/spi/spidev.rst.

Regards
Harald Mommer

Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Haixu Cui]

On 2/13/2024 9:53 PM, Harald Mommer wrote:
> +static struct spi_board_info board_info = {
> +	.modalias = "spi-virtio",
> +};

Hi Harald,
     Do you add "spi-virtio" in spidev_spi_ids in spidev.c when you 
doing the tests, to probe spidev driver?

Thanks

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Harald Mommer]

Hello Haixu,

no, I've not touched spidev.c. Nowhere. I took Vanilla next/stable and 
applied the patches I sent.

Run the driver as a (somewhat different but comparable) module on 6.5 on 
hardware over virtio-MMIO. Probes and goes live.

Tested on next/stable using a specially adapted QEMU version which 
allows the usage of our proprietary virtio SPI device over PCI in qemu. 
Probes and goes live.

There may be other patches in the setup we're using I'm not aware of but 
not this one.

Only in case you're using some locally developed virtio SPI device on 
qemu which uses PCI transport:

SPI has ID 45. Means 0x2d.

https://www.qemu.org/docs/master/specs/pci-ids.html

1af4:1040 to 1af4:10ef

    ID range for modern virtio devices. The PCI device ID is calculated
    from the virtio device ID by adding the 0x1040 offset. ...

lspci on qemu:

/ # lspci
...
00:03.0 Class 00ff: 1af4:106d
...

/ #

You see something like this?

Regards
Harald

On 04.03.24 08:11, Haixu Cui wrote:
>
>
> On 2/13/2024 9:53 PM, Harald Mommer wrote:
>> +static struct spi_board_info board_info = {
>> +    .modalias = "spi-virtio",
>> +};
>
> Hi Harald,
>     Do you add "spi-virtio" in spidev_spi_ids in spidev.c when you 
> doing the tests, to probe spidev driver?
>
> Thanks
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Haixu Cui]

Hello Harald,

Thank you for your detailed expatiation. To my knowledge, you took 
Vanilla as the front-end, and VMM is QEMU. Can you please explain 
further how do you test the SPI transfer without the Vanilla userspace 
interface? Thanks again.

Haixu Cui

On 3/4/2024 6:52 PM, Harald Mommer wrote:
> Hello Haixu,
> 
> no, I've not touched spidev.c. Nowhere. I took Vanilla next/stable and 
> applied the patches I sent.
> 
> Run the driver as a (somewhat different but comparable) module on 6.5 on 
> hardware over virtio-MMIO. Probes and goes live.
> 
> Tested on next/stable using a specially adapted QEMU version which 
> allows the usage of our proprietary virtio SPI device over PCI in qemu. 
> Probes and goes live.
> 
> There may be other patches in the setup we're using I'm not aware of but 
> not this one.
> 
> Only in case you're using some locally developed virtio SPI device on 
> qemu which uses PCI transport:
> 
> SPI has ID 45. Means 0x2d.
> 
> https://www.qemu.org/docs/master/specs/pci-ids.html
> 
> 1af4:1040 to 1af4:10ef
> 
>     ID range for modern virtio devices. The PCI device ID is calculated
>     from the virtio device ID by adding the 0x1040 offset. ...
> 
> lspci on qemu:
> 
> / # lspci
> ...
> 00:03.0 Class 00ff: 1af4:106d
> ...
> 
> / #
> 
> You see something like this?
> 
> Regards
> Harald
> 
> On 04.03.24 08:11, Haixu Cui wrote:
>>
>>
>> On 2/13/2024 9:53 PM, Harald Mommer wrote:
>>> +static struct spi_board_info board_info = {
>>> +    .modalias = "spi-virtio",
>>> +};
>>
>> Hi Harald,
>>     Do you add "spi-virtio" in spidev_spi_ids in spidev.c when you 
>> doing the tests, to probe spidev driver?
>>
>> Thanks
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

[Harald Mommer]

Hello,

I took next/stable as base giving the exact tag/sha of the current 
next/stable so that it's known what was used as base version even when 
next/stable moves. The ordinary next tags are currently not of best 
quality, gets better, therefore next/stable now. We were on v6.8-rc7 
yesterday with next/stable.

VMM is qemu for the driver you have. But it's a specially modified qemu 
which allows that we use our proprietary virtio SPI device as backend.

Proprietary virtio SPI device is started first, this is an own user 
process in our architecture. Subsequently the special internal qemu 
version is started. The virtio SPI driver is compiled as a module and 
inserted manually by a startup script by "modprobe spi-virtio". The 
driver goes live immediately.

In this simple setup I do not have udev rules (no service supporting 
udev => no rules) so no /dev/spidevX.Y automatically after the driver 
went live. What I'm using to test the latest driver before sending it to 
the mailing lists is really a naked kernel + a busybox running in a 
ramdisk. The udev rule I've sent are used on some more complete setup on 
real hardware.

So without udev I have to bring this device up manually:

In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 manually:

#!/bin/sh
SPIDEV=spi0.0
echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind

Afterwards there is /dev/spidev0.0.

In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
(somewhat hacked locally, and I mean "hacked" to be able to test 
somewhat more) are used to play around with /dev/spidev0.0.

I can do this on my Laptop which has no underlying SPI hardware which 
could be used as a backend for the virtio SPI device. The proprietary 
virtio SPI device has a test mode to support this. Using this test mode 
the driver does not communicate with a real backend SPI device but does 
an internal simulation. For example, if I do a half duplex read it 
always gives back the sequence 01 02 03 ...

For full duplex it gives back what has been read but with letter case 
changed, in loopback mode it gives back exactly what was sent. With this 
test mode I could develop a driver and parts of the device (device - 
real backend communication to an actual SPI device) on a board which had 
no user space /dev/spiX.Y available which could have served as backend 
for the virtio SPI device on the host.

Slightly different module version is tested on real hardware with the 
virtio SPI device not in test mode. "Tested on hardware" means that 
device + module work for our special use case (some hardware device 
using 8 bit word size) and the project team for which device and driver 
have been made did until now not complain.

Regards
Harald Mommer

On 05.03.24 08:46, Haixu Cui wrote:
> Hello Harald,
>
> Thank you for your detailed expatiation. To my knowledge, you took 
> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
> further how do you test the SPI transfer without the Vanilla userspace 
> interface? Thanks again.
>
> Haixu Cui

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

[Harald Mommer]

Hello,

looked again at my tinny setup and I've to add a small correction.

It's not the way that I've no udev at all there. What is in place there 
is busybox mdev.

Relevant part of /etc/init.d/rcS:

#!/bin/sh
mount -t proc none /proc
mount -t sysfs none /sys
depmod
modprobe spi-virtio
mdev -s
mdev -d

If I kill the "mdev -d" process my small script below does not make the 
/dev/spidev0.0 device node appear any more. Of course not, there must be 
some user mode process which does the job in the device directory.

Regards
Harald Mommer

On 05.03.24 11:57, Harald Mommer wrote:
> Hello,
>
> I took next/stable as base giving the exact tag/sha of the current 
> next/stable so that it's known what was used as base version even when 
> next/stable moves. The ordinary next tags are currently not of best 
> quality, gets better, therefore next/stable now. We were on v6.8-rc7 
> yesterday with next/stable.
>
> VMM is qemu for the driver you have. But it's a specially modified 
> qemu which allows that we use our proprietary virtio SPI device as 
> backend.
>
> Proprietary virtio SPI device is started first, this is an own user 
> process in our architecture. Subsequently the special internal qemu 
> version is started. The virtio SPI driver is compiled as a module and 
> inserted manually by a startup script by "modprobe spi-virtio". The 
> driver goes live immediately.
>
> In this simple setup I do not have udev rules (no service supporting 
> udev => no rules) so no /dev/spidevX.Y automatically after the driver 
> went live. What I'm using to test the latest driver before sending it 
> to the mailing lists is really a naked kernel + a busybox running in a 
> ramdisk. The udev rule I've sent are used on some more complete setup 
> on real hardware.
>
> So without udev I have to bring this device up manually:
>
> In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
> manually:
>
> #!/bin/sh
> SPIDEV=spi0.0
> echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
> echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind
>
> Afterwards there is /dev/spidev0.0.
>
> In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
> (somewhat hacked locally, and I mean "hacked" to be able to test 
> somewhat more) are used to play around with /dev/spidev0.0.
>
> I can do this on my Laptop which has no underlying SPI hardware which 
> could be used as a backend for the virtio SPI device. The proprietary 
> virtio SPI device has a test mode to support this. Using this test 
> mode the driver does not communicate with a real backend SPI device 
> but does an internal simulation. For example, if I do a half duplex 
> read it always gives back the sequence 01 02 03 ...
>
> For full duplex it gives back what has been read but with letter case 
> changed, in loopback mode it gives back exactly what was sent. With 
> this test mode I could develop a driver and parts of the device 
> (device - real backend communication to an actual SPI device) on a 
> board which had no user space /dev/spiX.Y available which could have 
> served as backend for the virtio SPI device on the host.
>
> Slightly different module version is tested on real hardware with the 
> virtio SPI device not in test mode. "Tested on hardware" means that 
> device + module work for our special use case (some hardware device 
> using 8 bit word size) and the project team for which device and 
> driver have been made did until now not complain.
>
> Regards
> Harald Mommer
>
> On 05.03.24 08:46, Haixu Cui wrote:
>> Hello Harald,
>>
>> Thank you for your detailed expatiation. To my knowledge, you took 
>> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
>> further how do you test the SPI transfer without the Vanilla 
>> userspace interface? Thanks again.
>>
>> Haixu Cui
>
>
-- 
Dipl.-Ing. Harald Mommer
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone:  +49 (30) 60 98 540-0 <== Zentrale
Fax:    +49 (30) 60 98 540-99
E-Mail: harald.mommer@opensynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

[Haixu Cui]

Hello Harald,

     In current driver, spi_new_device is used to instantiate the virtio 
SPI device, spidevX.Y is created manually, this way seems not flexible 
enough. Besides it's not easy to set the spi_board_info properly.

     Viresh Kumar has standardized the device tree node format for 
virtio-i2c and virtio-gpio:

 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml

     In this way, the driver is unified, board customization only 
depends on the device-tree node. It's easy to bring up spidev automatically.

     Look forward to your opinions. Thanks a lot.

Haixu Cui


On 3/6/2024 1:54 AM, Harald Mommer wrote:
> Hello,
> 
> looked again at my tinny setup and I've to add a small correction.
> 
> It's not the way that I've no udev at all there. What is in place there 
> is busybox mdev.
> 
> Relevant part of /etc/init.d/rcS:
> 
> #!/bin/sh
> mount -t proc none /proc
> mount -t sysfs none /sys
> depmod
> modprobe spi-virtio
> mdev -s
> mdev -d
> 
> If I kill the "mdev -d" process my small script below does not make the 
> /dev/spidev0.0 device node appear any more. Of course not, there must be 
> some user mode process which does the job in the device directory.
> 
> Regards
> Harald Mommer
> 
> On 05.03.24 11:57, Harald Mommer wrote:
>> Hello,
>>
>> I took next/stable as base giving the exact tag/sha of the current 
>> next/stable so that it's known what was used as base version even when 
>> next/stable moves. The ordinary next tags are currently not of best 
>> quality, gets better, therefore next/stable now. We were on v6.8-rc7 
>> yesterday with next/stable.
>>
>> VMM is qemu for the driver you have. But it's a specially modified 
>> qemu which allows that we use our proprietary virtio SPI device as 
>> backend.
>>
>> Proprietary virtio SPI device is started first, this is an own user 
>> process in our architecture. Subsequently the special internal qemu 
>> version is started. The virtio SPI driver is compiled as a module and 
>> inserted manually by a startup script by "modprobe spi-virtio". The 
>> driver goes live immediately.
>>
>> In this simple setup I do not have udev rules (no service supporting 
>> udev => no rules) so no /dev/spidevX.Y automatically after the driver 
>> went live. What I'm using to test the latest driver before sending it 
>> to the mailing lists is really a naked kernel + a busybox running in a 
>> ramdisk. The udev rule I've sent are used on some more complete setup 
>> on real hardware.
>>
>> So without udev I have to bring this device up manually:
>>
>> In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
>> manually:
>>
>> #!/bin/sh
>> SPIDEV=spi0.0
>> echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
>> echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind
>>
>> Afterwards there is /dev/spidev0.0.
>>
>> In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
>> (somewhat hacked locally, and I mean "hacked" to be able to test 
>> somewhat more) are used to play around with /dev/spidev0.0.
>>
>> I can do this on my Laptop which has no underlying SPI hardware which 
>> could be used as a backend for the virtio SPI device. The proprietary 
>> virtio SPI device has a test mode to support this. Using this test 
>> mode the driver does not communicate with a real backend SPI device 
>> but does an internal simulation. For example, if I do a half duplex 
>> read it always gives back the sequence 01 02 03 ...
>>
>> For full duplex it gives back what has been read but with letter case 
>> changed, in loopback mode it gives back exactly what was sent. With 
>> this test mode I could develop a driver and parts of the device 
>> (device - real backend communication to an actual SPI device) on a 
>> board which had no user space /dev/spiX.Y available which could have 
>> served as backend for the virtio SPI device on the host.
>>
>> Slightly different module version is tested on real hardware with the 
>> virtio SPI device not in test mode. "Tested on hardware" means that 
>> device + module work for our special use case (some hardware device 
>> using 8 bit word size) and the project team for which device and 
>> driver have been made did until now not complain.
>>
>> Regards
>> Harald Mommer
>>
>> On 05.03.24 08:46, Haixu Cui wrote:
>>> Hello Harald,
>>>
>>> Thank you for your detailed expatiation. To my knowledge, you took 
>>> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
>>> further how do you test the SPI transfer without the Vanilla 
>>> userspace interface? Thanks again.
>>>
>>> Haixu Cui
>>
>>

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

[Harald Mommer]

Hello Haixu,

not really sure what you mean and where the problem are. But we will 
find out.

What I did in the device tree of some hardware board was

virtio_mmio@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x0 0x4b013000 0x0 0x1000>;
         /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
         interrupts = <0 497 4>;
         spi,bus-num = <1234>;
     };

Simply added a line "spi,bus-num = <1234>;" in the device tree to 
configure the bus number.

(There is no device tree for my very small qemu setup to check 
next/latest, also no full udev, therefore I've to live there with the 
default bus-num which is 0.)

What I guess you mean is that the syntax of the device tree node should 
be changed having GPIO and I2C as template.

And as you need more parameters for the board info, not only this single 
line for the bus number. May this be somewhat for an enhancement in a 
subsequent version?

Why it's not easy to create the device node using the udev rule below in 
a full system with udev (vs. some minimal RAMDISK system) I don't 
understand. It's a single line, rest are comments.

# Bind spi-virtio device spiB.C to user mode SPI device /dev/spidevB.C
# Requires spi driver_override sysfs entry (Linux version 4.20+ and later)
#
# See also 
https://www.mail-archive.com/debian-arm@lists.debian.org/msg22090.html
# and Documentation/spi/spidev.rst
#
#ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S%p/subsystem/drivers/spidev/bind"
ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S/bus/spi/drivers/spidev/bind"

It may be helpful if you could provide a proposal how exactly the device 
tree entry should look. This would also show which information is needed 
in addition to the bus number.

What I guess is that you in the end it may look like this:

virtio_mmio@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x0 0x4b013000 0x0 0x1000>;
         /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
         interrupts = <0 497 4>;

         spi {
             bus-num = <1234>; /* Is it like this? */
             /* More stuff here especially for the board_info I've 
currently no need for and no idea about that it may be needed by others 
*/ /* ??? More info needed */
         }
     };

Regards
Harald Mommer

On 06.03.24 08:48, Haixu Cui wrote:
> Hello Harald,
>
>     In current driver, spi_new_device is used to instantiate the 
> virtio SPI device, spidevX.Y is created manually, this way seems not 
> flexible enough. Besides it's not easy to set the spi_board_info 
> properly.
>
>     Viresh Kumar has standardized the device tree node format for 
> virtio-i2c and virtio-gpio:
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml 
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml 
>
>
>     In this way, the driver is unified, board customization only 
> depends on the device-tree node. It's easy to bring up spidev 
> automatically.
>
>     Look forward to your opinions. Thanks a lot.
>
> Haixu Cui
>
>
> On 3/6/2024 1:54 AM, Harald Mommer wrote:
>> Hello,
>>
>> looked again at my tinny setup and I've to add a small correction.
>>
>> It's not the way that I've no udev at all there. What is in place 
>> there is busybox mdev.
>>
>> Relevant part of /etc/init.d/rcS:
>>
>> #!/bin/sh
>> mount -t proc none /proc
>> mount -t sysfs none /sys
>> depmod
>> modprobe spi-virtio
>> mdev -s
>> mdev -d
>>
>> If I kill the "mdev -d" process my small script below does not make 
>> the /dev/spidev0.0 device node appear any more. Of course not, there 
>> must be some user mode process which does the job in the device 
>> directory.
>>
>> Regards
>> Harald Mommer
>>
>> On 05.03.24 11:57, Harald Mommer wrote:
>>> Hello,
>>>
>>> I took next/stable as base giving the exact tag/sha of the current 
>>> next/stable so that it's known what was used as base version even 
>>> when next/stable moves. The ordinary next tags are currently not of 
>>> best quality, gets better, therefore next/stable now. We were on 
>>> v6.8-rc7 yesterday with next/stable.
>>>
>>> VMM is qemu for the driver you have. But it's a specially modified 
>>> qemu which allows that we use our proprietary virtio SPI device as 
>>> backend.
>>>
>>> Proprietary virtio SPI device is started first, this is an own user 
>>> process in our architecture. Subsequently the special internal qemu 
>>> version is started. The virtio SPI driver is compiled as a module 
>>> and inserted manually by a startup script by "modprobe spi-virtio". 
>>> The driver goes live immediately.
>>>
>>> In this simple setup I do not have udev rules (no service supporting 
>>> udev => no rules) so no /dev/spidevX.Y automatically after the 
>>> driver went live. What I'm using to test the latest driver before 
>>> sending it to the mailing lists is really a naked kernel + a busybox 
>>> running in a ramdisk. The udev rule I've sent are used on some more 
>>> complete setup on real hardware.
>>>
>>> So without udev I have to bring this device up manually:
>>>
>>> In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
>>> manually:
>>>
>>> #!/bin/sh
>>> SPIDEV=spi0.0
>>> echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
>>> echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind
>>>
>>> Afterwards there is /dev/spidev0.0.
>>>
>>> In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
>>> (somewhat hacked locally, and I mean "hacked" to be able to test 
>>> somewhat more) are used to play around with /dev/spidev0.0.
>>>
>>> I can do this on my Laptop which has no underlying SPI hardware 
>>> which could be used as a backend for the virtio SPI device. The 
>>> proprietary virtio SPI device has a test mode to support this. Using 
>>> this test mode the driver does not communicate with a real backend 
>>> SPI device but does an internal simulation. For example, if I do a 
>>> half duplex read it always gives back the sequence 01 02 03 ...
>>>
>>> For full duplex it gives back what has been read but with letter 
>>> case changed, in loopback mode it gives back exactly what was sent. 
>>> With this test mode I could develop a driver and parts of the device 
>>> (device - real backend communication to an actual SPI device) on a 
>>> board which had no user space /dev/spiX.Y available which could have 
>>> served as backend for the virtio SPI device on the host.
>>>
>>> Slightly different module version is tested on real hardware with 
>>> the virtio SPI device not in test mode. "Tested on hardware" means 
>>> that device + module work for our special use case (some hardware 
>>> device using 8 bit word size) and the project team for which device 
>>> and driver have been made did until now not complain.
>>>
>>> Regards
>>> Harald Mommer
>>>
>>> On 05.03.24 08:46, Haixu Cui wrote:
>>>> Hello Harald,
>>>>
>>>> Thank you for your detailed expatiation. To my knowledge, you took 
>>>> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
>>>> further how do you test the SPI transfer without the Vanilla 
>>>> userspace interface? Thanks again.
>>>>
>>>> Haixu Cui
>>>
>>>
>

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

[Haixu Cui]

Hello Harald,
     My concern is if possible to create the udev(spidev) by adding the 
device-tree node. Although the way of using the udev rule is fine, I 
think the way of adding device-tree node also suitable for some scenarios.

     Referring to Kumar's examples, I guess the virtio spi device-tree 
should be like:

     virtio-spi@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x4b013000 0x1000>;
         interrupts = <0 497 4>;

         spi {
             compatible = "virtio,device45";
             #address-cells = <1>;
             #size-cells = <0>;

             spidev@0 {
                 compatible = "xxx";
                 reg = <0>;
                 spi-max-frequency = <100000>;
             };
         };
     };

     Just like virtio-i2c node, virtio-spi@4b013000 has three levels. 
And the innermost, spidev node is to match spidev driver, to create 
spidev(udev) device. I am working on this recently, but got some 
stranger cases. Need more effort and time.

     Harald, do you have any idea about this way? I'm looking forward to 
it. Thanks a lot.

Haixu Cui




On 3/7/2024 12:18 AM, Harald Mommer wrote:
> Hello Haixu,
> 
> not really sure what you mean and where the problem are. But we will 
> find out.
> 
> What I did in the device tree of some hardware board was
> 
> virtio_mmio@4b013000 {
>          compatible = "virtio,mmio";
>          reg = <0x0 0x4b013000 0x0 0x1000>;
>          /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
>          interrupts = <0 497 4>;
>          spi,bus-num = <1234>;
>      };
> 
> Simply added a line "spi,bus-num = <1234>;" in the device tree to 
> configure the bus number.
> 
> (There is no device tree for my very small qemu setup to check 
> next/latest, also no full udev, therefore I've to live there with the 
> default bus-num which is 0.)
> 
> What I guess you mean is that the syntax of the device tree node should 
> be changed having GPIO and I2C as template.
> 
> And as you need more parameters for the board info, not only this single 
> line for the bus number. May this be somewhat for an enhancement in a 
> subsequent version?
> 
> Why it's not easy to create the device node using the udev rule below in 
> a full system with udev (vs. some minimal RAMDISK system) I don't 
> understand. It's a single line, rest are comments.
> 
> # Bind spi-virtio device spiB.C to user mode SPI device /dev/spidevB.C
> # Requires spi driver_override sysfs entry (Linux version 4.20+ and later)
> #
> # See also 
> https://www.mail-archive.com/debian-arm@lists.debian.org/msg22090.html
> # and Documentation/spi/spidev.rst
> #
> #ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
> PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
> %S%p/subsystem/drivers/spidev/bind"
> ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
> PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
> %S/bus/spi/drivers/spidev/bind"
> 
> It may be helpful if you could provide a proposal how exactly the device 
> tree entry should look. This would also show which information is needed 
> in addition to the bus number.
> 
> What I guess is that you in the end it may look like this:
> 
> virtio_mmio@4b013000 {
>          compatible = "virtio,mmio";
>          reg = <0x0 0x4b013000 0x0 0x1000>;
>          /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
>          interrupts = <0 497 4>;
> 
>          spi {
>              bus-num = <1234>; /* Is it like this? */
>              /* More stuff here especially for the board_info I've 
> currently no need for and no idea about that it may be needed by others 
> */ /* ??? More info needed */
>          }
>      };
> 
> Regards
> Harald Mommer
> 
> On 06.03.24 08:48, Haixu Cui wrote:
>> Hello Harald,
>>
>>     In current driver, spi_new_device is used to instantiate the 
>> virtio SPI device, spidevX.Y is created manually, this way seems not 
>> flexible enough. Besides it's not easy to set the spi_board_info 
>> properly.
>>
>>     Viresh Kumar has standardized the device tree node format for 
>> virtio-i2c and virtio-gpio:
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
>>
>>     In this way, the driver is unified, board customization only 
>> depends on the device-tree node. It's easy to bring up spidev 
>> automatically.
>>
>>     Look forward to your opinions. Thanks a lot.
>>
>> Haixu Cui
>>
>>
>> On 3/6/2024 1:54 AM, Harald Mommer wrote:
>>> Hello,
>>>
>>> looked again at my tinny setup and I've to add a small correction.
>>>
>>> It's not the way that I've no udev at all there. What is in place 
>>> there is busybox mdev.
>>>
>>> Relevant part of /etc/init.d/rcS:
>>>
>>> #!/bin/sh
>>> mount -t proc none /proc
>>> mount -t sysfs none /sys
>>> depmod
>>> modprobe spi-virtio
>>> mdev -s
>>> mdev -d
>>>
>>> If I kill the "mdev -d" process my small script below does not make 
>>> the /dev/spidev0.0 device node appear any more. Of course not, there 
>>> must be some user mode process which does the job in the device 
>>> directory.
>>>
>>> Regards
>>> Harald Mommer
>>>
>>> On 05.03.24 11:57, Harald Mommer wrote:
>>>> Hello,
>>>>
>>>> I took next/stable as base giving the exact tag/sha of the current 
>>>> next/stable so that it's known what was used as base version even 
>>>> when next/stable moves. The ordinary next tags are currently not of 
>>>> best quality, gets better, therefore next/stable now. We were on 
>>>> v6.8-rc7 yesterday with next/stable.
>>>>
>>>> VMM is qemu for the driver you have. But it's a specially modified 
>>>> qemu which allows that we use our proprietary virtio SPI device as 
>>>> backend.
>>>>
>>>> Proprietary virtio SPI device is started first, this is an own user 
>>>> process in our architecture. Subsequently the special internal qemu 
>>>> version is started. The virtio SPI driver is compiled as a module 
>>>> and inserted manually by a startup script by "modprobe spi-virtio". 
>>>> The driver goes live immediately.
>>>>
>>>> In this simple setup I do not have udev rules (no service supporting 
>>>> udev => no rules) so no /dev/spidevX.Y automatically after the 
>>>> driver went live. What I'm using to test the latest driver before 
>>>> sending it to the mailing lists is really a naked kernel + a busybox 
>>>> running in a ramdisk. The udev rule I've sent are used on some more 
>>>> complete setup on real hardware.
>>>>
>>>> So without udev I have to bring this device up manually:
>>>>
>>>> In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
>>>> manually:
>>>>
>>>> #!/bin/sh
>>>> SPIDEV=spi0.0
>>>> echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
>>>> echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind
>>>>
>>>> Afterwards there is /dev/spidev0.0.
>>>>
>>>> In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
>>>> (somewhat hacked locally, and I mean "hacked" to be able to test 
>>>> somewhat more) are used to play around with /dev/spidev0.0.
>>>>
>>>> I can do this on my Laptop which has no underlying SPI hardware 
>>>> which could be used as a backend for the virtio SPI device. The 
>>>> proprietary virtio SPI device has a test mode to support this. Using 
>>>> this test mode the driver does not communicate with a real backend 
>>>> SPI device but does an internal simulation. For example, if I do a 
>>>> half duplex read it always gives back the sequence 01 02 03 ...
>>>>
>>>> For full duplex it gives back what has been read but with letter 
>>>> case changed, in loopback mode it gives back exactly what was sent. 
>>>> With this test mode I could develop a driver and parts of the device 
>>>> (device - real backend communication to an actual SPI device) on a 
>>>> board which had no user space /dev/spiX.Y available which could have 
>>>> served as backend for the virtio SPI device on the host.
>>>>
>>>> Slightly different module version is tested on real hardware with 
>>>> the virtio SPI device not in test mode. "Tested on hardware" means 
>>>> that device + module work for our special use case (some hardware 
>>>> device using 8 bit word size) and the project team for which device 
>>>> and driver have been made did until now not complain.
>>>>
>>>> Regards
>>>> Harald Mommer
>>>>
>>>> On 05.03.24 08:46, Haixu Cui wrote:
>>>>> Hello Harald,
>>>>>
>>>>> Thank you for your detailed expatiation. To my knowledge, you took 
>>>>> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
>>>>> further how do you test the SPI transfer without the Vanilla 
>>>>> userspace interface? Thanks again.
>>>>>
>>>>> Haixu Cui
>>>>
>>>>
>>

Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

[Haixu Cui]

Hello Harald,

     Some updates:

         1. the virtio-spi device node intermediate layer, "spi", the
            compatible is not "virtio,device45", should be
            "virtio,device2d", shall be in lower case hexadecimal.

         2. the driver should support the way that creating the spi
            device via device-tree, but now the driver doesn't, it cannot
            parse the child node. I guess this is due to the missing of
            of_node property, please refer to the i2c virtio driver.

 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-virtio.c?h=v6.8#n216

         3. in current driver, spi_new_device to create spi devices, as

+        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);
+			spi_unregister_controller(ctrl);
+			err = -ENODEV;
+			goto err_return;
+		}
+	}

           but when I add spi device node in device tree, there will be
           conflicts here, between the device dynamically created via
           device tree node and the device static created by calling
           spi_new_device. As far as I am concerned, it's necessary to
           support of and(or) acpi.

         Let's discuss this topic firstly. Thanks a lot.

BR
Haixu Cui





On 3/11/2024 5:28 PM, Haixu Cui wrote:
> Hello Harald,
>      My concern is if possible to create the udev(spidev) by adding the 
> device-tree node. Although the way of using the udev rule is fine, I 
> think the way of adding device-tree node also suitable for some scenarios.
> 
>      Referring to Kumar's examples, I guess the virtio spi device-tree 
> should be like:
> 
>      virtio-spi@4b013000 {
>          compatible = "virtio,mmio";
>          reg = <0x4b013000 0x1000>;
>          interrupts = <0 497 4>;
> 
>          spi {
>              compatible = "virtio,device45";
>              #address-cells = <1>;
>              #size-cells = <0>;
> 
>              spidev@0 {
>                  compatible = "xxx";
>                  reg = <0>;
>                  spi-max-frequency = <100000>;
>              };
>          };
>      };
> 
>      Just like virtio-i2c node, virtio-spi@4b013000 has three levels. 
> And the innermost, spidev node is to match spidev driver, to create 
> spidev(udev) device. I am working on this recently, but got some 
> stranger cases. Need more effort and time.
> 
>      Harald, do you have any idea about this way? I'm looking forward to 
> it. Thanks a lot.
> 
> Haixu Cui
> 
> 
> 
> 
> On 3/7/2024 12:18 AM, Harald Mommer wrote:
>> Hello Haixu,
>>
>> not really sure what you mean and where the problem are. But we will 
>> find out.
>>
>> What I did in the device tree of some hardware board was
>>
>> virtio_mmio@4b013000 {
>>          compatible = "virtio,mmio";
>>          reg = <0x0 0x4b013000 0x0 0x1000>;
>>          /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
>>          interrupts = <0 497 4>;
>>          spi,bus-num = <1234>;
>>      };
>>
>> Simply added a line "spi,bus-num = <1234>;" in the device tree to 
>> configure the bus number.
>>
>> (There is no device tree for my very small qemu setup to check 
>> next/latest, also no full udev, therefore I've to live there with the 
>> default bus-num which is 0.)
>>
>> What I guess you mean is that the syntax of the device tree node 
>> should be changed having GPIO and I2C as template.
>>
>> And as you need more parameters for the board info, not only this 
>> single line for the bus number. May this be somewhat for an 
>> enhancement in a subsequent version?
>>
>> Why it's not easy to create the device node using the udev rule below 
>> in a full system with udev (vs. some minimal RAMDISK system) I don't 
>> understand. It's a single line, rest are comments.
>>
>> # Bind spi-virtio device spiB.C to user mode SPI device /dev/spidevB.C
>> # Requires spi driver_override sysfs entry (Linux version 4.20+ and 
>> later)
>> #
>> # See also 
>> https://www.mail-archive.com/debian-arm@lists.debian.org/msg22090.html
>> # and Documentation/spi/spidev.rst
>> #
>> #ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
>> PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
>> %S%p/subsystem/drivers/spidev/bind"
>> ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
>> PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
>> %S/bus/spi/drivers/spidev/bind"
>>
>> It may be helpful if you could provide a proposal how exactly the 
>> device tree entry should look. This would also show which information 
>> is needed in addition to the bus number.
>>
>> What I guess is that you in the end it may look like this:
>>
>> virtio_mmio@4b013000 {
>>          compatible = "virtio,mmio";
>>          reg = <0x0 0x4b013000 0x0 0x1000>;
>>          /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
>>          interrupts = <0 497 4>;
>>
>>          spi {
>>              bus-num = <1234>; /* Is it like this? */
>>              /* More stuff here especially for the board_info I've 
>> currently no need for and no idea about that it may be needed by 
>> others */ /* ??? More info needed */
>>          }
>>      };
>>
>> Regards
>> Harald Mommer
>>
>> On 06.03.24 08:48, Haixu Cui wrote:
>>> Hello Harald,
>>>
>>>     In current driver, spi_new_device is used to instantiate the 
>>> virtio SPI device, spidevX.Y is created manually, this way seems not 
>>> flexible enough. Besides it's not easy to set the spi_board_info 
>>> properly.
>>>
>>>     Viresh Kumar has standardized the device tree node format for 
>>> virtio-i2c and virtio-gpio:
>>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
>>>
>>>     In this way, the driver is unified, board customization only 
>>> depends on the device-tree node. It's easy to bring up spidev 
>>> automatically.
>>>
>>>     Look forward to your opinions. Thanks a lot.
>>>
>>> Haixu Cui
>>>
>>>
>>> On 3/6/2024 1:54 AM, Harald Mommer wrote:
>>>> Hello,
>>>>
>>>> looked again at my tinny setup and I've to add a small correction.
>>>>
>>>> It's not the way that I've no udev at all there. What is in place 
>>>> there is busybox mdev.
>>>>
>>>> Relevant part of /etc/init.d/rcS:
>>>>
>>>> #!/bin/sh
>>>> mount -t proc none /proc
>>>> mount -t sysfs none /sys
>>>> depmod
>>>> modprobe spi-virtio
>>>> mdev -s
>>>> mdev -d
>>>>
>>>> If I kill the "mdev -d" process my small script below does not make 
>>>> the /dev/spidev0.0 device node appear any more. Of course not, there 
>>>> must be some user mode process which does the job in the device 
>>>> directory.
>>>>
>>>> Regards
>>>> Harald Mommer
>>>>
>>>> On 05.03.24 11:57, Harald Mommer wrote:
>>>>> Hello,
>>>>>
>>>>> I took next/stable as base giving the exact tag/sha of the current 
>>>>> next/stable so that it's known what was used as base version even 
>>>>> when next/stable moves. The ordinary next tags are currently not of 
>>>>> best quality, gets better, therefore next/stable now. We were on 
>>>>> v6.8-rc7 yesterday with next/stable.
>>>>>
>>>>> VMM is qemu for the driver you have. But it's a specially modified 
>>>>> qemu which allows that we use our proprietary virtio SPI device as 
>>>>> backend.
>>>>>
>>>>> Proprietary virtio SPI device is started first, this is an own user 
>>>>> process in our architecture. Subsequently the special internal qemu 
>>>>> version is started. The virtio SPI driver is compiled as a module 
>>>>> and inserted manually by a startup script by "modprobe spi-virtio". 
>>>>> The driver goes live immediately.
>>>>>
>>>>> In this simple setup I do not have udev rules (no service 
>>>>> supporting udev => no rules) so no /dev/spidevX.Y automatically 
>>>>> after the driver went live. What I'm using to test the latest 
>>>>> driver before sending it to the mailing lists is really a naked 
>>>>> kernel + a busybox running in a ramdisk. The udev rule I've sent 
>>>>> are used on some more complete setup on real hardware.
>>>>>
>>>>> So without udev I have to bring this device up manually:
>>>>>
>>>>> In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
>>>>> manually:
>>>>>
>>>>> #!/bin/sh
>>>>> SPIDEV=spi0.0
>>>>> echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
>>>>> echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind
>>>>>
>>>>> Afterwards there is /dev/spidev0.0.
>>>>>
>>>>> In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
>>>>> (somewhat hacked locally, and I mean "hacked" to be able to test 
>>>>> somewhat more) are used to play around with /dev/spidev0.0.
>>>>>
>>>>> I can do this on my Laptop which has no underlying SPI hardware 
>>>>> which could be used as a backend for the virtio SPI device. The 
>>>>> proprietary virtio SPI device has a test mode to support this. 
>>>>> Using this test mode the driver does not communicate with a real 
>>>>> backend SPI device but does an internal simulation. For example, if 
>>>>> I do a half duplex read it always gives back the sequence 01 02 03 ...
>>>>>
>>>>> For full duplex it gives back what has been read but with letter 
>>>>> case changed, in loopback mode it gives back exactly what was sent. 
>>>>> With this test mode I could develop a driver and parts of the 
>>>>> device (device - real backend communication to an actual SPI 
>>>>> device) on a board which had no user space /dev/spiX.Y available 
>>>>> which could have served as backend for the virtio SPI device on the 
>>>>> host.
>>>>>
>>>>> Slightly different module version is tested on real hardware with 
>>>>> the virtio SPI device not in test mode. "Tested on hardware" means 
>>>>> that device + module work for our special use case (some hardware 
>>>>> device using 8 bit word size) and the project team for which device 
>>>>> and driver have been made did until now not complain.
>>>>>
>>>>> Regards
>>>>> Harald Mommer
>>>>>
>>>>> On 05.03.24 08:46, Haixu Cui wrote:
>>>>>> Hello Harald,
>>>>>>
>>>>>> Thank you for your detailed expatiation. To my knowledge, you took 
>>>>>> Vanilla as the front-end, and VMM is QEMU. Can you please explain 
>>>>>> further how do you test the SPI transfer without the Vanilla 
>>>>>> userspace interface? Thanks again.
>>>>>>
>>>>>> Haixu Cui
>>>>>
>>>>>
>>>

Re: [virtio-dev] [RFC PATCH v3 0/3] Virtio SPI Linux driver compliant to draft spec V10

[Cornelia Huck]

On Tue, Feb 13 2024, Harald Mommer <Harald.Mommer@opensynergy.com> wrote:

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

FWIW: this version of the SPI spec has been voted in for virtio 1.4 (and
is consequently available on the virtio-1.4 branch of the virtio spec.)
For all intents and purposes, this makes this spec final (modulo
possible future extensions).