| Message ID | aa4bf68fdd13b885a6dc1b98f88834916d51d97d.1626173013.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Parse virtio-device nodes from DT | expand |
On Tue, Jul 13, 2021 at 12:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > + > + virtio@3200 { > + compatible = "virtio,mmio"; > + reg = <0x3200 0x100>; > + interrupts = <43>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + i2c-virtio@0 { > + reg = <VIRTIO_ID_I2C_ADAPTER>; > + }; > + }; This works, but it seems oddly inconsistent with the way we do the same thing for PCI, USB and MMC devices that normally don't need device tree properties but can optionally have those. All of the above use the "compatible" property to identify the device, rather than using the "reg" property. Neither of them is actually great here, since we already know what the device is and how to talk to it, but I'd still prefer doing this with compatible = "virtio,34"; or similar, where 34 is the numerical value of VIRTIO_ID_I2C_ADAPTER. This would then be required in the virtio-i2c binding. I think you can skip the #address-cells/#size-cells then. Arnd
On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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. BTW, what's the usecase for these protocols? A standard interface to firmware controlled I2C, GPIO, etc.? > The device subnode must have the "reg" property, and its value must > match the virtio device ID used by the virtio mmio node. > > A phandle to this device subnode can then be used by the users of the > virtio device. > > Also add a symbolic link to uapi/linux/virtio_ids.h in order to use the > definitions here. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > .../devicetree/bindings/virtio/mmio.yaml | 41 +++++++++++++++++++ > include/dt-bindings/virtio/virtio_ids.h | 1 + > 2 files changed, 42 insertions(+) > create mode 120000 include/dt-bindings/virtio/virtio_ids.h > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml b/Documentation/devicetree/bindings/virtio/mmio.yaml > index d46597028cf1..e5f9fe6ecb5e 100644 > --- a/Documentation/devicetree/bindings/virtio/mmio.yaml > +++ b/Documentation/devicetree/bindings/virtio/mmio.yaml > @@ -31,6 +31,31 @@ title: virtio memory mapped devices > description: Required for devices making accesses thru an IOMMU. > maxItems: 1 > > + "#address-cells": > + const: 1 > + description: > + The cell is the device ID if a device subnode is used. > + > + "#size-cells": > + const: 0 > + > +patternProperties: > + '^[a-z0-9]+-virtio@[0-9]+$': > + type: object > + description: | > + Exactly one node describing the virtio device. The name of the node isn't > + significant but its phandle can be used to by an user of the virtio > + device. > + > + properties: > + reg: > + description: > + Must contain the Virtio ID of the device. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + required: > + - reg > + > required: > - compatible > - reg > @@ -57,4 +82,20 @@ additionalProperties: false > #iommu-cells = <1>; > }; > > + - | > + #include <dt-bindings/virtio/virtio_ids.h> > + > + virtio@3200 { > + compatible = "virtio,mmio"; > + reg = <0x3200 0x100>; > + interrupts = <43>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + i2c-virtio@0 { > + reg = <VIRTIO_ID_I2C_ADAPTER>; > + }; > + }; > + > ... > diff --git a/include/dt-bindings/virtio/virtio_ids.h b/include/dt-bindings/virtio/virtio_ids.h > new file mode 120000 > index 000000000000..6e59ba332216 > --- /dev/null > +++ b/include/dt-bindings/virtio/virtio_ids.h > @@ -0,0 +1 @@ > +../../uapi/linux/virtio_ids.h This will break the devicetree-rebasing tree I think. DT files shouldn't reference kernel files. Rob
On 13-07-21, 08:43, Rob Herring wrote: > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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 ? Are there cases where we would need to get the device pointer from the of_node ? Then we will have two here. > 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. > > diff --git a/include/dt-bindings/virtio/virtio_ids.h b/include/dt-bindings/virtio/virtio_ids.h > > new file mode 120000 > > index 000000000000..6e59ba332216 > > --- /dev/null > > +++ b/include/dt-bindings/virtio/virtio_ids.h > > @@ -0,0 +1 @@ > > +../../uapi/linux/virtio_ids.h > > This will break the devicetree-rebasing tree I think. DT files > shouldn't reference kernel files. We already do this for linux-event-codes.h and so I thought it is the right way of doing it :) Else we can create a new copy, which will be a mess, or use hardcoded values. -- viresh
On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 13-07-21, 08:43, Rob Herring wrote: > > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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. > Are there cases where we would need to get the device > pointer from the of_node ? Then we will have two here. Rarely... In any case, should these potential kernel issues be dictating the DT binding design? No! > > > 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? > > > diff --git a/include/dt-bindings/virtio/virtio_ids.h b/include/dt-bindings/virtio/virtio_ids.h > > > new file mode 120000 > > > index 000000000000..6e59ba332216 > > > --- /dev/null > > > +++ b/include/dt-bindings/virtio/virtio_ids.h > > > @@ -0,0 +1 @@ > > > +../../uapi/linux/virtio_ids.h > > > > This will break the devicetree-rebasing tree I think. DT files > > shouldn't reference kernel files. > > We already do this for linux-event-codes.h and so I thought it is the > right way of doing it :) Humm, maybe it's okay. Please double check then... > Else we can create a new copy, which will be a mess, or use hardcoded > values. Though you may not need the header based on what Arnd and I have suggested. Rob
On Tue, Jul 13, 2021 at 9:35 PM Rob Herring <robh+dt@kernel.org> wrote: > On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 13-07-21, 08:43, Rob Herring wrote: > > > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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@linaro.org> wrote: > > > > On 13-07-21, 08:43, Rob Herring wrote: > > > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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
On 13-07-21, 13:34, Rob Herring wrote: > On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > We already do this for linux-event-codes.h and so I thought it is the > > right way of doing it :) > > Humm, maybe it's okay. Please double check then... > > > Else we can create a new copy, which will be a mess, or use hardcoded > > values. > > Though you may not need the header based on what Arnd and I have suggested. Yeah, we may not need it at all. New node or not, reg property will get converted to a compatible anyway.. Thanks. -- viresh
On 13-07-21, 22:34, Arnd Bergmann wrote: > On Tue, Jul 13, 2021 at 9:35 PM Rob Herring <robh+dt@kernel.org> wrote: > > 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. I agree that even if the device is discoverable at runtime, we should still have some sort of stuff in DT to distinguish the devices, and "virtio,deviceDID" sounds good enough for that, considering that we already do it for USB, etc. And I am fine with both the ways, a new node or just using the parent node. So whatever you guys decide is fine. > > 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. Or it can be firmware controlled device as well, as Rob said earlier. I think that's what Vincent will be doing for SCMI. Though all I have tested until now is direct userspace access. -- viresh
On 13-07-21, 14:32, Arnd Bergmann wrote: > On Tue, Jul 13, 2021 at 12:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > + > > + virtio@3200 { > > + compatible = "virtio,mmio"; > > + reg = <0x3200 0x100>; > > + interrupts = <43>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + i2c-virtio@0 { > > + reg = <VIRTIO_ID_I2C_ADAPTER>; > > + }; > > + }; > > This works, but it seems oddly inconsistent with the way we do the same thing > for PCI, USB and MMC devices that normally don't need device tree properties but > can optionally have those. > > All of the above use the "compatible" property to identify the device, > rather than > using the "reg" property. Neither of them is actually great here, > since we already > know what the device is and how to talk to it, but I'd still prefer doing this > with > > compatible = "virtio,34"; > > or similar, where 34 is the numerical value of VIRTIO_ID_I2C_ADAPTER. > This would then be required in the virtio-i2c binding. > I think you can skip the #address-cells/#size-cells then. That works, sure. I think I misunderstood it when you said it earlier and thought that you are asking to add compatible in the parent node itself and so did it this way. Though that may be the way we will end up doing it now :) -- viresh
On Tue, Jul 13, 2021 at 10:34:03PM +0200, Arnd Bergmann wrote: > > > 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. Yes in retrospect I don't think the compatible property on the PCI endpoint node is necessary nor useful, we could deprecate it. The OS gets the IOMMU topology information early from 'iommus', 'iommu-map' and '#iommu-cells' properties, which is the only reason we need this PCI endpoint explicitly described in DT. The rest is discovered while probing just like virtio-mmio. Thanks, Jean
On 14-07-21, 07:56, Viresh Kumar wrote: > I agree that even if the device is discoverable at runtime, we should > still have some sort of stuff in DT to distinguish the devices, and > "virtio,deviceDID" sounds good enough for that, considering that we > already do it for USB, etc. > > And I am fine with both the ways, a new node or just using the parent > node. So whatever you guys decide is fine. I tried to write and see what it would look like after using the existing nodes for mmio/pci and here is what I got. (I couldn't find any virtio-pci bindings and so stayed away from adding any reference to it here). Does that look better ? -- viresh -------------------------8<------------------------- diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml b/Documentation/devicetree/bindings/virtio/mmio.yaml index d46597028cf1..324b810e51a5 100644 --- a/Documentation/devicetree/bindings/virtio/mmio.yaml +++ b/Documentation/devicetree/bindings/virtio/mmio.yaml @@ -15,7 +15,8 @@ title: virtio memory mapped devices properties: compatible: - const: virtio,mmio + contains: + const: virtio,mmio reg: maxItems: 1 @@ -36,7 +37,7 @@ title: virtio memory mapped devices - reg - interrupts -additionalProperties: false +additionalProperties: true examples: - | diff --git a/Documentation/devicetree/bindings/virtio/virtio-device.yaml b/Documentation/devicetree/bindings/virtio/virtio-device.yaml new file mode 100644 index 000000000000..9cfe090ea65f --- /dev/null +++ b/Documentation/devicetree/bindings/virtio/virtio-device.yaml @@ -0,0 +1,49 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/virtio/virtio-device.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Virtio device bindings + +maintainers: + - Viresh Kumar <viresh.kumar@linaro.org> + +description: + These bindings are applicable to virtio devices irrespective of the bus they + are bound to, like mmio or pci. + +allOf: + - $ref: /schemas/virtio/mmio.yaml# + +# We need a select here so we don't match all nodes with 'virtio,mmio' +select: + properties: + compatible: + contains: + pattern: '^virtio,[0-9]+$' + required: + - compatible + +properties: + compatible: + contains: + oneOf: + - items: + - const: virtio,mmio + - pattern: '^virtio,[0-9]+$' + +required: + - compatible + +additionalProperties: true + +examples: + - | + i2c: i2c-virtio@3000 { + compatible = "virtio,mmio", "virtio,34"; + reg = <0x3000 0x100>; + interrupts = <41>; + }; + +... diff --git a/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml b/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml new file mode 100644 index 000000000000..8115ba794557 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpio/gpio-virtio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Virtio GPIO controller + +maintainers: + - Viresh Kumar <viresh.kumar@linaro.org> + +allOf: + - $ref: /schemas/gpio/gpio.yaml# + - $ref: /schemas/virtio/virtio-device.yaml# + +# We need a select here so we don't match all nodes with 'virtio,mmio' +select: + properties: + compatible: + contains: + enum: + - virtio,41 + required: + - compatible + +properties: + compatible: + oneOf: + - items: + - const: virtio,mmio + - const: virtio,41 + + gpio-controller: true + + "#gpio-cells": + const: 2 + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + +required: + - compatible + - gpio-controller + - "#gpio-cells" + +unevaluatedProperties: false + +examples: + - | + gpio: gpio-virtio@3000 { + compatible = "virtio,mmio", "virtio,41"; + reg = <0x3000 0x100>; + interrupts = <41>; + + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + }; + +... diff --git a/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml b/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml new file mode 100644 index 000000000000..43e9910920d6 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml @@ -0,0 +1,54 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/i2c-virtio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Virtio I2C Adapter + +maintainers: + - Viresh Kumar <viresh.kumar@linaro.org> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + - $ref: /schemas/virtio/virtio-device.yaml# + +# We need a select here so we don't match all nodes with 'virtio,mmio' +select: + properties: + compatible: + contains: + enum: + - virtio,34 + required: + - compatible + +properties: + compatible: + oneOf: + - items: + - const: virtio,mmio + - const: virtio,34 + +required: + - compatible + +unevaluatedProperties: false + +examples: + - | + i2c: i2c-virtio@3000 { + compatible = "virtio,mmio", "virtio,34"; + reg = <0x3000 0x100>; + interrupts = <41>; + + #address-cells = <1>; + #size-cells = <0>; + + light-sensor@1c { + compatible = "dynaimage,al3320a"; + reg = <0x20>; + }; + }; + +...
On Tue, Jul 13, 2021 at 2:34 PM Arnd Bergmann <arnd@kernel.org> wrote: > > On Tue, Jul 13, 2021 at 9:35 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Jul 13, 2021 at 9:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 13-07-21, 08:43, Rob Herring wrote: > > > > On Tue, Jul 13, 2021 at 4:50 AM Viresh Kumar <viresh.kumar@linaro.org> 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
On Wed, Jul 14, 2021 at 5:43 PM Rob Herring <robh+dt@kernel.org> wrote: > On Tue, Jul 13, 2021 at 2:34 PM Arnd Bergmann <arnd@kernel.org> wrote: > > On Tue, Jul 13, 2021 at 9:35 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > 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. Ok, my mistake then, I though the VID/PID compatible was mandated > > 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. Right, one defines the transport and the other defines the device behind the transport. > > 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. It's the assigned PCI vendor/device pair for virtio, all virtio-pci devices have to be "pci1af4,1041", just like all virtio-mmio devices are "virtio,mmio". > > 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. It's not really a bus since there is only ever one device behind it. A better analogy would be your 'serdev' framework: You could have a 8250 or a pl011 uart, and behind that have a mouse, GPS receiver or bluetooth dongle. In Documentation/devicetree/bindings/serial/serial.yaml, you also have two nodes for a single device, so we could follow that example. Arnd
On 14-07-21, 23:07, Arnd Bergmann wrote: > On Wed, Jul 14, 2021 at 5:43 PM Rob Herring <robh+dt@kernel.org> wrote: > > 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. > > It's not really a bus since there is only ever one device behind it. > A better analogy would be your 'serdev' framework: You could > have a 8250 or a pl011 uart, and behind that have a mouse, GPS > receiver or bluetooth dongle. > > In Documentation/devicetree/bindings/serial/serial.yaml, you also > have two nodes for a single device, so we could follow that > example. So two device nodes is final then ? Pretty much like how this patchset did it already ? I need to get rid of reg thing and use "virtio,DID" though. -- viresh
On Mon, Jul 19, 2021 at 12:34 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 14-07-21, 23:07, Arnd Bergmann wrote: > > On Wed, Jul 14, 2021 at 5:43 PM Rob Herring <robh+dt@kernel.org> wrote: > > > 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. > > > > It's not really a bus since there is only ever one device behind it. > > A better analogy would be your 'serdev' framework: You could > > have a 8250 or a pl011 uart, and behind that have a mouse, GPS > > receiver or bluetooth dongle. > > > > In Documentation/devicetree/bindings/serial/serial.yaml, you also > > have two nodes for a single device, so we could follow that > > example. > > So two device nodes is final then ? Pretty much like how this patchset did it > already ? I need to get rid of reg thing and use "virtio,DID" though. Let's give Rob another day to reply here. I think two nodes is easier to get working than one node, even when we continue supporting the current iommu binding that relies on a single node, but we could make it work either way if necessary. Arnd
diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml b/Documentation/devicetree/bindings/virtio/mmio.yaml index d46597028cf1..e5f9fe6ecb5e 100644 --- a/Documentation/devicetree/bindings/virtio/mmio.yaml +++ b/Documentation/devicetree/bindings/virtio/mmio.yaml @@ -31,6 +31,31 @@ title: virtio memory mapped devices description: Required for devices making accesses thru an IOMMU. maxItems: 1 + "#address-cells": + const: 1 + description: + The cell is the device ID if a device subnode is used. + + "#size-cells": + const: 0 + +patternProperties: + '^[a-z0-9]+-virtio@[0-9]+$': + type: object + description: | + Exactly one node describing the virtio device. The name of the node isn't + significant but its phandle can be used to by an user of the virtio + device. + + properties: + reg: + description: + Must contain the Virtio ID of the device. + $ref: /schemas/types.yaml#/definitions/uint32 + + required: + - reg + required: - compatible - reg @@ -57,4 +82,20 @@ additionalProperties: false #iommu-cells = <1>; }; + - | + #include <dt-bindings/virtio/virtio_ids.h> + + virtio@3200 { + compatible = "virtio,mmio"; + reg = <0x3200 0x100>; + interrupts = <43>; + + #address-cells = <1>; + #size-cells = <0>; + + i2c-virtio@0 { + reg = <VIRTIO_ID_I2C_ADAPTER>; + }; + }; + ... diff --git a/include/dt-bindings/virtio/virtio_ids.h b/include/dt-bindings/virtio/virtio_ids.h new file mode 120000 index 000000000000..6e59ba332216 --- /dev/null +++ b/include/dt-bindings/virtio/virtio_ids.h @@ -0,0 +1 @@ +../../uapi/linux/virtio_ids.h \ No newline at end of file
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. The device subnode must have the "reg" property, and its value must match the virtio device ID used by the virtio mmio node. A phandle to this device subnode can then be used by the users of the virtio device. Also add a symbolic link to uapi/linux/virtio_ids.h in order to use the definitions here. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- .../devicetree/bindings/virtio/mmio.yaml | 41 +++++++++++++++++++ include/dt-bindings/virtio/virtio_ids.h | 1 + 2 files changed, 42 insertions(+) create mode 120000 include/dt-bindings/virtio/virtio_ids.h -- 2.31.1.272.g89b43f80a514