[virtio-dev] Re: [RFC PATCH] Virtio SPI Linux driver.

[virtio-dev] Re: [RFC PATCH] Virtio SPI Linux driver.

[Harald Mommer]

On 27.10.23 18:53, Mark Brown wrote:
> On Fri, Oct 27, 2023 at 06:10:13PM +0200, Harald Mommer wrote:
>
>> This is a virtio SPI Linux driver which is intended to be compliant with
>> the proposed virtio SPI draft specification V4.
> Do you have any pointers to the drafts?

There is a copy to the latest V4 draft at

https://lore.kernel.org/all/20231024125346.23546-1-quic_haixcui@quicinc.com/



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Haixu Cui]

Hi Harald,
     Please refer to my comments below.

On 10/28/2023 12:10 AM, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> 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@opensynergy.com>
> ---
>   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];
> +	/* I/O callback function pointers for the virtqueues */
> +	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_one_transfer(struct virtio_spi_req *spi_req,
> +				   struct spi_master *master,
> +				   struct spi_message *msg,
> +				   struct spi_transfer *xfer)
> +{
> +	struct virtio_spi_priv *priv = spi_master_get_devdata(master);
> +	struct device *dev = &priv->vdev->dev;
> +	struct virtqueue *vq = priv->vqs[VIRTIO_SPI_QUEUE_RQ];
> +	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->slave_id = spi->chip_select;
> +	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;
> +
> +#if (VIRTIO_SPI_CPHA != SPI_CPHA)
> +#error VIRTIO_SPI_CPHA != SPI_CPHA
> +#endif
> +
> +#if (VIRTIO_SPI_CPOL != SPI_CPOL)
> +#error VIRTIO_SPI_CPOL != SPI_CPOL
> +#endif
> +
> +#if (VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH)
> +#error VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH
> +#endif
> +
> +#if (VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST)
> +#error VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST
> +#endif
> +
> +	th->mode = spi->mode &
> +		   (SPI_LSB_FIRST | SPI_CS_HIGH | SPI_CPOL | SPI_CPHA);
> +	if ((spi->mode & SPI_LOOP) != 0)
> +		th->mode |= 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);
> +
> +	/*
> +	 * With the way it's specified in the virtio draft specification
> +	 * V4 the virtio device just MUST print a warning and ignores the delay
> +	 * timing settings it does not support.
> +	 * Implementation decision: Only warn once not to flood the logs.
> +	 * TODO: Comment on this
> +	 * By the wording of the speciification it is unclear which timings
> +	 * exactly are affected, there is a copy & paste mistake in the spec.
> +	 * TODO: Comment on this
> +	 * Somewhat unclear is whether the values in question are to be
> +	 * passed as is to the virtio device.
> +	 *
> +	 * Probably better specification for the device:
> +	 *   The device shall reject a message if tbd delay timing is not
> +	 *   supported but the requested value is not zero by some tbd error.
> +	 * Probably better specification for the driver:
> +	 *   If the device did not announce support of delay timings in the
> +	 *   config space the driver SHOULD not sent a delay timing not equal to
> +	 *   zero but should immediately reject the message.
> +	 */
> +	if (!priv->support_delays) {
> +		if (th->word_delay_ns)
> +			dev_warn_once(dev, "word_delay_ns != 0\n");
> +		if (th->cs_setup_ns)
> +			dev_warn_once(dev, "cs_setup_ns != 0\n");
> +		if (th->cs_change_delay_inactive_ns)
> +			dev_warn_once(dev,
> +				      "cs_change_delay_inactive_ns != 0\n");
> +	}
> +
> +	/* Set buffers */
> +	spi_req->tx_buf = xfer->tx_buf;
> +	spi_req->rx_buf = xfer->rx_buf;
> +#ifdef DEBUG
> +	spi_req->rx_len = xfer->len;
> +#endif
> +
> +	/* Prepare sending of virtio message */
> +	init_completion(&spi_req->completion);
> +
> +	sg_init_one(&sg_out_head, &spi_req->transfer_head,
> +		    sizeof(struct spi_transfer_head));
> +	sgs[outcnt] = &sg_out_head;
> +
> +	pr_debug("sgs[%u] len = %u", outcnt + incnt,
> +		 sgs[outcnt + incnt]->length);
> +	pr_debug("Dump of spi_transfer_head\n");
> +	print_hex_dump_debug(KBUILD_MODNAME " ", DUMP_PREFIX_NONE, 16, 1,
> +			     &spi_req->transfer_head,
> +			     sizeof(struct spi_transfer_head), true);
> +	outcnt++;
> +
> +	if (spi_req->tx_buf) {
> +		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> +		sgs[outcnt] = &sg_out_payload;
> +		pr_debug("sgs[%u] len = %u", outcnt + incnt,
> +			 sgs[outcnt + incnt]->length);
> +		pr_debug("Dump of TX payload\n");
> +		print_hex_dump_debug(KBUILD_MODNAME " ", DUMP_PREFIX_NONE, 16,
> +				     1, spi_req->tx_buf, xfer->len, true);
> +		outcnt++;
> +	}
> +
> +	if (spi_req->rx_buf) {
> +		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> +		sgs[outcnt + incnt] = &sg_in_payload;
> +		pr_debug("sgs[%u] len = %u", outcnt + incnt,
> +			 sgs[outcnt + incnt]->length);
> +		incnt++;
> +	}
> +
> +	sg_init_one(&sg_in_result, &spi_req->result,
> +		    sizeof(struct spi_transfer_result));
> +	sgs[outcnt + incnt] = &sg_in_result;
> +	pr_debug("sgs[%u] len = %u", outcnt + incnt,
> +		 sgs[outcnt + incnt]->length);
> +	incnt++;
> +
> +	pr_debug("%s: outcnt=%u, incnt=%u\n", __func__, outcnt, incnt);
> +
> +	ret = virtqueue_add_sgs(vq, sgs, outcnt, incnt, spi_req, GFP_KERNEL);
> +msg_done:
> +	if (ret)
> +		msg->status = ret;
> +
> +	return ret;
> +}
> +


This function here seems very similar with the default 
transfer_one_message interface provided by Linux driver, that is 
spi_transfer_one_message.

What if using the default interface? In default one, xfer->delay and
xfer->cs_change_delay are executed by software,  which means these two 
values don't need to pass to the backend.

> +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.
> +	 */
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		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) {
> +			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));
> +
> +	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);
> +	priv->support_delays = (support_delays != 0);
> +	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);
> +}
> +
> +/* 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);
> +}
> +
> +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));
> +	if (!master) {
> +		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
> +			__func__);
> +		goto err_return;
> +	}
> +
> +	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;
> +	/* Get bus_num and num_chipselect from the config space */
> +	virtio_spi_read_config(vdev);
> +	/* Maybe this method is not needed for virtio SPI */
> +	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 */
> +	master->cleanup = NULL; /* Function not needed for virtio-spi */
> +
> +	/* 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;

bus_num is not necessary actually, driver will dynamicly assign it if 
bus_num is -1(initial value), besides, it's not necessary to build the 
mapping relationship via bus_num.

> +
> +	/* 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;
> +		if (!spi_new_device(master, &board_info)) {
> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
> +			goto err_unregister_master;
> +		}
> +	}
> +
> +	/* Request device going live */
> +	virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */
> +
> +	dev_info(&vdev->dev, "Device live!\n");
> +
> +	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);
> +}
> +
> +#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);
> +}
> +#endif
> +
> +static struct virtio_device_id virtio_spi_id_table[] = {
> +	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
> +	{ 0 },
> +};
> +
> +static struct virtio_driver virtio_spi_driver = {
> +	.feature_table = NULL,
> +	.feature_table_size = 0u,
> +	.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,
> +	.config_changed = NULL,
> +#ifdef CONFIG_PM_SLEEP
> +	.freeze = virtio_spi_freeze,
> +	.restore = virtio_spi_restore,
> +#endif
> +};
> +
> +module_virtio_driver(virtio_spi_driver);
> +MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
> +
> +MODULE_AUTHOR("OpenSynergy GmbH");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SPI bus driver for Virtio SPI");

Best Regards
Haixcui

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Harald Mommer]

On 06.11.23 14:48, Mark Brown wrote:
> On Fri, Oct 27, 2023 at 06:10:16PM +0200, Harald Mommer wrote:
>
>> +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.
>> +
> This advice is going to be inappropriate for the majortiy of guests.
Reminder for me: Need still to address this, but this is not code I'm 
currently working on, so comes later.
>> +	// 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
> Remove this clang-format stuff.
Not needed any more, will be removed. Maybe I should remove this 
____cacheline_aligned. It's only there because I looked too deeply into 
struct virtio_i2c_req, not because I think this ____cacheline_aligned is 
decisive here.
>> +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 */
> In what way is one supposed to compare with the i2c driver?  What
> happens if the I2C driver changes?  It would be better to ensure that
> the code can be read and understood as a standalone thing.
It was a reminder for me from where I got some inspiration and to reveal 
that not everything was invented by me. Served it's purpose, all such 
comment references to foreign code is being removed.
>> +	/* Fill struct spi_transfer_head */
>> +	th->slave_id = spi->chip_select;
> If the spec just copied the Linux terminology it'd have few issues :(
Spec changed this in the meantime so I can do also.
>> +	th->bits_per_word = spi->bits_per_word;
>> +	th->cs_change = xfer->cs_change;
> The virtio spec for cs_change is *not* what the Linux cs_change field
> does, this will not do the right thing.

This is a hard one. Linux says (spi.h):

"@cs_change: affects chipselect after this transfer completes

...

All SPI transfers start with the relevant chipselect active. Normally
it stays selected until after the last transfer in a message. Drivers
can affect the chipselect signal using cs_change."

V8 of the draft spec says:

"cs_change indicates whether to deselect device before starting the next 
SPI transfer, 0 means chipselect
keep asserted and 1 means chipselect deasserted then asserted again."

What I understand here (unfortunately in both texts) is

- at the start of a transfer CS is made active

- it is kept active during the transfer

- when cs_change is 0 after the transfer CS is kept active (not changed)

- when cs_change is 1 after the transfer CS is de-asserted

So if cs_change is 0 keep CS asserted after transfer, otherwise 
de-assert after transfer.

I fear there is some subtle thing I haven't gotten yet but I don't 
understand / see it.What is it?

>> +	th->tx_nbits = xfer->tx_nbits;
>> +	th->rx_nbits = xfer->rx_nbits;
>> +	th->reserved[0] = 0;
>> +	th->reserved[1] = 0;
>> +	th->reserved[2] = 0;
>> +
>> +#if (VIRTIO_SPI_CPHA != SPI_CPHA)
>> +#error VIRTIO_SPI_CPHA != SPI_CPHA
>> +#endif
> BUILD_BUG_ON()

Thanks for this one, didn't know yet.


---------------------------------------------------------------------
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] [RFC PATCH v1 2/3] virtio-spi: Add virtio-spi.h (V4 draft specification).

[Viresh Kumar]

Hi Harald,

I have reviewed the specifications changes recently and here is an
attempt to go through the kernel code too.

I hope you would be sending a new version soon as there are few
changes in the spec already.

On 27-10-23, 18:10, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> Add initial virtio-spi.h header for virtio SPI. The header file is
> compliant to the virtio SPI draft specification V4.
> 
> Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
> ---
>  include/uapi/linux/virtio_spi.h | 130 ++++++++++++++++++++++++++++++++
>  1 file changed, 130 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..9cf4335784ef
> --- /dev/null
> +++ b/include/uapi/linux/virtio_spi.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */

Maybe this should be:

SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause

?

> +/*
> + * Copyright (C) 2023 OpenSynergy GmbH
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
> +#define _LINUX_VIRTIO_VIRTIO_SPI_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +// clang-format off

Why do we want to avoid clang-format here ?

> +/* Sample data on trailing clock edge */
> +#define VIRTIO_SPI_CPHA (1u << 0)
> +/* Clock is high when IDLE */
> +#define VIRTIO_SPI_CPOL (1u << 1)
> +/* Chip Select is active high */
> +#define VIRTIO_SPI_CS_HIGH (1u << 2)
> +/* Transmit LSB first */
> +#define VIRTIO_SPI_MODE_LSB_FIRST (1u << 3)
> +
> +/*
> + * Beware: From here on the bits do not match any more the Linux definitions!
> + */

Not sure if this is really required here. We are talking about the
interface defined by the Virtio protocol here and there can be
mismatch with Linux definitions.

> +/* Loopback mode */
> +#define VIRTIO_SPI_MODE_LOOP (1u << 4)
> +
> +/* All config fields are read-only for the Virtio SPI driver */
> +struct virtio_spi_config {

Can you please add proper doc style comments for the structures ?

> +	/* /dev/spidev<bus_num>.CS. For Linux must be >= 0 and <= S16_MAX */
> +	__le16 bus_num;
> +	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
> +	__le16 chip_select_max_number;
> +	/*
> +	 * 0: physical SPI master doesn't support cs timing setting"
> +	 * 1:_physical SPI master supports cs timing setting
> +	 * TODO: Comment on this, unclear and naming not good!
> +	 * Meant is probably word_delay_ns, cs_setup_ns and cs_delay_hold_ns
> +	 * while cs_change_delay_inactive_ns may be supportable everywhere
> +	 * Or all are meant. And the naming mismatch is the cs_ when the most
> +	 * critical word_delay_ns which cannot be supported everywhere is also
> +	 * affected.
> +	 */
> +	u8 cs_timing_setting_enable;
> +	/* Alignment and future extension */
> +	u8 reserved[3];
> +};
> +
> +/*
> + * @slave_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 slave_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_TRANS_ERR 1

Maybe just define them above the struct.

> +	__u8 result;
> +};
> +// clang-format on
> +
> +#endif /* #ifndef _COQOS_VIRTIO_VIRTIO_SPI_H */

s/_COQOS_VIRTIO_VIRTIO_SPI_H/_LINUX_VIRTIO_VIRTIO_SPI_H/

-- 
viresh

---------------------------------------------------------------------
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] [RFC PATCH v1 2/3] virtio-spi: Add virtio-spi.h (V4 draft specification).

[Harald Mommer]

Hello Viresh,

On 12.12.23 11:34, Viresh Kumar wrote:
> Hi Harald,
>
> I have reviewed the specifications changes recently and here is an
> attempt to go through the kernel code too.
>
> I hope you would be sending a new version soon as there are few
> changes in the spec already.

I'm working on V8. It's coming to an end, will still have to check some 
details but it's close. Internal review pending. Now there is a V9 and I 
will also have to look at this. Maybe I will send V8 and subsequently 
update to V9,

> On 27-10-23, 18:10, Harald Mommer wrote:
>> From: Harald Mommer <harald.mommer@opensynergy.com>
>>
>> Add initial virtio-spi.h header for virtio SPI. The header file is
>> compliant to the virtio SPI draft specification V4.
>>
>> Signed-off-by: Harald Mommer <harald.mommer@opensynergy.com>
>> ---
>>   include/uapi/linux/virtio_spi.h | 130 ++++++++++++++++++++++++++++++++
>>   1 file changed, 130 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..9cf4335784ef
>> --- /dev/null
>> +++ b/include/uapi/linux/virtio_spi.h
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
> Maybe this should be:
>
> SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>
> ?
Looking into what others do here. virtio_blk.h, virtio_input.h and 
virtio_iommu.h for example: None is using GPL-2.0 here. virtio_iommu.h 
is using exactly the same header as we do.
>> +/*
>> + * Copyright (C) 2023 OpenSynergy GmbH
>> + */
>> +#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
>> +#define _LINUX_VIRTIO_VIRTIO_SPI_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/virtio_types.h>
>> +#include <linux/virtio_ids.h>
>> +#include <linux/virtio_config.h>
>> +
>> +// clang-format off
> Why do we want to avoid clang-format here ?
Will be removed in the V8+ version.
>> +/* Sample data on trailing clock edge */
>> +#define VIRTIO_SPI_CPHA (1u << 0)
>> +/* Clock is high when IDLE */
>> +#define VIRTIO_SPI_CPOL (1u << 1)
>> +/* Chip Select is active high */
>> +#define VIRTIO_SPI_CS_HIGH (1u << 2)
>> +/* Transmit LSB first */
>> +#define VIRTIO_SPI_MODE_LSB_FIRST (1u << 3)
>> +
>> +/*
>> + * Beware: From here on the bits do not match any more the Linux definitions!
>> + */
> Not sure if this is really required here. We are talking about the
> interface defined by the Virtio protocol here and there can be
> mismatch with Linux definitions.

This is a friendliness for implementers so that they get awake here and 
don't walk into a trap just because the first 4 definitions were 
identical by coincidence.No, it's not required, I can also let people 
let go into the trap. Including myself. But you have a point here as the 
header is not really Linux specific but Virtio specific. And I have 4 
BUILD_BUG_ON in the C source code.

=> Out with this here.

>> +/* Loopback mode */
>> +#define VIRTIO_SPI_MODE_LOOP (1u << 4)
>> +
>> +/* All config fields are read-only for the Virtio SPI driver */
>> +struct virtio_spi_config {
> Can you please add proper doc style comments for the structures ?
Checking my current code. This is updated in the V8 version.
>> +	/* /dev/spidev<bus_num>.CS. For Linux must be >= 0 and <= S16_MAX */
>> +	__le16 bus_num;
>> +	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
>> +	__le16 chip_select_max_number;
>> +	/*
>> +	 * 0: physical SPI master doesn't support cs timing setting"
>> +	 * 1:_physical SPI master supports cs timing setting
>> +	 * TODO: Comment on this, unclear and naming not good!
>> +	 * Meant is probably word_delay_ns, cs_setup_ns and cs_delay_hold_ns
>> +	 * while cs_change_delay_inactive_ns may be supportable everywhere
>> +	 * Or all are meant. And the naming mismatch is the cs_ when the most
>> +	 * critical word_delay_ns which cannot be supported everywhere is also
>> +	 * affected.
>> +	 */
>> +	u8 cs_timing_setting_enable;
>> +	/* Alignment and future extension */
>> +	u8 reserved[3];
>> +};
>> +
>> +/*
>> + * @slave_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 slave_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_TRANS_ERR 1
> Maybe just define them above the struct.

People do here things differently in the virtio headers. Regardless what 
I do I would get comments.

In most virtio headers the constants are defined above the struct. Which 
makes it more difficult to see which definition is meant for which 
member and leads to comments. virtio_net.h does it frequently in the way 
it's done here. In the V8 version there will be much more definitions in 
the middle of the structures for constants which are needed but only 
textually defined in the specification. For those constants it's 
desirable to have them close to the structure members for which they are 
meant. I think I should stick to the way virtio_net.h does it.

>> +	__u8 result;
>> +};
>> +// clang-format on
>> +
>> +#endif /* #ifndef _COQOS_VIRTIO_VIRTIO_SPI_H */
> s/_COQOS_VIRTIO_VIRTIO_SPI_H/_LINUX_VIRTIO_VIRTIO_SPI_H/
Needs to be fixed.


---------------------------------------------------------------------
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] [RFC PATCH v1 2/3] virtio-spi: Add virtio-spi.h (V4 draft specification).

[Viresh Kumar]

On 12-12-23, 19:58, Harald Mommer wrote:
> On 12.12.23 11:34, Viresh Kumar wrote:
> I'm working on V8. It's coming to an end, will still have to check some
> details but it's close. Internal review pending. Now there is a V9 and I
> will also have to look at this. Maybe I will send V8 and subsequently update
> to V9,

I hope you are talking about V8/V9 of the spec here, as I only see one
version of the Linux driver on the list. Please keep me in cc if
possible.

> > On 27-10-23, 18:10, Harald Mommer wrote:
> > > +++ b/include/uapi/linux/virtio_spi.h
> > > @@ -0,0 +1,130 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause */
> > Maybe this should be:
> > 
> > SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > 
> > ?
> Looking into what others do here. virtio_blk.h, virtio_input.h and
> virtio_iommu.h for example: None is using GPL-2.0 here. virtio_iommu.h is
> using exactly the same header as we do.

Looked at all headers for SPDX License in include/uapi/ and this is
what I see (Yes there are many non SPDX licenses there):

    522 /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
    106 /* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
     18 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
     16 /* SPDX-License-Identifier: LGPL-2.1+ WITH Linux-syscall-note */
     16 /* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
     11 /* SPDX-License-Identifier: GPL-1.0+ WITH Linux-syscall-note */
      6 /* SPDX-License-Identifier: ((GPL-2.0-only WITH Linux-syscall-note) OR BSD-3-Clause) */
      5 /* SPDX-License-Identifier: BSD-3-Clause */
      4 /* SPDX-License-Identifier: LGPL-2.1 WITH Linux-syscall-note */
      4 /* SPDX-License-Identifier: LGPL-2.0+ WITH Linux-syscall-note */
      4 /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
      3 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR MIT */
      2 /* SPDX-License-Identifier: MIT */
      2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR Linux-OpenIB) */
      2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR CDDL-1.0) */
      2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
      2 /* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
      1 /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
      1 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause */
      1 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) */
      1 /* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note*/

Also Documentation/process/license-rules.rst says:

"The license described in the COPYING file applies to the kernel source
as a whole, though individual source files can have a different license
which is required to be compatible with the GPL-2.0::

...

Aside from that, individual files can be provided under a dual license,
e.g. one of the compatible GPL variants and alternatively under a
permissive license like BSD, MIT etc."

And so I thought we may want this to be a dual license.

> > > +/* All config fields are read-only for the Virtio SPI driver */
> > > +struct virtio_spi_config {
> > Can you please add proper doc style comments for the structures ?
> Checking my current code. This is updated in the V8 version.

V8 of this patch ?

-- 
viresh

---------------------------------------------------------------------
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] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).

[Viresh Kumar]

Hi Harald,

On 27-10-23, 18:10, Harald Mommer wrote:
> From: Harald Mommer <harald.mommer@opensynergy.com>
> 
> 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@opensynergy.com>
> ---
>  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];

There is no need to make this configurable since the specification
fixes it to 1. You can simplify this a bit.

> +	/* I/O callback function pointers for the virtqueues */
> +	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.
> +	 */

And why not send all the messages once and speed this thing up ? Just
like how I2C does it.

> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		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

Drop all temporary things please.

> +		if (spi_req->rx_buf) {
> +			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));

Instead of reading values separately, you can also read the entire
configuration structure in a single call to virtio_cread_bytes.

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.

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

Why is this a warning ? And not just debug or info ?

> +	priv->support_delays = (support_delays != 0);
> +	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);

Since the vq count is fixed by spec to 1, I think you can directly use
virtio_find_single_vq() and simplify this a bit.

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

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 virtio_spi_priv *priv;
> +	struct spi_master *master;
> +	int err;
> +	u16 csi;
> +
> +	err = -ENOMEM;

Why not do it with the definition itself ?

> +	master = spi_alloc_master(&vdev->dev, sizeof(struct virtio_spi_priv));

sizeof(*priv)

and there is a devm_* variant too that you can use.

> +	if (!master) {
> +		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
> +			__func__);

I think we removed print messages for allocation failure earlier, as
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.

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

No need to unset a field which is already NULL.

> +	/* Get bus_num and num_chipselect from the config space */
> +	virtio_spi_read_config(vdev);

Why call it in the middle of all the initialization. Can we do it
before virtio_spi_find_vqs() ?

> +	/* Maybe this method is not needed for virtio SPI */
> +	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 */

Some of the comments are not useful at all. The fields are self
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 */
> +
> +	/* 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;

I am not sure if you need explicit casting here and while converting
from u16 to s16.

> +
> +	/* Add chip selects to master device */
> +	for (csi = 0; csi < master->num_chipselect; csi++) {
> +		dev_info(&vdev->dev, "Setting up CS=%u\n", csi);

Should be a debug message ?

> +		board_info.chip_select = csi;
> +		if (!spi_new_device(master, &board_info)) {
> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
> +			goto err_unregister_master;

What about freeing already added devices (before we failed) ? Is that
done by the core automatically ?

> +		}
> +	}
> +
> +	/* Request device going live */
> +	virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */

Normally a driver shouldn't be calling it unless the probe function
uses the virtio device, like it is done in GPIO. Since it works for
you just fine, you can simply remove this.

> +
> +	dev_info(&vdev->dev, "Device live!\n");

Debug message ?

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

The ordering should be just the opposite. Free the users first and
then the resource.

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

You need to setup the queues first and then resume the master.

> +}
> +#endif
> +
> +static struct virtio_device_id virtio_spi_id_table[] = {
> +	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
> +	{ 0 },

The 0 value here is optional. This can be just {}.

> +};
> +
> +static struct virtio_driver virtio_spi_driver = {
> +	.feature_table = NULL,
> +	.feature_table_size = 0u,

You can skip defining them and they should be initialized to NULL/0
anyway.

> +	.driver.name = KBUILD_MODNAME,
> +	.driver.owner = THIS_MODULE,

Or:
	.driver = {
		.name	= KBUILD_MODNAME,
		.owner  = THIS_MODULE,
	},

> +	.id_table = virtio_spi_id_table,
> +	.validate = virtio_spi_validate,
> +	.probe = virtio_spi_probe,
> +	.remove = virtio_spi_remove,
> +	.config_changed = NULL,

Here too.

> +#ifdef CONFIG_PM_SLEEP
> +	.freeze = virtio_spi_freeze,
> +	.restore = virtio_spi_restore,
> +#endif

This is how we define them now a days.

b221df9c4e09 i2c: virtio: Remove #ifdef guards for PM related functions

> +};
> +
> +module_virtio_driver(virtio_spi_driver);

> +MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);

Maybe add right below the table without any blank line in between.

> +
> +MODULE_AUTHOR("OpenSynergy GmbH");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SPI bus driver for Virtio SPI");

Maybe just: "Virtio SPI bus driver"

> -- 
> 2.25.1
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

-- 
viresh

---------------------------------------------------------------------
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] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).

[Harald Mommer]

Hello Viresh,

On 13.12.23 10:53, Viresh Kumar wrote:
> Hi Harald,
>
> On 27-10-23, 18:10, Harald Mommer wrote:
>> From: Harald Mommer <harald.mommer@opensynergy.com>
>>
>> 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@opensynergy.com>
>> ---
>>   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];
> There is no need to make this configurable since the specification
> fixes it to 1. You can simplify this a bit.
Yes, i2c makes it "struct virtqueue *vq;" and this makes also sense here.
>> +	/* I/O callback function pointers for the virtqueues */
>> +	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.
>> +	 */
> And why not send all the messages once and speed this thing up ? Just
> like how I2C does it.

Because this is more complicated when I looked into i2c. First I wanted 
to have a working driver which can be delivered to our internal project. 
This internal project has no sophisticated performance requirements. 
Better to have something for the internal project when you have to 
deliver to them instead of having nothing at all because you wanted 
initially too much nobody asked for.

To minimize risk and not to extend the scope of my existing tickets too 
much I will address your comment not by a code change but by a comment 
in the code. Nothing is broken, this is an optimization only.

>> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>> +		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
> Drop all temporary things please.

This is an RFC patch, not an integration request. Until the spec is 
accepted by OASIS the driver is code under development tracking a moving 
target and will have to remain RFC. Removing debug aids is something to 
be addressed before we are going for non-RFC.

>> +		if (spi_req->rx_buf) {
>> +			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));
> Instead of reading values separately, you can also read the entire
> configuration structure in a single call to virtio_cread_bytes.

Cannot.

Virtio spec 4.2.2.2 Driver Requirements: MMIO Device Register Layout

...
For the device­-specific configuration space, the driver MUST use 8 bit 
wide accesses for 8 bit wide fields, 16 bit wide and aligned accesses 
for 16 bit wide fields and 32 bit wide and aligned accesses for 32 and 
64 bit wide fields.

>
> 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.
virtio_cread16() does also already exactly this conversion in the same 
way virtio_cread32() does.
>> +
>> +	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);
> Why is this a warning ? And not just debug or info ?

Here only stuff is printed when something is wrong. So this is not a 
debug or info.

For bus_num which will in the next version come optionally from the 
device tree the device tree value is silently ignored when it's out of 
range in my current code. Now I think about even to make it dev_err() 
making virtio_spi_probe() return an error. Not sure yet.

>> +	priv->support_delays = (support_delays != 0);
>> +	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);
> Since the vq count is fixed by spec to 1, I think you can directly use
> virtio_find_single_vq() and simplify this a bit.
Yes.
>> +}
>> +
>> +/* 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);
> virtio_reset_device(vdev)
Yes, not only shorter but does also additional things if 
CONFIG_VIRTIO_HARDEN_NOTIFICATION has been enabled.
>> +	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 virtio_spi_priv *priv;
>> +	struct spi_master *master;
>> +	int err;
>> +	u16 csi;
>> +
>> +	err = -ENOMEM;
> Why not do it with the definition itself ?

Replaced in the meantime all definitions containing the word "master" by 
the word controller when they existed so I'm not using the compatibility 
layer containing preprocessor definitions any more. For some reason 
there is no function spi_alloc_controller() so this here is the only one 
which cannot be replaced. It's already the function.

Update: In newer kernels there is now an identical spi_alloc_host(). Why 
now host instead of controller? I get mad.A lot of people still use 
spi_alloc_master().But this is a new driver so a change to 
spi_alloc_host() is most probably required.

>> +	master = spi_alloc_master(&vdev->dev, sizeof(struct virtio_spi_priv));
> sizeof(*priv)

Some people do it this way, others the other way.

> 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.
>> +	if (!master) {
>> +		dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
>> +			__func__);
> I think we removed print messages for allocation failure earlier, as
> the alloc core handles it now. This may not be required.
Seems to be that way. Other people print nothing.
>> +		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.

Again here, some people do it this way and other people the other way 
around.

>> +	}
>> +
>> +	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;
> No need to unset a field which is already NULL.
Yes. And reminder for me: I need to check this line, there is a change 
now in my WIP version which is probably wrong.
>> +	/* Get bus_num and num_chipselect from the config space */
>> +	virtio_spi_read_config(vdev);
> Why call it in the middle of all the initialization. Can we do it
> before virtio_spi_find_vqs() ?
We have to do it for V9 spec earlier as certain initializations depend 
now on config space values.
>> +	/* Maybe this method is not needed for virtio SPI */
>> +	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 */
> Some of the comments are not useful at all. The fields are self
> explanatory and don't need a comment, unless there is a reason for
> initializing it in a certain way that you want to mention.

Maybe. Here really seems to be over-commented in hindsight.

>> +	master->cleanup = NULL; /* Function not needed for virtio-spi */
>> +
>> +	/* 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;
> I am not sure if you need explicit casting here and while converting
> from u16 to s16.

First the code is made that way that master->bus_num is in the range 
0..S16_MAX so that we have no loss of anything in this assignment with 
or without cast.

Played around in user space with int16_t and uint16_t. Without cast

- MISRA C 2012 Rule 10.1 violation but we're not doing MISRA C here so 
this may be considered as irrelevant.

- Cert C I am not really sure, would be CERT-C INT31-C and the examples 
there show a cast. We are not doing CERT C here but I take this now as a 
small hint.

- FlexeLint warning "Info 732: Loss of sign (assignment) (short to 
unsigned short)" in the default settings. May be irrelevant for people 
never having used FlexeLint but I personally take this already as a 
stronger hint.

- Searched around: With "gcc -Wsign-conversion" or "gcc -Wconversion"

warning: conversion to ‘int16_t’ {aka ‘short int’} from ‘uint16_t’ {aka 
‘short unsigned int’} may change the sign of the result

Learned now that also for gcc exists a warning setting which makes the 
lack of the cast a warning. The cast may not be needed for the current 
settings but for my taste it should stay in.

>> +
>> +	/* Add chip selects to master device */
>> +	for (csi = 0; csi < master->num_chipselect; csi++) {
>> +		dev_info(&vdev->dev, "Setting up CS=%u\n", csi);
> Should be a debug message ?
Yes, this is too noisy.
>> +		board_info.chip_select = csi;
>> +		if (!spi_new_device(master, &board_info)) {
>> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> +			goto err_unregister_master;
> What about freeing already added devices (before we failed) ? Is that
> done by the core automatically ?

Looking what others do I see spi-xilinx.c not even checking for the 
return value...off-topic.

The function spi_unregister_controller() has a comment

"/* Prevent addition of new devices, unregister existing ones */"

and there is a line below "device_for_each_child(&ctlr->dev, NULL, 
__unregister);".

I would say it's done by the core.

>> +		}
>> +	}
>> +
>> +	/* Request device going live */
>> +	virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */
> Normally a driver shouldn't be calling it unless the probe function
> uses the virtio device, like it is done in GPIO. Since it works for
> you just fine, you can simply remove this.
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.
>> +
>> +	dev_info(&vdev->dev, "Device live!\n");
> Debug message ?

This or remove.

>> +
>> +	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);
> The ordering should be just the opposite. Free the users first and
> then the resource.
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.
>> +}
>> +
>> +#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);
> You need to setup the queues first and then resume the master.
Same driver / device side view mistake as made above which needs to be 
fixed.
>> +}
>> +#endif
>> +
>> +static struct virtio_device_id virtio_spi_id_table[] = {
>> +	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
>> +	{ 0 },
> The 0 value here is optional. This can be just {}.

Maybe. I just looked here what others did. And others (virtio_net.c for 
example but not only) have { 0 },

>> +};
>> +
>> +static struct virtio_driver virtio_spi_driver = {
>> +	.feature_table = NULL,
>> +	.feature_table_size = 0u,
> You can skip defining them and they should be initialized to NULL/0
> anyway.

Yes. What others do here is what you propose.

>> +	.driver.name = KBUILD_MODNAME,
>> +	.driver.owner = THIS_MODULE,
> Or:
> 	.driver = {
> 		.name	= KBUILD_MODNAME,
> 		.owner  = THIS_MODULE,
> 	},
Some people did it this way around, other people did it the other way 
around.
>> +	.id_table = virtio_spi_id_table,
>> +	.validate = virtio_spi_validate,
>> +	.probe = virtio_spi_probe,
>> +	.remove = virtio_spi_remove,
>> +	.config_changed = NULL,
> Here too.
Adapting to what's general habit.
>> +#ifdef CONFIG_PM_SLEEP
>> +	.freeze = virtio_spi_freeze,
>> +	.restore = virtio_spi_restore,
>> +#endif
> This is how we define them now a days.
>
> b221df9c4e09 i2c: virtio: Remove #ifdef guards for PM related functions
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.
>> +};
>> +
>> +module_virtio_driver(virtio_spi_driver);
>> +MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
> Maybe add right below the table without any blank line in between.
checkpatch.pl was happy. Looked also elsewhere. No issue here.
>> +
>> +MODULE_AUTHOR("OpenSynergy GmbH");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("SPI bus driver for Virtio SPI");
> Maybe just: "Virtio SPI bus driver"
Which is the wording i2c uses and sounds less clumsy.
>> -- 
>> 2.25.1
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


---------------------------------------------------------------------
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] [RFC PATCH v1 2/3] virtio-spi: Add virtio-spi.h (V4 draft specification).

[Harald Mommer]

Hello Viresh,

On 13.12.23 07:33, Viresh Kumar wrote:
> On 12-12-23, 19:58, Harald Mommer wrote:
>> On 12.12.23 11:34, Viresh Kumar wrote:
>> I'm working on V8. It's coming to an end, will still have to check some
>> details but it's close. Internal review pending. Now there is a V9 and I
>> will also have to look at this. Maybe I will send V8 and subsequently update
>> to V9,
> I hope you are talking about V8/V9 of the spec here, as I only see one
> version of the Linux driver on the list. Please keep me in cc if
> possible.

What you see is RFC PATCH v1 which meets the V4 draft specification. The 
only one which has been sent so far. For you this is latest. And now I 
got so much comments from you and also a spec update from V8 to V9 so 
that this will remain that way for some days. Need to do changes.

The next one will be RFC patch v2 and is planned to be made according to 
the V9 spec.

And most probably the next one will also attempt to be V9 spec compliant 
if there comes in the mean time a specification update. Moving target 
not good if you work agile, extension of scope all the time.

>>> On 27-10-23, 18:10, Harald Mommer wrote:
>>>> +++ b/include/uapi/linux/virtio_spi.h
>>>> @@ -0,0 +1,130 @@
>>>> +/* SPDX-License-Identifier: BSD-3-Clause */
>>> Maybe this should be:
>>>
>>> SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>>
>>> ?
>> Looking into what others do here. virtio_blk.h, virtio_input.h and
>> virtio_iommu.h for example: None is using GPL-2.0 here. virtio_iommu.h is
>> using exactly the same header as we do.
> Looked at all headers for SPDX License in include/uapi/ and this is
> what I see (Yes there are many non SPDX licenses there):
>
>      522 /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>      106 /* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>       18 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
>       16 /* SPDX-License-Identifier: LGPL-2.1+ WITH Linux-syscall-note */
>       16 /* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
>       11 /* SPDX-License-Identifier: GPL-1.0+ WITH Linux-syscall-note */
>        6 /* SPDX-License-Identifier: ((GPL-2.0-only WITH Linux-syscall-note) OR BSD-3-Clause) */
>        5 /* SPDX-License-Identifier: BSD-3-Clause */
>        4 /* SPDX-License-Identifier: LGPL-2.1 WITH Linux-syscall-note */
>        4 /* SPDX-License-Identifier: LGPL-2.0+ WITH Linux-syscall-note */
>        4 /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
>        3 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR MIT */
>        2 /* SPDX-License-Identifier: MIT */
>        2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR Linux-OpenIB) */
>        2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR CDDL-1.0) */
>        2 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
>        2 /* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
>        1 /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
>        1 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause */
>        1 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) */
>        1 /* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note*/
>
> Also Documentation/process/license-rules.rst says:
>
> "The license described in the COPYING file applies to the kernel source
> as a whole, though individual source files can have a different license
> which is required to be compatible with the GPL-2.0::
>
> ...
>
> Aside from that, individual files can be provided under a dual license,
> e.g. one of the compatible GPL variants and alternatively under a
> permissive license like BSD, MIT etc."
>
> And so I thought we may want this to be a dual license.

Please focus on include/uapi/linux/virtio_*.h files only.

You will see a lot BSD without mentioning GPL at all. So we are just 
doing what others did and what was accepted in the past. Changing the 
license is not a change I can do with a finger tip.

>>>> +/* All config fields are read-only for the Virtio SPI driver */
>>>> +struct virtio_spi_config {
>>> Can you please add proper doc style comments for the structures ?
>> Checking my current code. This is updated in the V8 version.
> V8 of this patch ?

V8 of the spec. Don't worry, you missed no code change, you are on 
public latest.



---------------------------------------------------------------------
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] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).

[Viresh Kumar]

+ Michael and Cornelia

Hello Harald,

On 14-12-23, 17:52, Harald Mommer wrote:
> On 13.12.23 10:53, Viresh Kumar wrote:
> > On 27-10-23, 18:10, Harald Mommer wrote:
> > > +	struct virtqueue *vqs[VIRTIO_SPI_QUEUE_COUNT];
> > There is no need to make this configurable since the specification
> > fixes it to 1. You can simplify this a bit.
> Yes, i2c makes it "struct virtqueue *vq;" and this makes also sense here.

Please add a blank line before and after your responses. It becomes very
difficult otherwise and one may end up missing some of the comments,
especially with bottom posting.

> > > +	/*
> > > +	 * Simple implementation: Process message by message and wait for each
> > > +	 * message to be completed by the device side.
> > > +	 */
> > And why not send all the messages once and speed this thing up ? Just
> > like how I2C does it.
> 
> Because this is more complicated when I looked into i2c. First I wanted to
> have a working driver which can be delivered to our internal project. This
> internal project has no sophisticated performance requirements. Better to
> have something for the internal project when you have to deliver to them
> instead of having nothing at all because you wanted initially too much
> nobody asked for.
> 
> To minimize risk and not to extend the scope of my existing tickets too much
> I will address your comment not by a code change but by a comment in the
> code. Nothing is broken, this is an optimization only.

I understand that the current implementation is good enough for your
use case. But you are trying to upstream this and there are more users
who will end up using it. Qualcomm is one for example, who is
upstreaming the spec.

I think the changes required shouldn't be huge and can be taken care
of easily, unless there is a big blocker. Given that I2C already
implements it that way, I think it would be easier to do it now from
the first implementation itself, since it isn't already merged.

Please see if that can be done, or I will write an incremental patch
for this.

> > > +#ifdef DEBUG
> > Drop all temporary things please.
> 
> This is an RFC patch, not an integration request. Until the spec is accepted
> by OASIS the driver is code under development tracking a moving target and
> will have to remain RFC. Removing debug aids is something to be addressed
> before we are going for non-RFC.

Since this is up for review, I would have liked to see a version
closer to what is supposed to be merged. It doesn't harm to keep all
such changes in a separate commit which you don't post to LKML. That
makes it easier for reviewers to go through stuff without getting
distracted with small things.

> > > +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));
> > Instead of reading values separately, you can also read the entire
> > configuration structure in a single call to virtio_cread_bytes.
> 
> Cannot.
> 
> Virtio spec 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> 
> ...
> For the device­-specific configuration space, the driver MUST use 8 bit wide
> accesses for 8 bit wide fields, 16 bit wide and aligned accesses for 16 bit
> wide fields and 32 bit wide and aligned accesses for 32 and 64 bit wide
> fields.

Fair point. I wonder if other implementations have got it wrong now.

Michael / Cornelia,

What's the right thing to do here ? I am not sure why that
driver-normative was even required for MMIO.

> > > +static int virtio_spi_probe(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_spi_priv *priv;
> > > +	struct spi_master *master;
> > > +	int err;
> > > +	u16 csi;
> > > +
> > > +	err = -ENOMEM;

I was only suggesting to use:

int err = -ENOMEM;

:)

> > Why not do it with the definition itself ?
> 
> Replaced in the meantime all definitions containing the word "master" by the
> word controller when they existed so I'm not using the compatibility layer
> containing preprocessor definitions any more. For some reason there is no
> function spi_alloc_controller() so this here is the only one which cannot be
> replaced. It's already the function.
> 
> Update: In newer kernels there is now an identical spi_alloc_host(). Why now
> host instead of controller?

IIUC, both master/slave are called controllers here.

And with below commit:

commit b8d3b056a78d ("spi: introduce new helpers with using modern naming")

Master => Host
Slave => Target

I guess eventually Master and Slave interfaces will be removed, and we
are in the transition phase for now.

> I get mad.A lot of people still use
> spi_alloc_master().But this is a new driver so a change to spi_alloc_host()
> is most probably required.

Right.

> > > +	master = spi_alloc_master(&vdev->dev, sizeof(struct virtio_spi_priv));
> > sizeof(*priv)
> 
> Some people do it this way, others the other way.

Documentation/process/coding-style.rst

14) Allocating memory

...

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

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

Yeah, that also has a devm_* variant.

Thanks.

-- 
viresh

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org