Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

[Rob Herring]

On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
>
> Hi Rob,
>
> On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> > > From: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > >
> > > This iommu module can be used by Unisoc's multimedia devices, such as
> > > display, Image codec(jpeg) and a few signal processors, including
> > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > > ---
> > >  .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..4fc99e81fa66
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > @@ -0,0 +1,72 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright 2020 Unisoc Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Unisoc IOMMU and Multi-media MMU
> > > +
> > > +maintainers:
> > > +  - Chunyan Zhang <zhang.lyra@xxxxxxxxx>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - sprd,iommu-v1
> > > +
> > > +  "#iommu-cells":
> > > +    const: 0
> > > +    description:
> > > +      Unisoc IOMMUs are all single-master IOMMU devices, therefore no
> > > +      additional information needs to associate with its master device.
> > > +      Please refer to the generic bindings document for more details,
> > > +      Documentation/devicetree/bindings/iommu/iommu.txt
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +    description:
> > > +      Not required if 'sprd,iommu-regs' is defined.
> > > +
> > > +  clocks:
> > > +    description:
> > > +      Reference to a gate clock phandle, since access to some of IOMMUs are
> > > +      controlled by gate clock, but this is not required.
> > > +
> > > +  sprd,iommu-regs:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +    description:
> > > +      Reference to a syscon phandle plus 1 cell, the syscon defines the
> > > +      register range used by the iommu and the media device, the cell
> > > +      defines the offset for iommu registers. Since iommu module shares
> > > +      the same register range with the media device which uses it.
> > > +
> > > +required:
> > > +  - compatible
> > > +  - "#iommu-cells"
> > > +
> > > +oneOf:
> > > +  - required:
> > > +      - reg
> > > +  - required:
> > > +      - sprd,iommu-regs
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    iommu_disp: iommu-disp {
> > > +      compatible = "sprd,iommu-v1";
> > > +      sprd,iommu-regs = <&dpu_regs 0x800>;
> >
> > If the IOMMU is contained within another device, then it should just be
> > a child node of that device.
>
> Yes, actually IOMMU can be seen as a child of multimedia devices, I
> considered moving IOMMU under into multimedia device node, but
> multimedia devices need IOMMU when probe[1], so I dropped that idea.

Don't design your binding around working-around linux issues.

> And they share the same register base, e.g.
>
> +               mm {
> +                       compatible = "simple-bus";
> +                       #address-cells = <2>;
> +                       #size-cells = <2>;
> +                       ranges;
> +
> +                       dpu_regs: syscon@63000000 {

Drop this node.

> +                               compatible = "sprd,sc9863a-dpuregs", "syscon";
> +                               reg = <0 0x63000000 0 0x1000>;
> +                       };
> +
> +                       dpu: dpu@63000000 {
> +                               compatible = "sprd,sharkl3-dpu";
> +                               sprd,disp-regs = <&dpu_regs>;

reg = <0 0x63000000 0 0x800>;

> +                               iommus = <&iommu_dispc>;
> +                       };
> +
> +                       iommu_dispc: iommu@63000000 {
> +                               compatible = "sprd,iommu-v1";
> +                               sprd,iommu-regs = <&dpu_regs 0x800>;

reg = <0 0x63000800 0 0x800>;

> +                               #iommu-cells = <0>;

Though given it seems there is only 1 client and this might really be
just 1 h/w block, you don't really need to use the iommu binding at
all. The DPU should be able to instantiate it's own IOMMU device.
There's other examples of this such as mali GPU though that is all one
driver, but that's a Linux implementation detail.

Rob