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

On Tue, Jul 13, 2021 at 9:35 PM Rob Herring [off-list ref] wrote:

On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar [off-list ref] wrote:

quoted

On 13-07-21, 08:43, Rob Herring wrote:

quoted

On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar [off-list ref] wrote:

On 13-07-21, 08:43, Rob Herring wrote:

quoted

On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar [off-list ref] wrote:

quoted

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