Hi Harald,Yes, i2c makes it "struct virtqueue *vq;" and this makes also sense here.
On 27-10-23, 18:10, Harald Mommer wrote:
From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>There is no need to make this configurable since the specification
This is the first public version of the virtio SPI Linux kernel driver
compliant to the "PATCH v4" draft virtio SPI specification.
Signed-off-by: Harald Mommer <Harald.Mommer@xxxxxxxxxxxxxxx>
---
drivers/spi/Kconfig | 10 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-virtio.c | 513 +++++++++++++++++++++++++++++++++++++++
3 files changed, 524 insertions(+)
create mode 100644 drivers/spi/spi-virtio.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 35dbfacecf1c..55f45c5a8d82 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1125,6 +1125,16 @@ config SPI_UNIPHIER
If your SoC supports SCSSI, say Y here.
+config SPI_VIRTIO
+ tristate "Virtio SPI SPI Controller"
+ depends on VIRTIO
+ help
+ This enables the Virtio SPI driver.
+
+ Virtio SPI is an SPI driver for virtual machines using Virtio.
+
+ If your Linux is a virtual machine using Virtio, say Y here.
+
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..12a4d96555f1
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,513 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SPI bus driver for the Virtio SPI controller
+ * Copyright (C) 2023 OpenSynergy GmbH
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/version.h>
+#include <linux/spi/spi.h>
+#include <linux/virtio_spi.h>
+
+/* SPI device queues */
+#define VIRTIO_SPI_QUEUE_RQ 0
+#define VIRTIO_SPI_QUEUE_COUNT 1
+
+/* virtio_spi private data structure */
+struct virtio_spi_priv {
+ /* The virtio device we're associated with */
+ struct virtio_device *vdev;
+ /* The virtqueues */
+ struct virtqueue *vqs[VIRTIO_SPI_QUEUE_COUNT];
fixes it to 1. You can simplify this a bit.
+ /* I/O callback function pointers for the virtqueues */And why not send all the messages once and speed this thing up ? Just
+ vq_callback_t *io_callbacks[VIRTIO_SPI_QUEUE_COUNT];
+ /* Support certain delay timing settings */
+ bool support_delays;
+};
+
+/* Compare with file i2c_virtio.c structure virtio_i2c_req */
+struct virtio_spi_req {
+ struct completion completion;
+#ifdef DEBUG
+ unsigned int rx_len;
+#endif
+ // clang-format off
+ 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;
+ // clang-format on
+};
+
+static struct spi_board_info board_info = {
+ .modalias = "spi-virtio",
+ .max_speed_hz = 125000000, /* Arbitrary very high limit */
+ .bus_num = 0, /* Patched later during initialization */
+ .chip_select = 0, /* Patched later during initialization */
+ .mode = SPI_MODE_0,
+};
+
+/* Compare with file i2c_virtio.c structure virtio_i2c_msg_done */
+static void virtio_spi_msg_done(struct virtqueue *vq)
+{
+ struct virtio_spi_req *req;
+ unsigned int len;
+
+ while ((req = virtqueue_get_buf(vq, &len)))
+ complete(&req->completion);
+}
+
+static int virtio_spi_transfer_one_message(struct spi_master *master,
+ struct spi_message *msg)
+{
+ struct virtio_spi_priv *priv = spi_master_get_devdata(master);
+ struct virtqueue *vq = priv->vqs[VIRTIO_SPI_QUEUE_RQ];
+ 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.
+ */
like how I2C does it.
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {Drop all temporary things please.
+ ret = virtio_spi_one_transfer(spi_req, master, msg, xfer);
+ if (ret)
+ goto msg_done;
+
+ virtqueue_kick(vq);
+
+ wait_for_completion(&spi_req->completion);
+
+ /* Read result from message */
+ ret = (int)spi_req->result.result;
+ if (ret)
+ goto msg_done;
+
+#ifdef DEBUG
+ if (spi_req->rx_buf) {Instead of reading values separately, you can also read the entire
+ pr_debug("Dump of RX payload\n");
+ print_hex_dump(KERN_DEBUG, KBUILD_MODNAME " ",
+ DUMP_PREFIX_NONE, 16, 1, spi_req->rx_buf,
+ spi_req->rx_len, true);
+ }
+#endif
+ }
+
+msg_done:
+ kfree(spi_req);
+no_mem:
+ msg->status = ret;
+ /*
+ * Looking into other SPI drivers like spi-atmel.c the function
+ * spi_finalize_current_message() is supposed to be called only once
+ * for all transfers in the list and not for each single message
+ */
+ spi_finalize_current_message(master);
+ dev_dbg(&priv->vdev->dev, "%s() returning %d\n", __func__, ret);
+ return ret;
+}
+
+static void virtio_spi_read_config(struct virtio_device *vdev)
+{
+ struct spi_master *master = dev_get_drvdata(&vdev->dev);
+ struct virtio_spi_priv *priv = vdev->priv;
+ u16 bus_num;
+ u16 cs_max_number;
+ u8 support_delays;
+
+ bus_num = virtio_cread16(vdev,
+ offsetof(struct virtio_spi_config, bus_num));
+ cs_max_number = virtio_cread16(vdev, offsetof(struct virtio_spi_config,
+ chip_select_max_number));
+ support_delays =
+ virtio_cread16(vdev, offsetof(struct virtio_spi_config,
+ cs_timing_setting_enable));
configuration structure in a single call to virtio_cread_bytes.
virtio_cread16() does also already exactly this conversion in the same way virtio_cread32() does.
Won't you also need to convert all the values using le16_to_cpu() ? I
have done it that way for drivers/gpio/gpio-virtio.c driver, just in
case it helps.
+Why is this a warning ? And not just debug or info ?
+ if (bus_num > S16_MAX) {
+ dev_warn(&vdev->dev, "Limiting bus_num = %u to %d\n", bus_num,
+ S16_MAX);
+ bus_num = S16_MAX;
+ }
+
+ if (support_delays > 1)
+ dev_warn(&vdev->dev, "cs_timing_setting_enable = %u\n",
+ support_delays);
Yes.+ priv->support_delays = (support_delays != 0);Since the vq count is fixed by spec to 1, I think you can directly use
+ master->bus_num = (s16)bus_num;
+ master->num_chipselect = cs_max_number;
+}
+
+static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
+{
+ static const char *const io_names[VIRTIO_SPI_QUEUE_COUNT] = { "spi-rq" };
+
+ priv->io_callbacks[VIRTIO_SPI_QUEUE_RQ] = virtio_spi_msg_done;
+
+ /* Find the queues. */
+ return virtio_find_vqs(priv->vdev, VIRTIO_SPI_QUEUE_COUNT, priv->vqs,
+ priv->io_callbacks, io_names, NULL);
virtio_find_single_vq() and simplify this a bit.
Yes, not only shorter but does also additional things if CONFIG_VIRTIO_HARDEN_NOTIFICATION has been enabled.+}virtio_reset_device(vdev)
+
+/* Compare with i2c-virtio.c function virtio_i2c_del_vqs() */
+/* Function must not be called before virtio_spi_find_vqs() has been run */
+static void virtio_spi_del_vq(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);Why not do it with the definition itself ?
+}
+
+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 virtio_spi_priv *priv;
+ struct spi_master *master;
+ int err;
+ u16 csi;
+
+ err = -ENOMEM;
+ master = spi_alloc_master(&vdev->dev, sizeof(struct virtio_spi_priv));sizeof(*priv)
and there is a devm_* variant too that you can use.Not that I'm aware of now.There is spi_alloc_host() as alternative frequently used now.
Seems to be that way. Other people print nothing.+ if (!master) {I think we removed print messages for allocation failure earlier, as
+ dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
+ __func__);
the alloc core handles it now. This may not be required.
+ goto err_return;We don't need to free any resources, maybe just return directly
without an unnecessary goto here. Yes it is normally cleaner to remove
all the resources at the bottom with a single return point, but we
normally return earlier if the resources were not required to be
freed.
Yes. And reminder for me: I need to check this line, there is a change now in my WIP version which is probably wrong.+ }No need to unset a field which is already NULL.
+
+ priv = spi_master_get_devdata(master);
+ priv->vdev = vdev;
+ vdev->priv = priv;
+ dev_set_drvdata(&vdev->dev, master);
+
+ /* the spi->mode bits understood by this driver: */
+ master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST |
+ SPI_LOOP | SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL |
+ SPI_RX_QUAD | SPI_TX_OCTAL | SPI_RX_OCTAL;
+
+ /* What most support. We don't know from the virtio device side */
+ master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 16);
+ /* There is no associated device tree node */
+ master->dev.of_node = NULL;
We have to do it for V9 spec earlier as certain initializations depend now on config space values.+ /* Get bus_num and num_chipselect from the config space */Why call it in the middle of all the initialization. Can we do it
+ virtio_spi_read_config(vdev);
before virtio_spi_find_vqs() ?
+ /* Maybe this method is not needed for virtio SPI */Some of the comments are not useful at all. The fields are self
+ master->setup = NULL; /* Function not needed for virtio-spi */
+ /* No restrictions to announce */
+ master->flags = 0;
+ /* Method to transfer a single SPI message */
+ master->transfer_one_message = virtio_spi_transfer_one_message;
+ /* Method to cleanup the driver */
explanatory and don't need a comment, unless there is a reason for
initializing it in a certain way that you want to mention.
+ master->cleanup = NULL; /* Function not needed for virtio-spi */I am not sure if you need explicit casting here and while converting
+
+ /* Initialize virtqueues */
+ err = virtio_spi_find_vqs(priv);
+ if (err) {
+ dev_err(&vdev->dev, "Cannot setup virtqueues\n");
+ goto err_master_put;
+ }
+
+ err = spi_register_master(master);
+ if (err) {
+ dev_err(&vdev->dev, "Cannot register master\n");
+ goto err_master_put;
+ }
+
+ /* spi_new_device() currently does not use bus_num but better set it */
+ board_info.bus_num = (u16)master->bus_num;
from u16 to s16.
Yes, this is too noisy.+Should be a debug message ?
+ /* Add chip selects to master device */
+ for (csi = 0; csi < master->num_chipselect; csi++) {
+ dev_info(&vdev->dev, "Setting up CS=%u\n", csi);
+ board_info.chip_select = csi;What about freeing already added devices (before we failed) ? Is that
+ if (!spi_new_device(master, &board_info)) {
+ dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
+ goto err_unregister_master;
done by the core automatically ?
Yes, this device does not do any queue request at the end of probe so the line is here not needed to get the device live before return.+ }Normally a driver shouldn't be calling it unless the probe function
+ }
+
+ /* Request device going live */
+ virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */
uses the virtio device, like it is done in GPIO. Since it works for
you just fine, you can simply remove this.
+Debug message ?
+ dev_info(&vdev->dev, "Device live!\n");
Yes. Actions are started by the master so first this guy has to be stopped subsequently everything it depends on. This is not the device side logic, I'm on the driver side.+The ordering should be just the opposite. Free the users first and
+ return 0;
+
+err_unregister_master:
+ spi_unregister_master(master);
+err_master_put:
+ spi_master_put(master);
+err_return:
+ return err;
+}
+
+static void virtio_spi_remove(struct virtio_device *vdev)
+{
+ struct spi_master *master = dev_get_drvdata(&vdev->dev);
+
+ virtio_spi_del_vq(vdev);
+ spi_unregister_master(master);
then the resource.
Same driver / device side view mistake as made above which needs to be fixed.+}You need to setup the queues first and then resume the master.
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * Compare with i2c-virtio.c function virtio_i2c_freeze()
+ * and with spi-atmel.c function atmel_spi_suspend()
+ */
+static int virtio_spi_freeze(struct virtio_device *vdev)
+{
+ struct device *dev = &vdev->dev;
+ struct spi_master *master = dev_get_drvdata(dev);
+ int ret;
+
+ /* Stop the queue running */
+ ret = spi_master_suspend(master);
+ if (ret) {
+ dev_warn(dev, "cannot suspend master (%d)\n", ret);
+ return ret;
+ }
+
+ virtio_spi_del_vq(vdev);
+ return 0;
+}
+
+/*
+ * Compare with i2c-virtio.c function virtio_i2c_restore()
+ * and with spi-atmel.c function atmel_spi_resume()
+ */
+static int virtio_spi_restore(struct virtio_device *vdev)
+{
+ struct device *dev = &vdev->dev;
+ struct spi_master *master = dev_get_drvdata(dev);
+ int ret;
+
+ /* Start the queue running */
+ ret = spi_master_resume(master);
+ if (ret)
+ dev_err(dev, "problem starting queue (%d)\n", ret);
+
+ return virtio_spi_find_vqs(vdev->priv);
+}The 0 value here is optional. This can be just {}.
+#endif
+
+static struct virtio_device_id virtio_spi_id_table[] = {
+ { VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
+ { 0 },
+};You can skip defining them and they should be initialized to NULL/0
+
+static struct virtio_driver virtio_spi_driver = {
+ .feature_table = NULL,
+ .feature_table_size = 0u,
anyway.
Some people did it this way around, other people did it the other way around.+ .driver.name = KBUILD_MODNAME,Or:
+ .driver.owner = THIS_MODULE,
.driver = {
.name = KBUILD_MODNAME,
.owner = THIS_MODULE,
},
Adapting to what's general habit.+ .id_table = virtio_spi_id_table,Here too.
+ .validate = virtio_spi_validate,
+ .probe = virtio_spi_probe,
+ .remove = virtio_spi_remove,
+ .config_changed = NULL,
I see. Not so recent. But recent enough that I don't have it yet on the target. Someone had to point me with the nose on it.+#ifdef CONFIG_PM_SLEEPThis is how we define them now a days.
+ .freeze = virtio_spi_freeze,
+ .restore = virtio_spi_restore,
+#endif
b221df9c4e09 i2c: virtio: Remove #ifdef guards for PM related functions
checkpatch.pl was happy. Looked also elsewhere. No issue here.+};Maybe add right below the table without any blank line in between.
+
+module_virtio_driver(virtio_spi_driver);
+MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
Which is the wording i2c uses and sounds less clumsy.+Maybe just: "Virtio SPI bus driver"
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SPI bus driver for Virtio SPI");
--
2.25.1
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
For additional commands, e-mail: virtio-dev-help@xxxxxxxxxxxxxxxxxxxx