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

[Philipp Zabel <p.zabel@pengutronix.de>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel