Re: [virtio-dev] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).
+ 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")
[virtio-dev] Updated schedule for OASIS mail list system upgrade
We Recently announced the migration to a new List Mail system for this list. We've modified the implementation process to allow the legacy system to continue running a bit longer and to have a shorter downtime during transition. *The system cut-over is now scheduled for the first week of January * Miss the original announcement? Here it is... The legacy list engine in use since 2003 will be replaced in this upgrade. You are receiving this message because you are a subscriber to the list receiving this message. This transition will provide significant improvements and will change how you interact with your mail lists. See below for more information about the changes, the transition schedule and what you can do to help. Changes to Expect The new platform is built in a modern web environment and mail delivery mechanism. with a number of new features to improve both your platform user experience and ability to engage with the OASIS community. Key features include: - Mail systems that you can depend on! The new tools support and comply with major deliverability and anti-spoofing standards. This will eliminate many of the sending and deliverability issues your group may be experiencing - Modern web interface and design for your committee internal work and public presentation of your work - If you are on this public list in addition to Technical Committees, Open Projects, and other community engagement mail lists you can manage this experience in one cohesive package - All archives created in the legacy email system will be migrated and available cohesively with your new work in the new platform. All URL links for assets in the old platform will redirect to the new location - After the cutover starts, you will be directed where to send future messages for this list and where to find the archives. Any message sent to a legacy list address after the cutover begins will receive an auto reply with the new list address and archive location. - After the migration you can continue using the mail list as before. If you wish to join additional public lists or modify your delivery to the new digest delivery, you will be required to create or login to an existing OASIS public community or member account to manage your subscriptions. This account system will be used to confirm your email address once (as opposed to each subscription) and will be the same account you may be using now for OASIS membership, submitting e-CLA forms etc. Legacy-style subscribe and unsubscribe email messages to the server will not be respected in the new system. The Transition Schedule During the cut-over the first week of January, There will be some system downtime during the cutover. During the downtime you will not receive email messages sent to this list or have access to the archive. All messages sent after the cut-over begins will receive an auto reply announcing the new list address and archive location. We will announce the timing specific as we get closer to the transition. What You Should Do Now If you no longer wish to subscribe to this list, send a message to [listname]-unsubscr...@lists.oasis-open.org Otherwise, watch for further messages in the coming days to include the new list mailing address and archive location. Feel free to contact me with any questions, and thanks for your patience with the inevitable short-term inconvenience implementing these improvements will bring. -- Scott McGrath Chief Operating Officer OASIS Open +1 781-929-7308 Mobile scott.mcgr...@oasis-open.org www.oasis-open.org
Re: [virtio-dev] [RFC PATCH v1 2/3] virtio-spi: Add virtio-spi.h (V4 draft specification).
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-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).
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
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
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* 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 = 12500, /* 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
[virtio-dev] Re: [virtio-comment] Changes coming to this mail list system
Yes, the cut-over to the new system is now expected the first week of January. We've delayed the process, in part, because we've found ways to shorten the downtime for the mail lists and are working on some of those implementation steps now in the background while the legacy systems continue to serve. Of course, closing in on the holidays os an issue as well factoring in the delay. Two other notes: I'll push the announcement to all other lists (non-member lists) this morning. The migration plan changes will better ensure messages sent during the downtime will not be lost and the downtime will be less than we originally expected. Hope that helps, Scott... On Thu, Dec 14, 2023 at 6:18 AM Cornelia Huck wrote: > On Tue, Dec 05 2023, Scott McGrath wrote: > > [dropped non-virtio lists, added the other virtio lists] > > > The Transition Schedule > > > > Starting 14 December, there may be as much as several days of system > > downtime. During the downtime you will not receive email messages sent to > > this list or have access to the archive. > > > > We will announce the timing specific as we get closer to the transition. > > I've received off-list notification that this change is now postponed to > January -- can you please confirm? It is important for us to plan how we > are going to proceed with the 1.3 version of the virtio spec. (And it > is of course also important for people reading those lists, who may not > have an account in the system.) > > -- Scott McGrath Chief Operating Officer OASIS Open +1 781-929-7308 Mobile scott.mcgr...@oasis-open.org www.oasis-open.org
[virtio-dev] Re: [virtio-comment] Changes coming to this mail list system
On Tue, Dec 05 2023, Scott McGrath wrote: [dropped non-virtio lists, added the other virtio lists] > The Transition Schedule > > Starting 14 December, there may be as much as several days of system > downtime. During the downtime you will not receive email messages sent to > this list or have access to the archive. > > We will announce the timing specific as we get closer to the transition. I've received off-list notification that this change is now postponed to January -- can you please confirm? It is important for us to plan how we are going to proceed with the 1.3 version of the virtio spec. (And it is of course also important for people reading those lists, who may not have an account in the system.) - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
