Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. - Correction

Haixu Cui <quic_haixcui@xxxxxxxxxxx> · Wed, 13 Mar 2024 15:05:44 +0800

Hello Harald,

    Some updates:

        1. the virtio-spi device node intermediate layer, "spi", the
           compatible is not "virtio,device45", should be
           "virtio,device2d", shall be in lower case hexadecimal.

        2. the driver should support the way that creating the spi
           device via device-tree, but now the driver doesn't, it cannot
           parse the child node. I guess this is due to the missing of
           of_node property, please refer to the i2c virtio driver.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-virtio.c?h=v6.8#n216

        3. in current driver, spi_new_device to create spi devices, as

+        for (csi = 0; csi < ctrl->num_chipselect; csi++) {
+		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
+		board_info.chip_select = csi;
+		/* TODO: Discuss setting of board_info.mode */
+		if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
+			board_info.mode = SPI_MODE_0;
+		else
+			board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
+		if (!spi_new_device(ctrl, &board_info)) {
+			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
+			spi_unregister_controller(ctrl);
+			err = -ENODEV;
+			goto err_return;
+		}
+	}

          but when I add spi device node in device tree, there will be
          conflicts here, between the device dynamically created via
          device tree node and the device static created by calling
          spi_new_device. As far as I am concerned, it's necessary to
          support of and(or) acpi.

        Let's discuss this topic firstly. Thanks a lot.

BR
Haixu Cui

On 3/11/2024 5:28 PM, Haixu Cui wrote:
Hello Harald,
     My concern is if possible to create the udev(spidev) by adding the 
device-tree node. Although the way of using the udev rule is fine, I 
think the way of adding device-tree node also suitable for some scenarios.

     Referring to Kumar's examples, I guess the virtio spi device-tree 
should be like:

     virtio-spi@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x4b013000 0x1000>;
         interrupts = <0 497 4>;

         spi {
             compatible = "virtio,device45";
             #address-cells = <1>;
             #size-cells = <0>;

             spidev@0 {
                 compatible = "xxx";
                 reg = <0>;
                 spi-max-frequency = <100000>;
             };
         };
     };

     Just like virtio-i2c node, virtio-spi@4b013000 has three levels. 
And the innermost, spidev node is to match spidev driver, to create 
spidev(udev) device. I am working on this recently, but got some 
stranger cases. Need more effort and time.

     Harald, do you have any idea about this way? I'm looking forward to 
it. Thanks a lot.

Haixu Cui

On 3/7/2024 12:18 AM, Harald Mommer wrote:
Hello Haixu,

not really sure what you mean and where the problem are. But we will 
find out.

What I did in the device tree of some hardware board was

virtio_mmio@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x0 0x4b013000 0x0 0x1000>;
         /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
         interrupts = <0 497 4>;
         spi,bus-num = <1234>;
     };

Simply added a line "spi,bus-num = <1234>;" in the device tree to 
configure the bus number.

(There is no device tree for my very small qemu setup to check 
next/latest, also no full udev, therefore I've to live there with the 
default bus-num which is 0.)

What I guess you mean is that the syntax of the device tree node 
should be changed having GPIO and I2C as template.

And as you need more parameters for the board info, not only this 
single line for the bus number. May this be somewhat for an 
enhancement in a subsequent version?

Why it's not easy to create the device node using the udev rule below 
in a full system with udev (vs. some minimal RAMDISK system) I don't 
understand. It's a single line, rest are comments.

# Bind spi-virtio device spiB.C to user mode SPI device /dev/spidevB.C
# Requires spi driver_override sysfs entry (Linux version 4.20+ and 
later)
#
# See also 
https://www.mail-archive.com/debian-arm@xxxxxxxxxxxxxxxx/msg22090.html
# and Documentation/spi/spidev.rst
#
#ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S%p/subsystem/drivers/spidev/bind"
ACTION=="add", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:spi-virtio", 
PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > 
%S/bus/spi/drivers/spidev/bind"

It may be helpful if you could provide a proposal how exactly the 
device tree entry should look. This would also show which information 
is needed in addition to the bus number.

What I guess is that you in the end it may look like this:

virtio_mmio@4b013000 {
         compatible = "virtio,mmio";
         reg = <0x0 0x4b013000 0x0 0x1000>;
         /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
         interrupts = <0 497 4>;

         spi {
             bus-num = <1234>; /* Is it like this? */
             /* More stuff here especially for the board_info I've 
currently no need for and no idea about that it may be needed by 
others */ /* ??? More info needed */
         }
     };

Regards
Harald Mommer

On 06.03.24 08:48, Haixu Cui wrote:
Hello Harald,

    In current driver, spi_new_device is used to instantiate the 
virtio SPI device, spidevX.Y is created manually, this way seems not 
flexible enough. Besides it's not easy to set the spi_board_info 
properly.

    Viresh Kumar has standardized the device tree node format for 
virtio-i2c and virtio-gpio:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml

    In this way, the driver is unified, board customization only 
depends on the device-tree node. It's easy to bring up spidev 
automatically.

    Look forward to your opinions. Thanks a lot.

Haixu Cui

On 3/6/2024 1:54 AM, Harald Mommer wrote:
Hello,

looked again at my tinny setup and I've to add a small correction.

It's not the way that I've no udev at all there. What is in place 
there is busybox mdev.

Relevant part of /etc/init.d/rcS:

#!/bin/sh
mount -t proc none /proc
mount -t sysfs none /sys
depmod
modprobe spi-virtio
mdev -s
mdev -d

If I kill the "mdev -d" process my small script below does not make 
the /dev/spidev0.0 device node appear any more. Of course not, there 
must be some user mode process which does the job in the device 
directory.

Regards
Harald Mommer

On 05.03.24 11:57, Harald Mommer wrote:
Hello,

I took next/stable as base giving the exact tag/sha of the current 
next/stable so that it's known what was used as base version even 
when next/stable moves. The ordinary next tags are currently not of 
best quality, gets better, therefore next/stable now. We were on 
v6.8-rc7 yesterday with next/stable.

VMM is qemu for the driver you have. But it's a specially modified 
qemu which allows that we use our proprietary virtio SPI device as 
backend.

Proprietary virtio SPI device is started first, this is an own user 
process in our architecture. Subsequently the special internal qemu 
version is started. The virtio SPI driver is compiled as a module 
and inserted manually by a startup script by "modprobe spi-virtio". 
The driver goes live immediately.

In this simple setup I do not have udev rules (no service 
supporting udev => no rules) so no /dev/spidevX.Y automatically 
after the driver went live. What I'm using to test the latest 
driver before sending it to the mailing lists is really a naked 
kernel + a busybox running in a ramdisk. The udev rule I've sent 
are used on some more complete setup on real hardware.

So without udev I have to bring this device up manually:

In /etc/spidev-up.sh there is a script tp bring up /dev/spidev0.0 
manually:

#!/bin/sh
SPIDEV=spi0.0
echo spidev > /sys/bus/spi/devices/$SPIDEV/driver_override
echo $SPIDEV > /sys/bus/spi/drivers/spidev/bind

Afterwards there is /dev/spidev0.0.

In linux/tools/spi there are spidev_test.c and spidev_fdx.c. Those 
(somewhat hacked locally, and I mean "hacked" to be able to test 
somewhat more) are used to play around with /dev/spidev0.0.

I can do this on my Laptop which has no underlying SPI hardware 
which could be used as a backend for the virtio SPI device. The 
proprietary virtio SPI device has a test mode to support this. 
Using this test mode the driver does not communicate with a real 
backend SPI device but does an internal simulation. For example, if 
I do a half duplex read it always gives back the sequence 01 02 03 ...

For full duplex it gives back what has been read but with letter 
case changed, in loopback mode it gives back exactly what was sent. 
With this test mode I could develop a driver and parts of the 
device (device - real backend communication to an actual SPI 
device) on a board which had no user space /dev/spiX.Y available 
which could have served as backend for the virtio SPI device on the 
host.

Slightly different module version is tested on real hardware with 
the virtio SPI device not in test mode. "Tested on hardware" means 
that device + module work for our special use case (some hardware 
device using 8 bit word size) and the project team for which device 
and driver have been made did until now not complain.

Regards
Harald Mommer

On 05.03.24 08:46, Haixu Cui wrote:
Hello Harald,

Thank you for your detailed expatiation. To my knowledge, you took 
Vanilla as the front-end, and VMM is QEMU. Can you please explain 
further how do you test the SPI transfer without the Vanilla 
userspace interface? Thanks again.

Haixu Cui