From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7DCA022F0B for ; Wed, 19 Jul 2023 21:37:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EF68CC433C8 for ; Wed, 19 Jul 2023 21:37:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689802663; bh=l31xmKKZKXpdELnjITMbNJIiJOmw33va5T4SCwmraDs=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ehK5AhVFNt8CzbXFYdFHh5jzB81wh7yj1WYC+1x/CKL4LzcFjsShmKSk/jl9zejkw b6lfgY+/XxgPHGqavUtZ5PhvvN3AnagCd7A/AfZRs7trWOpFLVUa2oKSr6WbDrhUUf +FM5z3756FcjGhuMyU/sStaPvraEi3ZScmmx7aVyw//h9SiTW2w6EairnoLPkDn40T yv83I6obL4m0YOmV65Rhbf4SfA87hqGVcdg1grwCsrL04JN5qjckLKnmPqCWOP4OhO GNldWU2xI3qgihrhsaA933Dob/b9j5RzidJDRj71V/2+l80YRZjUaurjXwFxnzzGsd gdR7L2gtF4qkw== Received: by mail-lj1-f170.google.com with SMTP id 38308e7fff4ca-2b73564e98dso1077391fa.3 for ; Wed, 19 Jul 2023 14:37:42 -0700 (PDT) X-Gm-Message-State: ABy/qLaQKDokzq3Une2QydUKr1uJGBKEsOD2bqghqxwf+zUVJ3l+YQdK Co3OZU2fXOWxlfzoSeTvHGaz6ehCQt5z6vOpQQ== X-Google-Smtp-Source: APBJJlEFlih0GsgJb5Kb1nv8BRzBOuXoE5Uxgako0V+TCO6aW6WabNUjR9BZ/pRBaaA+TCXjVBCw3zJAiM2xffQZrLs= X-Received: by 2002:a2e:9c5a:0:b0:2b6:d0fc:ee18 with SMTP id t26-20020a2e9c5a000000b002b6d0fcee18mr790661ljj.19.1689802660935; Wed, 19 Jul 2023 14:37:40 -0700 (PDT) Precedence: bulk X-Mailing-List: iommu@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20230719-unnoticed-scion-744fdf509151@spud> In-Reply-To: <20230719-unnoticed-scion-744fdf509151@spud> From: Rob Herring Date: Wed, 19 Jul 2023 15:37:27 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 03/11] dt-bindings: Add RISC-V IOMMU bindings To: Conor Dooley , Tomasz Jeznach Cc: Joerg Roedel , Will Deacon , Robin Murphy , Paul Walmsley , Anup Patel , Albert Ou , linux@rivosinc.com, linux-kernel@vger.kernel.org, Sebastien Boeuf , iommu@lists.linux.dev, Palmer Dabbelt , Nick Kossifidis , linux-riscv@lists.infradead.org, krzysztof.kozlowski+dt@linaro.org, devicetree@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jul 19, 2023 at 2:19=E2=80=AFPM Conor Dooley wro= te: > > Hey Tomasz, > > On Wed, Jul 19, 2023 at 12:33:47PM -0700, Tomasz Jeznach wrote: > > From: Anup Patel > > > > 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 > > 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 > > 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 ta= ble, > > + command queue and fault reporting as wired interrupt or MSIx event f= or > > + 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@ventanamicro.= com/ > > > + - const: riscv,iommu > > + > > + - description: RISC-V IOMMU as a PCI device connected to root po= rt > > + 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 br= idge Your IOMMU is also a PCI-PCI bridge? Is that a normal PCI thing? > > + details as described in Documentation/devicetree/bindings/pci/pc= i.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 assumpt= ion. > > + > > + interrupts: > > + minItems: 1 > > + maxItems: 16 > > What are any of these interrupts? > > > + description: > > + The presence of this property implies that given RISC-V IOMMU us= es > > + wired interrupts to notify the RISC-V HARTS (or CPUs). > > + > > + msi-parent: > > + description: > > + The presence of this property implies that given RISC-V IOMMU us= es > > + 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 =3D "vendor,chip-iommu", "riscv,iommu"; > > + reg =3D <0x1bccd000 0x1000>; > > + interrupt-parent =3D <&aplic_smode>; > > + interrupts =3D <32 4>, <33 4>, <34 4>, <35 4>; > > + #iommu-cells =3D <2>; > > + }; > > + > > + /* Device with two IOMMU device IDs, 0 and 7 */ > > + master1 { > > + iommus =3D <&immu1 0 1>, <&immu1 7 1>; > > + }; > > + > > + - | > > + /* Example 2 (IOMMU platform device with MSIs) */ > > + immu2: iommu@1bcdd000 { > > + compatible =3D "vendor,chip-iommu", "riscv,iommu"; > > + reg =3D <0x1bccd000 0x1000>; > > + msi-parent =3D <&imsics_smode>; > > + #iommu-cells =3D <2>; > > + }; > > + > > + bus { > > + #address-cells =3D <2>; > > + #size-cells =3D <2>; > > + > > + /* Device with IOMMU device IDs ranging from 32 to 64 */ > > + master1 { > > + iommus =3D <&immu2 32 32>; > > + }; > > + > > + pcie@40000000 { > > + compatible =3D "pci-host-cam-generic"; > > + device_type =3D "pci"; > > + #address-cells =3D <3>; > > + #size-cells =3D <2>; > > + bus-range =3D <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