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

[Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>]

Hello,

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


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

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


Will fix.


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


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

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


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

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


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


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


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


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


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

In this code there is

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

....

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

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


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


Indeed missing. I guess

  vdev->config->del_vqs(vdev);

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

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

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


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

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

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

and at the end of the function a

  spi_controller_put(ctrl);

but nothing of this here as reading the comments

devm_spi_alloc_host() => __devm_spi_alloc_controller()

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

no spi_controller_put() with devm.

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

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