Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen <[email protected]>
Signed-off-by: Conghui Chen <[email protected]>
Signed-off-by: Jie Deng <[email protected]>
---
Changes v13 -> v14
- Put the headers in virtio_i2c.h in alphabetical order.
- Dropped I2C_FUNC_SMBUS_QUICK support.
- Dropped few unnecessary variables and checks.
- Use "num" everywhere instead of num or nr, to be consistent.
- Added few comments which make the design more clear.
Changes v12 -> v13
- Use _BITUL() instead of BIT().
- Rename "virtio_i2c_send_reqs" to "virtio_i2c_prepare_reqs".
- Optimize the return value of "virtio_i2c_complete_reqs".
Changes v11 -> v12
- Do not sent msg_buf for zero-length request.
- Send requests to host only if all the number of transfers requested prepared successfully.
- Remove the line #include <linux/bits.h> in virtio_i2c.h.
Changes v10 -> v11
- Remove vi->adap.class = I2C_CLASS_DEPRECATED.
- Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
- Remove "struct mutex lock" in "struct virtio_i2c".
- Support zero-length request.
- Remove unnecessary logs.
- Remove vi->adap.timeout = HZ / 10, just use the default value.
- Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
- Add the virtio_device index to adapter's naming mechanism.
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 285 ++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/virtio_i2c.h | 41 ++++++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 341 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 10acece..e47616a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ select VIRTIO
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 69e9963..9843756 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 0000000..0139cdc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include <linux/acpi.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_i2c.h>
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
+ uint8_t *buf ____cacheline_aligned;
+ struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(&vi->completion);
+}
+
+static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int num)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i;
+
+ for (i = 0; i < num; i++) {
+ int outcnt = 0, incnt = 0;
+
+ /*
+ * We don't support 0 length messages and so masked out
+ * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
+ */
+ if (!msgs[i].len)
+ break;
+
+ /*
+ * Only 7-bit mode supported for this moment. For the address
+ * format, Please check the Virtio I2C Specification.
+ */
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != num - 1)
+ reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
+
+ sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = &out_hdr;
+
+ reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+ if (!reqs[i].buf)
+ break;
+
+ sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
+ if (msgs[i].flags & I2C_M_RD)
+ sgs[outcnt + incnt++] = &msg_buf;
+ else
+ sgs[outcnt++] = &msg_buf;
+
+ sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+ sgs[outcnt + incnt++] = &in_hdr;
+
+ if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
+ break;
+ }
+ }
+
+ return i;
+}
+
+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int num,
+ bool timedout)
+{
+ struct virtio_i2c_req *req;
+ bool failed = timedout;
+ unsigned int len;
+ int i, j = 0;
+
+ for (i = 0; i < num; i++) {
+ /* Detach the ith request from the vq */
+ req = virtqueue_get_buf(vq, &len);
+
+ /*
+ * Condition req == &reqs[i] should always meet since we have
+ * total num requests in the vq. reqs[i] can never be NULL here.
+ */
+ if (!failed && (WARN_ON(req != &reqs[i]) ||
+ req->in_hdr.status != VIRTIO_I2C_MSG_OK))
+ failed = true;
+
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+
+ if (!failed)
+ j++;
+ }
+
+ return timedout ? -ETIMEDOUT : j;
+}
+
+static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ struct virtio_i2c *vi = i2c_get_adapdata(adap);
+ struct virtqueue *vq = vi->vq;
+ struct virtio_i2c_req *reqs;
+ unsigned long time_left;
+ int count;
+
+ reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+ if (!reqs)
+ return -ENOMEM;
+
+ count = virtio_i2c_prepare_reqs(vq, reqs, msgs, num);
+ if (!count)
+ goto err_free;
+
+ /*
+ * For the case where count < num, i.e. we weren't able to queue all the
+ * msgs, ideally we should abort right away and return early, but some
+ * of the messages are already sent to the remote I2C controller and the
+ * virtqueue will be left in undefined state in that case. We kick the
+ * remote here to clear the virtqueue, so we can try another set of
+ * messages later on.
+ */
+
+ reinit_completion(&vi->completion);
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
+ if (!time_left)
+ dev_err(&adap->dev, "virtio i2c backend timeout.\n");
+
+ count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left);
+
+err_free:
+ kfree(reqs);
+ return count;
+}
+
+static void virtio_i2c_del_vqs(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);
+}
+
+static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
+{
+ struct virtio_device *vdev = vi->vdev;
+
+ vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
+ return PTR_ERR_OR_ZERO(vi->vq);
+}
+
+static u32 virtio_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+}
+
+static struct i2c_algorithm virtio_algorithm = {
+ .master_xfer = virtio_i2c_xfer,
+ .functionality = virtio_i2c_func,
+};
+
+static int virtio_i2c_probe(struct virtio_device *vdev)
+{
+ struct device *pdev = vdev->dev.parent;
+ struct virtio_i2c *vi;
+ int ret;
+
+ vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
+ if (!vi)
+ return -ENOMEM;
+
+ vdev->priv = vi;
+ vi->vdev = vdev;
+
+ init_completion(&vi->completion);
+
+ ret = virtio_i2c_setup_vqs(vi);
+ if (ret)
+ return ret;
+
+ vi->adap.owner = THIS_MODULE;
+ snprintf(vi->adap.name, sizeof(vi->adap.name),
+ "i2c_virtio at virtio bus %d", vdev->index);
+ vi->adap.algo = &virtio_algorithm;
+ vi->adap.dev.parent = &vdev->dev;
+ i2c_set_adapdata(&vi->adap, vi);
+
+ /*
+ * Setup ACPI node for controlled devices which will be probed through
+ * ACPI.
+ */
+ ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
+
+ ret = i2c_add_adapter(&vi->adap);
+ if (ret)
+ virtio_i2c_del_vqs(vdev);
+
+ return ret;
+}
+
+static void virtio_i2c_remove(struct virtio_device *vdev)
+{
+ struct virtio_i2c *vi = vdev->priv;
+
+ i2c_del_adapter(&vi->adap);
+ virtio_i2c_del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_I2C_ADAPTER, VIRTIO_DEV_ANY_ID },
+ {}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+#ifdef CONFIG_PM_SLEEP
+static int virtio_i2c_freeze(struct virtio_device *vdev)
+{
+ virtio_i2c_del_vqs(vdev);
+ return 0;
+}
+
+static int virtio_i2c_restore(struct virtio_device *vdev)
+{
+ return virtio_i2c_setup_vqs(vdev->priv);
+}
+#endif
+
+static struct virtio_driver virtio_i2c_driver = {
+ .id_table = id_table,
+ .probe = virtio_i2c_probe,
+ .remove = virtio_i2c_remove,
+ .driver = {
+ .name = "i2c_virtio",
+ },
+#ifdef CONFIG_PM_SLEEP
+ .freeze = virtio_i2c_freeze,
+ .restore = virtio_i2c_restore,
+#endif
+};
+module_virtio_driver(virtio_i2c_driver);
+
+MODULE_AUTHOR("Jie Deng <[email protected]>");
+MODULE_AUTHOR("Conghui Chen <[email protected]>");
+MODULE_DESCRIPTION("Virtio i2c bus driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
new file mode 100644
index 0000000..7c6a6fc
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
+/*
+ * Definitions for virtio I2C Adpter
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_I2C_H
+#define _UAPI_LINUX_VIRTIO_I2C_H
+
+#include <linux/const.h>
+#include <linux/types.h>
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT _BITUL(0)
+
+/**
+ * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
+ * @addr: the controlled device address
+ * @padding: used to pad to full dword
+ * @flags: used for feature extensibility
+ */
+struct virtio_i2c_out_hdr {
+ __le16 addr;
+ __le16 padding;
+ __le32 flags;
+};
+
+/**
+ * struct virtio_i2c_in_hdr - the virtio I2C message IN header
+ * @status: the processing result from the backend
+ */
+struct virtio_i2c_in_hdr {
+ __u8 status;
+};
+
+/* The final status written by the device */
+#define VIRTIO_I2C_MSG_OK 0
+#define VIRTIO_I2C_MSG_ERR 1
+
+#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 4fe842c..3b5f05a 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -55,6 +55,7 @@
#define VIRTIO_ID_FS 26 /* virtio filesystem */
#define VIRTIO_ID_PMEM 27 /* virtio pmem */
#define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
+#define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */
#define VIRTIO_ID_BT 40 /* virtio bluetooth */
#endif /* _LINUX_VIRTIO_IDS_H */
--
2.7.4
On 09-07-21, 10:25, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <[email protected]>
> Signed-off-by: Conghui Chen <[email protected]>
> Signed-off-by: Jie Deng <[email protected]>
> ---
> Changes v13 -> v14
> - Put the headers in virtio_i2c.h in alphabetical order.
> - Dropped I2C_FUNC_SMBUS_QUICK support.
> - Dropped few unnecessary variables and checks.
> - Use "num" everywhere instead of num or nr, to be consistent.
> - Added few comments which make the design more clear.
Thanks a lot for following this up so far :)
> +static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int num)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int outcnt = 0, incnt = 0;
> +
> + /*
> + * We don't support 0 length messages and so masked out
> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> + */
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> + * format, Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != num - 1)
> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = &msg_buf;
> + else
> + sgs[outcnt++] = &msg_buf;
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + break;
> + }
> + }
> +
> + return i;
> +}
Wolfram, in case you wonder why we don't error out early as discussed earlier,
then ...
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
...
> + /*
> + * For the case where count < num, i.e. we weren't able to queue all the
> + * msgs, ideally we should abort right away and return early, but some
> + * of the messages are already sent to the remote I2C controller and the
> + * virtqueue will be left in undefined state in that case. We kick the
> + * remote here to clear the virtqueue, so we can try another set of
> + * messages later on.
> + */
... here is the reasoning for that.
Please see if you can still get it merged into 5.14-rc1/2. Thanks.
Reviewed-by: Viresh Kumar <[email protected]>
Tested-by: Viresh Kumar <[email protected]>
--
viresh
On Fri, Jul 09, 2021 at 09:14:07AM +0530, Viresh Kumar wrote:
> On 09-07-21, 10:25, Jie Deng wrote:
> > Add an I2C bus driver for virtio para-virtualization.
> >
> > The controller can be emulated by the backend driver in
> > any device model software by following the virtio protocol.
> >
> > The device specification can be found on
> > https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
> >
> > By following the specification, people may implement different
> > backend drivers to emulate different controllers according to
> > their needs.
> >
> > Co-developed-by: Conghui Chen <[email protected]>
> > Signed-off-by: Conghui Chen <[email protected]>
> > Signed-off-by: Jie Deng <[email protected]>
> > ---
> > Changes v13 -> v14
> > - Put the headers in virtio_i2c.h in alphabetical order.
> > - Dropped I2C_FUNC_SMBUS_QUICK support.
> > - Dropped few unnecessary variables and checks.
> > - Use "num" everywhere instead of num or nr, to be consistent.
> > - Added few comments which make the design more clear.
>
> Thanks a lot for following this up so far :)
>
> > +static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> > + struct virtio_i2c_req *reqs,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> > + int i;
> > +
> > + for (i = 0; i < num; i++) {
> > + int outcnt = 0, incnt = 0;
> > +
> > + /*
> > + * We don't support 0 length messages and so masked out
> > + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> > + */
> > + if (!msgs[i].len)
> > + break;
> > +
> > + /*
> > + * Only 7-bit mode supported for this moment. For the address
> > + * format, Please check the Virtio I2C Specification.
> > + */
> > + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> > +
> > + if (i != num - 1)
> > + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> > +
> > + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> > + sgs[outcnt++] = &out_hdr;
> > +
> > + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> > + if (!reqs[i].buf)
> > + break;
> > +
> > + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> > +
> > + if (msgs[i].flags & I2C_M_RD)
> > + sgs[outcnt + incnt++] = &msg_buf;
> > + else
> > + sgs[outcnt++] = &msg_buf;
> > +
> > + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> > + sgs[outcnt + incnt++] = &in_hdr;
> > +
> > + if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
> > + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> > + break;
> > + }
> > + }
> > +
> > + return i;
> > +}
>
> Wolfram, in case you wonder why we don't error out early as discussed earlier,
> then ...
>
> > +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > + int num)
> > +{
>
> ...
>
> > + /*
> > + * For the case where count < num, i.e. we weren't able to queue all the
> > + * msgs, ideally we should abort right away and return early, but some
> > + * of the messages are already sent to the remote I2C controller and the
> > + * virtqueue will be left in undefined state in that case. We kick the
> > + * remote here to clear the virtqueue, so we can try another set of
> > + * messages later on.
> > + */
>
> ... here is the reasoning for that.
>
> Please see if you can still get it merged into 5.14-rc1/2. Thanks.
>
> Reviewed-by: Viresh Kumar <[email protected]>
> Tested-by: Viresh Kumar <[email protected]>
Well a new driver so maybe rc2 is still ok ...
Acked-by: Michael S. Tsirkin <[email protected]>
> --
> viresh
On Fri, Jul 09, 2021 at 10:25:30AM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <[email protected]>
> Signed-off-by: Conghui Chen <[email protected]>
> Signed-off-by: Jie Deng <[email protected]>
> ---
> Changes v13 -> v14
> - Put the headers in virtio_i2c.h in alphabetical order.
> - Dropped I2C_FUNC_SMBUS_QUICK support.
> - Dropped few unnecessary variables and checks.
> - Use "num" everywhere instead of num or nr, to be consistent.
> - Added few comments which make the design more clear.
>
> Changes v12 -> v13
> - Use _BITUL() instead of BIT().
> - Rename "virtio_i2c_send_reqs" to "virtio_i2c_prepare_reqs".
> - Optimize the return value of "virtio_i2c_complete_reqs".
>
> Changes v11 -> v12
> - Do not sent msg_buf for zero-length request.
> - Send requests to host only if all the number of transfers requested prepared successfully.
> - Remove the line #include <linux/bits.h> in virtio_i2c.h.
>
> Changes v10 -> v11
> - Remove vi->adap.class = I2C_CLASS_DEPRECATED.
> - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
> - Remove "struct mutex lock" in "struct virtio_i2c".
> - Support zero-length request.
> - Remove unnecessary logs.
> - Remove vi->adap.timeout = HZ / 10, just use the default value.
> - Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
> - Add the virtio_device index to adapter's naming mechanism.
>
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 285 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_i2c.h | 41 ++++++
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 341 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 10acece..e47616a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + select VIRTIO
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 69e9963..9843756 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..0139cdc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_i2c.h>
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
> + uint8_t *buf ____cacheline_aligned;
> + struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int num)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int outcnt = 0, incnt = 0;
> +
> + /*
> + * We don't support 0 length messages and so masked out
> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> + */
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> + * format, Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != num - 1)
> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = &msg_buf;
> + else
> + sgs[outcnt++] = &msg_buf;
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + break;
> + }
I think we should tweak this such that we add multiple buffers but
only make them visible to host after all add commands were successful.
With split this is possible by deffering avail idx update,
with packed by deferring update of the avail bit in the descriptor.
I'll write a patch to add an API like that to virtio, then we
can switch to that.
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int num,
> + bool timedout)
> +{
> + struct virtio_i2c_req *req;
> + bool failed = timedout;
> + unsigned int len;
> + int i, j = 0;
> +
> + for (i = 0; i < num; i++) {
> + /* Detach the ith request from the vq */
> + req = virtqueue_get_buf(vq, &len);
> +
> + /*
> + * Condition req == &reqs[i] should always meet since we have
> + * total num requests in the vq. reqs[i] can never be NULL here.
> + */
> + if (!failed && (WARN_ON(req != &reqs[i]) ||
> + req->in_hdr.status != VIRTIO_I2C_MSG_OK))
> + failed = true;
> +
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
> +
> + if (!failed)
> + j++;
> + }
> +
> + return timedout ? -ETIMEDOUT : j;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int count;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + count = virtio_i2c_prepare_reqs(vq, reqs, msgs, num);
> + if (!count)
> + goto err_free;
> +
> + /*
> + * For the case where count < num, i.e. we weren't able to queue all the
> + * msgs, ideally we should abort right away and return early, but some
> + * of the messages are already sent to the remote I2C controller and the
> + * virtqueue will be left in undefined state in that case. We kick the
> + * remote here to clear the virtqueue, so we can try another set of
> + * messages later on.
> + */
> +
> + reinit_completion(&vi->completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> + if (!time_left)
> + dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> +
> + count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left);
> +
> +err_free:
> + kfree(reqs);
> + return count;
> +}
> +
> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)
> +{
> + vdev->config->reset(vdev);
> + vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
> +{
> + struct virtio_device *vdev = vi->vdev;
> +
> + vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
> + return PTR_ERR_OR_ZERO(vi->vq);
> +}
> +
> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static struct i2c_algorithm virtio_algorithm = {
> + .master_xfer = virtio_i2c_xfer,
> + .functionality = virtio_i2c_func,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> + struct device *pdev = vdev->dev.parent;
> + struct virtio_i2c *vi;
> + int ret;
> +
> + vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
> + if (!vi)
> + return -ENOMEM;
> +
> + vdev->priv = vi;
> + vi->vdev = vdev;
> +
> + init_completion(&vi->completion);
> +
> + ret = virtio_i2c_setup_vqs(vi);
> + if (ret)
> + return ret;
> +
> + vi->adap.owner = THIS_MODULE;
> + snprintf(vi->adap.name, sizeof(vi->adap.name),
> + "i2c_virtio at virtio bus %d", vdev->index);
> + vi->adap.algo = &virtio_algorithm;
> + vi->adap.dev.parent = &vdev->dev;
> + i2c_set_adapdata(&vi->adap, vi);
> +
> + /*
> + * Setup ACPI node for controlled devices which will be probed through
> + * ACPI.
> + */
> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
> +
> + ret = i2c_add_adapter(&vi->adap);
> + if (ret)
> + virtio_i2c_del_vqs(vdev);
> +
> + return ret;
> +}
> +
> +static void virtio_i2c_remove(struct virtio_device *vdev)
> +{
> + struct virtio_i2c *vi = vdev->priv;
> +
> + i2c_del_adapter(&vi->adap);
> + virtio_i2c_del_vqs(vdev);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_I2C_ADAPTER, VIRTIO_DEV_ANY_ID },
> + {}
> +};
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> + virtio_i2c_del_vqs(vdev);
> + return 0;
> +}
> +
> +static int virtio_i2c_restore(struct virtio_device *vdev)
> +{
> + return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +#endif
> +
> +static struct virtio_driver virtio_i2c_driver = {
> + .id_table = id_table,
> + .probe = virtio_i2c_probe,
> + .remove = virtio_i2c_remove,
> + .driver = {
> + .name = "i2c_virtio",
> + },
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_i2c_freeze,
> + .restore = virtio_i2c_restore,
> +#endif
> +};
> +module_virtio_driver(virtio_i2c_driver);
> +
> +MODULE_AUTHOR("Jie Deng <[email protected]>");
> +MODULE_AUTHOR("Conghui Chen <[email protected]>");
> +MODULE_DESCRIPTION("Virtio i2c bus driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> new file mode 100644
> index 0000000..7c6a6fc
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include <linux/const.h>
> +#include <linux/types.h>
> +
> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT _BITUL(0)
> +
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> + __le16 addr;
> + __le16 padding;
> + __le32 flags;
> +};
> +
> +/**
> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_in_hdr {
> + __u8 status;
> +};
> +
> +/* The final status written by the device */
> +#define VIRTIO_I2C_MSG_OK 0
> +#define VIRTIO_I2C_MSG_ERR 1
> +
> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 4fe842c..3b5f05a 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -55,6 +55,7 @@
> #define VIRTIO_ID_FS 26 /* virtio filesystem */
> #define VIRTIO_ID_PMEM 27 /* virtio pmem */
> #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */
> #define VIRTIO_ID_BT 40 /* virtio bluetooth */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> --
> 2.7.4
On 13-07-21, 11:38, Michael S. Tsirkin wrote:
> I think we should tweak this such that we add multiple buffers but
> only make them visible to host after all add commands were successful.
> With split this is possible by deffering avail idx update,
> with packed by deferring update of the avail bit in the descriptor.
> I'll write a patch to add an API like that to virtio, then we
> can switch to that.
That will be great, exactly what we wanted but didn't ask for :)
--
viresh
On 2021/7/13 23:38, Michael S. Tsirkin wrote:
>
> I think we should tweak this such that we add multiple buffers but
> only make them visible to host after all add commands were successful.
> With split this is possible by deffering avail idx update,
> with packed by deferring update of the avail bit in the descriptor.
> I'll write a patch to add an API like that to virtio, then we
> can switch to that.
>
That's great ! Looking forward to seeing that API.
On 09-07-21, 10:25, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <[email protected]>
> Signed-off-by: Conghui Chen <[email protected]>
> Signed-off-by: Jie Deng <[email protected]>
> ---
> Changes v13 -> v14
> - Put the headers in virtio_i2c.h in alphabetical order.
> - Dropped I2C_FUNC_SMBUS_QUICK support.
> - Dropped few unnecessary variables and checks.
> - Use "num" everywhere instead of num or nr, to be consistent.
> - Added few comments which make the design more clear.
Wolfram,
Is it still possible to queue this for 5.14 ?
--
viresh
On Thu, Jul 22, 2021 at 10:44:33AM +0530, Viresh Kumar wrote:
> On 09-07-21, 10:25, Jie Deng wrote:
> > Add an I2C bus driver for virtio para-virtualization.
> >
> > The controller can be emulated by the backend driver in
> > any device model software by following the virtio protocol.
> >
> > The device specification can be found on
> > https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
> >
> > By following the specification, people may implement different
> > backend drivers to emulate different controllers according to
> > their needs.
> >
> > Co-developed-by: Conghui Chen <[email protected]>
> > Signed-off-by: Conghui Chen <[email protected]>
> > Signed-off-by: Jie Deng <[email protected]>
> > ---
> > Changes v13 -> v14
> > - Put the headers in virtio_i2c.h in alphabetical order.
> > - Dropped I2C_FUNC_SMBUS_QUICK support.
> > - Dropped few unnecessary variables and checks.
> > - Use "num" everywhere instead of num or nr, to be consistent.
> > - Added few comments which make the design more clear.
>
> Wolfram,
>
> Is it still possible to queue this for 5.14 ?
No new features are allowed for 5.14, you know this. It's but fixes
only now.
thanks,
greg k-h
On 22-07-21, 08:06, Greg KH wrote:
> No new features are allowed for 5.14, you know this. It's but fixes
> only now.
I was trying to be (overly) optimistic here since this was a fairly
independent driver which won't break anything else, and had been
pending on the list since many months now.
But yeah, I know the rule and understand its purpose :)
5.15 it is then.
--
viresh
Hi,
so only minor stuff left from my side.
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + select VIRTIO
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
The sorting is not good. I think your entry should go to the bottom of
the Kconfig file.
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 69e9963..9843756 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>
> +# VIRTIO I2C host controller driver
This comment can go, I'd say.
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> + /*
> + * We don't support 0 length messages and so masked out
> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> + */
> + if (!msgs[i].len)
> + break;
I recommend using struct i2c_adapter_quirks with I2C_AQ_NO_ZERO_LEN. But
let's wait first if zero length are possible or not.
Also, checkpatch:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
So, is one of you interested in maintaining this driver?
All the best,
Wolfram
On 2021/7/22 23:35, Wolfram Sang wrote:
> Hi,
>
> so only minor stuff left from my side.
>
>> @@ -21,6 +21,17 @@ config I2C_ALI1535
>> This driver can also be built as a module. If so, the module
>> will be called i2c-ali1535.
>>
>> +config I2C_VIRTIO
>> + tristate "Virtio I2C Adapter"
>> + select VIRTIO
>> + help
>> + If you say yes to this option, support will be included for the virtio
>> + I2C adapter driver. The hardware can be emulated by any device model
>> + software according to the virtio protocol.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called i2c-virtio.
>> +
>> config I2C_ALI1563
>> tristate "ALI 1563"
>> depends on PCI
> The sorting is not good. I think your entry should go to the bottom of
> the Kconfig file.
OK. I will move it to the bottom.
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 69e9963..9843756 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
>> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
>> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>>
>> +# VIRTIO I2C host controller driver
> This comment can go, I'd say.
I will remove this line. Thank you.
>
>> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
>> +
>> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
>
>> + /*
>> + * We don't support 0 length messages and so masked out
>> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
>> + */
>> + if (!msgs[i].len)
>> + break;
> I recommend using struct i2c_adapter_quirks with I2C_AQ_NO_ZERO_LEN. But
> let's wait first if zero length are possible or not.
I think we can add an i2c_adapter_quirks for this moment. Support for
I2C_FUNC_SMBUS_QUICK
can be added incrementally if needed.
>
> Also, checkpatch:
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>
> So, is one of you interested in maintaining this driver?
>
> All the best,
>
> Wolfram
I will play this role. I see Viresh also spend a lot of time on this driver.
So I'd like to ask Viresh, are you willing to be a co-maintainer ?
Regards,
Jie
On 23-07-21, 10:21, Jie Deng wrote:
> I think we can add an i2c_adapter_quirks for this moment. Support for
> I2C_FUNC_SMBUS_QUICK
> can be added incrementally if needed.
+1.
We just need to get this merged :)
> I will play this role. I see Viresh also spend a lot of time on this driver.
>
> So I'd like to ask Viresh, are you willing to be a co-maintainer ?
I will be happy to be listed as one. Really appreciate it Jie.
--
viresh
On Fri, Jul 09, 2021 at 10:25:30AM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <[email protected]>
> Signed-off-by: Conghui Chen <[email protected]>
> Signed-off-by: Jie Deng <[email protected]>
Same as with qemu bits, I am confused as to what is the status
of proposed spec changes and whether the driver will work
with them. Let's let the dust settle on them then, then
resubmit?
> ---
> Changes v13 -> v14
> - Put the headers in virtio_i2c.h in alphabetical order.
> - Dropped I2C_FUNC_SMBUS_QUICK support.
> - Dropped few unnecessary variables and checks.
> - Use "num" everywhere instead of num or nr, to be consistent.
> - Added few comments which make the design more clear.
>
> Changes v12 -> v13
> - Use _BITUL() instead of BIT().
> - Rename "virtio_i2c_send_reqs" to "virtio_i2c_prepare_reqs".
> - Optimize the return value of "virtio_i2c_complete_reqs".
>
> Changes v11 -> v12
> - Do not sent msg_buf for zero-length request.
> - Send requests to host only if all the number of transfers requested prepared successfully.
> - Remove the line #include <linux/bits.h> in virtio_i2c.h.
>
> Changes v10 -> v11
> - Remove vi->adap.class = I2C_CLASS_DEPRECATED.
> - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
> - Remove "struct mutex lock" in "struct virtio_i2c".
> - Support zero-length request.
> - Remove unnecessary logs.
> - Remove vi->adap.timeout = HZ / 10, just use the default value.
> - Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
> - Add the virtio_device index to adapter's naming mechanism.
>
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 285 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_i2c.h | 41 ++++++
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 341 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 10acece..e47616a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + select VIRTIO
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 69e9963..9843756 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..0139cdc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_i2c.h>
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
> + uint8_t *buf ____cacheline_aligned;
> + struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int num)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int outcnt = 0, incnt = 0;
> +
> + /*
> + * We don't support 0 length messages and so masked out
> + * I2C_FUNC_SMBUS_QUICK in virtio_i2c_func().
> + */
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> + * format, Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != num - 1)
> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = &msg_buf;
> + else
> + sgs[outcnt++] = &msg_buf;
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + if (virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL)) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + break;
> + }
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int num,
> + bool timedout)
> +{
> + struct virtio_i2c_req *req;
> + bool failed = timedout;
> + unsigned int len;
> + int i, j = 0;
> +
> + for (i = 0; i < num; i++) {
> + /* Detach the ith request from the vq */
> + req = virtqueue_get_buf(vq, &len);
> +
> + /*
> + * Condition req == &reqs[i] should always meet since we have
> + * total num requests in the vq. reqs[i] can never be NULL here.
> + */
> + if (!failed && (WARN_ON(req != &reqs[i]) ||
> + req->in_hdr.status != VIRTIO_I2C_MSG_OK))
> + failed = true;
> +
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
> +
> + if (!failed)
> + j++;
> + }
> +
> + return timedout ? -ETIMEDOUT : j;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int count;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + count = virtio_i2c_prepare_reqs(vq, reqs, msgs, num);
> + if (!count)
> + goto err_free;
> +
> + /*
> + * For the case where count < num, i.e. we weren't able to queue all the
> + * msgs, ideally we should abort right away and return early, but some
> + * of the messages are already sent to the remote I2C controller and the
> + * virtqueue will be left in undefined state in that case. We kick the
> + * remote here to clear the virtqueue, so we can try another set of
> + * messages later on.
> + */
> +
> + reinit_completion(&vi->completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> + if (!time_left)
> + dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> +
> + count = virtio_i2c_complete_reqs(vq, reqs, msgs, count, !time_left);
> +
> +err_free:
> + kfree(reqs);
> + return count;
> +}
> +
> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)
> +{
> + vdev->config->reset(vdev);
> + vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
> +{
> + struct virtio_device *vdev = vi->vdev;
> +
> + vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
> + return PTR_ERR_OR_ZERO(vi->vq);
> +}
> +
> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static struct i2c_algorithm virtio_algorithm = {
> + .master_xfer = virtio_i2c_xfer,
> + .functionality = virtio_i2c_func,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> + struct device *pdev = vdev->dev.parent;
> + struct virtio_i2c *vi;
> + int ret;
> +
> + vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
> + if (!vi)
> + return -ENOMEM;
> +
> + vdev->priv = vi;
> + vi->vdev = vdev;
> +
> + init_completion(&vi->completion);
> +
> + ret = virtio_i2c_setup_vqs(vi);
> + if (ret)
> + return ret;
> +
> + vi->adap.owner = THIS_MODULE;
> + snprintf(vi->adap.name, sizeof(vi->adap.name),
> + "i2c_virtio at virtio bus %d", vdev->index);
> + vi->adap.algo = &virtio_algorithm;
> + vi->adap.dev.parent = &vdev->dev;
> + i2c_set_adapdata(&vi->adap, vi);
> +
> + /*
> + * Setup ACPI node for controlled devices which will be probed through
> + * ACPI.
> + */
> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
> +
> + ret = i2c_add_adapter(&vi->adap);
> + if (ret)
> + virtio_i2c_del_vqs(vdev);
> +
> + return ret;
> +}
> +
> +static void virtio_i2c_remove(struct virtio_device *vdev)
> +{
> + struct virtio_i2c *vi = vdev->priv;
> +
> + i2c_del_adapter(&vi->adap);
> + virtio_i2c_del_vqs(vdev);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_I2C_ADAPTER, VIRTIO_DEV_ANY_ID },
> + {}
> +};
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> + virtio_i2c_del_vqs(vdev);
> + return 0;
> +}
> +
> +static int virtio_i2c_restore(struct virtio_device *vdev)
> +{
> + return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +#endif
> +
> +static struct virtio_driver virtio_i2c_driver = {
> + .id_table = id_table,
> + .probe = virtio_i2c_probe,
> + .remove = virtio_i2c_remove,
> + .driver = {
> + .name = "i2c_virtio",
> + },
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_i2c_freeze,
> + .restore = virtio_i2c_restore,
> +#endif
> +};
> +module_virtio_driver(virtio_i2c_driver);
> +
> +MODULE_AUTHOR("Jie Deng <[email protected]>");
> +MODULE_AUTHOR("Conghui Chen <[email protected]>");
> +MODULE_DESCRIPTION("Virtio i2c bus driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> new file mode 100644
> index 0000000..7c6a6fc
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include <linux/const.h>
> +#include <linux/types.h>
> +
> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT _BITUL(0)
> +
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> + __le16 addr;
> + __le16 padding;
> + __le32 flags;
> +};
> +
> +/**
> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_in_hdr {
> + __u8 status;
> +};
> +
> +/* The final status written by the device */
> +#define VIRTIO_I2C_MSG_OK 0
> +#define VIRTIO_I2C_MSG_ERR 1
> +
> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 4fe842c..3b5f05a 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -55,6 +55,7 @@
> #define VIRTIO_ID_FS 26 /* virtio filesystem */
> #define VIRTIO_ID_PMEM 27 /* virtio pmem */
> #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */
> #define VIRTIO_ID_BT 40 /* virtio bluetooth */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> --
> 2.7.4
On 04-09-21, 16:01, Michael S. Tsirkin wrote:
> Same as with qemu bits, I am confused as to what is the status
> of proposed spec changes and whether the driver will work
> with them.
This is already merged as well.
The current version simply fails to transmit the messages in case the
length of a buffer is 0. I have patch ready to make it work with the
proposed spec changes, just waiting for them to be accepted.
> Let's let the dust settle on them then, then
> resubmit?
It doesn't break anything for now since we don't have much users and
we know zero length transfers don't work. I will submit the necessary
changes once spec is finalized.
--
viresh
> The current version simply fails to transmit the messages in case the
> length of a buffer is 0. I have patch ready to make it work with the
> proposed spec changes, just waiting for them to be accepted.
For the record, it fails gracefully. The driver has the proper quirk
flag set, so the I2C core knows it doesn't zupport 0 length messages.
On 2021/9/6 12:43, Viresh Kumar wrote:
>
>> Let's let the dust settle on them then, then
>> resubmit?
> It doesn't break anything for now since we don't have much users and
> we know zero length transfers don't work. I will submit the necessary
> changes once spec is finalized.
Agree. The currently merged spec and driver are consistent.
We can support zero length request once the following spec change is
accepted.
[PATCH V3] virtio: i2c: Allow zero-length transactions
(https://lists.oasis-open.org/archives/virtio-dev/202109/msg00004.html)
So, can we start a ballot for this new feature ?