Re: [PATCH 1/5] dt-bindings: virtio: mmio: Add support for device subnode

[Rob Herring]

On Tue, Jul 13, 2021 at 2:34 PM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
>
> On Tue, Jul 13, 2021 at 9:35 PM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> > On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > >
> > > On 13-07-21, 08:43, Rob Herring wrote:
> > > > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > > >
> > > > > Allow virtio,mmio nodes to contain device specific subnodes. Since each
> > > > > virtio,mmio node can represent a single virtio device, each virtio node
> > > > > is allowed to contain a maximum of one device specific subnode.
> > > >
> > > > Doesn't sound like we need 2 nodes here. Just add I2C devices as child
> > > > nodes. You could add a more specific compatible string, but the
> > > > protocol is discoverable, so that shouldn't be necessary.
> > >
> > > I am not sure if it will be a problem, but you can clarify it better.
> >
> > > The parent node (virtio,mmio) is used to create a platform device,
> > > virtio-mmio, (and so assigned as its of_node) and we create the
> > > virtio-device from probe() of this virtio-mmio device.
> > >
> > > Is it going to be a problem if two devices in kernel use the same
> > > of_node ?
> >
> > There shouldn't be. We have nodes be multiple providers (e.g clocks
> > and resets) already.
>
> I think this would be a little different, but it can still work. There is in
> fact already some precedent of doing this, with Jean-Philippe's virtio-iommu
> binding, which is documented in both
>
> Documentation/devicetree/bindings/virtio/iommu.txt
> Documentation/devicetree/bindings/virtio/mmio.txt
>
> Unfortunately, those are still slightly different from where I think we should
> be going here, but it's probably close enough to fit into the general
> system.
>
> What we have with virtio-iommu is two special hacks:
>  - on virtio-mmio, a node with 'compatible="virtio,mmio"' may optionally
>    have an '#iommu-cells=<1>', in which case we assume it's an iommu.
>  - for virtio-pci, the node has the standard PCI 'reg' property but a special
>    'compatible="virtio,pci-iommu"' property that I think is different from any
>    other PCI node.

How is that different? PCI device can be a VID/PID compatible or
omitted, but can also be a "typical" compatible string.

> I think for other virtio devices, we should come up with a way to define a
> binding per device (i2c, gpio, ...) without needing to cram this into the
> "virtio,mmio" binding or coming up with special compatible strings for
> PCI devices.
>
> Having a child device for the virtio device type gives a better separation
> here, since it lets you have two nodes with 'compatible' strings that each
> make sense for their respective parent buses: The parent is either a PCI
> device or a plain mmio based device, and the child is a virtio device with
> its own namespace for compatible values. As you say, the downside is
> that this requires an extra node that is redundant because there is always
> a 1:1 relation with its parent.
>
> Having a combined node gets rid of the redundancy but if we want to
> identify the device for the purpose of defining a custom binding, it would have
> to have two compatible strings, something like
>
> compatible="virtio,mmio", "virtio,device34";

The order seems backwards here. 'virtio,device34' is more specific.
Though I guess the meanings are orthogonal.

> for a virtio-mmio device of device-id 34 (i2c), or a PCI device with
>
> compatible="pci1af4,1041", "virtio,device34";

But this seems the right order. Though does '1041' have any specific
meaning or device IDs are just dynamically assigned? It seems to be
the latter from my brief scan of the code.

> which also does not quite feel right.

I guess it comes down to is 'virtio,mmio' providing a bus or is it
just a device? I guess a bus (so 2 nodes) does make sense here.
'virtio,mmio' defines how you access/discover the virtio queues (the
bus) and the functional device (i2c, gpio, iommu, etc.) is accessed
via the virtio queues.

Rob