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

From: Harald Mommer
Date: Tue Mar 19 2024 - 11:06:16 EST


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@4b013000 {
        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@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>; /* <<<=== 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.