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

[Robin Murphy <robin.murphy@arm.com>]

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.

>>>> +
>>>> +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.

>> 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 :)

> 
>> +                               #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.

Robin.