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

[Chunyan Zhang <zhang.lyra@gmail.com>]

On Tue, 16 Feb 2021 at 23:10, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-02-10 19:21, Rob Herring wrote:
> > On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> >>
> >> Hi Rob,
> >>
> >> On Fri, 5 Feb 2021 at 07:25, Rob Herring <robh@kernel.org> wrote:
> >>>
> >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> >>>> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >>>>
> >>>> 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@unisoc.com>
> >>>> ---
> >>>>   .../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@gmail.com>
> >>>> +
> >>>> +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"
>
> OK, so apparently the hardware is not quite as trivial as my initial
> impression, and you should have interrupts as well.

Ok, I will have a look.

>
> >>>> +
> >>>> +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.
>
> Having stumbled across the DRM driver patches the other day, I now see
> where this is coming from, and it's even worse than that - this whole
> binding seems to be largely working around bad driver design.
>

I guess you mean bad h/w design (not bad driver design)?
Having this syscon node just because I don't want a same register
range to be mapped to multiple virtual address ranges, and that's the
case for many media devices and their IOMMUs. If this issue exsists
for one device only, I can even endure, but that's not unfortunately.
But anyway, as you all think this is not a good way, I will change to
use reg property.

> >> 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>;
>
> In fact judging by the other driver it looks like the length only needs
> to be 0x200 here (but maybe there's more to come in future).
>
> >> +                               iommus = <&iommu_dispc>;
> >> +                       };
> >> +
> >> +                       iommu_dispc: iommu@63000000 {
> >> +                               compatible = "sprd,iommu-v1";
> >> +                               sprd,iommu-regs = <&dpu_regs 0x800>;
> >
> > reg = <0 0x63000800 0 0x800>;
>
> ...and this one looks to need less than 0x80, even :)

There're some registers not be added in the current driver indeed. The
specification defines registers up to 0x7c.

>
> >
> >> +                               #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.
>
> FWIW that's really a very different situation - the MMUs in a Mali GPU
> are fundamental parts of its internal pipelines and would never make
> sense to handle as separate devices (if it were even feasible to try).
> An IOMMU like this one is typically a logically-distinct block stuck to
> the external bus interface of any old device, rewriting transactions
> that said device has already issued - it's telling that it needs to
> allocate the prot_page scratchpad for "faulting" transactions to still
> flow somewhere, implying that it's not even involved enough to be able
> to terminate them.
>
> As such I think it *does* make complete sense to describe even
> "dedicated" IOMMUs like this one, Rockchip, Exynos, etc. in DT.
> Otherwise you'd be effectively forcing OSes to turn half their
> display/media drivers into mini board files with secret knowledge of
> which blocks are integrated with IOMMUs on which SoCs.
>

 Thanks for helping me explain the situation.

Regards,
Chunyan

> Robin.