Received-SPF: pass (google.com: domain of linux-kernel+bounces-98618-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <919477ee-929e-45d3-85fc-82cac071a0ec@quicinc.com>
Date: Mon, 11 Mar 2024 17:28:42 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [virtio-dev] Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver. -
 Correction
To: Harald Mommer <harald.mommer@opensynergy.com>
CC: <quic_ztu@quicinc.com>, Matti Moell <Matti.Moell@opensynergy.com>,
        "Mikhail Golubev" <Mikhail.Golubev@opensynergy.com>,
        <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
        Viresh Kumar
	<viresh.kumar@linaro.org>,
        "Mark Brown" <broonie@kernel.org>
References: <20240213135350.5878-1-Harald.Mommer@opensynergy.com>
 <20240213135350.5878-4-Harald.Mommer@opensynergy.com>
 <e2e5a7a0-7a2b-40d3-acf7-6f0b91bd5d40@quicinc.com>
 <2a911f96-eed9-4adc-9043-27fd1d3a7ca2@opensynergy.com>
 <c7c939c9-50e0-4d86-bd44-d8b4b90ca14d@quicinc.com>
 <7bfbf09f-67d1-43d0-8ecd-aedf3b9e2287@opensynergy.com>
 <58d1487a-fd3b-4b31-a133-3181359e0c61@opensynergy.com>
 <a8e5463f-787b-44c3-842f-c2d7f703d26e@quicinc.com>
 <b5e3ed19-15a2-4dfd-a1af-28d11ebefbcc@opensynergy.com>
Content-Language: en-US
From: Haixu Cui <quic_haixcui@quicinc.com>
In-Reply-To: <b5e3ed19-15a2-4dfd-a1af-28d11ebefbcc@opensynergy.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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@lists.debian.org/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
>>>>
>>>>
>>