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

[Rob Herring]

On Wed, Jul 19, 2023 at 2:19 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> Hey Tomasz,
>
> On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote:
> > From: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> >
> > 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 <apatel@xxxxxxxxxxxxxxxx>
>
> 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 <tjeznach@xxxxxxxxxxxx>
>
> 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/20230719113542.2293295-7-apatel@xxxxxxxxxxxxxxxx/
>
> > +          - 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