diff mbox

[RFC,v2,1/4] dt-bindings: drm/mediatek: Add Mediatek display subsystem dts binding

Message ID 1442592722-29004-2-git-send-email-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel Sept. 18, 2015, 4:11 p.m. UTC
From: CK Hu <ck.hu@mediatek.com>

Add device tree binding documentation for the display subsystem in
Mediatek MT8173 SoCs.

Signed-off-by: CK Hu <ck.hu@mediatek.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../bindings/drm/mediatek/mediatek,disp.txt        | 131 +++++++++++++++++++++
 .../bindings/drm/mediatek/mediatek,dsi.txt         |  29 +++++
 2 files changed, 160 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
 create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt

Comments

Rob Herring (Arm) Sept. 18, 2015, 8:33 p.m. UTC | #1
On Fri, Sep 18, 2015 at 11:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> From: CK Hu <ck.hu@mediatek.com>
>
> Add device tree binding documentation for the display subsystem in
> Mediatek MT8173 SoCs.
>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../bindings/drm/mediatek/mediatek,disp.txt        | 131 +++++++++++++++++++++
>  .../bindings/drm/mediatek/mediatek,dsi.txt         |  29 +++++
>  2 files changed, 160 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
>  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> new file mode 100644
> index 0000000..a3811bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> @@ -0,0 +1,131 @@
> +Mediatek display subsystem
> +==========================
> +
> +The Mediatek display subsystem consists of various DISP function blocks in the
> +MMSYS register space. The connections between them can be configured by output
> +and input selectors in the MMSYS_CONFIG register space and register updates can
> +be synchronized to video frame boundaries with help of a DISP_MUTEX function
> +block.
> +
> +The display-subsystem node binds together all individual device nodes that
> +comprise the DISP subsystem.
> +
> +Required properties:
> +
> +- compatible: "mediatek,<chip>-disp"

> +- components: Should contain a list of phandles pointing to the DISP function
> +  block device nodes.
> +- component-names: Should contain the name of the function block pointed to
> +  by the components phandle of the same index.

NAK. Group these nodes under a parent node, use of-graph or just don't
put this into DT. Don't invent a new way.


> +- mmsys-config: Should contain a phandle pointing to the MMSYS node.
> +- disp-mutex: Should contain a phandle pointing to the DISP_MUTEX node.
> +
> +Example:
> +
> +display-subsystem {
> +       compatible = "mediatek,mt8173-disp";
> +       components = <&ovl0>, <&rdma0>, <&color0>, <&aal>,
> +                    <&ufoe>, <&dsi0>, <&od>;
> +       component-names = "ovl0", "rdma0", "color0", "aal",
> +                         "ufoe", "dsi0", "od";
> +       mmsys-config = <&mmsys>;
> +       disp-mutex = <&mutex>;
> +};
> +
> +DISP function blocks
> +====================
> +
> +A display stream starts at a source function block that reads pixel data from
> +memory and ends with a sink function block that drives pixels on a display
> +interface, or writes pixels back to memory. All DISP function blocks have
> +their own register space, interrupt, and clock gate. The blocks that can
> +access memory additionally have to list the IOMMU and local arbiter they are
> +connected to.
> +
> +For a description of the display interface sink function blocks, see
> +Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> +
> +Required properties (all function blocks):
> +- compatible: "mediatek,<chip>-disp-<function>", one of
> +       "mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
> +       "mediatek,<chip>-disp-rdma"  - read DMA / line buffer
> +       "mediatek,<chip>-disp-color" - color processor
> +       "mediatek,<chip>-disp-aal"   - adaptive ambient light controller
> +       "mediatek,<chip>-disp-gamma" - gamma correction
> +       "mediatek,<chip>-disp-ufoe"  - data compression engine
> +       "mediatek,<chip>-disp-mutex" - display mutex
> +       "mediatek,<chip>-disp-od"    - overdrive
> +- reg: Physical base address and length of the function block register space
> +- interrupts: The interrupt signal from the function block.
> +- clocks: device clocks
> +  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> +- compatible: "mediatek,<chip>-ddp"
> +- power-domains: a phandle to DDP power domain node.
> +
> +Required properties (DMA function blocks):
> +- compatible: Should be one of
> +       "mediatek,<chip>-disp-ovl"
> +       "mediatek,<chip>-disp-rdma"
> +- larb: Should a phandles pointing to the local arbiter device as defined in
> +  Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
> +- iommus: required a iommu node
> +
> +Examples:
> +
> +ovl0: ovl@1400c000 {
> +       compatible = "mediatek,mt8173-disp-ovl";
> +       reg = <0 0x1400c000 0 0x1000>;
> +       interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_OVL0>;
> +       iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_OVL0>;
> +};
> +
> +rdma0: rdma@1400e000 {
> +       compatible = "mediatek,mt8173-disp-rdma";
> +       reg = <0 0x1400e000 0 0x1000>;
> +       interrupts = <GIC_SPI 182 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_RDMA0>;
> +       iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_RDMA0>;
> +};
> +
> +color0: color@14013000 {
> +       compatible = "mediatek,mt8173-disp-color";
> +       reg = <0 0x14013000 0 0x1000>;
> +       interrupts = <GIC_SPI 187 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_COLOR0>;
> +};
> +
> +aal: aal@14015000 {
> +       compatible = "mediatek,mt8173-disp-aal";
> +       reg = <0 0x14015000 0 0x1000>;
> +       interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_AAL>;
> +};
> +
> +gamma: gamma@14016000 {
> +       compatible = "mediatek,mt8173-disp-gamma";
> +       reg = <0 0x14016000 0 0x1000>;
> +       interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_GAMMA>;
> +};
> +
> +ufoe: ufoe@1401a000 {
> +       compatible = "mediatek,mt8173-disp-ufoe";
> +       reg = <0 0x1401a000 0 0x1000>;
> +       interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_LOW>;
> +       clocks = <&mmsys CLK_MM_DISP_UFOE>;
> +};
> +
> +mutex: mutex@14020000 {
> +       compatible = "mediatek,mt8173-disp-mutex";
> +       reg = <0 0x14020000 0 0x1000>;
> +       interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_LOW>;
> +       power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> +       clocks = <&mmsys CLK_MM_MUTEX_32K>;
> +};
> +
> +od: od@14023000 {
> +       compatible = "mediatek,mt8173-disp-od";
> +       reg = <0 0x14023000 0 0x1000>;
> +       clocks = <&mmsys CLK_MM_DISP_OD>;
> +};
> diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> new file mode 100644
> index 0000000..e892ef1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> @@ -0,0 +1,29 @@
> +Mediatek DSI Device
> +===================
> +
> +The Mediatek DSI function block is a sink of the display subsystem and can
> +drive up to 4-lane MIPI DSI output. Two DSIs can be synchronized for dual-
> +channel output.
> +
> +Required properties:
> +- compatible: "mediatek,<chip>-dsi"
> +- reg: Physical base address and length of the controller's registers
> +- clocks: device clocks
> +  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> +- clock-names: must contain "engine" and "digital".

This leaves wondering which one is used for DSI bit clock.

> +
> +Example:
> +
> +dsi0: dsi@1401b000 {
> +       compatible = "mediatek,mt8173-dsi";
> +       reg = <0 0x1401b000 0 0x1000>,  /* DSI0 */
> +             <0 0x10215000 0 0x1000>;  /* MIPI_TX0 */
> +       clocks = <&mmsys MM_DSI0_ENGINE>, <&mmsys MM_DSI0_DIGITAL>;
> +       clock-names = "engine", "digital";
> +
> +       port {

Missing from the binding description.

> +               dsi0_out: endpoint {
> +                       remote-endpoint = <&panel_in>;
> +               };
> +       };
> +};
> --
> 2.5.1
>
Philipp Zabel Sept. 21, 2015, 8:11 a.m. UTC | #2
Hi Rob,

thank you for the comments.

Am Freitag, den 18.09.2015, 15:33 -0500 schrieb Rob Herring:
[...]
> > +The display-subsystem node binds together all individual device nodes that
> > +comprise the DISP subsystem.
> > +
> > +Required properties:
> > +
> > +- compatible: "mediatek,<chip>-disp"
> 
> > +- components: Should contain a list of phandles pointing to the DISP function
> > +  block device nodes.
> > +- component-names: Should contain the name of the function block pointed to
> > +  by the components phandle of the same index.
> 
> NAK. Group these nodes under a parent node, use of-graph or just don't
> put this into DT. Don't invent a new way.

Ok. The reason I haven't grouped all the display nodes together in the
first place is that they aren't contiguous in the register space. So
instead of:

	ovl@1400c000 {
		compatible = "mediatek,mt8173-disp-ovl";
	};
	...
	ufoe@1401a000 {
		compatible = "mediatek,mt8173-disp-ufoe";
	};

	pwm@1401e000 {
		compatible = "mediatek,mt8173-disp-pwm";
	};

	larb@14021000 {
		compatible = "mediatek,mt8173-smi-larb";
	};

	od@14023000 {
		compatible = "mediatek,mt8173-disp-od";
	};

We'd have:

	display-subsystem@1400c00 {
		compatible = "mediatek,mt8173-disp", "simple-bus";

		ovl@1400c000 {
			compatible = "mediatek,mt8173-disp-ovl";
		};
		...
		ufoe@1401a000 {
			compatible = "mediatek,mt8173-disp-ufoe";
		};

		od@14023000 {
			compatible = "mediatek,mt8173-disp-od";
		};
	};

	pwm@1401e000 {
		compatible = "mediatek,mt8173-disp-pwm";
	};

	larb@14021000 {
		compatible = "mediatek,mt8173-smi-larb";
	};

Note how the display-subsystem node overlaps the larb node. Is that
acceptable?

[...]
> > diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> > new file mode 100644
> > index 0000000..e892ef1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> > @@ -0,0 +1,29 @@
> > +Mediatek DSI Device
> > +===================
> > +
> > +The Mediatek DSI function block is a sink of the display subsystem and can
> > +drive up to 4-lane MIPI DSI output. Two DSIs can be synchronized for dual-
> > +channel output.
> > +
> > +Required properties:
> > +- compatible: "mediatek,<chip>-dsi"
> > +- reg: Physical base address and length of the controller's registers
> > +- clocks: device clocks
> > +  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> > +- clock-names: must contain "engine" and "digital".
> 
> This leaves wondering which one is used for DSI bit clock.

The MIPI_TX0 module controls the MIPI D-PHY. It contains a PLL that
provides the bit clock to the DSI module.

From the documentation, it looks to me like the AP_PLL_CON0[6] bit in
the mediatek,mt8173-apmixedsys region gates the 26 MHz reference clock
to MIPI_TX. It is enabled by default. Currently there is no gate clock
registered for that bit.
Can somebody confirm that this gate clock should be added to the
mediatek,mt8173-apmixedsys bindings?

> > +
> > +Example:
> > +
> > +dsi0: dsi@1401b000 {
> > +       compatible = "mediatek,mt8173-dsi";
> > +       reg = <0 0x1401b000 0 0x1000>,  /* DSI0 */
> > +             <0 0x10215000 0 0x1000>;  /* MIPI_TX0 */

Thinking about it, MIPI_TX0 is for PHY control. Should this be moved
into its own node and the phy bindings used to let the DSI driver find
it?

> > +       clocks = <&mmsys MM_DSI0_ENGINE>, <&mmsys MM_DSI0_DIGITAL>;
> > +       clock-names = "engine", "digital";
> > +
> > +       port {
> 
> Missing from the binding description.

Thanks, I'll fix that next round.

> > +               dsi0_out: endpoint {
> > +                       remote-endpoint = <&panel_in>;
> > +               };
> > +       };
> > +};

best regards
Philipp
Philipp Zabel Sept. 30, 2015, 3:30 p.m. UTC | #3
Am Freitag, den 18.09.2015, 15:33 -0500 schrieb Rob Herring:
> On Fri, Sep 18, 2015 at 11:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > From: CK Hu <ck.hu@mediatek.com>
> >
> > Add device tree binding documentation for the display subsystem in
> > Mediatek MT8173 SoCs.
> >
> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  .../bindings/drm/mediatek/mediatek,disp.txt        | 131 +++++++++++++++++++++
> >  .../bindings/drm/mediatek/mediatek,dsi.txt         |  29 +++++
> >  2 files changed, 160 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> > new file mode 100644
> > index 0000000..a3811bd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> > @@ -0,0 +1,131 @@
> > +Mediatek display subsystem
> > +==========================
> > +
> > +The Mediatek display subsystem consists of various DISP function blocks in the
> > +MMSYS register space. The connections between them can be configured by output
> > +and input selectors in the MMSYS_CONFIG register space and register updates can
> > +be synchronized to video frame boundaries with help of a DISP_MUTEX function
> > +block.
> > +
> > +The display-subsystem node binds together all individual device nodes that
> > +comprise the DISP subsystem.
> > +
> > +Required properties:
> > +
> > +- compatible: "mediatek,<chip>-disp"
> 
> > +- components: Should contain a list of phandles pointing to the DISP function
> > +  block device nodes.
> > +- component-names: Should contain the name of the function block pointed to
> > +  by the components phandle of the same index.
> 
> NAK. Group these nodes under a parent node, use of-graph or just don't
> put this into DT. Don't invent a new way.

If I connect the DISP nodes using of-graph bindings, the full graph will
look somewhat like this (including the currently unused function blocks,
all properties but ports and endpoints removed for brevity):

	ovl0@1400c000 {
		port {
			ovl0_to_color0: endpoint@0 {
				remote-endpoint = <&color0_from_ovl0>;
			};

			ovl0_to_wdma0: endpoint@1 {
				remote-endpoint = <&wdma0_from_ovl0>;
			};
		};
	};

	ovl1@1400d000 {
		port {
			ovl1_to_color1: endpoint@0 {
				remote-endpoint = <&color1_from_ovl1>;
			};

			ovl1_to_wdma1: endpoint@1 {
				remote-endpoint = <&wdma1_from_ovl1>;
			};
		};
	};

	rdma0@1400e000 {
		port@0 {
			rdma0_from_od: endpoint {
				remote-endpoint = <&od_to_rdma0>;
			};
		};

		port@1 {
			rdma0_to_ufoe: endpoint@0 {
				remote-endpoint = <&ufoe_from_rdma0>;
			};

			rdma0_to_split0: endpoint@1 {
				remote-endpoint = <&split0_from_rdma0>;
			};

			rdma0_to_color0: endpoint@2 {
				remote-endpoint = <&color0_from_rdma0>;
			};
		};
	};

	rdma1@1400f000 {
		port@0 {
			rdma1_from_gamma: endpoint {
				remote-endpoint = <&gamma_to_rdma1>;
			};
		};

		port@1 {
			rdma1_to_dsi0: endpoint@0 {
				remote-endpoint = <&dsi0_from_rdma1>;
			};

			rdma1_to_dsi1: endpoint@1 {
				remote-endpoint = <&dsi1_from_rdma1>;
			};

			rdma1_to_dpi0: endpoint@2 {
				remote-endpoint = <&dpi0_from_rdma1>;
			};

			rdma1_to_color1: endpoint@3 {
				remote-endpoint = <&color1_from_rdma1>;
			};
		};
	};

	rdma2@14010000 {
		port {
			rdma2_to_dsi1: endpoint@0 {
				remote-endpoint = <&dsi1_from_rdma2>;
			};

			rdma2_to_dpi0: endpoint@1 {
				remote-endpoint = <&dpi0_from_rdma2>;
			};
		};
	};

	wdma0@14011000 {
		port {
			wdma0_from_ovl0: endpoint@0 {
				remote-endpoint = <&ovl0_to_wdma0>;
			};

			wdma0_from_od: endpoint@1 {
				remote-endpoint = <&od_to_wdma0>;
			};

			wdma0_from_ufoe: endpoint@2 {
				remote-endpoint = <&ufoe_to_wdma0>;
			};
		};
	};

	wdma1@14012000 {
		port {
			wdma1_from_ovl1: endpoint@0 {
				remote-endpoint = <&ovl1_to_wdma1>;
			};

			wdma1_from_gamma: endpoint@1 {
				remote-endpoint = <&gamma_to_wdma1>;
			};
		};
	};

	color0@14013000 {
		port@0 {
			color0_from_rdma0: endpoint@0 {
				remote-endpoint = <&rdma0_to_color0>;
			};

			color0_from_ovl0: endpoint@1 {
				remote-endpoint = <&ovl0_to_color0>;
			};
		};

		port@1 {
			color0_to_aal: endpoint@0 {
				remote-endpoint = <&aal_from_color0>;
			};

			color0_to_merge: endpoint@1 {
				remote-endpoint = <&merge_from_color0>;
			};
		};
	};

	color1@14014000 {
		port@0 {
			color1_from_rdma1: endpoint@0 {
				remote-endpoint = <&rdma1_to_color1>;
			};

			color1_from_ovl1: endpoint@1 {
				remote-endpoint = <&ovl1_to_color1>;
			};
		};

		port@1 {
			color1_to_gamma: endpoint@0 {
				remote-endpoint = <&gamma_from_color1>;
			};

			color1_to_merge: endpoint@1 {
				remote-endpoint = <&merge_from_color1>;
			};
		};
	};

	aal@14015000 {
		port@0 {
			aal_from_color0: endpoint@0 {
				remote-endpoint = <&color0_to_aal>;
			};

			aal_from_merge: endpoint@1 {
				remote-endpoint = <&merge_to_aal>;
			};
		};

		port@1 {
			aal_to_od: endpoint {
				remote-endpoint = <&od_from_aal>;
			};
		};
	};

	gamma@14016000 {
		port@0 {
			gamma_from_color1: endpoint {
				remote-endpoint = <&color1_to_gamma>;
			};
		};

		port@1 {
			gamma_to_rdma1: endpoint@0 {
				remote-endpoint = <&rdma1_from_gamma>;
			};

			gamma_to_dsi0: endpoint@1 {
				remote-endpoint = <&dsi0_from_gamma>;
			};

			gamma_to_dsi1: endpoint@2 {
				remote-endpoint = <&dsi1_from_gamma>;
			};

			gamma_to_dpi0: endpoint@3 {
				remote-endpoint = <&dpi0_from_gamma>;
			};

			gamma_to_wdma1: endpoint@4 {
				remote-endpoint = <&wdma1_from_gamma>;
			};
		};
	};

	merge@14017000 {
		port@0 {
			merge_from_color0: endpoint {
				remote-endpoint = <&color0_to_merge>;
			};
		};

		port@1 {
			merge_from_color1: endpoint {
				remote-endpoint = <&color1_to_merge>;
			};
		};

		port@2 {
			merge_to_aal: endpoint {
				remote-endpoint = <&aal_from_merge>;
			};
		};
	};

	split0@14018000 {
		port@0 {
			split0_from_rdma0: endpoint@0 {
				remote-endpoint = <&rdma0_to_split0>;
			};

			split0_from_od: endpoint@1 {
				remote-endpoint = <&od_to_split0>;
			};
		};

		port@1 {
			split0_to_ufoe: endpoint {
				remote-endpoint = <&ufoe_from_split0>;
			};
		};

		port@2 {
			split0_to_dsi0: endpoint@0 {
				remote-endpoint = <&dsi0_from_split0>;
			};

			split0_to_dsi1: endpoint@1 {
				remote-endpoint = <&dsi1_from_split0>;
			};

			split0_to_dpi0: endpoint@2 {
				remote-endpoint = <&dpi0_from_split0>;
			};
		};
	};

	split1@14019000 {
		port@0 {
			split1_from_ufoe: endpoint {
				remote-endpoint = <&ufoe_to_split1>;
			};
		};

		port@1 {
			split1_to_dsi0: endpoint {
				remote-endpoint = <&dsi0_from_split1>;
			};
		};

		port@2 {
			split1_to_dsi1: endpoint {
				remote-endpoint = <&dsi1_from_split1>;
			};
		};
	};

	ufoe@1401a000 {
		port@0 {
			ufoe_from_rdma0: endpoint@0 {
				remote-endpoint = <&rdma0_to_ufoe>;
			};

			ufoe_from_od: endpoint@1 {
				remote-endpoint = <&od_to_ufoe>;
			};

			ufoe_from_split0: endpoint@2 {
				remote-endpoint = <&split0_to_ufoe>;
			};
		};

		port@1 {
			ufoe_to_dsi0: endpoint@0 {
				remote-endpoint = <&dsi0_from_ufoe>;
			};

			ufoe_to_split1: endpoint@1 {
				remote-endpoint = <&split1_from_ufoe>;
			};

			ufoe_to_dpi0: endpoint@2 {
				remote-endpoint = <&dpi0_from_ufoe>;
			};

			ufoe_to_wdma0: endpoint@3 {
				remote-endpoint = <&wdma0_from_ufoe>;
			};

			ufoe_to_dsi1: endpoint@4 {
				remote-endpoint = <&dsi1_from_ufoe>;
			};
		};
	};

	dsi0: dsi0@1401b000 {
		port@0 {
			dsi0_from_ufoe: endpoint@0 {
				remote-endpoint = <&ufoe_to_dsi0>;
			};

			dsi0_from_split1: endpoint@1 {
				remote-endpoint = <&split1_to_dsi0>;
			};

			dsi0_from_rdma1: endpoint@2 {
				remote-endpoint = <&rdma1_to_dsi0>;
			};

			dsi0_from_gamma: endpoint@3 {
				remote-endpoint = <&gamma_to_dsi0>;
			};

			dsi0_from_split0: endpoint@4 {
				remote-endpoint = <&split0_to_dsi0>;
			};
		};
	};

	dsi1: dsi1@1401c000 {
		port@0 {
			dsi1_from_split1: endpoint@0 {
				remote-endpoint = <&split1_to_dsi1>;
			};

			dsi1_from_rdma1: endpoint@1 {
				remote-endpoint = <&rdma1_to_dsi1>;
			};

			dsi1_from_gamma: endpoint@2 {
				remote-endpoint = <&gamma_to_dsi1>;
			};

			dsi1_from_split0: endpoint@3 {
				remote-endpoint = <&split0_to_dsi1>;
			};

			dsi1_from_rdma2: endpoint@4 {
				remote-endpoint = <&rdma2_to_dsi1>;
			};

			dsi1_from_ufoe: endpoint@5 {
				remote-endpoint = <&ufoe_to_dsi1>;
			};
		};
	};

	dpi0: dpi0@1401d000 {
		port@0 {
			dpi0_from_ufoe: endpoint@0 {
				remote-endpoint = <&ufoe_to_dpi0>;
			};

			dpi0_from_rdma1: endpoint@1 {
				remote-endpoint = <&rdma1_to_dpi0>;
			};

			dpi0_from_gamma: endpoint@2 {
				remote-endpoint = <&gamma_to_dpi0>;
			};

			dpi0_from_split0: endpoint@3 {
				remote-endpoint = <&split0_to_dpi0>;
			};

			dpi0_from_rdma2: endpoint@4 {
				remote-endpoint = <&rdma2_to_dpi0>;
			};
		};
	};

	od@14023000 {
		port@0 {
			od_from_aal: endpoint {
				remote-endpoint = <&aal_to_od>;
			};
		};

		port@1 {
			od_to_rdma0: endpoint@0 {
				remote-endpoint = <&rdma0_from_od>;
			};

			od_to_ufoe: endpoint@1 {
				remote-endpoint = <&ufoe_from_od>;
			};

			od_to_split0: endpoint@2 {
				remote-endpoint = <&split0_from_od>;
			};

			od_to_wdma0: endpoint@3 {
				remote-endpoint = <&wdma0_from_od>;
			};
		};
	};

Adding the graph to the mediatek oak-rev3 device tree increases the size
from currently 33 KiB to 41.5 KiB.
As a proponent of the of-graph bindings I certainly like to describe the
hardware connections using DT, but I'm not sure if maybe this is a bit
verbose.

regards
Philipp
Rob Herring (Arm) Sept. 30, 2015, 4:57 p.m. UTC | #4
On Wed, Sep 30, 2015 at 10:30 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 18.09.2015, 15:33 -0500 schrieb Rob Herring:
>> On Fri, Sep 18, 2015 at 11:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > From: CK Hu <ck.hu@mediatek.com>
>> >
>> > Add device tree binding documentation for the display subsystem in
>> > Mediatek MT8173 SoCs.
>> >
>> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
>> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> > ---
>> >  .../bindings/drm/mediatek/mediatek,disp.txt        | 131 +++++++++++++++++++++
>> >  .../bindings/drm/mediatek/mediatek,dsi.txt         |  29 +++++
>> >  2 files changed, 160 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
>> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
>> > new file mode 100644
>> > index 0000000..a3811bd
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
>> > @@ -0,0 +1,131 @@
>> > +Mediatek display subsystem
>> > +==========================
>> > +
>> > +The Mediatek display subsystem consists of various DISP function blocks in the
>> > +MMSYS register space. The connections between them can be configured by output
>> > +and input selectors in the MMSYS_CONFIG register space and register updates can
>> > +be synchronized to video frame boundaries with help of a DISP_MUTEX function
>> > +block.
>> > +
>> > +The display-subsystem node binds together all individual device nodes that
>> > +comprise the DISP subsystem.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: "mediatek,<chip>-disp"
>>
>> > +- components: Should contain a list of phandles pointing to the DISP function
>> > +  block device nodes.
>> > +- component-names: Should contain the name of the function block pointed to
>> > +  by the components phandle of the same index.
>>
>> NAK. Group these nodes under a parent node, use of-graph or just don't
>> put this into DT. Don't invent a new way.
>
> If I connect the DISP nodes using of-graph bindings, the full graph will
> look somewhat like this (including the currently unused function blocks,
> all properties but ports and endpoints removed for brevity):
>
>         ovl0@1400c000 {
>                 port {
>                         ovl0_to_color0: endpoint@0 {
>                                 remote-endpoint = <&color0_from_ovl0>;
>                         };
>
>                         ovl0_to_wdma0: endpoint@1 {
>                                 remote-endpoint = <&wdma0_from_ovl0>;
>                         };
>                 };
>         };
>
>         ovl1@1400d000 {
>                 port {
>                         ovl1_to_color1: endpoint@0 {
>                                 remote-endpoint = <&color1_from_ovl1>;
>                         };
>
>                         ovl1_to_wdma1: endpoint@1 {
>                                 remote-endpoint = <&wdma1_from_ovl1>;
>                         };
>                 };
>         };
>
>         rdma0@1400e000 {
>                 port@0 {
>                         rdma0_from_od: endpoint {
>                                 remote-endpoint = <&od_to_rdma0>;
>                         };
>                 };
>
>                 port@1 {
>                         rdma0_to_ufoe: endpoint@0 {
>                                 remote-endpoint = <&ufoe_from_rdma0>;
>                         };
>
>                         rdma0_to_split0: endpoint@1 {
>                                 remote-endpoint = <&split0_from_rdma0>;
>                         };
>
>                         rdma0_to_color0: endpoint@2 {
>                                 remote-endpoint = <&color0_from_rdma0>;
>                         };
>                 };
>         };
>
>         rdma1@1400f000 {
>                 port@0 {
>                         rdma1_from_gamma: endpoint {
>                                 remote-endpoint = <&gamma_to_rdma1>;
>                         };
>                 };
>
>                 port@1 {
>                         rdma1_to_dsi0: endpoint@0 {
>                                 remote-endpoint = <&dsi0_from_rdma1>;
>                         };
>
>                         rdma1_to_dsi1: endpoint@1 {
>                                 remote-endpoint = <&dsi1_from_rdma1>;
>                         };
>
>                         rdma1_to_dpi0: endpoint@2 {
>                                 remote-endpoint = <&dpi0_from_rdma1>;
>                         };
>
>                         rdma1_to_color1: endpoint@3 {
>                                 remote-endpoint = <&color1_from_rdma1>;
>                         };
>                 };
>         };
>
>         rdma2@14010000 {
>                 port {
>                         rdma2_to_dsi1: endpoint@0 {
>                                 remote-endpoint = <&dsi1_from_rdma2>;
>                         };
>
>                         rdma2_to_dpi0: endpoint@1 {
>                                 remote-endpoint = <&dpi0_from_rdma2>;
>                         };
>                 };
>         };
>
>         wdma0@14011000 {
>                 port {
>                         wdma0_from_ovl0: endpoint@0 {
>                                 remote-endpoint = <&ovl0_to_wdma0>;
>                         };
>
>                         wdma0_from_od: endpoint@1 {
>                                 remote-endpoint = <&od_to_wdma0>;
>                         };
>
>                         wdma0_from_ufoe: endpoint@2 {
>                                 remote-endpoint = <&ufoe_to_wdma0>;
>                         };
>                 };
>         };
>
>         wdma1@14012000 {
>                 port {
>                         wdma1_from_ovl1: endpoint@0 {
>                                 remote-endpoint = <&ovl1_to_wdma1>;
>                         };
>
>                         wdma1_from_gamma: endpoint@1 {
>                                 remote-endpoint = <&gamma_to_wdma1>;
>                         };
>                 };
>         };
>
>         color0@14013000 {
>                 port@0 {
>                         color0_from_rdma0: endpoint@0 {
>                                 remote-endpoint = <&rdma0_to_color0>;
>                         };
>
>                         color0_from_ovl0: endpoint@1 {
>                                 remote-endpoint = <&ovl0_to_color0>;
>                         };
>                 };
>
>                 port@1 {
>                         color0_to_aal: endpoint@0 {
>                                 remote-endpoint = <&aal_from_color0>;
>                         };
>
>                         color0_to_merge: endpoint@1 {
>                                 remote-endpoint = <&merge_from_color0>;
>                         };
>                 };
>         };
>
>         color1@14014000 {
>                 port@0 {
>                         color1_from_rdma1: endpoint@0 {
>                                 remote-endpoint = <&rdma1_to_color1>;
>                         };
>
>                         color1_from_ovl1: endpoint@1 {
>                                 remote-endpoint = <&ovl1_to_color1>;
>                         };
>                 };
>
>                 port@1 {
>                         color1_to_gamma: endpoint@0 {
>                                 remote-endpoint = <&gamma_from_color1>;
>                         };
>
>                         color1_to_merge: endpoint@1 {
>                                 remote-endpoint = <&merge_from_color1>;
>                         };
>                 };
>         };
>
>         aal@14015000 {
>                 port@0 {
>                         aal_from_color0: endpoint@0 {
>                                 remote-endpoint = <&color0_to_aal>;
>                         };
>
>                         aal_from_merge: endpoint@1 {
>                                 remote-endpoint = <&merge_to_aal>;
>                         };
>                 };
>
>                 port@1 {
>                         aal_to_od: endpoint {
>                                 remote-endpoint = <&od_from_aal>;
>                         };
>                 };
>         };
>
>         gamma@14016000 {
>                 port@0 {
>                         gamma_from_color1: endpoint {
>                                 remote-endpoint = <&color1_to_gamma>;
>                         };
>                 };
>
>                 port@1 {
>                         gamma_to_rdma1: endpoint@0 {
>                                 remote-endpoint = <&rdma1_from_gamma>;
>                         };
>
>                         gamma_to_dsi0: endpoint@1 {
>                                 remote-endpoint = <&dsi0_from_gamma>;
>                         };
>
>                         gamma_to_dsi1: endpoint@2 {
>                                 remote-endpoint = <&dsi1_from_gamma>;
>                         };
>
>                         gamma_to_dpi0: endpoint@3 {
>                                 remote-endpoint = <&dpi0_from_gamma>;
>                         };
>
>                         gamma_to_wdma1: endpoint@4 {
>                                 remote-endpoint = <&wdma1_from_gamma>;
>                         };
>                 };
>         };
>
>         merge@14017000 {
>                 port@0 {
>                         merge_from_color0: endpoint {
>                                 remote-endpoint = <&color0_to_merge>;
>                         };
>                 };
>
>                 port@1 {
>                         merge_from_color1: endpoint {
>                                 remote-endpoint = <&color1_to_merge>;
>                         };
>                 };
>
>                 port@2 {
>                         merge_to_aal: endpoint {
>                                 remote-endpoint = <&aal_from_merge>;
>                         };
>                 };
>         };
>
>         split0@14018000 {
>                 port@0 {
>                         split0_from_rdma0: endpoint@0 {
>                                 remote-endpoint = <&rdma0_to_split0>;
>                         };
>
>                         split0_from_od: endpoint@1 {
>                                 remote-endpoint = <&od_to_split0>;
>                         };
>                 };
>
>                 port@1 {
>                         split0_to_ufoe: endpoint {
>                                 remote-endpoint = <&ufoe_from_split0>;
>                         };
>                 };
>
>                 port@2 {
>                         split0_to_dsi0: endpoint@0 {
>                                 remote-endpoint = <&dsi0_from_split0>;
>                         };
>
>                         split0_to_dsi1: endpoint@1 {
>                                 remote-endpoint = <&dsi1_from_split0>;
>                         };
>
>                         split0_to_dpi0: endpoint@2 {
>                                 remote-endpoint = <&dpi0_from_split0>;
>                         };
>                 };
>         };
>
>         split1@14019000 {
>                 port@0 {
>                         split1_from_ufoe: endpoint {
>                                 remote-endpoint = <&ufoe_to_split1>;
>                         };
>                 };
>
>                 port@1 {
>                         split1_to_dsi0: endpoint {
>                                 remote-endpoint = <&dsi0_from_split1>;
>                         };
>                 };
>
>                 port@2 {
>                         split1_to_dsi1: endpoint {
>                                 remote-endpoint = <&dsi1_from_split1>;
>                         };
>                 };
>         };
>
>         ufoe@1401a000 {
>                 port@0 {
>                         ufoe_from_rdma0: endpoint@0 {
>                                 remote-endpoint = <&rdma0_to_ufoe>;
>                         };
>
>                         ufoe_from_od: endpoint@1 {
>                                 remote-endpoint = <&od_to_ufoe>;
>                         };
>
>                         ufoe_from_split0: endpoint@2 {
>                                 remote-endpoint = <&split0_to_ufoe>;
>                         };
>                 };
>
>                 port@1 {
>                         ufoe_to_dsi0: endpoint@0 {
>                                 remote-endpoint = <&dsi0_from_ufoe>;
>                         };
>
>                         ufoe_to_split1: endpoint@1 {
>                                 remote-endpoint = <&split1_from_ufoe>;
>                         };
>
>                         ufoe_to_dpi0: endpoint@2 {
>                                 remote-endpoint = <&dpi0_from_ufoe>;
>                         };
>
>                         ufoe_to_wdma0: endpoint@3 {
>                                 remote-endpoint = <&wdma0_from_ufoe>;
>                         };
>
>                         ufoe_to_dsi1: endpoint@4 {
>                                 remote-endpoint = <&dsi1_from_ufoe>;
>                         };
>                 };
>         };
>
>         dsi0: dsi0@1401b000 {
>                 port@0 {
>                         dsi0_from_ufoe: endpoint@0 {
>                                 remote-endpoint = <&ufoe_to_dsi0>;
>                         };
>
>                         dsi0_from_split1: endpoint@1 {
>                                 remote-endpoint = <&split1_to_dsi0>;
>                         };
>
>                         dsi0_from_rdma1: endpoint@2 {
>                                 remote-endpoint = <&rdma1_to_dsi0>;
>                         };
>
>                         dsi0_from_gamma: endpoint@3 {
>                                 remote-endpoint = <&gamma_to_dsi0>;
>                         };
>
>                         dsi0_from_split0: endpoint@4 {
>                                 remote-endpoint = <&split0_to_dsi0>;
>                         };
>                 };
>         };
>
>         dsi1: dsi1@1401c000 {
>                 port@0 {
>                         dsi1_from_split1: endpoint@0 {
>                                 remote-endpoint = <&split1_to_dsi1>;
>                         };
>
>                         dsi1_from_rdma1: endpoint@1 {
>                                 remote-endpoint = <&rdma1_to_dsi1>;
>                         };
>
>                         dsi1_from_gamma: endpoint@2 {
>                                 remote-endpoint = <&gamma_to_dsi1>;
>                         };
>
>                         dsi1_from_split0: endpoint@3 {
>                                 remote-endpoint = <&split0_to_dsi1>;
>                         };
>
>                         dsi1_from_rdma2: endpoint@4 {
>                                 remote-endpoint = <&rdma2_to_dsi1>;
>                         };
>
>                         dsi1_from_ufoe: endpoint@5 {
>                                 remote-endpoint = <&ufoe_to_dsi1>;
>                         };
>                 };
>         };
>
>         dpi0: dpi0@1401d000 {
>                 port@0 {
>                         dpi0_from_ufoe: endpoint@0 {
>                                 remote-endpoint = <&ufoe_to_dpi0>;
>                         };
>
>                         dpi0_from_rdma1: endpoint@1 {
>                                 remote-endpoint = <&rdma1_to_dpi0>;
>                         };
>
>                         dpi0_from_gamma: endpoint@2 {
>                                 remote-endpoint = <&gamma_to_dpi0>;
>                         };
>
>                         dpi0_from_split0: endpoint@3 {
>                                 remote-endpoint = <&split0_to_dpi0>;
>                         };
>
>                         dpi0_from_rdma2: endpoint@4 {
>                                 remote-endpoint = <&rdma2_to_dpi0>;
>                         };
>                 };
>         };
>
>         od@14023000 {
>                 port@0 {
>                         od_from_aal: endpoint {
>                                 remote-endpoint = <&aal_to_od>;
>                         };
>                 };
>
>                 port@1 {
>                         od_to_rdma0: endpoint@0 {
>                                 remote-endpoint = <&rdma0_from_od>;
>                         };
>
>                         od_to_ufoe: endpoint@1 {
>                                 remote-endpoint = <&ufoe_from_od>;
>                         };
>
>                         od_to_split0: endpoint@2 {
>                                 remote-endpoint = <&split0_from_od>;
>                         };
>
>                         od_to_wdma0: endpoint@3 {
>                                 remote-endpoint = <&wdma0_from_od>;
>                         };
>                 };
>         };
>
> Adding the graph to the mediatek oak-rev3 device tree increases the size
> from currently 33 KiB to 41.5 KiB.
> As a proponent of the of-graph bindings I certainly like to describe the
> hardware connections using DT, but I'm not sure if maybe this is a bit
> verbose.

We need a graph visualizer...

I think there has to be a balance with how much is put into the DT. If
the relationship between blocks is pretty fixed then it doesn't really
help much to put all this into DT. If every SOC or board can have
differing combinations of blocks or connections between blocks then
the graph makes sense.

Rob
Rob Herring (Arm) Sept. 30, 2015, 5:13 p.m. UTC | #5
On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Rob,
>
> thank you for the comments.
>
> Am Freitag, den 18.09.2015, 15:33 -0500 schrieb Rob Herring:
> [...]
>> > +The display-subsystem node binds together all individual device nodes that
>> > +comprise the DISP subsystem.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: "mediatek,<chip>-disp"
>>
>> > +- components: Should contain a list of phandles pointing to the DISP function
>> > +  block device nodes.
>> > +- component-names: Should contain the name of the function block pointed to
>> > +  by the components phandle of the same index.
>>
>> NAK. Group these nodes under a parent node, use of-graph or just don't
>> put this into DT. Don't invent a new way.
>
> Ok. The reason I haven't grouped all the display nodes together in the
> first place is that they aren't contiguous in the register space. So
> instead of:
>
>         ovl@1400c000 {
>                 compatible = "mediatek,mt8173-disp-ovl";
>         };
>         ...
>         ufoe@1401a000 {
>                 compatible = "mediatek,mt8173-disp-ufoe";
>         };
>
>         pwm@1401e000 {
>                 compatible = "mediatek,mt8173-disp-pwm";
>         };
>
>         larb@14021000 {
>                 compatible = "mediatek,mt8173-smi-larb";
>         };
>
>         od@14023000 {
>                 compatible = "mediatek,mt8173-disp-od";
>         };
>
> We'd have:
>
>         display-subsystem@1400c00 {
>                 compatible = "mediatek,mt8173-disp", "simple-bus";
>
>                 ovl@1400c000 {
>                         compatible = "mediatek,mt8173-disp-ovl";
>                 };
>                 ...
>                 ufoe@1401a000 {
>                         compatible = "mediatek,mt8173-disp-ufoe";
>                 };
>
>                 od@14023000 {
>                         compatible = "mediatek,mt8173-disp-od";
>                 };
>         };
>
>         pwm@1401e000 {
>                 compatible = "mediatek,mt8173-disp-pwm";
>         };
>
>         larb@14021000 {
>                 compatible = "mediatek,mt8173-smi-larb";
>         };
>
> Note how the display-subsystem node overlaps the larb node. Is that
> acceptable?

Given what the graph looks like, perhaps. However, do you really need
a container node? It only serves to provide a list of nodes (e.g. all
the children) to include as components. There are other ways to
determine this list. You could find all nodes just searching
compatible strings for each component. You just need to bind the drm
driver to some other DT node. Is there no node you can pick as the
master component?

Rob
Philipp Zabel Oct. 1, 2015, 8:59 a.m. UTC | #6
Am Mittwoch, den 30.09.2015, 11:57 -0500 schrieb Rob Herring:
> On Wed, Sep 30, 2015 at 10:30 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Freitag, den 18.09.2015, 15:33 -0500 schrieb Rob Herring:
> >> On Fri, Sep 18, 2015 at 11:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> > From: CK Hu <ck.hu@mediatek.com>
> >> >
> >> > Add device tree binding documentation for the display subsystem in
> >> > Mediatek MT8173 SoCs.
> >> >
> >> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> > ---
> >> >  .../bindings/drm/mediatek/mediatek,disp.txt        | 131 +++++++++++++++++++++
> >> >  .../bindings/drm/mediatek/mediatek,dsi.txt         |  29 +++++
> >> >  2 files changed, 160 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> >> >  create mode 100644 Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> >> > new file mode 100644
> >> > index 0000000..a3811bd
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
> >> > @@ -0,0 +1,131 @@
> >> > +Mediatek display subsystem
> >> > +==========================
> >> > +
> >> > +The Mediatek display subsystem consists of various DISP function blocks in the
> >> > +MMSYS register space. The connections between them can be configured by output
> >> > +and input selectors in the MMSYS_CONFIG register space and register updates can
> >> > +be synchronized to video frame boundaries with help of a DISP_MUTEX function
> >> > +block.
> >> > +
> >> > +The display-subsystem node binds together all individual device nodes that
> >> > +comprise the DISP subsystem.
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible: "mediatek,<chip>-disp"
> >>
> >> > +- components: Should contain a list of phandles pointing to the DISP function
> >> > +  block device nodes.
> >> > +- component-names: Should contain the name of the function block pointed to
> >> > +  by the components phandle of the same index.
> >>
> >> NAK. Group these nodes under a parent node, use of-graph or just don't
> >> put this into DT. Don't invent a new way.
> >
> > If I connect the DISP nodes using of-graph bindings, the full graph will
> > look somewhat like this (including the currently unused function blocks,
> > all properties but ports and endpoints removed for brevity):
> >
> >         ovl0@1400c000 {
> >                 port {
> >                         ovl0_to_color0: endpoint@0 {
> >                                 remote-endpoint = <&color0_from_ovl0>;
> >                         };
> >
> >                         ovl0_to_wdma0: endpoint@1 {
> >                                 remote-endpoint = <&wdma0_from_ovl0>;
> >                         };
> >                 };
> >         };
> >
> >         ovl1@1400d000 {
> >                 port {
> >                         ovl1_to_color1: endpoint@0 {
> >                                 remote-endpoint = <&color1_from_ovl1>;
> >                         };
> >
> >                         ovl1_to_wdma1: endpoint@1 {
> >                                 remote-endpoint = <&wdma1_from_ovl1>;
> >                         };
> >                 };
> >         };
> >
> >         rdma0@1400e000 {
> >                 port@0 {
> >                         rdma0_from_od: endpoint {
> >                                 remote-endpoint = <&od_to_rdma0>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         rdma0_to_ufoe: endpoint@0 {
> >                                 remote-endpoint = <&ufoe_from_rdma0>;
> >                         };
> >
> >                         rdma0_to_split0: endpoint@1 {
> >                                 remote-endpoint = <&split0_from_rdma0>;
> >                         };
> >
> >                         rdma0_to_color0: endpoint@2 {
> >                                 remote-endpoint = <&color0_from_rdma0>;
> >                         };
> >                 };
> >         };
> >
> >         rdma1@1400f000 {
> >                 port@0 {
> >                         rdma1_from_gamma: endpoint {
> >                                 remote-endpoint = <&gamma_to_rdma1>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         rdma1_to_dsi0: endpoint@0 {
> >                                 remote-endpoint = <&dsi0_from_rdma1>;
> >                         };
> >
> >                         rdma1_to_dsi1: endpoint@1 {
> >                                 remote-endpoint = <&dsi1_from_rdma1>;
> >                         };
> >
> >                         rdma1_to_dpi0: endpoint@2 {
> >                                 remote-endpoint = <&dpi0_from_rdma1>;
> >                         };
> >
> >                         rdma1_to_color1: endpoint@3 {
> >                                 remote-endpoint = <&color1_from_rdma1>;
> >                         };
> >                 };
> >         };
> >
> >         rdma2@14010000 {
> >                 port {
> >                         rdma2_to_dsi1: endpoint@0 {
> >                                 remote-endpoint = <&dsi1_from_rdma2>;
> >                         };
> >
> >                         rdma2_to_dpi0: endpoint@1 {
> >                                 remote-endpoint = <&dpi0_from_rdma2>;
> >                         };
> >                 };
> >         };
> >
> >         wdma0@14011000 {
> >                 port {
> >                         wdma0_from_ovl0: endpoint@0 {
> >                                 remote-endpoint = <&ovl0_to_wdma0>;
> >                         };
> >
> >                         wdma0_from_od: endpoint@1 {
> >                                 remote-endpoint = <&od_to_wdma0>;
> >                         };
> >
> >                         wdma0_from_ufoe: endpoint@2 {
> >                                 remote-endpoint = <&ufoe_to_wdma0>;
> >                         };
> >                 };
> >         };
> >
> >         wdma1@14012000 {
> >                 port {
> >                         wdma1_from_ovl1: endpoint@0 {
> >                                 remote-endpoint = <&ovl1_to_wdma1>;
> >                         };
> >
> >                         wdma1_from_gamma: endpoint@1 {
> >                                 remote-endpoint = <&gamma_to_wdma1>;
> >                         };
> >                 };
> >         };
> >
> >         color0@14013000 {
> >                 port@0 {
> >                         color0_from_rdma0: endpoint@0 {
> >                                 remote-endpoint = <&rdma0_to_color0>;
> >                         };
> >
> >                         color0_from_ovl0: endpoint@1 {
> >                                 remote-endpoint = <&ovl0_to_color0>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         color0_to_aal: endpoint@0 {
> >                                 remote-endpoint = <&aal_from_color0>;
> >                         };
> >
> >                         color0_to_merge: endpoint@1 {
> >                                 remote-endpoint = <&merge_from_color0>;
> >                         };
> >                 };
> >         };
> >
> >         color1@14014000 {
> >                 port@0 {
> >                         color1_from_rdma1: endpoint@0 {
> >                                 remote-endpoint = <&rdma1_to_color1>;
> >                         };
> >
> >                         color1_from_ovl1: endpoint@1 {
> >                                 remote-endpoint = <&ovl1_to_color1>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         color1_to_gamma: endpoint@0 {
> >                                 remote-endpoint = <&gamma_from_color1>;
> >                         };
> >
> >                         color1_to_merge: endpoint@1 {
> >                                 remote-endpoint = <&merge_from_color1>;
> >                         };
> >                 };
> >         };
> >
> >         aal@14015000 {
> >                 port@0 {
> >                         aal_from_color0: endpoint@0 {
> >                                 remote-endpoint = <&color0_to_aal>;
> >                         };
> >
> >                         aal_from_merge: endpoint@1 {
> >                                 remote-endpoint = <&merge_to_aal>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         aal_to_od: endpoint {
> >                                 remote-endpoint = <&od_from_aal>;
> >                         };
> >                 };
> >         };
> >
> >         gamma@14016000 {
> >                 port@0 {
> >                         gamma_from_color1: endpoint {
> >                                 remote-endpoint = <&color1_to_gamma>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         gamma_to_rdma1: endpoint@0 {
> >                                 remote-endpoint = <&rdma1_from_gamma>;
> >                         };
> >
> >                         gamma_to_dsi0: endpoint@1 {
> >                                 remote-endpoint = <&dsi0_from_gamma>;
> >                         };
> >
> >                         gamma_to_dsi1: endpoint@2 {
> >                                 remote-endpoint = <&dsi1_from_gamma>;
> >                         };
> >
> >                         gamma_to_dpi0: endpoint@3 {
> >                                 remote-endpoint = <&dpi0_from_gamma>;
> >                         };
> >
> >                         gamma_to_wdma1: endpoint@4 {
> >                                 remote-endpoint = <&wdma1_from_gamma>;
> >                         };
> >                 };
> >         };
> >
> >         merge@14017000 {
> >                 port@0 {
> >                         merge_from_color0: endpoint {
> >                                 remote-endpoint = <&color0_to_merge>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         merge_from_color1: endpoint {
> >                                 remote-endpoint = <&color1_to_merge>;
> >                         };
> >                 };
> >
> >                 port@2 {
> >                         merge_to_aal: endpoint {
> >                                 remote-endpoint = <&aal_from_merge>;
> >                         };
> >                 };
> >         };
> >
> >         split0@14018000 {
> >                 port@0 {
> >                         split0_from_rdma0: endpoint@0 {
> >                                 remote-endpoint = <&rdma0_to_split0>;
> >                         };
> >
> >                         split0_from_od: endpoint@1 {
> >                                 remote-endpoint = <&od_to_split0>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         split0_to_ufoe: endpoint {
> >                                 remote-endpoint = <&ufoe_from_split0>;
> >                         };
> >                 };
> >
> >                 port@2 {
> >                         split0_to_dsi0: endpoint@0 {
> >                                 remote-endpoint = <&dsi0_from_split0>;
> >                         };
> >
> >                         split0_to_dsi1: endpoint@1 {
> >                                 remote-endpoint = <&dsi1_from_split0>;
> >                         };
> >
> >                         split0_to_dpi0: endpoint@2 {
> >                                 remote-endpoint = <&dpi0_from_split0>;
> >                         };
> >                 };
> >         };
> >
> >         split1@14019000 {
> >                 port@0 {
> >                         split1_from_ufoe: endpoint {
> >                                 remote-endpoint = <&ufoe_to_split1>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         split1_to_dsi0: endpoint {
> >                                 remote-endpoint = <&dsi0_from_split1>;
> >                         };
> >                 };
> >
> >                 port@2 {
> >                         split1_to_dsi1: endpoint {
> >                                 remote-endpoint = <&dsi1_from_split1>;
> >                         };
> >                 };
> >         };
> >
> >         ufoe@1401a000 {
> >                 port@0 {
> >                         ufoe_from_rdma0: endpoint@0 {
> >                                 remote-endpoint = <&rdma0_to_ufoe>;
> >                         };
> >
> >                         ufoe_from_od: endpoint@1 {
> >                                 remote-endpoint = <&od_to_ufoe>;
> >                         };
> >
> >                         ufoe_from_split0: endpoint@2 {
> >                                 remote-endpoint = <&split0_to_ufoe>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         ufoe_to_dsi0: endpoint@0 {
> >                                 remote-endpoint = <&dsi0_from_ufoe>;
> >                         };
> >
> >                         ufoe_to_split1: endpoint@1 {
> >                                 remote-endpoint = <&split1_from_ufoe>;
> >                         };
> >
> >                         ufoe_to_dpi0: endpoint@2 {
> >                                 remote-endpoint = <&dpi0_from_ufoe>;
> >                         };
> >
> >                         ufoe_to_wdma0: endpoint@3 {
> >                                 remote-endpoint = <&wdma0_from_ufoe>;
> >                         };
> >
> >                         ufoe_to_dsi1: endpoint@4 {
> >                                 remote-endpoint = <&dsi1_from_ufoe>;
> >                         };
> >                 };
> >         };
> >
> >         dsi0: dsi0@1401b000 {
> >                 port@0 {
> >                         dsi0_from_ufoe: endpoint@0 {
> >                                 remote-endpoint = <&ufoe_to_dsi0>;
> >                         };
> >
> >                         dsi0_from_split1: endpoint@1 {
> >                                 remote-endpoint = <&split1_to_dsi0>;
> >                         };
> >
> >                         dsi0_from_rdma1: endpoint@2 {
> >                                 remote-endpoint = <&rdma1_to_dsi0>;
> >                         };
> >
> >                         dsi0_from_gamma: endpoint@3 {
> >                                 remote-endpoint = <&gamma_to_dsi0>;
> >                         };
> >
> >                         dsi0_from_split0: endpoint@4 {
> >                                 remote-endpoint = <&split0_to_dsi0>;
> >                         };
> >                 };
> >         };
> >
> >         dsi1: dsi1@1401c000 {
> >                 port@0 {
> >                         dsi1_from_split1: endpoint@0 {
> >                                 remote-endpoint = <&split1_to_dsi1>;
> >                         };
> >
> >                         dsi1_from_rdma1: endpoint@1 {
> >                                 remote-endpoint = <&rdma1_to_dsi1>;
> >                         };
> >
> >                         dsi1_from_gamma: endpoint@2 {
> >                                 remote-endpoint = <&gamma_to_dsi1>;
> >                         };
> >
> >                         dsi1_from_split0: endpoint@3 {
> >                                 remote-endpoint = <&split0_to_dsi1>;
> >                         };
> >
> >                         dsi1_from_rdma2: endpoint@4 {
> >                                 remote-endpoint = <&rdma2_to_dsi1>;
> >                         };
> >
> >                         dsi1_from_ufoe: endpoint@5 {
> >                                 remote-endpoint = <&ufoe_to_dsi1>;
> >                         };
> >                 };
> >         };
> >
> >         dpi0: dpi0@1401d000 {
> >                 port@0 {
> >                         dpi0_from_ufoe: endpoint@0 {
> >                                 remote-endpoint = <&ufoe_to_dpi0>;
> >                         };
> >
> >                         dpi0_from_rdma1: endpoint@1 {
> >                                 remote-endpoint = <&rdma1_to_dpi0>;
> >                         };
> >
> >                         dpi0_from_gamma: endpoint@2 {
> >                                 remote-endpoint = <&gamma_to_dpi0>;
> >                         };
> >
> >                         dpi0_from_split0: endpoint@3 {
> >                                 remote-endpoint = <&split0_to_dpi0>;
> >                         };
> >
> >                         dpi0_from_rdma2: endpoint@4 {
> >                                 remote-endpoint = <&rdma2_to_dpi0>;
> >                         };
> >                 };
> >         };
> >
> >         od@14023000 {
> >                 port@0 {
> >                         od_from_aal: endpoint {
> >                                 remote-endpoint = <&aal_to_od>;
> >                         };
> >                 };
> >
> >                 port@1 {
> >                         od_to_rdma0: endpoint@0 {
> >                                 remote-endpoint = <&rdma0_from_od>;
> >                         };
> >
> >                         od_to_ufoe: endpoint@1 {
> >                                 remote-endpoint = <&ufoe_from_od>;
> >                         };
> >
> >                         od_to_split0: endpoint@2 {
> >                                 remote-endpoint = <&split0_from_od>;
> >                         };
> >
> >                         od_to_wdma0: endpoint@3 {
> >                                 remote-endpoint = <&wdma0_from_od>;
> >                         };
> >                 };
> >         };
> >
> > Adding the graph to the mediatek oak-rev3 device tree increases the size
> > from currently 33 KiB to 41.5 KiB.
> > As a proponent of the of-graph bindings I certainly like to describe the
> > hardware connections using DT, but I'm not sure if maybe this is a bit
> > verbose.
> 
> We need a graph visualizer...

Yes :)

> I think there has to be a balance with how much is put into the DT. If
> the relationship between blocks is pretty fixed then it doesn't really
> help much to put all this into DT. If every SOC or board can have
> differing combinations of blocks or connections between blocks then
> the graph makes sense.

I expect the graph to be constant for a given SoC like MT8173.

regards
Philipp
Philipp Zabel Oct. 1, 2015, 8:59 a.m. UTC | #7
Am Mittwoch, den 30.09.2015, 12:13 -0500 schrieb Rob Herring:
> On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Note how the display-subsystem node overlaps the larb node. Is that
> > acceptable?
> 
> Given what the graph looks like, perhaps. However, do you really need
> a container node? It only serves to provide a list of nodes (e.g. all
> the children) to include as components. There are other ways to
> determine this list. You could find all nodes just searching
> compatible strings for each component. You just need to bind the drm
> driver to some other DT node. Is there no node you can pick as the
> master component?

There is the mmsys clock-controller node at the top of the MMSYS address
space (0x14000000-0x14ffffff):

	mmsys: clock-controller@14000000 {
		compatible = "mediatek,mt8173-mmsys", "syscon";
		reg = <0 0x14000000 0 0x1000>;
		#clock-cells = <1>;
	};

Its register space also contains the MMSYS_CONFIG region that controls
the multiplexers between the display function blocks, so that would be a
good candidate.
No driver binds to this node yet, the clocks are registered with
CLK_OF_DECLARE.

I'll try to bind to this node and have the driver find sibling nodes
using their compatible strings.

thanks
Philipp
Rob Herring (Arm) Oct. 1, 2015, 12:58 p.m. UTC | #8
On Thu, Oct 1, 2015 at 3:59 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Mittwoch, den 30.09.2015, 12:13 -0500 schrieb Rob Herring:
>> On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Note how the display-subsystem node overlaps the larb node. Is that
>> > acceptable?
>>
>> Given what the graph looks like, perhaps. However, do you really need
>> a container node? It only serves to provide a list of nodes (e.g. all
>> the children) to include as components. There are other ways to
>> determine this list. You could find all nodes just searching
>> compatible strings for each component. You just need to bind the drm
>> driver to some other DT node. Is there no node you can pick as the
>> master component?
>
> There is the mmsys clock-controller node at the top of the MMSYS address
> space (0x14000000-0x14ffffff):
>
>         mmsys: clock-controller@14000000 {
>                 compatible = "mediatek,mt8173-mmsys", "syscon";
>                 reg = <0 0x14000000 0 0x1000>;
>                 #clock-cells = <1>;
>         };
>
> Its register space also contains the MMSYS_CONFIG region that controls
> the multiplexers between the display function blocks, so that would be a
> good candidate.
> No driver binds to this node yet, the clocks are registered with
> CLK_OF_DECLARE.
>
> I'll try to bind to this node and have the driver find sibling nodes
> using their compatible strings.

That doesn't seem like a good choice since there are other functions
in the block. I was thinking one of the display related blocks like
whatever block provides the main crtc functions.

Rob
Daniel Kurtz Oct. 1, 2015, 2:29 p.m. UTC | #9
On Thu, Oct 1, 2015 at 8:58 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Oct 1, 2015 at 3:59 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> Am Mittwoch, den 30.09.2015, 12:13 -0500 schrieb Rob Herring:
>>> On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>> > Note how the display-subsystem node overlaps the larb node. Is that
>>> > acceptable?
>>>
>>> Given what the graph looks like, perhaps. However, do you really need
>>> a container node? It only serves to provide a list of nodes (e.g. all
>>> the children) to include as components. There are other ways to
>>> determine this list. You could find all nodes just searching
>>> compatible strings for each component. You just need to bind the drm
>>> driver to some other DT node. Is there no node you can pick as the
>>> master component?
>>
>> There is the mmsys clock-controller node at the top of the MMSYS address
>> space (0x14000000-0x14ffffff):
>>
>>         mmsys: clock-controller@14000000 {
>>                 compatible = "mediatek,mt8173-mmsys", "syscon";
>>                 reg = <0 0x14000000 0 0x1000>;
>>                 #clock-cells = <1>;
>>         };
>>
>> Its register space also contains the MMSYS_CONFIG region that controls
>> the multiplexers between the display function blocks, so that would be a
>> good candidate.
>> No driver binds to this node yet, the clocks are registered with
>> CLK_OF_DECLARE.
>>
>> I'll try to bind to this node and have the driver find sibling nodes
>> using their compatible strings.
>
> That doesn't seem like a good choice since there are other functions
> in the block. I was thinking one of the display related blocks like
> whatever block provides the main crtc functions.

The two "OVL" nodes correspond to the "crtc" functions of the two
display pipes in this SoC, we would setup the display-subsystem node
like this:

display-subsystem {
  compatible = "mediatek,display-subsystem";
  ports = <&ovl0>, <&ovl1>;
};

And, any node that needs to poke around in the mmsys_config area, like
an ovl, could access it using a phandle <&mmsys> to the common
"mediatek,mt8173-mmsys" device, like this:

ovl0: ovl@1400c000 {
       compatible = "mediatek,mt8173-ovl";
       reg = <0 0x1400c000 0 0x1000>;
       interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_LOW>;
       clocks = <&mmsys CLK_MM_DISP_OVL0>;
       iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_OVL0>;
       mediatek,mmsys = <&mmsys>;
       status = "disabled";
};

One idea to reduce the size of the of_graph would be to just model the
entrance/exit points from the MM subsystem in dt.  So, instead of:
  ovl0 -> color0 -> aal -> od -> ufoe -> dsi0 -> dsi-edp-bridge
  ovl1 -> color1 -> gamma0 -> dpi0 -> hdmi

We can do something like this:
  ovl0 -> dsi0 -> dsi-edp-bridge
  ovl1 -> dpi0 -> hdmi

But ... this might be too limiting for some applications.
So, I think we should just live with the big graph.
I would, however, recommend that you move the graph to its own
mt8173-display-graph.dtsi file which would be #included into a board
specific .dts file.

If you let the board .dts include the graph, then we can possibly
shrink the resulting board specific device tree -
At first we would make the full graph in one .dtsi as above
"mt8173-display-graph.dtsi".
This is the full reference graph that will always work.

Then, if a particular board used a fixed configuration, the system
integrator could create a smaller graph with just the graph nodes that
they care about.

For example, if mt8173-evb only supports a single HDMI output, then
perhaps you would just
set up the endpoints for this single fixed function display pipe in
mt8173-evb-display-graph.dtsi:

&ovl1 {
    status = "okay";
    port {
        ovl1_to_color1: endpoint@0 {
            remote-endpoint = <&color1_from_ovl1>;
        };
    };
};

&color1 {
    status = "okay";
    port@0 {
        color1_from_ovl1: endpoint@1 {
            remote-endpoint = <&ovl1_to_color1>;
        };
    };

    port@1 {
        color1_to_gamma: endpoint@0 {
            remote-endpoint = <&gamma_from_color1>;
        };
    };
};

&gamma {
    status = "okay";
    port@0 {
        gamma_from_color1: endpoint {
            remote-endpoint = <&color1_to_gamma>;
        };
    };

    port@1 {
        gamma_to_dpi0: endpoint@3 {
            remote-endpoint = <&dpi0_from_gamma>;
        };
    };
};

&dpi0 {
    status = "okay";
    port@0 {
        dpi0_from_gamma: endpoint@2 {
            remote-endpoint = <&gamma_to_dpi0>;
        };
    };

    port@1 {
        dpi0_to_hdmi: endpoint {
            remote-endpoint = <&hdmi_from_dpi0>;
        };
    };
};

Notice that this also only enables the nodes that we care about (all
of the other nodes would be disabled by default).

I this would allow full flexibility but also drastically cut down on
the resulting .dts size.

The individual component drivers would then march down their graphs
ensuring that all of the corresponding components have been bound, and
then themselves appropriately based on their set of endpoints that are
available.

What do you think?

-Dan


>
> Rob
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Philipp Zabel Oct. 2, 2015, 7:18 a.m. UTC | #10
Am Donnerstag, den 01.10.2015, 07:58 -0500 schrieb Rob Herring:
> On Thu, Oct 1, 2015 at 3:59 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Mittwoch, den 30.09.2015, 12:13 -0500 schrieb Rob Herring:
> >> On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> > Note how the display-subsystem node overlaps the larb node. Is that
> >> > acceptable?
> >>
> >> Given what the graph looks like, perhaps. However, do you really need
> >> a container node? It only serves to provide a list of nodes (e.g. all
> >> the children) to include as components. There are other ways to
> >> determine this list. You could find all nodes just searching
> >> compatible strings for each component. You just need to bind the drm
> >> driver to some other DT node. Is there no node you can pick as the
> >> master component?
> >
> > There is the mmsys clock-controller node at the top of the MMSYS address
> > space (0x14000000-0x14ffffff):
> >
> >         mmsys: clock-controller@14000000 {
> >                 compatible = "mediatek,mt8173-mmsys", "syscon";
> >                 reg = <0 0x14000000 0 0x1000>;
> >                 #clock-cells = <1>;
> >         };
> >
> > Its register space also contains the MMSYS_CONFIG region that controls
> > the multiplexers between the display function blocks, so that would be a
> > good candidate.
> > No driver binds to this node yet, the clocks are registered with
> > CLK_OF_DECLARE.
> >
> > I'll try to bind to this node and have the driver find sibling nodes
> > using their compatible strings.
> 
> That doesn't seem like a good choice since there are other functions
> in the block. I was thinking one of the display related blocks like
> whatever block provides the main crtc functions.

Why exactly isn't this a good choice? Since all display function blocks
are spread throughout the MMSYS address space, using the mmsys driver to
create the DRM/component master platform driver that collects its
subdevices sounded logical.

Another candidate would be the mutex node, which can synchronize the
configuration updates to the function blocks in the MMSYS region to
frame borders.

The hardware blocks that most closely correspond to crtc functionality
are the two OVL blocks, as Daniel points out. They provide DMA and plane
composition. But they are equivalent, and there is no single central
node, same as with the two IPUs on i.MX6.
Also, I'd like to keep the possibility of not tying OVL blocks to crtc
functionality too closely, because there are three more separate RDMA
units, at least one of which could be used to implement a third pipe in
the future.

Either way, if we go this path (one driver scanning DT for known
component driver compatible values), whichever node the master driver
binds to is a Linux implementation detail, and it should be completely
hidden from the device tree and binding specs.

regards
Philipp
Philipp Zabel Oct. 2, 2015, 7:40 a.m. UTC | #11
Am Donnerstag, den 01.10.2015, 22:29 +0800 schrieb Daniel Kurtz:
> On Thu, Oct 1, 2015 at 8:58 PM, Rob Herring <robh@kernel.org> wrote:
> > I was thinking one of the display related blocks like
> > whatever block provides the main crtc functions.
>
> The two "OVL" nodes correspond to the "crtc" functions of the two
> display pipes in this SoC, we would setup the display-subsystem node
> like this:
>
> display-subsystem {
>   compatible = "mediatek,display-subsystem";

Yes, the problem with the ovl nodes is that there are two equivalent
ones. Having two equivalent ipu nodes on i.MX6 was the reason to
introduce the display-subsystem node in the first place, but I'd very
much prefer to avoid it, if possible.

[...]
> And, any node that needs to poke around in the mmsys_config area, like
> an ovl, could access it using a phandle <&mmsys> to the common
> "mediatek,mt8173-mmsys" device, like this:
> 
> ovl0: ovl@1400c000 {
>        compatible = "mediatek,mt8173-ovl";
>        reg = <0 0x1400c000 0 0x1000>;
>        interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_LOW>;
>        clocks = <&mmsys CLK_MM_DISP_OVL0>;
>        iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_OVL0>;
>        mediatek,mmsys = <&mmsys>;
>        status = "disabled";
> };

I think the ovl node has no business linking to mmsys_config.
It's the drm driver code that sets up the pipeline, including the crtc
-> encoder connections

> One idea to reduce the size of the of_graph would be to just model the
> entrance/exit points from the MM subsystem in dt.  So, instead of:
>   ovl0 -> color0 -> aal -> od -> ufoe -> dsi0 -> dsi-edp-bridge
>   ovl1 -> color1 -> gamma0 -> dpi0 -> hdmi
> 
> We can do something like this:
>   ovl0 -> dsi0 -> dsi-edp-bridge
>   ovl1 -> dpi0 -> hdmi
> 
> But ... this might be too limiting for some applications.

I'm already worried about having left out the DISP_PATH0 and DISP_PATH1
multiplexers from my DT graph example. There are so many ways to do this
slightly wrong and then having to live with the stable bindings, that
I'd rather not put an approximation of the real hardware into the device
tree.
Now that I've heard it is acceptable for one driver to look for its
components by their compatible value, I'm leaning towards not describing
the graph in the DT.

> So, I think we should just live with the big graph.
> I would, however, recommend that you move the graph to its own
> mt8173-display-graph.dtsi file which would be #included into a board
> specific .dts file.

Separating the graph out into its own include file would help to make
the main dtsi more readable, though.

> If you let the board .dts include the graph, then we can possibly
> shrink the resulting board specific device tree -
> At first we would make the full graph in one .dtsi as above
> "mt8173-display-graph.dtsi".
> This is the full reference graph that will always work.
>
> Then, if a particular board used a fixed configuration, the system
> integrator could create a smaller graph with just the graph nodes that
> they care about.
> 
> For example, if mt8173-evb only supports a single HDMI output, then
> perhaps you would just
> set up the endpoints for this single fixed function display pipe in
> mt8173-evb-display-graph.dtsi:
[...]

Especially for development boards I'd rather keep all possibilites.
Sure, if nothing is connected to the DSIs, disable them and drop thier
possible connections, but I'd still keep ovl0 for example, because it
could be connected to hdmi, even though the driver doesn't support it.
I realize this was probably just an example, so yes, I think this is a
valid method of reducing device tree size for a given board if
necessary.

regards
Philipp
Daniel Kurtz Oct. 2, 2015, 1:47 p.m. UTC | #12
On Fri, Oct 2, 2015 at 3:40 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Donnerstag, den 01.10.2015, 22:29 +0800 schrieb Daniel Kurtz:
>> On Thu, Oct 1, 2015 at 8:58 PM, Rob Herring <robh@kernel.org> wrote:
>> > I was thinking one of the display related blocks like
>> > whatever block provides the main crtc functions.
>>
>> The two "OVL" nodes correspond to the "crtc" functions of the two
>> display pipes in this SoC, we would setup the display-subsystem node
>> like this:
>>
>> display-subsystem {
>>   compatible = "mediatek,display-subsystem";
>
> Yes, the problem with the ovl nodes is that there are two equivalent
> ones. Having two equivalent ipu nodes on i.MX6 was the reason to
> introduce the display-subsystem node in the first place, but I'd very
> much prefer to avoid it, if possible.

Oh, interesting.  How is it a problem that there are two equivalent ovl nodes?
Why do you prefer to avoid the display-subsystem node?

BTW, what I really meant was for my example was that the
'display-subsystem' 'ports' node lists phandles representing the
'root' nodes for the for the display-subsystem graph.
This matches what we did for rk3288.dtsi:

display-subsystem {
  compatible = "mediatek,display-subsystem";
  ports = <&ovl0_out>, <&ovl1_out>;
};

> [...]
>> And, any node that needs to poke around in the mmsys_config area, like
>> an ovl, could access it using a phandle <&mmsys> to the common
>> "mediatek,mt8173-mmsys" device, like this:
>>
>> ovl0: ovl@1400c000 {
>>        compatible = "mediatek,mt8173-ovl";
>>        reg = <0 0x1400c000 0 0x1000>;
>>        interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_LOW>;
>>        clocks = <&mmsys CLK_MM_DISP_OVL0>;
>>        iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_OVL0>;
>>        mediatek,mmsys = <&mmsys>;
>>        status = "disabled";
>> };
>
> I think the ovl node has no business linking to mmsys_config.
> It's the drm driver code that sets up the pipeline, including the crtc
> -> encoder connections

My original thinking is that the "ovl" node should link to the
mmsys_config since the mmsys_config has the ovl output select
registers.
An 'ovl' driver would then use the phandle to the syscon to find a
regmap to safely access the registers it needs.

>> One idea to reduce the size of the of_graph would be to just model the
>> entrance/exit points from the MM subsystem in dt.  So, instead of:
>>   ovl0 -> color0 -> aal -> od -> ufoe -> dsi0 -> dsi-edp-bridge
>>   ovl1 -> color1 -> gamma0 -> dpi0 -> hdmi
>>
>> We can do something like this:
>>   ovl0 -> dsi0 -> dsi-edp-bridge
>>   ovl1 -> dpi0 -> hdmi
>>
>> But ... this might be too limiting for some applications.
>
> I'm already worried about having left out the DISP_PATH0 and DISP_PATH1
> multiplexers from my DT graph example. There are so many ways to do this
> slightly wrong and then having to live with the stable bindings, that
> I'd rather not put an approximation of the real hardware into the device
> tree.
> Now that I've heard it is acceptable for one driver to look for its
> components by their compatible value, I'm leaning towards not describing
> the graph in the DT.

Sure, it would be interesting to see how that would look, too.

-Dan
Rob Herring (Arm) Oct. 2, 2015, 2:24 p.m. UTC | #13
On Fri, Oct 2, 2015 at 2:18 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Donnerstag, den 01.10.2015, 07:58 -0500 schrieb Rob Herring:
>> On Thu, Oct 1, 2015 at 3:59 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Am Mittwoch, den 30.09.2015, 12:13 -0500 schrieb Rob Herring:
>> >> On Mon, Sep 21, 2015 at 3:11 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> >> > Note how the display-subsystem node overlaps the larb node. Is that
>> >> > acceptable?
>> >>
>> >> Given what the graph looks like, perhaps. However, do you really need
>> >> a container node? It only serves to provide a list of nodes (e.g. all
>> >> the children) to include as components. There are other ways to
>> >> determine this list. You could find all nodes just searching
>> >> compatible strings for each component. You just need to bind the drm
>> >> driver to some other DT node. Is there no node you can pick as the
>> >> master component?
>> >
>> > There is the mmsys clock-controller node at the top of the MMSYS address
>> > space (0x14000000-0x14ffffff):
>> >
>> >         mmsys: clock-controller@14000000 {
>> >                 compatible = "mediatek,mt8173-mmsys", "syscon";
>> >                 reg = <0 0x14000000 0 0x1000>;
>> >                 #clock-cells = <1>;
>> >         };
>> >
>> > Its register space also contains the MMSYS_CONFIG region that controls
>> > the multiplexers between the display function blocks, so that would be a
>> > good candidate.
>> > No driver binds to this node yet, the clocks are registered with
>> > CLK_OF_DECLARE.
>> >
>> > I'll try to bind to this node and have the driver find sibling nodes
>> > using their compatible strings.
>>
>> That doesn't seem like a good choice since there are other functions
>> in the block. I was thinking one of the display related blocks like
>> whatever block provides the main crtc functions.
>
> Why exactly isn't this a good choice? Since all display function blocks
> are spread throughout the MMSYS address space, using the mmsys driver to
> create the DRM/component master platform driver that collects its
> subdevices sounded logical.

Given it is a syscon, I was assuming there are other unrelated
functions. If everything in it is display related, then it would be a
fine choice.

> Another candidate would be the mutex node, which can synchronize the
> configuration updates to the function blocks in the MMSYS region to
> frame borders.
>
> The hardware blocks that most closely correspond to crtc functionality
> are the two OVL blocks, as Daniel points out. They provide DMA and plane
> composition. But they are equivalent, and there is no single central
> node, same as with the two IPUs on i.MX6.

Just curious, are 2 IPUs represented as 1 or 2 struct drm_device?

> Also, I'd like to keep the possibility of not tying OVL blocks to crtc
> functionality too closely, because there are three more separate RDMA
> units, at least one of which could be used to implement a third pipe in
> the future.
>
> Either way, if we go this path (one driver scanning DT for known
> component driver compatible values), whichever node the master driver
> binds to is a Linux implementation detail, and it should be completely
> hidden from the device tree and binding specs.

Agreed.

Rob
Philipp Zabel Oct. 2, 2015, 2:33 p.m. UTC | #14
Hi Daniel,

Am Freitag, den 02.10.2015, 21:47 +0800 schrieb Daniel Kurtz:
> On Fri, Oct 2, 2015 at 3:40 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Donnerstag, den 01.10.2015, 22:29 +0800 schrieb Daniel Kurtz:
> >> On Thu, Oct 1, 2015 at 8:58 PM, Rob Herring <robh@kernel.org> wrote:
> >> > I was thinking one of the display related blocks like
> >> > whatever block provides the main crtc functions.
> >>
> >> The two "OVL" nodes correspond to the "crtc" functions of the two
> >> display pipes in this SoC, we would setup the display-subsystem node
> >> like this:
> >>
> >> display-subsystem {
> >>   compatible = "mediatek,display-subsystem";
> >
> > Yes, the problem with the ovl nodes is that there are two equivalent
> > ones. Having two equivalent ipu nodes on i.MX6 was the reason to
> > introduce the display-subsystem node in the first place, but I'd very
> > much prefer to avoid it, if possible.
> 
> Oh, interesting. How is it a problem that there are two equivalent ovl nodes?

Which one of the two should create the drm device?

> Why do you prefer to avoid the display-subsystem node?

Because then we don't have to argue about what it should look like ;)

Mostly because nodes like this don't describe an actual hardware
devices, but rather how the hardware designers intended other, actually
existing hardware devices to work together (in the best case), or just
how the Linux driver developers wanted to use the components together
(in the worst case).

If the display-subsystem node is necessary or useful, I won't argue
against it. If we can do without, I'll be happier.

> BTW, what I really meant was for my example was that the
> 'display-subsystem' 'ports' node lists phandles representing the
> 'root' nodes for the for the display-subsystem graph.
> This matches what we did for rk3288.dtsi:
>
> display-subsystem {
>   compatible = "mediatek,display-subsystem";
>   ports = <&ovl0_out>, <&ovl1_out>;
> };

Oh, ok then. I'm worried again that we would be describing what the
driver should load, not what the hardware looks like. Note that the
ovl_out ports have no significance outside of the display subsystem. In
the RK3288 and i.MX6 cases, the 'external' ports of the display core are
listed, to which the encoders are connected. On MT8173 this would
correspond to:

display-subsystem {
    compatible = "mediatek,display-subsystem";
    ports = <&dsi0_out>, <&dsi1_out>, <&dpi0_out>;
};

[...]
> > I think the ovl node has no business linking to mmsys_config.
> > It's the drm driver code that sets up the pipeline, including the crtc
> > -> encoder connections
> 
> My original thinking is that the "ovl" node should link to the
> mmsys_config since the mmsys_config has the ovl output select
> registers.

I see. If each function block takes care of the attached SEL input
selectors and SOUT/MOUT output selectors, wouldn't all of them have to
link to the mmsys_config node, then?
That's certainly a possibility, but in this case I feel like having a
single, central instance to handle building the paths and arbitrating
register access with help of the disp_mutex would be easier to
understand.

regards
Philipp
Philipp Zabel Oct. 2, 2015, 2:51 p.m. UTC | #15
Hi Rob,

Am Freitag, den 02.10.2015, 09:24 -0500 schrieb Rob Herring:
[...]
> >> > I'll try to bind to this node and have the driver find sibling nodes
> >> > using their compatible strings.
> >>
> >> That doesn't seem like a good choice since there are other functions
> >> in the block. I was thinking one of the display related blocks like
> >> whatever block provides the main crtc functions.
> >
> > Why exactly isn't this a good choice? Since all display function blocks
> > are spread throughout the MMSYS address space, using the mmsys driver to
> > create the DRM/component master platform driver that collects its
> > subdevices sounded logical.
> 
> Given it is a syscon, I was assuming there are other unrelated
> functions. If everything in it is display related, then it would be a
> fine choice.

Oh, ok. Next to the display blocks there's also a similar graph of MDP
("multimedia data path") blocks that can perform memory-to-memory
scaling and rotation that use the same multiplexers and mutex, and the
HDMI encoder controls the FIFO between its parallel output and the HDMI
PHY via (separate) registers in the MMSYS_CONFIG region. Still, I think
these are sufficiently display related.

> > Another candidate would be the mutex node, which can synchronize the
> > configuration updates to the function blocks in the MMSYS region to
> > frame borders.
> >
> > The hardware blocks that most closely correspond to crtc functionality
> > are the two OVL blocks, as Daniel points out. They provide DMA and plane
> > composition. But they are equivalent, and there is no single central
> > node, same as with the two IPUs on i.MX6.
> 
> Just curious, are 2 IPUs represented as 1 or 2 struct drm_device?

One. There's bus multiplexer fabric between the IPU outputs and the
LVDS/HDMI encoders, so it's all connected.

regards
Philipp
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
new file mode 100644
index 0000000..a3811bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,disp.txt
@@ -0,0 +1,131 @@ 
+Mediatek display subsystem
+==========================
+
+The Mediatek display subsystem consists of various DISP function blocks in the
+MMSYS register space. The connections between them can be configured by output
+and input selectors in the MMSYS_CONFIG register space and register updates can
+be synchronized to video frame boundaries with help of a DISP_MUTEX function
+block.
+
+The display-subsystem node binds together all individual device nodes that
+comprise the DISP subsystem.
+
+Required properties:
+
+- compatible: "mediatek,<chip>-disp"
+- components: Should contain a list of phandles pointing to the DISP function
+  block device nodes.
+- component-names: Should contain the name of the function block pointed to
+  by the components phandle of the same index.
+- mmsys-config: Should contain a phandle pointing to the MMSYS node.
+- disp-mutex: Should contain a phandle pointing to the DISP_MUTEX node.
+
+Example:
+
+display-subsystem {
+	compatible = "mediatek,mt8173-disp";
+	components = <&ovl0>, <&rdma0>, <&color0>, <&aal>,
+		     <&ufoe>, <&dsi0>, <&od>;
+	component-names = "ovl0", "rdma0", "color0", "aal",
+			  "ufoe", "dsi0", "od";
+	mmsys-config = <&mmsys>;
+	disp-mutex = <&mutex>;
+};
+
+DISP function blocks
+====================
+
+A display stream starts at a source function block that reads pixel data from
+memory and ends with a sink function block that drives pixels on a display
+interface, or writes pixels back to memory. All DISP function blocks have
+their own register space, interrupt, and clock gate. The blocks that can
+access memory additionally have to list the IOMMU and local arbiter they are
+connected to.
+
+For a description of the display interface sink function blocks, see
+Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
+
+Required properties (all function blocks):
+- compatible: "mediatek,<chip>-disp-<function>", one of
+	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
+	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
+	"mediatek,<chip>-disp-color" - color processor
+	"mediatek,<chip>-disp-aal"   - adaptive ambient light controller
+	"mediatek,<chip>-disp-gamma" - gamma correction
+	"mediatek,<chip>-disp-ufoe"  - data compression engine
+	"mediatek,<chip>-disp-mutex" - display mutex
+	"mediatek,<chip>-disp-od"    - overdrive
+- reg: Physical base address and length of the function block register space
+- interrupts: The interrupt signal from the function block.
+- clocks: device clocks
+  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- compatible: "mediatek,<chip>-ddp"
+- power-domains: a phandle to DDP power domain node.
+
+Required properties (DMA function blocks):
+- compatible: Should be one of
+	"mediatek,<chip>-disp-ovl"
+	"mediatek,<chip>-disp-rdma"
+- larb: Should a phandles pointing to the local arbiter device as defined in
+  Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
+- iommus: required a iommu node
+
+Examples:
+
+ovl0: ovl@1400c000 {
+	compatible = "mediatek,mt8173-disp-ovl";
+	reg = <0 0x1400c000 0 0x1000>;
+	interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_OVL0>;
+	iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_OVL0>;
+};
+
+rdma0: rdma@1400e000 {
+	compatible = "mediatek,mt8173-disp-rdma";
+	reg = <0 0x1400e000 0 0x1000>;
+	interrupts = <GIC_SPI 182 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_RDMA0>;
+	iommus = <&iommu M4U_LARB0_ID M4U_PORT_DISP_RDMA0>;
+};
+
+color0: color@14013000 {
+	compatible = "mediatek,mt8173-disp-color";
+	reg = <0 0x14013000 0 0x1000>;
+	interrupts = <GIC_SPI 187 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_COLOR0>;
+};
+
+aal: aal@14015000 {
+	compatible = "mediatek,mt8173-disp-aal";
+	reg = <0 0x14015000 0 0x1000>;
+	interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_AAL>;
+};
+
+gamma: gamma@14016000 {
+	compatible = "mediatek,mt8173-disp-gamma";
+	reg = <0 0x14016000 0 0x1000>;
+	interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_GAMMA>;
+};
+
+ufoe: ufoe@1401a000 {
+	compatible = "mediatek,mt8173-disp-ufoe";
+	reg = <0 0x1401a000 0 0x1000>;
+	interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&mmsys CLK_MM_DISP_UFOE>;
+};
+
+mutex: mutex@14020000 {
+	compatible = "mediatek,mt8173-disp-mutex";
+	reg = <0 0x14020000 0 0x1000>;
+	interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_LOW>;
+	power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
+	clocks = <&mmsys CLK_MM_MUTEX_32K>;
+};
+
+od: od@14023000 {
+	compatible = "mediatek,mt8173-disp-od";
+	reg = <0 0x14023000 0 0x1000>;
+	clocks = <&mmsys CLK_MM_DISP_OD>;
+};
diff --git a/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
new file mode 100644
index 0000000..e892ef1
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
@@ -0,0 +1,29 @@ 
+Mediatek DSI Device
+===================
+
+The Mediatek DSI function block is a sink of the display subsystem and can
+drive up to 4-lane MIPI DSI output. Two DSIs can be synchronized for dual-
+channel output.
+
+Required properties:
+- compatible: "mediatek,<chip>-dsi"
+- reg: Physical base address and length of the controller's registers
+- clocks: device clocks
+  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- clock-names: must contain "engine" and "digital".
+
+Example:
+
+dsi0: dsi@1401b000 {
+	compatible = "mediatek,mt8173-dsi";
+	reg = <0 0x1401b000 0 0x1000>,	/* DSI0 */
+	      <0 0x10215000 0 0x1000>;  /* MIPI_TX0 */
+	clocks = <&mmsys MM_DSI0_ENGINE>, <&mmsys MM_DSI0_DIGITAL>;
+	clock-names = "engine", "digital";
+
+	port {
+		dsi0_out: endpoint {
+			remote-endpoint = <&panel_in>;
+		};
+	};
+};