[v4] Virtio SPI Linux driver

[PATCH v4 3/3] SPI: Add virtio SPI driver

Posted by Haixu Cui 1 month, 2 weeks ago

This is the virtio SPI Linux kernel driver.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 MAINTAINERS              |   1 +
 drivers/spi/Kconfig      |  11 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-virtio.c | 448 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 461 insertions(+)
 create mode 100644 drivers/spi/spi-virtio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3e289677ca18..ba276c71ec44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26763,6 +26763,7 @@ F:	sound/virtio/*
 VIRTIO SPI DRIVER
 M:	Haixu Cui <quic_haixcui@quicinc.com>
 S:	Maintained
+F:	drivers/spi/spi-virtio.c
 F:	include/uapi/linux/virtio_spi.h
 
 VIRTUAL BOX GUEST DEVICE DRIVER
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 891729c9c564..7b609013fb05 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1224,6 +1224,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
+	  If you say yes to this option, support will be included for the virtio
+	  SPI controller driver. The hardware can be emulated by any device model
+	  software according to the virtio protocol.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spi-virtio.
+
 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 062c85989c8c..27a7cf68d55d 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -158,6 +158,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..e413c9cc73de
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SPI bus driver for the Virtio SPI controller
+ * Copyright (C) 2023 OpenSynergy GmbH
+ * Copyright (C) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/virtio_spi.h>
+
+#define VIRTIO_SPI_MODE_MASK \
+	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
+
+struct virtio_spi_req {
+	struct completion completion;
+	struct spi_transfer_head transfer_head	____cacheline_aligned;
+	const u8 *tx_buf;
+	u8 *rx_buf;
+	struct spi_transfer_result result	____cacheline_aligned;
+};
+
+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;
+};
+
+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);
+}
+
+/*
+ * virtio_spi_set_delays - Set delay parameters for SPI transfer
+ *
+ * This function sets various delay parameters for SPI transfer,
+ * including delay after CS asserted, timing intervals between
+ * adjacent words within a transfer, delay before and after CS
+ * deasserted. It converts these delay parameters to nanoseconds
+ * using spi_delay_to_ns and stores the results in spi_transfer_head
+ * structure.
+ * If the conversion fails, the function logs a warning message and
+ * returns an error code.
+ *       .   .      .    .    .   .   .   .   .   .
+ * Delay + A +      + B  +    + C + D + E + F + A +
+ *       .   .      .    .    .   .   .   .   .   .
+ *    ___.   .      .    .    .   .   .___.___.   .
+ * CS#   |___.______.____.____.___.___|   .   |___._____________
+ *       .   .      .    .    .   .   .   .   .   .
+ *       .   .      .    .    .   .   .   .   .   .
+ * SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______
+ *
+ * NOTE: 1st transfer has two words, the delay between these two words are
+ * 'B' in the diagram.
+ *
+ * A => struct spi_device -> cs_setup
+ * B => max{struct spi_transfer -> word_delay, struct spi_device -> word_delay}
+ *   Note: spi_device and spi_transfer both have word_delay, Linux
+ *         choose the bigger one, refer to _spi_xfer_word_delay_update function
+ * C => struct spi_transfer -> delay
+ * D => struct spi_device -> cs_hold
+ * E => struct spi_device -> cs_inactive
+ * F => struct spi_transfer -> cs_change_delay
+ *
+ * So the corresponding relationship:
+ * A   <===> cs_setup_ns (after CS asserted)
+ * B   <===> word_delay_ns (delay between adjacent words within a transfer)
+ * C+D <===> cs_delay_hold_ns (before CS deasserted)
+ * E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two
+ * values are also recommended in the Linux driver to be added up)
+ */
+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(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(cs_word_delay_spi);
+	else
+		th->word_delay_ns = cpu_to_le32(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(delay + 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(cs_inactive + cs_change_delay);
+
+	return 0;
+}
+
+static int virtio_spi_transfer_one(struct spi_controller *ctrl,
+				   struct spi_device *spi,
+				   struct spi_transfer *xfer)
+{
+	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
+	struct virtio_spi_req *spi_req;
+	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;
+
+	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
+	if (!spi_req)
+		return -ENOMEM;
+
+	init_completion(&spi_req->completion);
+
+	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;
+	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;
+
+	static_assert(VIRTIO_SPI_CPHA == SPI_CPHA,
+		      "VIRTIO_SPI_CPHA must match SPI_CPHA");
+	static_assert(VIRTIO_SPI_CPOL == SPI_CPOL,
+		      "VIRTIO_SPI_CPOL must match SPI_CPOL");
+	static_assert(VIRTIO_SPI_CS_HIGH == SPI_CS_HIGH,
+		      "VIRTIO_SPI_CS_HIGH must match SPI_CS_HIGH");
+	static_assert(VIRTIO_SPI_MODE_LSB_FIRST == SPI_LSB_FIRST,
+		      "VIRTIO_SPI_MODE_LSB_FIRST must match SPI_LSB_FIRST");
+
+	th->mode = cpu_to_le32(spi->mode & VIRTIO_SPI_MODE_MASK);
+	if (spi->mode & SPI_LOOP)
+		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
+
+	th->freq = cpu_to_le32(xfer->speed_hz);
+
+	ret = virtio_spi_set_delays(th, spi, xfer);
+	if (ret)
+		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] = &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);
+	if (ret)
+		goto msg_done;
+
+	/* Simple implementation: There can be only one transfer in flight */
+	virtqueue_kick(priv->vq);
+
+	wait_for_completion(&spi_req->completion);
+
+	/* Read result from message and translate return code */
+	switch (spi_req->result.result) {
+	case VIRTIO_SPI_TRANS_OK:
+		break;
+	case VIRTIO_SPI_PARAM_ERR:
+		ret = -EINVAL;
+		break;
+	case VIRTIO_SPI_TRANS_ERR:
+		ret = -EIO;
+		break;
+	default:
+		ret = -EIO;
+		break;
+	}
+
+msg_done:
+	kfree(spi_req);
+	if (ret)
+		ctrl->cur_msg->status = ret;
+
+	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)
+		ctrl->mode_bits |= VIRTIO_SPI_CPHA;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1)
+		ctrl->mode_bits |= VIRTIO_SPI_CPOL;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST)
+		ctrl->mode_bits |= SPI_LSB_FIRST;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK)
+		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)
+		ctrl->mode_bits |= SPI_TX_DUAL;
+	if (tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD)
+		ctrl->mode_bits |= SPI_TX_QUAD;
+	if (tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL)
+		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)
+		ctrl->mode_bits |= SPI_RX_DUAL;
+	if (rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD)
+		ctrl->mode_bits |= SPI_RX_QUAD;
+	if (rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL)
+		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 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_probe(struct virtio_device *vdev)
+{
+	struct virtio_spi_priv *priv;
+	struct spi_controller *ctrl;
+	int err;
+	u32 bus_num;
+
+	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
+	if (!ctrl)
+		return -ENOMEM;
+
+	priv = spi_controller_get_devdata(ctrl);
+	priv->vdev = vdev;
+	vdev->priv = priv;
+
+	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
+
+	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
+	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
+
+	dev_set_drvdata(&vdev->dev, ctrl);
+
+	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
+	if (!err && bus_num <= S16_MAX)
+		ctrl->bus_num = bus_num;
+	else
+		ctrl->bus_num = -1;
+
+	virtio_spi_read_config(vdev);
+
+	ctrl->transfer_one = virtio_spi_transfer_one;
+
+	err = virtio_spi_find_vqs(priv);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
+		return err;
+	}
+
+	/* Register cleanup for virtqueues using devm */
+	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
+		return err;
+	}
+
+	/* Use devm version to register controller */
+	err = devm_spi_register_controller(&vdev->dev, ctrl);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int virtio_spi_freeze(struct device *dev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
+	int ret;
+
+	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 device *dev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, 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 },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
+
+static const struct dev_pm_ops virtio_spi_pm_ops = {
+	.freeze = pm_sleep_ptr(virtio_spi_freeze),
+	.restore = pm_sleep_ptr(virtio_spi_restore),
+};
+
+static struct virtio_driver virtio_spi_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.pm = &virtio_spi_pm_ops,
+	},
+	.id_table = virtio_spi_id_table,
+	.probe = virtio_spi_probe,
+};
+module_virtio_driver(virtio_spi_driver);
+
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_AUTHOR("Haixu Cui <quic_haixcui@quicinc.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Virtio SPI bus driver");
-- 
2.34.1

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

Posted by kernel test robot 1 month, 2 weeks ago

Hi Haixu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on linus/master v6.17-rc2 next-20250820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Haixu-Cui/virtio-Add-ID-for-virtio-SPI/20250820-165441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20250820084944.84505-4-quic_haixcui%40quicinc.com
patch subject: [PATCH v4 3/3] SPI: Add virtio SPI driver
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20250821/202508211051.cuOjaH00-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 93d24b6b7b148c47a2fa228a4ef31524fa1d9f3f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250821/202508211051.cuOjaH00-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508211051.cuOjaH00-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/spi/spi-virtio.c:373:45: warning: cast from 'void (*)(struct virtio_device *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
     373 |         err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
         |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/device/devres.h:166:34: note: expanded from macro 'devm_add_action_or_reset'
     166 |         __devm_add_action_or_reset(dev, action, data, #action)
         |                                         ^~~~~~
   1 warning generated.


vim +373 drivers/spi/spi-virtio.c

   333	
   334	static int virtio_spi_probe(struct virtio_device *vdev)
   335	{
   336		struct virtio_spi_priv *priv;
   337		struct spi_controller *ctrl;
   338		int err;
   339		u32 bus_num;
   340	
   341		ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
   342		if (!ctrl)
   343			return -ENOMEM;
   344	
   345		priv = spi_controller_get_devdata(ctrl);
   346		priv->vdev = vdev;
   347		vdev->priv = priv;
   348	
   349		device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
   350	
   351		/* Setup ACPI node for controlled devices which will be probed through ACPI. */
   352		ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
   353	
   354		dev_set_drvdata(&vdev->dev, ctrl);
   355	
   356		err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
   357		if (!err && bus_num <= S16_MAX)
   358			ctrl->bus_num = bus_num;
   359		else
   360			ctrl->bus_num = -1;
   361	
   362		virtio_spi_read_config(vdev);
   363	
   364		ctrl->transfer_one = virtio_spi_transfer_one;
   365	
   366		err = virtio_spi_find_vqs(priv);
   367		if (err) {
   368			dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
   369			return err;
   370		}
   371	
   372		/* Register cleanup for virtqueues using devm */
 > 373		err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
   374		if (err) {
   375			dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
   376			return err;
   377		}
   378	
   379		/* Use devm version to register controller */
   380		err = devm_spi_register_controller(&vdev->dev, ctrl);
   381		if (err) {
   382			dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
   383			return err;
   384		}
   385	
   386		return 0;
   387	}
   388	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

Posted by Andy Shevchenko 1 month, 2 weeks ago

On Wed, Aug 20, 2025 at 04:49:44PM +0800, Haixu Cui wrote:
> This is the virtio SPI Linux kernel driver.

...

> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/virtio_spi.h>

> +#define VIRTIO_SPI_MODE_MASK \
> +	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)

We have SPI_MODE_X_MASK.

...

> +struct virtio_spi_req {
> +	struct completion completion;
> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
> +	const u8 *tx_buf;
> +	u8 *rx_buf;
> +	struct spi_transfer_result result	____cacheline_aligned;
> +};

Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
any potential improvement?

I think the fields can be reshuffled to go last and only one needs that
attribute.

struct virtio_spi_req {
	struct completion completion;
	const u8 *tx_buf;
	u8 *rx_buf;
	struct spi_transfer_head transfer_head	____cacheline_aligned;
	struct spi_transfer_result result;
};

...

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

Hmm... Not a problem in your code, but ns can be quite high for low speed
links, there is a potential overflow...

> +		dev_warn(&spi->dev, "Cannot convert cs_setup\n");
> +		return cs_setup;
> +	}
> +	th->cs_setup_ns = cpu_to_le32(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(cs_word_delay_spi);
> +	else
> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);

Why not max() ?

> +	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(delay + 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(cs_inactive + cs_change_delay);
> +
> +	return 0;
> +}

...

> +static int virtio_spi_transfer_one(struct spi_controller *ctrl,
> +				   struct spi_device *spi,
> +				   struct spi_transfer *xfer)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct virtio_spi_req *spi_req;
> +	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;

Are 'u':s important in this case/

> +	int ret;
> +
> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> +	if (!spi_req)
> +		return -ENOMEM;
> +
> +	init_completion(&spi_req->completion);
> +
> +	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;
> +	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;
> +
> +	static_assert(VIRTIO_SPI_CPHA == SPI_CPHA,
> +		      "VIRTIO_SPI_CPHA must match SPI_CPHA");
> +	static_assert(VIRTIO_SPI_CPOL == SPI_CPOL,
> +		      "VIRTIO_SPI_CPOL must match SPI_CPOL");
> +	static_assert(VIRTIO_SPI_CS_HIGH == SPI_CS_HIGH,
> +		      "VIRTIO_SPI_CS_HIGH must match SPI_CS_HIGH");
> +	static_assert(VIRTIO_SPI_MODE_LSB_FIRST == SPI_LSB_FIRST,
> +		      "VIRTIO_SPI_MODE_LSB_FIRST must match SPI_LSB_FIRST");
> +
> +	th->mode = cpu_to_le32(spi->mode & VIRTIO_SPI_MODE_MASK);
> +	if (spi->mode & SPI_LOOP)
> +		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> +	th->freq = cpu_to_le32(xfer->speed_hz);
> +
> +	ret = virtio_spi_set_delays(th, spi, xfer);
> +	if (ret)
> +		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] = &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);
> +	if (ret)
> +		goto msg_done;
> +
> +	/* Simple implementation: There can be only one transfer in flight */
> +	virtqueue_kick(priv->vq);
> +
> +	wait_for_completion(&spi_req->completion);
> +
> +	/* Read result from message and translate return code */
> +	switch (spi_req->result.result) {
> +	case VIRTIO_SPI_TRANS_OK:
> +		break;
> +	case VIRTIO_SPI_PARAM_ERR:
> +		ret = -EINVAL;
> +		break;
> +	case VIRTIO_SPI_TRANS_ERR:
> +		ret = -EIO;
> +		break;
> +	default:
> +		ret = -EIO;
> +		break;
> +	}
> +
> +msg_done:

> +	kfree(spi_req);

Can be called via __free() to simplify the error handling,

> +	if (ret)
> +		ctrl->cur_msg->status = ret;
> +
> +	return ret;
> +}

...

> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_spi_priv *priv;
> +	struct spi_controller *ctrl;

> +	int err;

Out of a sudden it's named 'err'. Please, go through the code and make
style/naming/etc consistent.

> +	u32 bus_num;
> +
> +	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	priv = spi_controller_get_devdata(ctrl);
> +	priv->vdev = vdev;
> +	vdev->priv = priv;

> +	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));

> +	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
> +	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));

This is strange. Either you need to put parent above in device_set_node() or
drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
going on here...

> +	dev_set_drvdata(&vdev->dev, ctrl);
> +
> +	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
> +	if (!err && bus_num <= S16_MAX)

This is wrong. What is the bus_num value when err != 0?
And why do we even care about this?

> +		ctrl->bus_num = bus_num;
> +	else
> +		ctrl->bus_num = -1;
> +
> +	virtio_spi_read_config(vdev);
> +
> +	ctrl->transfer_one = virtio_spi_transfer_one;
> +
> +	err = virtio_spi_find_vqs(priv);
> +	if (err) {

> +		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
> +		return err;

		return dev_err_probe(...);

> +	}

> +	/* Register cleanup for virtqueues using devm */
> +	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
> +	if (err) {
> +		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
> +		return err;
> +	}
> +
> +	/* Use devm version to register controller */
> +	err = devm_spi_register_controller(&vdev->dev, ctrl);
> +	if (err) {
> +		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
> +		return err;

As per above.

> +	}
> +
> +	return 0;
> +}

...

> +static int virtio_spi_freeze(struct device *dev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);

Use dev_to_virtio()

> +	int ret;
> +
> +	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 device *dev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);

As per above.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

Posted by Haixu Cui 1 month, 1 week ago

Hi Andy,

Thank you very much for your review and suggestions, I truly appreciate it!

I've carefully considered your comments and have replied directly under 
your comments with the corresponding changes. These updates will be 
included in next version of patch. Please let me know if there's 
anything else I should address.

Thanks again for your guidance and support.

Best Regards
Haixu Cui

On 8/20/2025 10:39 PM, Andy Shevchenko wrote:

> 
>> +#define VIRTIO_SPI_MODE_MASK \
>> +	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
> 
> We have SPI_MODE_X_MASK.
> 

#define VIRTIO_SPI_MODE_MASK \
         (SPI_MODE_X_MASK | SPI_CS_HIGH | SPI_LSB_FIRST)


>> +struct virtio_spi_req {
>> +	struct completion completion;
>> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
>> +	const u8 *tx_buf;
>> +	u8 *rx_buf;
>> +	struct spi_transfer_result result	____cacheline_aligned;
>> +};
> 
> Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
> any potential improvement?
> 
> I think the fields can be reshuffled to go last and only one needs that
> attribute.
> 
> struct virtio_spi_req {
> 	struct completion completion;
> 	const u8 *tx_buf;
> 	u8 *rx_buf;
> 	struct spi_transfer_head transfer_head	____cacheline_aligned;
> 	struct spi_transfer_result result;
> };
> 
Ok, I’ll adjust the structure as recommended.



>> +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) {
> 
> Hmm... Not a problem in your code, but ns can be quite high for low speed
> links, there is a potential overflow...

You're right—ns values can be quite high on low-speed links, and there's 
a potential risk of overflow. I’ll keep a close eye on this and I've 
noted this as a follow-up action item.



> 
>> +	if (cs_word_delay_spi > cs_word_delay_xfer)
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
>> +	else
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);
> 
> Why not max() ?

update code with max() using:
th->word_delay_ns = cpu_to_le32(max(cs_word_delay_spi, cs_word_delay_xfer));



>> +	unsigned int outcnt = 0u;
>> +	unsigned int incnt = 0u;
> 
> Are 'u':s important in this case/

Will remove 'u' here.


>> +msg_done:
> 
>> +	kfree(spi_req);
> 
> Can be called via __free() to simplify the error handling,
> 

Yes, will update spi_req definition as follows then remove 
kfree(spi_req) here:

struct virtio_spi_req *spi_req __free(kfree);



>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +	struct virtio_spi_priv *priv;
>> +	struct spi_controller *ctrl;
> 
>> +	int err;
> 
> Out of a sudden it's named 'err'. Please, go through the code and make
> style/naming/etc consistent.
> 
I'll update the naming to consistently use ret throughout, replacing err 
and other variants where appropriate



>> +	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
> 
>> +	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
>> +	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
> 
> This is strange. Either you need to put parent above in device_set_node() or
> drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
> going on here...

I'll remove the ACPI_COMPANION_SET() line and kept only 
device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev)); to ensure consistency.

> 
>> +	dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> +	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
>> +	if (!err && bus_num <= S16_MAX)
> 
> This is wrong. What is the bus_num value when err != 0?
> And why do we even care about this?
> 
>> +		ctrl->bus_num = bus_num;
>> +	else
>> +		ctrl->bus_num = -1;
>> +

Update as follows:
ret = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
         if (ret || bus_num > S16_MAX)
                 ctrl->bus_num = -1;
         else
                 ctrl->bus_num = bus_num;



>> +	err = virtio_spi_find_vqs(priv);
>> +	if (err) {
> 
>> +		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
>> +		return err;
> 
> 		return dev_err_probe(...);
> 
Acknowledged.




>> +	/* Register cleanup for virtqueues using devm */
>> +	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
>> +		return err;
>> +	}
>> +
>> +	/* Use devm version to register controller */
>> +	err = devm_spi_register_controller(&vdev->dev, ctrl);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
>> +		return err;
> 
> As per above.
Acknowledged

> 
>> +static int virtio_spi_freeze(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> Use dev_to_virtio()
Acknowledged




>> +static int virtio_spi_restore(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> As per above.
Acknowledged

[PATCH v4 1/3] virtio: Add ID for virtio SPI
[PATCH v4 2/3] virtio-spi: Add virtio-spi.h
[PATCH v4 3/3] SPI: Add virtio SPI driver