Hi,

Please see my comments inline.

On Fri, May 15, 2015 at 6:43 PM, Yong Wu [off-list ref] wrote:

nit: Could you follow one of existing styles of DT binding
documentation? You might be able to use [1] as an example.

[1] http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/iommu/arm,smmu.txt
.

+              EMI (External Memory Interface)
+               |
+              m4u (Multimedia Memory Management Unit)
+               |
+              smi (Smart Multimedia Interface)
+               |
+        +---------------+-------
+        |               |
+        |               |
+    vdec larb       disp larb      ... SoCs have different local arbiter(larb).
+        |               |
+        |               |
+   +----+----+    +-----+-----+
+   |    |    |    |     |     |    ...
+   |    |    |    |     |     |    ...
+   |    |    |    |     |     |    ...
+  MC   PP   VLD  OVL0 RDMA0 WDMA0  ... There are different ports in each larb.
+

This diagram is still quite meaningless without proper description of
all the functionality off all the blocks and interfaces between them.

+Required properties:
+- compatible : must be "mediatek,mt8173-m4u".
+- reg : m4u register base and size.
+- interrupts : the interrupt of m4u.
+- clocks : must contain one entry for each clock-names.
+- clock-names : must be "bclk", It is the block clock of m4u.
+- larb-portes-nr : must contain the number of the portes for each larb(local
+       arbiter). The number is defined in dt-binding/iommu/mt8173-iommu-port.h.

typo: Should it be "ports" instead of "portes"?

Anyway, shouldn't this be a property of larb nodes? I.e. the larb0
node would have a port-count property set to M4U_LARB0_PORT_NR.

+- larb : must contain the local arbiters of the current platform. Refer to
+       bindings/soc/mediatek/mediatek,smi.txt. It must sort according to the
+       local arbiter index, like larb0, larb1, larb2...
+- iommu-cells : must be 1. Specifies the client PortID as defined in
+       dt-binding/iommu/mt8173-iommu-port.h

Looking at the driver, wouldn't a 2 cell system be more appropriate
here? With first cell indexing the larbs and second the ports within
that larb.

[snip]

+#define M4U_LARB0_PORT_NR      8
+#define M4U_LARB1_PORT_NR      10
+#define M4U_LARB2_PORT_NR      21
+#define M4U_LARB3_PORT_NR      15
+#define M4U_LARB4_PORT_NR      6
+#define M4U_LARB5_PORT_NR      9
+
+#define M4U_LARB0_PORT(n)      (n)
+#define M4U_LARB1_PORT(n)      ((n) + M4U_LARB0_PORT_NR + M4U_LARB0_PORT(0))
+#define M4U_LARB2_PORT(n)      ((n) + M4U_LARB1_PORT_NR + M4U_LARB1_PORT(0))
+#define M4U_LARB3_PORT(n)      ((n) + M4U_LARB2_PORT_NR + M4U_LARB2_PORT(0))
+#define M4U_LARB4_PORT(n)      ((n) + M4U_LARB3_PORT_NR + M4U_LARB3_PORT(0))
+#define M4U_LARB5_PORT(n)      ((n) + M4U_LARB4_PORT_NR + M4U_LARB4_PORT(0))

This looks like some artificial indexing. Does it have any
correspondence with actual hardware configuration?

+
+/* larb0 */
+#define        M4U_PORT_DISP_OVL0              M4U_LARB0_PORT(0)
+#define        M4U_PORT_DISP_RDMA0             M4U_LARB0_PORT(1)
+#define        M4U_PORT_DISP_WDMA0             M4U_LARB0_PORT(2)

[snip]

+#define        M4U_PORT_VENC_CUR_CHROMA_SET2   M4U_LARB5_PORT(6)
+#define        M4U_PORT_VENC_RD_COMA_SET2      M4U_LARB5_PORT(7)
+#define        M4U_PORT_VENC_SV_COMA_SET2      M4U_LARB5_PORT(8)

This convinces me even more that this binding actually needs
#iommu-cells to be 2 and the indexing system I described above.

Best regards,
Tomasz