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

From: Robin Murphy
Date: Tue Feb 16 2021 - 10:11:45 EST

On 2021-02-10 19:21, Rob Herring wrote:
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"

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.