linux-kernel - Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5dedcd26-ed59-415f-b978-87a546a0816d@opensynergy.com>
Date: Tue, 19 Mar 2024 16:05:55 +0100
From: Harald Mommer <harald.mommer@...nsynergy.com>
To: Haixu Cui <quic_haixcui@...cinc.com>, Mark Brown <broonie@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>, linux-spi@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: quic_ztu@...cinc.com, Matti Moell <Matti.Moell@...nsynergy.com>,
 Mikhail Golubev <Mikhail.Golubev@...nsynergy.com>
Subject: Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.

Hello,

virtio-dev still down...

I have to admit when it comes to device trees it's most of the time a 
fight. You will see below, fighting.

On 18.03.24 10:39, Haixu Cui wrote:
> Hello,
>
>     As Mark recommended, I reserve the virtio_spi_probe function only 
> and list my comments.
>
> On 3/4/2024 11:43 PM, Harald Mommer wrote:
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +    struct device_node *np = vdev->dev.parent->of_node;
>> +    struct virtio_spi_priv *priv;
>> +    struct spi_controller *ctrl;
>> +    int err;
>> +    u32 bus_num;
>> +    u16 csi;
>> +
>> +    ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
>> +    if (!ctrl)
>> +        return -ENOMEM;
>> +
>> +    priv = spi_controller_get_devdata(ctrl);
>> +    priv->vdev = vdev;
>> +    vdev->priv = priv;
>> +    dev_set_drvdata(&vdev->dev, ctrl);
>
>     ctrl->dev.of_node is not set, so the child node cannot be parsed. 
> I would say, it's necessary to support the virtio spi device node in 
> the format:


What you most probably want to have here is

   ctrl->dev.of_node = np;

for reasons I don't understand completely yet but have some idea. I may 
play around to get it.


>
>     virtio-spi@...13000 {
>         compatible = "virtio,mmio";
>         reg = <0x4b013000 0x1000>;
>         interrupts = <0 497 4>;
>
>         spi {
>             compatible = "virtio,device2d";
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             spidev {
>                 compatible = "xxx";
>                 reg = <0>;
>                 spi-max-frequency = <100000>;
>             };
>         };
>     };


Looks in my setup like this:

virtio_mmio@...13000 {
   compatible = "virtio,mmio";
   reg = <0x0 0x4b013000 0x0 0x1000>;
   /* GIC_SPI = 0, IRQ_TYPE_LEVEL_HIGH = 4 */
   interrupts = <0 497 4>;
   spi,bus-num = <1234>; /* <<<=== This here has been added */
};

The "spi,bus-num" is missing in your setup so you use the default of 0.

For what is "reg = <0>;" good? Or is it a dummy and only has to be present?

I don't understand spi-max-frequency in the device tree entry as we get 
this value from the config space.


>> +
>> +    init_completion(&priv->spi_req.completion);
>> +
>> +    err = of_property_read_u32(np, "spi,bus-num", &bus_num);


np already set so this assignment should play no role here when reading 
the entry.


>> +    if (!err && bus_num <= S16_MAX)
>> +        ctrl->bus_num = (s16)bus_num;
>> +
>> +    virtio_spi_read_config(vdev);
>> +
>> +    ctrl->transfer_one = virtio_spi_transfer_one;
>> +
>> +    err = virtio_spi_find_vqs(priv);
>> +    if (err) {
>> +        dev_err(&vdev->dev, "Cannot setup virtqueues\n");
>> +        return err;
>> +    }
>> +
>> +    err = spi_register_controller(ctrl);
>> +    if (err) {
>> +        dev_err(&vdev->dev, "Cannot register controller\n");
>> +        goto err_return;
>> +    }
>> +
>> +    board_info.max_speed_hz = priv->max_freq_hz;
>> +    /* spi_new_device() currently does not use bus_num but better 
>> set it */
>> +    board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> +    /* Add chip selects to controller */
>
>         Creating the spi devices here statically, will introduce 
> conflicts if the same spi devices also in the device-tree.


Of course I need to create the SPI devices here statically. The 
"spi,bus-num = <1234>;" in the device tree has to be defined in a way 
that there are no conflicts. I do not understand this.


>
>> +    for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> +        dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> +        board_info.chip_select = csi;
>> +
>> +        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;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +err_return:
>> +    vdev->config->del_vqs(vdev);
>> +    return err;
>> +}
>> +
>

What I needed to support for our needs was a user space SPI interface. 
You probably have a different setup and additional more complex device 
tree configuration needs I do not understand (yet). I'm somewhat lost in 
the moment.