linux-kernel - Re: [PATCH v2 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAAFQd5AF5UVb35PEg+GjzLAqqr0q=Wg+XONRmK4XR=HLuJv2rA@mail.gmail.com>
Date:	Mon, 25 May 2015 15:31:48 +0900
From:	Tomasz Figa <tfiga@...omium.org>
To:	Yong Wu <yong.wu@...iatek.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Joerg Roedel <joro@...tes.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Robin Murphy <robin.murphy@....com>,
	Will Deacon <will.deacon@....com>,
	Daniel Kurtz <djkurtz@...gle.com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Mark Rutland <mark.rutland@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	linux-mediatek@...ts.infradead.org,
	Sasha Hauer <kernel@...gutronix.de>,
	srv_heupstream@...iatek.com, devicetree@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
	pebolle@...cali.nl, arnd@...db.de, mitchelh@...eaurora.org,
	k.zhang@...iatek.com, youhua.li@...iatek.com
Subject: Re: [PATCH v2 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU

Hi,

Please see my comments inline.

On Fri, May 15, 2015 at 6:43 PM, Yong Wu <yong.wu@...iatek.com> wrote:
> This patch add mediatek iommu dts binding document.
>
> Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> ---
>  .../devicetree/bindings/iommu/mediatek,iommu.txt   |  51 ++++++++++
>  include/dt-bindings/iommu/mt8173-iommu-port.h      | 112 +++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
>  create mode 100644 include/dt-bindings/iommu/mt8173-iommu-port.h
>
> diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> new file mode 100644
> index 0000000..f2cc7c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> @@ -0,0 +1,51 @@
> +/******************************************************/
> +/*    Mediatek IOMMU Hardware Block Diagram           */
> +/******************************************************/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/