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

[Arnd Bergmann]

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.

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";

for a virtio-mmio device of device-id 34 (i2c), or a PCI device with

compatible="pci1af4,1041", "virtio,device34";

which also does not quite feel right.

>> 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:
> >
> > > BTW, what's the usecase for these protocols? A standard interface to
> > > firmware controlled I2C, GPIO, etc.?
> >
> > Right now we are looking to control devices in the host machine from
> > guests. That's what Linaro's project stratos is doing. There are other
> > people who want to use this for other kind of remote control stuff,
> > maybe from firmware.
>
> Project stratos means nothing to me.
>
> Direct userspace access to I2C, GPIO, etc. has its issues, we're going
> to repeat that with guests?

Passing through the i2c or gpio access from a Linux host is just one
way to use it, you could do the same with an emulated i2c device
from qemu, and you could have a fake i2c device behind a virtio-i2c
controller.

         Arnd