[03/11] dt-bindings: Add RISC-V IOMMU bindings

Commit Message

Tomasz Jeznach July 19, 2023, 7:33 p.m. UTC

From: Anup Patel <[e-mail]>

We add DT bindings document for RISC-V IOMMU platform and PCI devices
defined by the RISC-V IOMMU specification.

Signed-off-by: Anup Patel <[e-mail]>
---
 .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml

Comments

Conor Dooley July 19, 2023, 8:19 p.m. UTC | #1

Hey Tomasz,

On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote:
> From: Anup Patel <[e-mail]>
> 
> We add DT bindings document for RISC-V IOMMU platform and PCI devices
> defined by the RISC-V IOMMU specification.
> 
> Signed-off-by: Anup Patel <[e-mail]>

Your signoff is missing from here.

Secondly, as get_maintainer.pl would have told you, dt-bindings patches
need to be sent to the dt-binding maintainers and list.
+CC maintainers & list.

Thirdly, dt-binding patches should come before their users.

> ---
>  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> new file mode 100644
> index 000000000000..8a9aedb61768
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> @@ -0,0 +1,146 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V IOMMU Implementation
> +
> +maintainers:
> +  - Tomasz Jeznach <[e-mail]>

What about Anup, who seems to have written this?
Or your co-authors of the drivers?

> +
> +description:
> +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> +  which can be a regular platform device or a PCI device connected to
> +  the host root port.
> +
> +  The RISC-V IOMMU provides two stage translation, device directory table,
> +  command queue and fault reporting as wired interrupt or MSIx event for
> +  both PCI and platform devices.
> +
> +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: RISC-V IOMMU as a platform device
> +        items:
> +          - enum:
> +              - vendor,chip-iommu

These dummy compatibles are not valid, as was pointed out to Anup on
the AIA series. Please go look at what was done there instead:
https://lore.kernel.org/all/[e-mail]/

> +          - const: riscv,iommu
> +
> +      - description: RISC-V IOMMU as a PCI device connected to root port
> +        items:
> +          - enum:
> +              - vendor,chip-pci-iommu
> +          - const: riscv,pci-iommu

I'm not really au fait with the arm smmu stuff, but do any of its
versions support being connected to a root port? 

> +  reg:
> +    maxItems: 1
> +    description:
> +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> +      address of registers.
> +
> +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> +
> +  '#iommu-cells':
> +    const: 2
> +    description: |

|s are only needed where formatting needs to be preserved.

> +      Each IOMMU specifier represents the base device ID and number of
> +      device IDs.
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 16

What are any of these interrupts?

> +    description:
> +      The presence of this property implies that given RISC-V IOMMU uses
> +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> +
> +  msi-parent:
> +    description:
> +      The presence of this property implies that given RISC-V IOMMU uses
> +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> +      considered only when the interrupts property is absent.
> +
> +  dma-coherent:

RISC-V is dma-coherent by default, should this not be dma-noncoherent
instead?

> +    description:
> +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> +      are cache coherent with the CPU.
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#iommu-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* Example 1 (IOMMU platform device with wired interrupts) */
> +    immu1: iommu@1bccd000 {

Why is this "immu"? typo or intentional?

> +        compatible = "vendor,chip-iommu", "riscv,iommu";
> +        reg = <0x1bccd000 0x1000>;
> +        interrupt-parent = <&aplic_smode>;
> +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> +        #iommu-cells = <2>;
> +    };
> +
> +    /* Device with two IOMMU device IDs, 0 and 7 */
> +    master1 {
> +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> +    };
> +
> +  - |
> +    /* Example 2 (IOMMU platform device with MSIs) */
> +    immu2: iommu@1bcdd000 {
> +        compatible = "vendor,chip-iommu", "riscv,iommu";
> +        reg = <0x1bccd000 0x1000>;
> +        msi-parent = <&imsics_smode>;
> +        #iommu-cells = <2>;
> +    };
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> +        master1 {
> +                iommus = <&immu2 32 32>;
> +        };
> +
> +        pcie@40000000 {
> +            compatible = "pci-host-cam-generic";
> +            device_type = "pci";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            bus-range = <0x0 0x1>;
> +
> +            /* CPU_PHYSICAL(2)  SIZE(2) */

These sort of comments seem to just repeat what address-cells &
size-cells has already said, no?

Thanks,
Conor.

Conor Dooley July 19, 2023, 8:57 p.m. UTC | #2

On Wed, Jul 19, 2023 at 01:52:28PM -0700, Tomasz Jeznach wrote:
> On Wed, Jul 19, 2023 at 1:19 PM Conor Dooley <[e-mail]> wrote:
> 
> > Hey Tomasz,
> >
> > On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote:
> > > From: Anup Patel <[e-mail]>
> > >
> > > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > > defined by the RISC-V IOMMU specification.
> > >
> > > Signed-off-by: Anup Patel <[e-mail]>
> >
> > Your signoff is missing from here.
> >
> > Secondly, as get_maintainer.pl would have told you, dt-bindings patches
> > need to be sent to the dt-binding maintainers and list.
> > +CC maintainers & list.
> >
> > Thirdly, dt-binding patches should come before their users.
> >
> 
> 
> Thank you for pointing out and adding DT maintainers.
> The signoff is definitely missing, and I'll will amend with other fixes /
> reordering.

Yeah, please wait until you get actual feedback on the drivers etc
though before you do that.

Also, don't send html emails to the mailing lists. They will be rejected
and those outside of direct-cc will not see the emails.

> > > ---
> > >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..8a9aedb61768
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > @@ -0,0 +1,146 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V IOMMU Implementation
> > > +
> > > +maintainers:
> > > +  - Tomasz Jeznach <[e-mail]>
> >
> > What about Anup, who seems to have written this?
> > Or your co-authors of the drivers?
> >
> >
> Anup provided only device tree riscv,iommu bindings proposal, but handed
> over its maintenance.
> 
> > +
> > > +description:
> > > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> > > +  which can be a regular platform device or a PCI device connected to
> > > +  the host root port.
> > > +
> > > +  The RISC-V IOMMU provides two stage translation, device directory
> > table,
> > > +  command queue and fault reporting as wired interrupt or MSIx event for
> > > +  both PCI and platform devices.
> > > +
> > > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - description: RISC-V IOMMU as a platform device
> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-iommu
> >
> > These dummy compatibles are not valid, as was pointed out to Anup on
> > the AIA series. Please go look at what was done there instead:
> >
> > https://lore.kernel.org/all/[e-mail]/
> >
> >
> Thank you, good pointer, seams like the same comments apply here. Will go
> through the discussion and update.
> 
> 
> > > +          - const: riscv,iommu
> > > +
> > > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-pci-iommu
> > > +          - const: riscv,pci-iommu
> >
> > I'm not really au fait with the arm smmu stuff, but do any of its
> > versions support being connected to a root port?
> >
> >
> RISC-V IOMMU allows them to be connected to the root port, or presented as
> a platform device.

That is not quite what I asked... What I want to know is why we are
doing something different to Arm's SMMU stuff & whether it is because
RISC-V has extra capabilities, or the binding itself is flawed.

(There's no more comments from me below, just making sure the mail's
contents reaches lore)

Cheers,
Conor.

> > > +  reg:
> > > +    maxItems: 1
> > > +    description:
> > > +      For RISC-V IOMMU as a platform device, this represents the MMIO
> > base
> > > +      address of registers.
> > > +
> > > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI
> > bridge
> > > +      details as described in
> > Documentation/devicetree/bindings/pci/pci.txt
> > > +
> > > +  '#iommu-cells':
> > > +    const: 2
> > > +    description: |
> >
> > |s are only needed where formatting needs to be preserved.
> >
> > > +      Each IOMMU specifier represents the base device ID and number of
> > > +      device IDs.
> > > +
> > > +  interrupts:
> > > +    minItems: 1
> > > +    maxItems: 16
> >
> > What are any of these interrupts?
> >
> >
> I'll add a description to the file. In short queue interfaces signalling to
> the driver.
> 
> 
> > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > > +
> > > +  msi-parent:
> > > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > > +      considered only when the interrupts property is absent.
> > > +
> > > +  dma-coherent:
> >
> > RISC-V is dma-coherent by default, should this not be dma-noncoherent
> > instead?
> >
> >
> Very valid comment. I'm ok to reverse the flag unless anyone objects.
> 
> 
> > > +    description:
> > > +      Present if page table walks and DMA accessed made by the RISC-V
> > IOMMU
> > > +      are cache coherent with the CPU.
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - '#iommu-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > > +    immu1: iommu@1bccd000 {
> >
> > Why is this "immu"? typo or intentional?
> >
> 
> I guess there was no particular naming schema here, but I might defer this
> question to the author.
> 
> 
> >
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        interrupt-parent = <&aplic_smode>;
> > > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > > +    master1 {
> > > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > > +    };
> > > +
> > > +  - |
> > > +    /* Example 2 (IOMMU platform device with MSIs) */
> > > +    immu2: iommu@1bcdd000 {
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        msi-parent = <&imsics_smode>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    bus {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > > +        master1 {
> > > +                iommus = <&immu2 32 32>;
> > > +        };
> > > +
> > > +        pcie@40000000 {
> > > +            compatible = "pci-host-cam-generic";
> > > +            device_type = "pci";
> > > +            #address-cells = <3>;
> > > +            #size-cells = <2>;
> > > +            bus-range = <0x0 0x1>;
> > > +
> > > +            /* CPU_PHYSICAL(2)  SIZE(2) */
> >
> > These sort of comments seem to just repeat what address-cells &
> > size-cells has already said, no?
> >
> >
> Correct.
> 
> 
> 
> > Thanks,
> > Conor.
> >
> 
> 
> Thank you Conor for prompt response and comments.
> I'll address them in the next version.
> 
> - Tomasz

Rob Herring July 19, 2023, 9:37 p.m. UTC | #3

On Wed, Jul 19, 2023 at 2:19 PM Conor Dooley <[e-mail]> wrote:
>
> Hey Tomasz,
>
> On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote:
> > From: Anup Patel <[e-mail]>
> >
> > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > defined by the RISC-V IOMMU specification.
> >
> > Signed-off-by: Anup Patel <[e-mail]>
>
> Your signoff is missing from here.
>
> Secondly, as get_maintainer.pl would have told you, dt-bindings patches
> need to be sent to the dt-binding maintainers and list.
> +CC maintainers & list.
>
> Thirdly, dt-binding patches should come before their users.
>
> > ---
> >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > new file mode 100644
> > index 000000000000..8a9aedb61768
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > @@ -0,0 +1,146 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V IOMMU Implementation
> > +
> > +maintainers:
> > +  - Tomasz Jeznach <[e-mail]>
>
> What about Anup, who seems to have written this?
> Or your co-authors of the drivers?
>
> > +
> > +description:
> > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms

typo

> > +  which can be a regular platform device or a PCI device connected to
> > +  the host root port.
> > +
> > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > +  command queue and fault reporting as wired interrupt or MSIx event for
> > +  both PCI and platform devices.

TBC, you want a PCI device that's an IOMMU and the IOMMU serves
(provides translation for) PCI devices?

> > +
> > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description: RISC-V IOMMU as a platform device

"platform device" is a Linux term. Don't use Linux terms in bindings.

> > +        items:
> > +          - enum:
> > +              - vendor,chip-iommu
>
> These dummy compatibles are not valid, as was pointed out to Anup on
> the AIA series. Please go look at what was done there instead:
> https://lore.kernel.org/all/[e-mail]/
>
> > +          - const: riscv,iommu
> > +
> > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > +        items:
> > +          - enum:
> > +              - vendor,chip-pci-iommu
> > +          - const: riscv,pci-iommu
>
> I'm not really au fait with the arm smmu stuff, but do any of its
> versions support being connected to a root port?

PCI devices have a defined format for the compatible string based on
VID/PID. For PCI, also usually don't need to be described in DT
because they are discoverable. The exception is when there's parts
which aren't. Which parts aren't?

> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > +      address of registers.
> > +
> > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge

Your IOMMU is also a PCI-PCI bridge? Is that a normal PCI thing?


> > +      details as described in Documentation/devicetree/bindings/pci/pci.txt

Don't refer to pci.txt. It is going to be removed.

> > +
> > +  '#iommu-cells':
> > +    const: 2
> > +    description: |
>
> |s are only needed where formatting needs to be preserved.
>
> > +      Each IOMMU specifier represents the base device ID and number of
> > +      device IDs.

Doesn't that assume device IDs are contiguous? Generally not a safe assumption.

> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 16
>
> What are any of these interrupts?
>
> > +    description:
> > +      The presence of this property implies that given RISC-V IOMMU uses
> > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > +
> > +  msi-parent:
> > +    description:
> > +      The presence of this property implies that given RISC-V IOMMU uses
> > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > +      considered only when the interrupts property is absent.

This doesn't make sense for a PCI device. PCI defines its own way to
describe MSI support.

> > +
> > +  dma-coherent:
>
> RISC-V is dma-coherent by default, should this not be dma-noncoherent
> instead?
>
> > +    description:
> > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > +      are cache coherent with the CPU.
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#iommu-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > +    immu1: iommu@1bccd000 {
>
> Why is this "immu"? typo or intentional?
>
> > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > +        reg = <0x1bccd000 0x1000>;
> > +        interrupt-parent = <&aplic_smode>;
> > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > +        #iommu-cells = <2>;
> > +    };
> > +
> > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > +    master1 {
> > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > +    };
> > +
> > +  - |
> > +    /* Example 2 (IOMMU platform device with MSIs) */
> > +    immu2: iommu@1bcdd000 {
> > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > +        reg = <0x1bccd000 0x1000>;
> > +        msi-parent = <&imsics_smode>;
> > +        #iommu-cells = <2>;
> > +    };
> > +
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > +        master1 {
> > +                iommus = <&immu2 32 32>;
> > +        };
> > +
> > +        pcie@40000000 {
> > +            compatible = "pci-host-cam-generic";
> > +            device_type = "pci";
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            bus-range = <0x0 0x1>;
> > +
> > +            /* CPU_PHYSICAL(2)  SIZE(2) */

I'm guessing there was more after this, but I don't have it...

Guessing, immu2 is a PCI device, but it translates for master1 which
is not a PCI device? Weird. Why would anyone build such a thing?


Rob

Tomasz Jeznach July 19, 2023, 11:04 p.m. UTC | #4

On Wed, Jul 19, 2023 at 2:37 PM Rob Herring <[e-mail]> wrote:
>
> On Wed, Jul 19, 2023 at 2:19 PM Conor Dooley <[e-mail]> wrote:
> >
> > Hey Tomasz,
> >
> > On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote:
> > > From: Anup Patel <[e-mail]>
> > >
> > > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > > defined by the RISC-V IOMMU specification.
> > >
> > > Signed-off-by: Anup Patel <[e-mail]>
> >
> > Your signoff is missing from here.
> >
> > Secondly, as get_maintainer.pl would have told you, dt-bindings patches
> > need to be sent to the dt-binding maintainers and list.
> > +CC maintainers & list.
> >
> > Thirdly, dt-binding patches should come before their users.
> >
> > > ---
> > >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..8a9aedb61768
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > @@ -0,0 +1,146 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V IOMMU Implementation
> > > +
> > > +maintainers:
> > > +  - Tomasz Jeznach <[e-mail]>
> >
> > What about Anup, who seems to have written this?
> > Or your co-authors of the drivers?
> >
> > > +
> > > +description:
> > > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
>
> typo
>

ack

> > > +  which can be a regular platform device or a PCI device connected to
> > > +  the host root port.
> > > +
> > > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > > +  command queue and fault reporting as wired interrupt or MSIx event for
> > > +  both PCI and platform devices.
>
> TBC, you want a PCI device that's an IOMMU and the IOMMU serves
> (provides translation for) PCI devices?
>

Yes, IOMMU as a PCIe device providing address translation services for
connect PCIe root complex.

> > > +
> > > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - description: RISC-V IOMMU as a platform device
>
> "platform device" is a Linux term. Don't use Linux terms in bindings.
>

ack.


> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-iommu
> >
> > These dummy compatibles are not valid, as was pointed out to Anup on
> > the AIA series. Please go look at what was done there instead:
> > https://lore.kernel.org/all/[e-mail]/
> >
> > > +          - const: riscv,iommu
> > > +
> > > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-pci-iommu
> > > +          - const: riscv,pci-iommu
> >
> > I'm not really au fait with the arm smmu stuff, but do any of its
> > versions support being connected to a root port?
>
> PCI devices have a defined format for the compatible string based on
> VID/PID. For PCI, also usually don't need to be described in DT
> because they are discoverable. The exception is when there's parts
> which aren't. Which parts aren't?
>

We've put 'riscv,pci-iommu' node here to describe relationship between PCIe
devices and IOMMU(s), needed for the pcie root complex description (iommu-map).
If there is a better way to reference PCI-IOMMU without adding
pci-iommu definition
that would solve the problem. Every other property of pci-iommu should
be discoverable.

> > > +  reg:
> > > +    maxItems: 1
> > > +    description:
> > > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > > +      address of registers.
> > > +
> > > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
>
> Your IOMMU is also a PCI-PCI bridge? Is that a normal PCI thing?
>

It's allowed to be integrated with root complex / IO bridge, but it is
as a separate PCIe device.
I'll clarify the description.

>
> > > +      details as described in Documentation/devicetree/bindings/pci/pci.txt
>
> Don't refer to pci.txt. It is going to be removed.
>

ack.

> > > +
> > > +  '#iommu-cells':
> > > +    const: 2
> > > +    description: |
> >
> > |s are only needed where formatting needs to be preserved.
> >
> > > +      Each IOMMU specifier represents the base device ID and number of
> > > +      device IDs.
>
> Doesn't that assume device IDs are contiguous? Generally not a safe assumption.
>

ack.

> > > +
> > > +  interrupts:
> > > +    minItems: 1
> > > +    maxItems: 16
> >
> > What are any of these interrupts?
> >
> > > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > > +
> > > +  msi-parent:
> > > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > > +      considered only when the interrupts property is absent.
>
> This doesn't make sense for a PCI device. PCI defines its own way to
> describe MSI support.
>

Agree, this is for IOMMU as a non-PCI device, capable of sending MSI.
Follows 'MSI clients' notes from
devicetree/bindings/interrupt-controller/msi.txt
Is this a proper way to describe this relationship?

> > > +
> > > +  dma-coherent:
> >
> > RISC-V is dma-coherent by default, should this not be dma-noncoherent
> > instead?
> >
> > > +    description:
> > > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > > +      are cache coherent with the CPU.
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - '#iommu-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > > +    immu1: iommu@1bccd000 {
> >
> > Why is this "immu"? typo or intentional?
> >
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        interrupt-parent = <&aplic_smode>;
> > > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > > +    master1 {
> > > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > > +    };
> > > +
> > > +  - |
> > > +    /* Example 2 (IOMMU platform device with MSIs) */
> > > +    immu2: iommu@1bcdd000 {
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        msi-parent = <&imsics_smode>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    bus {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > > +        master1 {
> > > +                iommus = <&immu2 32 32>;
> > > +        };
> > > +
> > > +        pcie@40000000 {
> > > +            compatible = "pci-host-cam-generic";
> > > +            device_type = "pci";
> > > +            #address-cells = <3>;
> > > +            #size-cells = <2>;
> > > +            bus-range = <0x0 0x1>;
> > > +
> > > +            /* CPU_PHYSICAL(2)  SIZE(2) */
>
> I'm guessing there was more after this, but I don't have it...

Complete patch 3 is at:
https://lore.kernel.org/linux-iommu/[e-mail]/T/#mbf8dc4098fb09b87b2618c5c545ae882f11b114b

>
> Guessing, immu2 is a PCI device, but it translates for master1 which
> is not a PCI device? Weird. Why would anyone build such a thing?
>

In this example immu2 is a non-PCI device. Agree, otherwise would be weird.

>
> Rob

Thank you,
- Tomasz

Zong Li July 24, 2023, 8:03 a.m. UTC | #5

On Thu, Jul 20, 2023 at 3:35 AM Tomasz Jeznach <[e-mail]> wrote:
>
> From: Anup Patel <[e-mail]>
>
> We add DT bindings document for RISC-V IOMMU platform and PCI devices
> defined by the RISC-V IOMMU specification.
>
> Signed-off-by: Anup Patel <[e-mail]>
> ---
>  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
>
> diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> new file mode 100644
> index 000000000000..8a9aedb61768
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> @@ -0,0 +1,146 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V IOMMU Implementation
> +
> +maintainers:
> +  - Tomasz Jeznach <[e-mail]>
> +
> +description:
> +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> +  which can be a regular platform device or a PCI device connected to
> +  the host root port.
> +
> +  The RISC-V IOMMU provides two stage translation, device directory table,
> +  command queue and fault reporting as wired interrupt or MSIx event for
> +  both PCI and platform devices.
> +
> +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: RISC-V IOMMU as a platform device
> +        items:
> +          - enum:
> +              - vendor,chip-iommu
> +          - const: riscv,iommu
> +
> +      - description: RISC-V IOMMU as a PCI device connected to root port
> +        items:
> +          - enum:
> +              - vendor,chip-pci-iommu
> +          - const: riscv,pci-iommu
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> +      address of registers.
> +
> +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> +
> +  '#iommu-cells':
> +    const: 2
> +    description: |
> +      Each IOMMU specifier represents the base device ID and number of
> +      device IDs.
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 16
> +    description:
> +      The presence of this property implies that given RISC-V IOMMU uses
> +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> +
> +  msi-parent:
> +    description:
> +      The presence of this property implies that given RISC-V IOMMU uses
> +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> +      considered only when the interrupts property is absent.
> +
> +  dma-coherent:
> +    description:
> +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> +      are cache coherent with the CPU.
> +
> +  power-domains:
> +    maxItems: 1
> +

In RISC-V IOMMU, certain devices can be set to bypass mode when the
IOMMU is in translation mode. To identify the devices that require
bypass mode by default, does it be sensible to add a property to
indicate this behavior?

> +required:
> +  - compatible
> +  - reg
> +  - '#iommu-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* Example 1 (IOMMU platform device with wired interrupts) */
> +    immu1: iommu@1bccd000 {
> +        compatible = "vendor,chip-iommu", "riscv,iommu";
> +        reg = <0x1bccd000 0x1000>;
> +        interrupt-parent = <&aplic_smode>;
> +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> +        #iommu-cells = <2>;
> +    };
> +
> +    /* Device with two IOMMU device IDs, 0 and 7 */
> +    master1 {
> +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> +    };
> +
> +  - |
> +    /* Example 2 (IOMMU platform device with MSIs) */
> +    immu2: iommu@1bcdd000 {
> +        compatible = "vendor,chip-iommu", "riscv,iommu";
> +        reg = <0x1bccd000 0x1000>;
> +        msi-parent = <&imsics_smode>;
> +        #iommu-cells = <2>;
> +    };
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> +        master1 {
> +                iommus = <&immu2 32 32>;
> +        };
> +
> +        pcie@40000000 {
> +            compatible = "pci-host-cam-generic";
> +            device_type = "pci";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            bus-range = <0x0 0x1>;
> +
> +            /* CPU_PHYSICAL(2)  SIZE(2) */
> +            reg = <0x0 0x40000000 0x0 0x1000000>;
> +
> +            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
> +            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
> +                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
> +
> +            #interrupt-cells = <0x1>;
> +
> +            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
> +            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
> +                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
> +                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
> +                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
> +
> +            /* PCI_DEVICE(3)  INT#(1) */
> +            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
> +
> +            msi-parent = <&imsics_smode>;
> +
> +            /* Devices with bus number 0-127 are mastered via immu2 */
> +            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
> +        };
> +    };
> +...
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Anup Patel July 24, 2023, 10:02 a.m. UTC | #6

On Mon, Jul 24, 2023 at 1:33 PM Zong Li <[e-mail]> wrote:
>
> On Thu, Jul 20, 2023 at 3:35 AM Tomasz Jeznach <[e-mail]> wrote:
> >
> > From: Anup Patel <[e-mail]>
> >
> > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > defined by the RISC-V IOMMU specification.
> >
> > Signed-off-by: Anup Patel <[e-mail]>
> > ---
> >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > new file mode 100644
> > index 000000000000..8a9aedb61768
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > @@ -0,0 +1,146 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V IOMMU Implementation
> > +
> > +maintainers:
> > +  - Tomasz Jeznach <[e-mail]>
> > +
> > +description:
> > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> > +  which can be a regular platform device or a PCI device connected to
> > +  the host root port.
> > +
> > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > +  command queue and fault reporting as wired interrupt or MSIx event for
> > +  both PCI and platform devices.
> > +
> > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description: RISC-V IOMMU as a platform device
> > +        items:
> > +          - enum:
> > +              - vendor,chip-iommu
> > +          - const: riscv,iommu
> > +
> > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > +        items:
> > +          - enum:
> > +              - vendor,chip-pci-iommu
> > +          - const: riscv,pci-iommu
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > +      address of registers.
> > +
> > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> > +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> > +
> > +  '#iommu-cells':
> > +    const: 2
> > +    description: |
> > +      Each IOMMU specifier represents the base device ID and number of
> > +      device IDs.
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 16
> > +    description:
> > +      The presence of this property implies that given RISC-V IOMMU uses
> > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > +
> > +  msi-parent:
> > +    description:
> > +      The presence of this property implies that given RISC-V IOMMU uses
> > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > +      considered only when the interrupts property is absent.
> > +
> > +  dma-coherent:
> > +    description:
> > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > +      are cache coherent with the CPU.
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
>
> In RISC-V IOMMU, certain devices can be set to bypass mode when the
> IOMMU is in translation mode. To identify the devices that require
> bypass mode by default, does it be sensible to add a property to
> indicate this behavior?

Bypass mode for a device is a property of that device (similar to dma-coherent)
and not of the IOMMU. Other architectures (ARM and x86) never added such
a device property for bypass mode so I guess it is NOT ADVISABLE to do it.

If this is REALLY required then we can do something similar to the QCOM
SMMU driver where they have a whitelist of devices which are allowed to
be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
compatible string and any device outside this whitelist is blocked by default.

Regards,
Anup

>
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#iommu-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > +    immu1: iommu@1bccd000 {
> > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > +        reg = <0x1bccd000 0x1000>;
> > +        interrupt-parent = <&aplic_smode>;
> > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > +        #iommu-cells = <2>;
> > +    };
> > +
> > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > +    master1 {
> > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > +    };
> > +
> > +  - |
> > +    /* Example 2 (IOMMU platform device with MSIs) */
> > +    immu2: iommu@1bcdd000 {
> > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > +        reg = <0x1bccd000 0x1000>;
> > +        msi-parent = <&imsics_smode>;
> > +        #iommu-cells = <2>;
> > +    };
> > +
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > +        master1 {
> > +                iommus = <&immu2 32 32>;
> > +        };
> > +
> > +        pcie@40000000 {
> > +            compatible = "pci-host-cam-generic";
> > +            device_type = "pci";
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            bus-range = <0x0 0x1>;
> > +
> > +            /* CPU_PHYSICAL(2)  SIZE(2) */
> > +            reg = <0x0 0x40000000 0x0 0x1000000>;
> > +
> > +            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
> > +            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
> > +                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
> > +
> > +            #interrupt-cells = <0x1>;
> > +
> > +            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
> > +            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
> > +                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
> > +                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
> > +                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
> > +
> > +            /* PCI_DEVICE(3)  INT#(1) */
> > +            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
> > +
> > +            msi-parent = <&imsics_smode>;
> > +
> > +            /* Devices with bus number 0-127 are mastered via immu2 */
> > +            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
> > +        };
> > +    };
> > +...
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Zong Li July 24, 2023, 11:31 a.m. UTC | #7

On Mon, Jul 24, 2023 at 6:02 PM Anup Patel <[e-mail]> wrote:
>
> On Mon, Jul 24, 2023 at 1:33 PM Zong Li <[e-mail]> wrote:
> >
> > On Thu, Jul 20, 2023 at 3:35 AM Tomasz Jeznach <[e-mail]> wrote:
> > >
> > > From: Anup Patel <[e-mail]>
> > >
> > > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > > defined by the RISC-V IOMMU specification.
> > >
> > > Signed-off-by: Anup Patel <[e-mail]>
> > > ---
> > >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..8a9aedb61768
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > @@ -0,0 +1,146 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V IOMMU Implementation
> > > +
> > > +maintainers:
> > > +  - Tomasz Jeznach <[e-mail]>
> > > +
> > > +description:
> > > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> > > +  which can be a regular platform device or a PCI device connected to
> > > +  the host root port.
> > > +
> > > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > > +  command queue and fault reporting as wired interrupt or MSIx event for
> > > +  both PCI and platform devices.
> > > +
> > > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - description: RISC-V IOMMU as a platform device
> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-iommu
> > > +          - const: riscv,iommu
> > > +
> > > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > > +        items:
> > > +          - enum:
> > > +              - vendor,chip-pci-iommu
> > > +          - const: riscv,pci-iommu
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +    description:
> > > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > > +      address of registers.
> > > +
> > > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> > > +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> > > +
> > > +  '#iommu-cells':
> > > +    const: 2
> > > +    description: |
> > > +      Each IOMMU specifier represents the base device ID and number of
> > > +      device IDs.
> > > +
> > > +  interrupts:
> > > +    minItems: 1
> > > +    maxItems: 16
> > > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > > +
> > > +  msi-parent:
> > > +    description:
> > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > > +      considered only when the interrupts property is absent.
> > > +
> > > +  dma-coherent:
> > > +    description:
> > > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > > +      are cache coherent with the CPU.
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> >
> > In RISC-V IOMMU, certain devices can be set to bypass mode when the
> > IOMMU is in translation mode. To identify the devices that require
> > bypass mode by default, does it be sensible to add a property to
> > indicate this behavior?
>
> Bypass mode for a device is a property of that device (similar to dma-coherent)
> and not of the IOMMU. Other architectures (ARM and x86) never added such
> a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
>
> If this is REALLY required then we can do something similar to the QCOM
> SMMU driver where they have a whitelist of devices which are allowed to
> be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> compatible string and any device outside this whitelist is blocked by default.
>

I have considered that adding the property of bypass mode to that
device would be more appropriate. However, if we want to define this
property for the device, it might need to go through the generic IOMMU
dt-bindings, but I'm not sure if other IOMMU devices need this. I am
bringing up this topic here because I would like to explore if there
are any solutions on the IOMMU side, such as a property that indicates
the phandle of devices wishing to set bypass mode, somewhat similar to
the whitelist you mentioned earlier. Do you think we should address
this? After all, this is a case of RISC-V IOMMU supported.

> Regards,
> Anup
>
> >
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - '#iommu-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > > +    immu1: iommu@1bccd000 {
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        interrupt-parent = <&aplic_smode>;
> > > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > > +    master1 {
> > > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > > +    };
> > > +
> > > +  - |
> > > +    /* Example 2 (IOMMU platform device with MSIs) */
> > > +    immu2: iommu@1bcdd000 {
> > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > +        reg = <0x1bccd000 0x1000>;
> > > +        msi-parent = <&imsics_smode>;
> > > +        #iommu-cells = <2>;
> > > +    };
> > > +
> > > +    bus {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > > +        master1 {
> > > +                iommus = <&immu2 32 32>;
> > > +        };
> > > +
> > > +        pcie@40000000 {
> > > +            compatible = "pci-host-cam-generic";
> > > +            device_type = "pci";
> > > +            #address-cells = <3>;
> > > +            #size-cells = <2>;
> > > +            bus-range = <0x0 0x1>;
> > > +
> > > +            /* CPU_PHYSICAL(2)  SIZE(2) */
> > > +            reg = <0x0 0x40000000 0x0 0x1000000>;
> > > +
> > > +            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
> > > +            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
> > > +                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
> > > +
> > > +            #interrupt-cells = <0x1>;
> > > +
> > > +            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
> > > +            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
> > > +                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
> > > +                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
> > > +                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
> > > +
> > > +            /* PCI_DEVICE(3)  INT#(1) */
> > > +            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
> > > +
> > > +            msi-parent = <&imsics_smode>;
> > > +
> > > +            /* Devices with bus number 0-127 are mastered via immu2 */
> > > +            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
> > > +        };
> > > +    };
> > > +...
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [e-mail]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Anup Patel July 24, 2023, 12:10 p.m. UTC | #8

On Mon, Jul 24, 2023 at 5:01 PM Zong Li <[e-mail]> wrote:
>
> On Mon, Jul 24, 2023 at 6:02 PM Anup Patel <[e-mail]> wrote:
> >
> > On Mon, Jul 24, 2023 at 1:33 PM Zong Li <[e-mail]> wrote:
> > >
> > > On Thu, Jul 20, 2023 at 3:35 AM Tomasz Jeznach <[e-mail]> wrote:
> > > >
> > > > From: Anup Patel <[e-mail]>
> > > >
> > > > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > > > defined by the RISC-V IOMMU specification.
> > > >
> > > > Signed-off-by: Anup Patel <[e-mail]>
> > > > ---
> > > >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> > > >  1 file changed, 146 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > > new file mode 100644
> > > > index 000000000000..8a9aedb61768
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > > @@ -0,0 +1,146 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: RISC-V IOMMU Implementation
> > > > +
> > > > +maintainers:
> > > > +  - Tomasz Jeznach <[e-mail]>
> > > > +
> > > > +description:
> > > > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> > > > +  which can be a regular platform device or a PCI device connected to
> > > > +  the host root port.
> > > > +
> > > > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > > > +  command queue and fault reporting as wired interrupt or MSIx event for
> > > > +  both PCI and platform devices.
> > > > +
> > > > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - description: RISC-V IOMMU as a platform device
> > > > +        items:
> > > > +          - enum:
> > > > +              - vendor,chip-iommu
> > > > +          - const: riscv,iommu
> > > > +
> > > > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > > > +        items:
> > > > +          - enum:
> > > > +              - vendor,chip-pci-iommu
> > > > +          - const: riscv,pci-iommu
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +    description:
> > > > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > > > +      address of registers.
> > > > +
> > > > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> > > > +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> > > > +
> > > > +  '#iommu-cells':
> > > > +    const: 2
> > > > +    description: |
> > > > +      Each IOMMU specifier represents the base device ID and number of
> > > > +      device IDs.
> > > > +
> > > > +  interrupts:
> > > > +    minItems: 1
> > > > +    maxItems: 16
> > > > +    description:
> > > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > > > +
> > > > +  msi-parent:
> > > > +    description:
> > > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > > > +      considered only when the interrupts property is absent.
> > > > +
> > > > +  dma-coherent:
> > > > +    description:
> > > > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > > > +      are cache coherent with the CPU.
> > > > +
> > > > +  power-domains:
> > > > +    maxItems: 1
> > > > +
> > >
> > > In RISC-V IOMMU, certain devices can be set to bypass mode when the
> > > IOMMU is in translation mode. To identify the devices that require
> > > bypass mode by default, does it be sensible to add a property to
> > > indicate this behavior?
> >
> > Bypass mode for a device is a property of that device (similar to dma-coherent)
> > and not of the IOMMU. Other architectures (ARM and x86) never added such
> > a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
> >
> > If this is REALLY required then we can do something similar to the QCOM
> > SMMU driver where they have a whitelist of devices which are allowed to
> > be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> > compatible string and any device outside this whitelist is blocked by default.
> >
>
> I have considered that adding the property of bypass mode to that
> device would be more appropriate. However, if we want to define this
> property for the device, it might need to go through the generic IOMMU
> dt-bindings, but I'm not sure if other IOMMU devices need this. I am
> bringing up this topic here because I would like to explore if there
> are any solutions on the IOMMU side, such as a property that indicates
> the phandle of devices wishing to set bypass mode, somewhat similar to
> the whitelist you mentioned earlier. Do you think we should address
> this? After all, this is a case of RISC-V IOMMU supported.

Bypass mode is a common feature across IOMMUs. Other IOMMUs don't
have a special property for bypass mode at device-level or at IOMMU level,
which clearly indicates that defining a RISC-V specific property is not the
right way to go.

The real question is how do we set IOMMU_DOMAIN_IDENTITY (i.e.
bypass/identity domain) as the default domain for certain devices ?

One possible option is to implement def_domain_type() IOMMU operation
for RISC-V IOMMU which will return IOMMU_DOMAIN_IDENTITY for
certain devices based on compatible string matching (i.e. whitelist of
devices). As an example, refer qcom_smmu_def_domain_type()
of drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c

Regards,
Anup





>
> > Regards,
> > Anup
> >
> > >
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - '#iommu-cells'
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > > > +    immu1: iommu@1bccd000 {
> > > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > > +        reg = <0x1bccd000 0x1000>;
> > > > +        interrupt-parent = <&aplic_smode>;
> > > > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > > > +        #iommu-cells = <2>;
> > > > +    };
> > > > +
> > > > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > > > +    master1 {
> > > > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > > > +    };
> > > > +
> > > > +  - |
> > > > +    /* Example 2 (IOMMU platform device with MSIs) */
> > > > +    immu2: iommu@1bcdd000 {
> > > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > > +        reg = <0x1bccd000 0x1000>;
> > > > +        msi-parent = <&imsics_smode>;
> > > > +        #iommu-cells = <2>;
> > > > +    };
> > > > +
> > > > +    bus {
> > > > +        #address-cells = <2>;
> > > > +        #size-cells = <2>;
> > > > +
> > > > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > > > +        master1 {
> > > > +                iommus = <&immu2 32 32>;
> > > > +        };
> > > > +
> > > > +        pcie@40000000 {
> > > > +            compatible = "pci-host-cam-generic";
> > > > +            device_type = "pci";
> > > > +            #address-cells = <3>;
> > > > +            #size-cells = <2>;
> > > > +            bus-range = <0x0 0x1>;
> > > > +
> > > > +            /* CPU_PHYSICAL(2)  SIZE(2) */
> > > > +            reg = <0x0 0x40000000 0x0 0x1000000>;
> > > > +
> > > > +            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
> > > > +            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
> > > > +                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
> > > > +
> > > > +            #interrupt-cells = <0x1>;
> > > > +
> > > > +            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
> > > > +            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
> > > > +                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
> > > > +                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
> > > > +                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
> > > > +
> > > > +            /* PCI_DEVICE(3)  INT#(1) */
> > > > +            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
> > > > +
> > > > +            msi-parent = <&imsics_smode>;
> > > > +
> > > > +            /* Devices with bus number 0-127 are mastered via immu2 */
> > > > +            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
> > > > +        };
> > > > +    };
> > > > +...
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > [e-mail]
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Zong Li July 24, 2023, 1:23 p.m. UTC | #9

On Mon, Jul 24, 2023 at 8:10 PM Anup Patel <[e-mail]> wrote:
>
> On Mon, Jul 24, 2023 at 5:01 PM Zong Li <[e-mail]> wrote:
> >
> > On Mon, Jul 24, 2023 at 6:02 PM Anup Patel <[e-mail]> wrote:
> > >
> > > On Mon, Jul 24, 2023 at 1:33 PM Zong Li <[e-mail]> wrote:
> > > >
> > > > On Thu, Jul 20, 2023 at 3:35 AM Tomasz Jeznach <[e-mail]> wrote:
> > > > >
> > > > > From: Anup Patel <[e-mail]>
> > > > >
> > > > > We add DT bindings document for RISC-V IOMMU platform and PCI devices
> > > > > defined by the RISC-V IOMMU specification.
> > > > >
> > > > > Signed-off-by: Anup Patel <[e-mail]>
> > > > > ---
> > > > >  .../bindings/iommu/riscv,iommu.yaml           | 146 ++++++++++++++++++
> > > > >  1 file changed, 146 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..8a9aedb61768
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
> > > > > @@ -0,0 +1,146 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: RISC-V IOMMU Implementation
> > > > > +
> > > > > +maintainers:
> > > > > +  - Tomasz Jeznach <[e-mail]>
> > > > > +
> > > > > +description:
> > > > > +  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
> > > > > +  which can be a regular platform device or a PCI device connected to
> > > > > +  the host root port.
> > > > > +
> > > > > +  The RISC-V IOMMU provides two stage translation, device directory table,
> > > > > +  command queue and fault reporting as wired interrupt or MSIx event for
> > > > > +  both PCI and platform devices.
> > > > > +
> > > > > +  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    oneOf:
> > > > > +      - description: RISC-V IOMMU as a platform device
> > > > > +        items:
> > > > > +          - enum:
> > > > > +              - vendor,chip-iommu
> > > > > +          - const: riscv,iommu
> > > > > +
> > > > > +      - description: RISC-V IOMMU as a PCI device connected to root port
> > > > > +        items:
> > > > > +          - enum:
> > > > > +              - vendor,chip-pci-iommu
> > > > > +          - const: riscv,pci-iommu
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +    description:
> > > > > +      For RISC-V IOMMU as a platform device, this represents the MMIO base
> > > > > +      address of registers.
> > > > > +
> > > > > +      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
> > > > > +      details as described in Documentation/devicetree/bindings/pci/pci.txt
> > > > > +
> > > > > +  '#iommu-cells':
> > > > > +    const: 2
> > > > > +    description: |
> > > > > +      Each IOMMU specifier represents the base device ID and number of
> > > > > +      device IDs.
> > > > > +
> > > > > +  interrupts:
> > > > > +    minItems: 1
> > > > > +    maxItems: 16
> > > > > +    description:
> > > > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > > > +      wired interrupts to notify the RISC-V HARTS (or CPUs).
> > > > > +
> > > > > +  msi-parent:
> > > > > +    description:
> > > > > +      The presence of this property implies that given RISC-V IOMMU uses
> > > > > +      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
> > > > > +      considered only when the interrupts property is absent.
> > > > > +
> > > > > +  dma-coherent:
> > > > > +    description:
> > > > > +      Present if page table walks and DMA accessed made by the RISC-V IOMMU
> > > > > +      are cache coherent with the CPU.
> > > > > +
> > > > > +  power-domains:
> > > > > +    maxItems: 1
> > > > > +
> > > >
> > > > In RISC-V IOMMU, certain devices can be set to bypass mode when the
> > > > IOMMU is in translation mode. To identify the devices that require
> > > > bypass mode by default, does it be sensible to add a property to
> > > > indicate this behavior?
> > >
> > > Bypass mode for a device is a property of that device (similar to dma-coherent)
> > > and not of the IOMMU. Other architectures (ARM and x86) never added such
> > > a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
> > >
> > > If this is REALLY required then we can do something similar to the QCOM
> > > SMMU driver where they have a whitelist of devices which are allowed to
> > > be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> > > compatible string and any device outside this whitelist is blocked by default.
> > >
> >
> > I have considered that adding the property of bypass mode to that
> > device would be more appropriate. However, if we want to define this
> > property for the device, it might need to go through the generic IOMMU
> > dt-bindings, but I'm not sure if other IOMMU devices need this. I am
> > bringing up this topic here because I would like to explore if there
> > are any solutions on the IOMMU side, such as a property that indicates
> > the phandle of devices wishing to set bypass mode, somewhat similar to
> > the whitelist you mentioned earlier. Do you think we should address
> > this? After all, this is a case of RISC-V IOMMU supported.
>
> Bypass mode is a common feature across IOMMUs. Other IOMMUs don't
> have a special property for bypass mode at device-level or at IOMMU level,
> which clearly indicates that defining a RISC-V specific property is not the
> right way to go.
>
> The real question is how do we set IOMMU_DOMAIN_IDENTITY (i.e.
> bypass/identity domain) as the default domain for certain devices ?
>
> One possible option is to implement def_domain_type() IOMMU operation
> for RISC-V IOMMU which will return IOMMU_DOMAIN_IDENTITY for
> certain devices based on compatible string matching (i.e. whitelist of
> devices). As an example, refer qcom_smmu_def_domain_type()
> of drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>

That is indeed one way to approach it, and we can modify the
compatible string when we want to change the mode. However, it would
be preferable to explore a more flexible approach to achieve this
goal. By doing so, we can avoid hard coding anything in the driver or
having to rebuild the kernel  whenever we want to change the mode for
certain devices. While I have considered extending a cell in the
'iommus' property to indicate a device's desire to set bypass mode, it
doesn't comply with the iommu documentation and could lead to
ambiguous definitions.

If, at present, we are unable to find a suitable solution, perhaps
let's keep this topic in mind until we discover a more appropriate
approach. In the meantime, we can continue to explore other
possibilities to implement it. Thanks.

> Regards,
> Anup
>
>
>
>
>
> >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - '#iommu-cells'
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    /* Example 1 (IOMMU platform device with wired interrupts) */
> > > > > +    immu1: iommu@1bccd000 {
> > > > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > > > +        reg = <0x1bccd000 0x1000>;
> > > > > +        interrupt-parent = <&aplic_smode>;
> > > > > +        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
> > > > > +        #iommu-cells = <2>;
> > > > > +    };
> > > > > +
> > > > > +    /* Device with two IOMMU device IDs, 0 and 7 */
> > > > > +    master1 {
> > > > > +        iommus = <&immu1 0 1>, <&immu1 7 1>;
> > > > > +    };
> > > > > +
> > > > > +  - |
> > > > > +    /* Example 2 (IOMMU platform device with MSIs) */
> > > > > +    immu2: iommu@1bcdd000 {
> > > > > +        compatible = "vendor,chip-iommu", "riscv,iommu";
> > > > > +        reg = <0x1bccd000 0x1000>;
> > > > > +        msi-parent = <&imsics_smode>;
> > > > > +        #iommu-cells = <2>;
> > > > > +    };
> > > > > +
> > > > > +    bus {
> > > > > +        #address-cells = <2>;
> > > > > +        #size-cells = <2>;
> > > > > +
> > > > > +        /* Device with IOMMU device IDs ranging from 32 to 64 */
> > > > > +        master1 {
> > > > > +                iommus = <&immu2 32 32>;
> > > > > +        };
> > > > > +
> > > > > +        pcie@40000000 {
> > > > > +            compatible = "pci-host-cam-generic";
> > > > > +            device_type = "pci";
> > > > > +            #address-cells = <3>;
> > > > > +            #size-cells = <2>;
> > > > > +            bus-range = <0x0 0x1>;
> > > > > +
> > > > > +            /* CPU_PHYSICAL(2)  SIZE(2) */
> > > > > +            reg = <0x0 0x40000000 0x0 0x1000000>;
> > > > > +
> > > > > +            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
> > > > > +            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
> > > > > +                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
> > > > > +
> > > > > +            #interrupt-cells = <0x1>;
> > > > > +
> > > > > +            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
> > > > > +            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
> > > > > +                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
> > > > > +                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
> > > > > +                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
> > > > > +
> > > > > +            /* PCI_DEVICE(3)  INT#(1) */
> > > > > +            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
> > > > > +
> > > > > +            msi-parent = <&imsics_smode>;
> > > > > +
> > > > > +            /* Devices with bus number 0-127 are mastered via immu2 */
> > > > > +            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
> > > > > +        };
> > > > > +    };
> > > > > +...
> > > > > --
> > > > > 2.34.1
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > linux-riscv mailing list
> > > > > [e-mail]
> > > > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Baolu Lu July 26, 2023, 3:21 a.m. UTC | #10

On 2023/7/24 21:23, Zong Li wrote:
>>>>> In RISC-V IOMMU, certain devices can be set to bypass mode when the
>>>>> IOMMU is in translation mode. To identify the devices that require
>>>>> bypass mode by default, does it be sensible to add a property to
>>>>> indicate this behavior?
>>>> Bypass mode for a device is a property of that device (similar to dma-coherent)
>>>> and not of the IOMMU. Other architectures (ARM and x86) never added such
>>>> a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
>>>>
>>>> If this is REALLY required then we can do something similar to the QCOM
>>>> SMMU driver where they have a whitelist of devices which are allowed to
>>>> be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
>>>> compatible string and any device outside this whitelist is blocked by default.
>>>>
>>> I have considered that adding the property of bypass mode to that
>>> device would be more appropriate. However, if we want to define this
>>> property for the device, it might need to go through the generic IOMMU
>>> dt-bindings, but I'm not sure if other IOMMU devices need this. I am
>>> bringing up this topic here because I would like to explore if there
>>> are any solutions on the IOMMU side, such as a property that indicates
>>> the phandle of devices wishing to set bypass mode, somewhat similar to
>>> the whitelist you mentioned earlier. Do you think we should address
>>> this? After all, this is a case of RISC-V IOMMU supported.
>> Bypass mode is a common feature across IOMMUs. Other IOMMUs don't
>> have a special property for bypass mode at device-level or at IOMMU level,
>> which clearly indicates that defining a RISC-V specific property is not the
>> right way to go.
>>
>> The real question is how do we set IOMMU_DOMAIN_IDENTITY (i.e.
>> bypass/identity domain) as the default domain for certain devices ?
>>
>> One possible option is to implement def_domain_type() IOMMU operation
>> for RISC-V IOMMU which will return IOMMU_DOMAIN_IDENTITY for
>> certain devices based on compatible string matching (i.e. whitelist of
>> devices). As an example, refer qcom_smmu_def_domain_type()
>> of drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>
> That is indeed one way to approach it, and we can modify the
> compatible string when we want to change the mode. However, it would
> be preferable to explore a more flexible approach to achieve this
> goal. By doing so, we can avoid hard coding anything in the driver or
> having to rebuild the kernel  whenever we want to change the mode for
> certain devices. While I have considered extending a cell in the
> 'iommus' property to indicate a device's desire to set bypass mode, it
> doesn't comply with the iommu documentation and could lead to
> ambiguous definitions.

Hard coding the matching strings in the iommu driver is definitely not a
preferable way. A feasible solution from current code's point of view is
that platform opt-in the device's special requirements through DT or
ACPI. And in the def_domain_type callback, let the iommu core know that,
hence it can allocate a right type of domain for the device.

Thoughts?

Best regards,
baolu

Zong Li July 26, 2023, 4:26 a.m. UTC | #11

On Wed, Jul 26, 2023 at 11:21 AM Baolu Lu <[e-mail]> wrote:
>
> On 2023/7/24 21:23, Zong Li wrote:
> >>>>> In RISC-V IOMMU, certain devices can be set to bypass mode when the
> >>>>> IOMMU is in translation mode. To identify the devices that require
> >>>>> bypass mode by default, does it be sensible to add a property to
> >>>>> indicate this behavior?
> >>>> Bypass mode for a device is a property of that device (similar to dma-coherent)
> >>>> and not of the IOMMU. Other architectures (ARM and x86) never added such
> >>>> a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
> >>>>
> >>>> If this is REALLY required then we can do something similar to the QCOM
> >>>> SMMU driver where they have a whitelist of devices which are allowed to
> >>>> be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> >>>> compatible string and any device outside this whitelist is blocked by default.
> >>>>
> >>> I have considered that adding the property of bypass mode to that
> >>> device would be more appropriate. However, if we want to define this
> >>> property for the device, it might need to go through the generic IOMMU
> >>> dt-bindings, but I'm not sure if other IOMMU devices need this. I am
> >>> bringing up this topic here because I would like to explore if there
> >>> are any solutions on the IOMMU side, such as a property that indicates
> >>> the phandle of devices wishing to set bypass mode, somewhat similar to
> >>> the whitelist you mentioned earlier. Do you think we should address
> >>> this? After all, this is a case of RISC-V IOMMU supported.
> >> Bypass mode is a common feature across IOMMUs. Other IOMMUs don't
> >> have a special property for bypass mode at device-level or at IOMMU level,
> >> which clearly indicates that defining a RISC-V specific property is not the
> >> right way to go.
> >>
> >> The real question is how do we set IOMMU_DOMAIN_IDENTITY (i.e.
> >> bypass/identity domain) as the default domain for certain devices ?
> >>
> >> One possible option is to implement def_domain_type() IOMMU operation
> >> for RISC-V IOMMU which will return IOMMU_DOMAIN_IDENTITY for
> >> certain devices based on compatible string matching (i.e. whitelist of
> >> devices). As an example, refer qcom_smmu_def_domain_type()
> >> of drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>
> > That is indeed one way to approach it, and we can modify the
> > compatible string when we want to change the mode. However, it would
> > be preferable to explore a more flexible approach to achieve this
> > goal. By doing so, we can avoid hard coding anything in the driver or
> > having to rebuild the kernel  whenever we want to change the mode for
> > certain devices. While I have considered extending a cell in the
> > 'iommus' property to indicate a device's desire to set bypass mode, it
> > doesn't comply with the iommu documentation and could lead to
> > ambiguous definitions.
>
> Hard coding the matching strings in the iommu driver is definitely not a
> preferable way. A feasible solution from current code's point of view is
> that platform opt-in the device's special requirements through DT or
> ACPI. And in the def_domain_type callback, let the iommu core know that,
> hence it can allocate a right type of domain for the device.
>
> Thoughts?
>

It would be nice if we can deal with it at this time. As we discussed
earlier, we might need to consider how to indicate that, such as
putting a property in device side or iommu side, and whether we need
to define it in generic dt-binding instead of RISC-V specific
dt-binding.

> Best regards,
> baolu

Jason Gunthorpe July 26, 2023, 12:17 p.m. UTC | #12

On Wed, Jul 26, 2023 at 12:26:14PM +0800, Zong Li wrote:
> On Wed, Jul 26, 2023 at 11:21 AM Baolu Lu <[e-mail]> wrote:
> >
> > On 2023/7/24 21:23, Zong Li wrote:
> > >>>>> In RISC-V IOMMU, certain devices can be set to bypass mode when the
> > >>>>> IOMMU is in translation mode. To identify the devices that require
> > >>>>> bypass mode by default, does it be sensible to add a property to
> > >>>>> indicate this behavior?
> > >>>> Bypass mode for a device is a property of that device (similar to dma-coherent)
> > >>>> and not of the IOMMU. Other architectures (ARM and x86) never added such
> > >>>> a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
> > >>>>
> > >>>> If this is REALLY required then we can do something similar to the QCOM
> > >>>> SMMU driver where they have a whitelist of devices which are allowed to
> > >>>> be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> > >>>> compatible string and any device outside this whitelist is
> > >>>> blocked by default.

I have a draft patch someplace that consolidated all this quirk
checking into the core code. Generally the expectation is that any
device behind an iommu is fully functional in all modes. The existing
quirks are for HW defects that make some devices not work properly. In
this case the right outcome seems to be effectively blocking them from
using the iommu.

So, you should explain a lot more what "require bypass mode" means in
the RISCV world and why any device would need it.

Jason

Zong Li July 27, 2023, 2:42 a.m. UTC | #13

On Wed, Jul 26, 2023 at 8:17 PM Jason Gunthorpe <[e-mail]> wrote:
>
> On Wed, Jul 26, 2023 at 12:26:14PM +0800, Zong Li wrote:
> > On Wed, Jul 26, 2023 at 11:21 AM Baolu Lu <[e-mail]> wrote:
> > >
> > > On 2023/7/24 21:23, Zong Li wrote:
> > > >>>>> In RISC-V IOMMU, certain devices can be set to bypass mode when the
> > > >>>>> IOMMU is in translation mode. To identify the devices that require
> > > >>>>> bypass mode by default, does it be sensible to add a property to
> > > >>>>> indicate this behavior?
> > > >>>> Bypass mode for a device is a property of that device (similar to dma-coherent)
> > > >>>> and not of the IOMMU. Other architectures (ARM and x86) never added such
> > > >>>> a device property for bypass mode so I guess it is NOT ADVISABLE to do it.
> > > >>>>
> > > >>>> If this is REALLY required then we can do something similar to the QCOM
> > > >>>> SMMU driver where they have a whitelist of devices which are allowed to
> > > >>>> be in bypass mode (i.e. IOMMU_DOMAIN_IDENTITY) based their device
> > > >>>> compatible string and any device outside this whitelist is
> > > >>>> blocked by default.
>
> I have a draft patch someplace that consolidated all this quirk
> checking into the core code. Generally the expectation is that any
> device behind an iommu is fully functional in all modes. The existing
> quirks are for HW defects that make some devices not work properly. In
> this case the right outcome seems to be effectively blocking them from
> using the iommu.
>
> So, you should explain a lot more what "require bypass mode" means in
> the RISCV world and why any device would need it.

Perhaps this question could be related to the scenarios in which
devices wish to be in bypass mode when the IOMMU is in translation
mode, and why IOMMU defines/supports this case. Currently, I could
envision a scenario where a device is already connected to the IOMMU
in hardware, but it is not functioning correctly, or there are
performance impacts. If modifying the hardware is not feasible, a
default configuration that allows bypass mode could be provided as a
solution. There might be other scenarios that I might have overlooked.
It seems to me since IOMMU supports this configuration, it would be
advantageous to have an approach to achieve it, and DT might be a
flexible way.

>
> Jason

Jason Gunthorpe Aug. 9, 2023, 2:57 p.m. UTC | #14

On Thu, Jul 27, 2023 at 10:42:47AM +0800, Zong Li wrote:

> Perhaps this question could be related to the scenarios in which
> devices wish to be in bypass mode when the IOMMU is in translation
> mode, and why IOMMU defines/supports this case. Currently, I could
> envision a scenario where a device is already connected to the IOMMU
> in hardware, but it is not functioning correctly, or there are
> performance impacts. If modifying the hardware is not feasible, a
> default configuration that allows bypass mode could be provided as a
> solution. There might be other scenarios that I might have overlooked.
> It seems to me since IOMMU supports this configuration, it would be
> advantageous to have an approach to achieve it, and DT might be a
> flexible way.

So far we've taken the approach that broken hardware is quirked in the
kernel by matching OF compatible string pattners. This is HW that is
completely broken and the IOMMU doesn't work at all for it.

HW that is slow or whatever is not quirked and this is an admin policy
choice where the system should land on the security/performance
spectrum.

So I'm not sure adding DT makes sense here.

Jason

Zong Li Aug. 15, 2023, 1:28 a.m. UTC | #15

On Wed, Aug 9, 2023 at 10:57 PM Jason Gunthorpe <[e-mail]> wrote:
>
> On Thu, Jul 27, 2023 at 10:42:47AM +0800, Zong Li wrote:
>
> > Perhaps this question could be related to the scenarios in which
> > devices wish to be in bypass mode when the IOMMU is in translation
> > mode, and why IOMMU defines/supports this case. Currently, I could
> > envision a scenario where a device is already connected to the IOMMU
> > in hardware, but it is not functioning correctly, or there are
> > performance impacts. If modifying the hardware is not feasible, a
> > default configuration that allows bypass mode could be provided as a
> > solution. There might be other scenarios that I might have overlooked.
> > It seems to me since IOMMU supports this configuration, it would be
> > advantageous to have an approach to achieve it, and DT might be a
> > flexible way.
>
> So far we've taken the approach that broken hardware is quirked in the
> kernel by matching OF compatible string pattners. This is HW that is
> completely broken and the IOMMU doesn't work at all for it.
>
> HW that is slow or whatever is not quirked and this is an admin policy
> choice where the system should land on the security/performance
> spectrum.
>
> So I'm not sure adding DT makes sense here.
>

Hi Jason,
Sorry for being late here, I hadn't noticed this reply earlier. The
approach seems to address the situation. Could you kindly provide
information about the location of the patches? I was wondering about
further details regarding this particular implementation. Thanks

> Jason

Jason Gunthorpe Aug. 15, 2023, 6:38 p.m. UTC | #16

On Tue, Aug 15, 2023 at 09:28:54AM +0800, Zong Li wrote:
> On Wed, Aug 9, 2023 at 10:57 PM Jason Gunthorpe <[e-mail]> wrote:
> >
> > On Thu, Jul 27, 2023 at 10:42:47AM +0800, Zong Li wrote:
> >
> > > Perhaps this question could be related to the scenarios in which
> > > devices wish to be in bypass mode when the IOMMU is in translation
> > > mode, and why IOMMU defines/supports this case. Currently, I could
> > > envision a scenario where a device is already connected to the IOMMU
> > > in hardware, but it is not functioning correctly, or there are
> > > performance impacts. If modifying the hardware is not feasible, a
> > > default configuration that allows bypass mode could be provided as a
> > > solution. There might be other scenarios that I might have overlooked.
> > > It seems to me since IOMMU supports this configuration, it would be
> > > advantageous to have an approach to achieve it, and DT might be a
> > > flexible way.
> >
> > So far we've taken the approach that broken hardware is quirked in the
> > kernel by matching OF compatible string pattners. This is HW that is
> > completely broken and the IOMMU doesn't work at all for it.
> >
> > HW that is slow or whatever is not quirked and this is an admin policy
> > choice where the system should land on the security/performance
> > spectrum.
> >
> > So I'm not sure adding DT makes sense here.
> >
> 
> Hi Jason,
> Sorry for being late here, I hadn't noticed this reply earlier. The
> approach seems to address the situation. Could you kindly provide
> information about the location of the patches? I was wondering about
> further details regarding this particular implementation. Thanks

There are a couple versions, eg  
 arm_smmu_def_domain_type()
 qcom_smmu_def_domain_type()

Jason

Zong Li Aug. 16, 2023, 2:16 a.m. UTC | #17

On Wed, Aug 16, 2023 at 2:38 AM Jason Gunthorpe <[e-mail]> wrote:
>
> On Tue, Aug 15, 2023 at 09:28:54AM +0800, Zong Li wrote:
> > On Wed, Aug 9, 2023 at 10:57 PM Jason Gunthorpe <[e-mail]> wrote:
> > >
> > > On Thu, Jul 27, 2023 at 10:42:47AM +0800, Zong Li wrote:
> > >
> > > > Perhaps this question could be related to the scenarios in which
> > > > devices wish to be in bypass mode when the IOMMU is in translation
> > > > mode, and why IOMMU defines/supports this case. Currently, I could
> > > > envision a scenario where a device is already connected to the IOMMU
> > > > in hardware, but it is not functioning correctly, or there are
> > > > performance impacts. If modifying the hardware is not feasible, a
> > > > default configuration that allows bypass mode could be provided as a
> > > > solution. There might be other scenarios that I might have overlooked.
> > > > It seems to me since IOMMU supports this configuration, it would be
> > > > advantageous to have an approach to achieve it, and DT might be a
> > > > flexible way.
> > >
> > > So far we've taken the approach that broken hardware is quirked in the
> > > kernel by matching OF compatible string pattners. This is HW that is
> > > completely broken and the IOMMU doesn't work at all for it.
> > >
> > > HW that is slow or whatever is not quirked and this is an admin policy
> > > choice where the system should land on the security/performance
> > > spectrum.
> > >
> > > So I'm not sure adding DT makes sense here.
> > >
> >
> > Hi Jason,
> > Sorry for being late here, I hadn't noticed this reply earlier. The
> > approach seems to address the situation. Could you kindly provide
> > information about the location of the patches? I was wondering about
> > further details regarding this particular implementation. Thanks
>
> There are a couple versions, eg
>  arm_smmu_def_domain_type()
>  qcom_smmu_def_domain_type()
>

I thought what you mentioned earlier is that there is a new approach
being considered for this. I think what you point out is the same as
Anup mentioned. However, as I mentioned earlier, I am exploring a more
flexible approach to achieve this objective. This way, we can avoid
hard coding anything (i.e.list compatible string) in the driver or
requiring a kernel rebuild every time we need to change the mode for
specific devices. For example, the driver could parse the device node
to determine and record if a device will be set to bypass, and then
the .def_domain_type could be used to set to IOMMU_DOMAIN_IDENTITY by
the record. I'm not sure if it makes sense for everyone, it seems to
me that it would be great if there is a way to do this. :)

> Jason

Baolu Lu Aug. 16, 2023, 4:10 a.m. UTC | #18

On 2023/8/16 10:16, Zong Li wrote:
> On Wed, Aug 16, 2023 at 2:38 AM Jason Gunthorpe<[e-mail]>  wrote:
>> On Tue, Aug 15, 2023 at 09:28:54AM +0800, Zong Li wrote:
>>> On Wed, Aug 9, 2023 at 10:57 PM Jason Gunthorpe<[e-mail]>  wrote:
>>>> On Thu, Jul 27, 2023 at 10:42:47AM +0800, Zong Li wrote:
>>>>
>>>>> Perhaps this question could be related to the scenarios in which
>>>>> devices wish to be in bypass mode when the IOMMU is in translation
>>>>> mode, and why IOMMU defines/supports this case. Currently, I could
>>>>> envision a scenario where a device is already connected to the IOMMU
>>>>> in hardware, but it is not functioning correctly, or there are
>>>>> performance impacts. If modifying the hardware is not feasible, a
>>>>> default configuration that allows bypass mode could be provided as a
>>>>> solution. There might be other scenarios that I might have overlooked.
>>>>> It seems to me since IOMMU supports this configuration, it would be
>>>>> advantageous to have an approach to achieve it, and DT might be a
>>>>> flexible way.
>>>> So far we've taken the approach that broken hardware is quirked in the
>>>> kernel by matching OF compatible string pattners. This is HW that is
>>>> completely broken and the IOMMU doesn't work at all for it.
>>>>
>>>> HW that is slow or whatever is not quirked and this is an admin policy
>>>> choice where the system should land on the security/performance
>>>> spectrum.
>>>>
>>>> So I'm not sure adding DT makes sense here.
>>>>
>>> Hi Jason,
>>> Sorry for being late here, I hadn't noticed this reply earlier. The
>>> approach seems to address the situation. Could you kindly provide
>>> information about the location of the patches? I was wondering about
>>> further details regarding this particular implementation. Thanks
>> There are a couple versions, eg
>>   arm_smmu_def_domain_type()
>>   qcom_smmu_def_domain_type()
>>
> I thought what you mentioned earlier is that there is a new approach
> being considered for this. I think what you point out is the same as
> Anup mentioned. However, as I mentioned earlier, I am exploring a more
> flexible approach to achieve this objective. This way, we can avoid
> hard coding anything (i.e.list compatible string) in the driver or
> requiring a kernel rebuild every time we need to change the mode for
> specific devices. For example, the driver could parse the device node
> to determine and record if a device will be set to bypass, and then
> the .def_domain_type could be used to set to IOMMU_DOMAIN_IDENTITY by
> the record. I'm not sure if it makes sense for everyone, it seems to
> me that it would be great if there is a way to do this. 

Patch

diff --git a/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
new file mode 100644
index 000000000000..8a9aedb61768
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
@@ -0,0 +1,146 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V IOMMU Implementation
+
+maintainers:
+  - Tomasz Jeznach <[e-mail]>
+
+description:
+  The RISC-V IOMMU specificaiton defines an IOMMU for RISC-V platforms
+  which can be a regular platform device or a PCI device connected to
+  the host root port.
+
+  The RISC-V IOMMU provides two stage translation, device directory table,
+  command queue and fault reporting as wired interrupt or MSIx event for
+  both PCI and platform devices.
+
+  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
+
+properties:
+  compatible:
+    oneOf:
+      - description: RISC-V IOMMU as a platform device
+        items:
+          - enum:
+              - vendor,chip-iommu
+          - const: riscv,iommu
+
+      - description: RISC-V IOMMU as a PCI device connected to root port
+        items:
+          - enum:
+              - vendor,chip-pci-iommu
+          - const: riscv,pci-iommu
+
+  reg:
+    maxItems: 1
+    description:
+      For RISC-V IOMMU as a platform device, this represents the MMIO base
+      address of registers.
+
+      For RISC-V IOMMU as a PCI device, this represents the PCI-PCI bridge
+      details as described in Documentation/devicetree/bindings/pci/pci.txt
+
+  '#iommu-cells':
+    const: 2
+    description: |
+      Each IOMMU specifier represents the base device ID and number of
+      device IDs.
+
+  interrupts:
+    minItems: 1
+    maxItems: 16
+    description:
+      The presence of this property implies that given RISC-V IOMMU uses
+      wired interrupts to notify the RISC-V HARTS (or CPUs).
+
+  msi-parent:
+    description:
+      The presence of this property implies that given RISC-V IOMMU uses
+      MSIx to notify the RISC-V HARTs (or CPUs). This property should be
+      considered only when the interrupts property is absent.
+
+  dma-coherent:
+    description:
+      Present if page table walks and DMA accessed made by the RISC-V IOMMU
+      are cache coherent with the CPU.
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#iommu-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    /* Example 1 (IOMMU platform device with wired interrupts) */
+    immu1: iommu@1bccd000 {
+        compatible = "vendor,chip-iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        interrupt-parent = <&aplic_smode>;
+        interrupts = <32 4>, <33 4>, <34 4>, <35 4>;
+        #iommu-cells = <2>;
+    };
+
+    /* Device with two IOMMU device IDs, 0 and 7 */
+    master1 {
+        iommus = <&immu1 0 1>, <&immu1 7 1>;
+    };
+
+  - |
+    /* Example 2 (IOMMU platform device with MSIs) */
+    immu2: iommu@1bcdd000 {
+        compatible = "vendor,chip-iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        msi-parent = <&imsics_smode>;
+        #iommu-cells = <2>;
+    };
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        /* Device with IOMMU device IDs ranging from 32 to 64 */
+        master1 {
+                iommus = <&immu2 32 32>;
+        };
+
+        pcie@40000000 {
+            compatible = "pci-host-cam-generic";
+            device_type = "pci";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            bus-range = <0x0 0x1>;
+
+            /* CPU_PHYSICAL(2)  SIZE(2) */
+            reg = <0x0 0x40000000 0x0 0x1000000>;
+
+            /* BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2) */
+            ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
+                     <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
+
+            #interrupt-cells = <0x1>;
+
+            /* PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(2) */
+            interrupt-map = <   0x0 0x0 0x0  0x1  &aplic_smode  0x4 0x1>,
+                            < 0x800 0x0 0x0  0x1  &aplic_smode  0x5 0x1>,
+                            <0x1000 0x0 0x0  0x1  &aplic_smode  0x6 0x1>,
+                            <0x1800 0x0 0x0  0x1  &aplic_smode  0x7 0x1>;
+
+            /* PCI_DEVICE(3)  INT#(1) */
+            interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
+
+            msi-parent = <&imsics_smode>;
+
+            /* Devices with bus number 0-127 are mastered via immu2 */
+            iommu-map = <0x0000 &immu2 0x0000 0x8000>;
+        };
+    };
+...