| Message ID | 1431683009-18158-2-git-send-email-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Please see my comments inline. On Fri, May 15, 2015 at 6:43 PM, Yong Wu <yong.wu@mediatek.com> wrote: > This patch add mediatek iommu dts binding document. > > Signed-off-by: Yong Wu <yong.wu@mediatek.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
Hi Tomasz, Thanks very much for your suggestion!. please help check my comment. On Mon, 2015-05-25 at 15:31 +0900, Tomasz Figa wrote: > Hi, > > Please see my comments inline. > > On Fri, May 15, 2015 at 6:43 PM, Yong Wu <yong.wu@mediatek.com> wrote: > > This patch add mediatek iommu dts binding document. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.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 How about below: * Mediatek IOMMU Architecture Implementation Mediatek Socs may contain a implementation of Multimedia Memory Management Unit(M4U),which use ARM Short-descriptor translation table to achieve address translation. The IOMMU Hardware Block Diagram, please check below: > . > > > + EMI (External Memory Interface) > > + | > > + m4u (Multimedia Memory Management Unit) > > + | > > + smi (Smart Multimedia Interface) > > + | > > + +---------------+------- > > + | | > > + | | > > + vdec larb disp larb ... SoCs have different smi 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. How about it if like this: //======== As above, The Multimedia HW will go through SMI and M4U while it access EMI. SMI is a brige between m4u and the Multimedia HW. It contain smi local arbiter and smi common. It will control whether the Multimedia HW should go though the m4u for translation or bypass it and talk directly with EMI.And also SMI will help control the clocks for each local arbiter. About smi local arbiter(larb), Normally we specify a local arbiter(larb) for each multimedia HW like display, video decode, and camera. And there are different ports in each larb. Take a example, There are some ports like MC, PP, VLD, AVC_MV, PRED_RD, PRED_WR in the video local arbiter, all the ports are according to the video HW. //====== > > > +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. Thanks very much. This is very helpful! if we use 2, we can enter smi driver to enable/disable iommu easily. and the code will be easier to read. Then How about the descriptor if I write like this: //==== iommu-cells : must be 2.There are 2 cell needed to enable/disable iommu. The first is local arbiter index(larbid), and the second is port index(portid) within local arbiter. Specifies the larbid and portid as defined in dt-binding/iommu/mt8173-iommu-port.h. //==== Is it ok? > > [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? No. We don't have the correspondence hardware config about this. we only find out the local arbiter id and port id according to the M4U_LARB0_PORT,M4U_LARB1_PORT,... So if we use 2 in the iommu-cells. we don't need to remember the port id offset for each local arbiter. I will delete them. Then I could define like this. #define M4U_LARB0_ID (0) #define M4U_LARB1_ID (1) #define M4U_LARB2_ID (2) /* larb0 */ #define M4U_PORT_DISP_OVL0 (0) #define M4U_PORT_DISP_RDMA0 (1) #define M4U_PORT_DISP_WDMA0 (2) ... /* larb1 */ #define M4U_PORT_HW_VDEC_MC_EXT (0) #define M4U_PORT_HW_VDEC_PP_EXT (1) ... Then we could write like this in the client's dtsi. vdec:vdec@xxxx{ reg = <xxxxx>; iommu = <&iommu M4U_LARB1_ID M4U_PORT_HW_VDEC_MC_EXT>, <&iommu M4U_LARB1_ID M4U_PORT_HW_VDEC_PP_EXT>; } Is it ok? > > > + > > +/* 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
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 */ +/******************************************************/ + 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. + +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. +- 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 + +Example: + iommu: mmsys_iommu@10205000 { + compatible = "mediatek,mt8173-m4u"; + reg = <0 0x10205000 0 0x1000>; + interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>; + clocks = <&infrasys INFRA_M4U>; + clock-names = "bclk"; + larb-portes-nr = <M4U_LARB0_PORT_NR + M4U_LARB1_PORT_NR + M4U_LARB2_PORT_NR + M4U_LARB3_PORT_NR + M4U_LARB4_PORT_NR + M4U_LARB5_PORT_NR>; + larb = <&larb0 &larb1 &larb2 &larb3 &larb4 &larb5>; + #iommu-cells = <1>; + }; \ No newline at end of file diff --git a/include/dt-bindings/iommu/mt8173-iommu-port.h b/include/dt-bindings/iommu/mt8173-iommu-port.h new file mode 100644 index 0000000..09bac4f --- /dev/null +++ b/include/dt-bindings/iommu/mt8173-iommu-port.h @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2014-2015 MediaTek Inc. + * Author: Yong Wu <yong.wu@mediatek.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef __DTS_IOMMU_PORT_MT8173_H +#define __DTS_IOMMU_PORT_MT8173_H + +#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)) + +/* 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) +#define M4U_PORT_DISP_OD_R M4U_LARB0_PORT(3) +#define M4U_PORT_DISP_OD_W M4U_LARB0_PORT(4) +#define M4U_PORT_MDP_RDMA0 M4U_LARB0_PORT(5) +#define M4U_PORT_MDP_WDMA M4U_LARB0_PORT(6) +#define M4U_PORT_MDP_WROT0 M4U_LARB0_PORT(7) + +/* larb1 */ +#define M4U_PORT_HW_VDEC_MC_EXT M4U_LARB1_PORT(0) +#define M4U_PORT_HW_VDEC_PP_EXT M4U_LARB1_PORT(1) +#define M4U_PORT_HW_VDEC_UFO_EXT M4U_LARB1_PORT(2) +#define M4U_PORT_HW_VDEC_VLD_EXT M4U_LARB1_PORT(3) +#define M4U_PORT_HW_VDEC_VLD2_EXT M4U_LARB1_PORT(4) +#define M4U_PORT_HW_VDEC_AVC_MV_EXT M4U_LARB1_PORT(5) +#define M4U_PORT_HW_VDEC_PRED_RD_EXT M4U_LARB1_PORT(6) +#define M4U_PORT_HW_VDEC_PRED_WR_EXT M4U_LARB1_PORT(7) +#define M4U_PORT_HW_VDEC_PPWRAP_EXT M4U_LARB1_PORT(8) +#define M4U_PORT_HW_VDEC_TILE M4U_LARB1_PORT(9) + +/* larb2 */ +#define M4U_PORT_IMGO M4U_LARB2_PORT(0) +#define M4U_PORT_RRZO M4U_LARB2_PORT(1) +#define M4U_PORT_AAO M4U_LARB2_PORT(2) +#define M4U_PORT_LCSO M4U_LARB2_PORT(3) +#define M4U_PORT_ESFKO M4U_LARB2_PORT(4) +#define M4U_PORT_IMGO_D M4U_LARB2_PORT(5) +#define M4U_PORT_LSCI M4U_LARB2_PORT(6) +#define M4U_PORT_LSCI_D M4U_LARB2_PORT(7) +#define M4U_PORT_BPCI M4U_LARB2_PORT(8) +#define M4U_PORT_BPCI_D M4U_LARB2_PORT(9) +#define M4U_PORT_UFDI M4U_LARB2_PORT(10) +#define M4U_PORT_IMGI M4U_LARB2_PORT(11) +#define M4U_PORT_IMG2O M4U_LARB2_PORT(12) +#define M4U_PORT_IMG3O M4U_LARB2_PORT(13) +#define M4U_PORT_VIPI M4U_LARB2_PORT(14) +#define M4U_PORT_VIP2I M4U_LARB2_PORT(15) +#define M4U_PORT_VIP3I M4U_LARB2_PORT(16) +#define M4U_PORT_LCEI M4U_LARB2_PORT(17) +#define M4U_PORT_RB M4U_LARB2_PORT(18) +#define M4U_PORT_RP M4U_LARB2_PORT(19) +#define M4U_PORT_WR M4U_LARB2_PORT(20) + +/* larb3 */ +#define M4U_PORT_VENC_RCPU M4U_LARB3_PORT(0) +#define M4U_PORT_VENC_REC M4U_LARB3_PORT(1) +#define M4U_PORT_VENC_BSDMA M4U_LARB3_PORT(2) +#define M4U_PORT_VENC_SV_COMV M4U_LARB3_PORT(3) +#define M4U_PORT_VENC_RD_COMV M4U_LARB3_PORT(4) +#define M4U_PORT_JPGENC_RDMA M4U_LARB3_PORT(5) +#define M4U_PORT_JPGENC_BSDMA M4U_LARB3_PORT(6) +#define M4U_PORT_JPGDEC_WDMA M4U_LARB3_PORT(7) +#define M4U_PORT_JPGDEC_BSDMA M4U_LARB3_PORT(8) +#define M4U_PORT_VENC_CUR_LUMA M4U_LARB3_PORT(9) +#define M4U_PORT_VENC_CUR_CHROMA M4U_LARB3_PORT(10) +#define M4U_PORT_VENC_REF_LUMA M4U_LARB3_PORT(11) +#define M4U_PORT_VENC_REF_CHROMA M4U_LARB3_PORT(12) +#define M4U_PORT_VENC_NBM_RDMA M4U_LARB3_PORT(13) +#define M4U_PORT_VENC_NBM_WDMA M4U_LARB3_PORT(14) + +/* larb4 */ +#define M4U_PORT_DISP_OVL1 M4U_LARB4_PORT(0) +#define M4U_PORT_DISP_RDMA1 M4U_LARB4_PORT(1) +#define M4U_PORT_DISP_RDMA2 M4U_LARB4_PORT(2) +#define M4U_PORT_DISP_WDMA1 M4U_LARB4_PORT(3) +#define M4U_PORT_MDP_RDMA1 M4U_LARB4_PORT(4) +#define M4U_PORT_MDP_WROT1 M4U_LARB4_PORT(5) + +/* larb5 */ +#define M4U_PORT_VENC_RCPU_SET2 M4U_LARB5_PORT(0) +#define M4U_PORT_VENC_REC_FRM_SET2 M4U_LARB5_PORT(1) +#define M4U_PORT_VENC_REF_LUMA_SET2 M4U_LARB5_PORT(2) +#define M4U_PORT_VENC_REC_CHROMA_SET2 M4U_LARB5_PORT(3) +#define M4U_PORT_VENC_BSDMA_SET2 M4U_LARB5_PORT(4) +#define M4U_PORT_VENC_CUR_LUMA_SET2 M4U_LARB5_PORT(5) +#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) + +#endif
This patch add mediatek iommu dts binding document. Signed-off-by: Yong Wu <yong.wu@mediatek.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