RE: [PATCH 06/10] dt-bindings: display: xlnx: Add ZynqMP DP subsystem bindings

From: Hyun Kwon <hyunk-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
To: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Michal Simek
	<michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>,
	"dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: RE: [PATCH 06/10] dt-bindings: display: xlnx: Add ZynqMP DP subsystem bindings
Date: Thu, 11 Jan 2018 02:06:24 +0000	[thread overview]
Message-ID: <BY1PR0201MB1000E20D9F60CADCF810F35FD6160@BY1PR0201MB1000.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20180109040713.rusxuzhkhp262zgf@rob-hp-laptop>

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> Of Rob Herring
> Sent: Monday, January 08, 2018 8:07 PM
> To: Hyun Kwon <hyunk@xilinx.com>
> Cc: devicetree@vger.kernel.org; Michal Simek <michal.simek@xilinx.com>;
> dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 06/10] dt-bindings: display: xlnx: Add ZynqMP DP
> subsystem bindings
> 
> On Thu, Jan 04, 2018 at 06:05:55PM -0800, Hyun Kwon wrote:
> > This add a dt binding for ZynqMP DP subsystem.
> >
> > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > ---
> >  .../bindings/display/xlnx/xlnx,zynqmp-dpsub.txt    | 94
> ++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-
> dpsub.txt b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-
> dpsub.txt
> > new file mode 100644
> > index 0000000..4e478ca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-
> dpsub.txt
> > @@ -0,0 +1,94 @@
> > +Xilinx ZynqMP DisplayPort subsystem
> > +-----------------------------------
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "xlnx,zynqmp-dpsub-1.7".
> > +
> > +- reg: Physical base address and length of the registers set for the device.
> > +- reg-names: Must be "dp", "blend", "av_buf", and "aud" to map logical
> register
> > +  partitions.
> > +
> > +- interrupts: Interrupt number.
> > +- interrupts-parent: phandle for interrupt controller.
> > +
> > +- clocks: phandles for axi, audio, non-live video, and live video clocks.
> > +  axi clock is required. Audio clock is optional. If not present, audio will
> > +  be disabled. One of non-live or live video clock should be present.
> > +- clock-names: The identification strings are required. "aclk" for axi clock.
> > +  "dp_aud_clk" for audio clock. "dp_vtc_pixel_clk_in" for non-live video
> clock.
> > +  "dp_live_video_in_clk" for live video clock (clock from programmable
> logic).
> 
> "_clk" is redundant.

This is to reflect the name of signal spelled out in the hardware spec, so I'd like to keep it this way unless you are still against it.

> 
> > +
> > +- phys: phandles for phy specifier.
> > +- phy-names: The identifier strings. "dp-phy" followed by index.
> > +
> > +- power-domains: phandle for the corresponding power domain
> > +
> > +- ports: crtc and encoder ports are required using DT bindings defined in
> > +  Documentation/devicetree/bindings/graph.txt.
> 
> Isn't the connection crtc->encoder?

It's bidirectional: crtc <-> encoder. Currently, as in this example, it's only connected between internal ports, but any of those ports can be connected with external ports too.

> 
> Also, crtc is pretty much a DRM term. Don't use that in bindings.
> Describe the h/w blocks.

Sure. Will fix.

> 
> > +
> > +- vid-layer, gfx-layer: Required to represent available layers
> > +
> > +Required layer properties
> > +
> > +- dmas: phandles for DMA channels as defined in
> > +  Documentation/devicetree/bindings/dma/dma.txt.
> > +- dma-names: The identifier strings are required. "graphics0" for graphics
> > +  layer. "video" followed by index for video layer
> > +
> > +Optional child node
> > +
> > +- The driver populates any child device node in this node. This can be
> used,
> > +  for example, to populate the sound device from the DisplayPort
> subsystem
> > +  driver.
> > +
> > +Example:
> > +	zynqmp_dpsub: zynqmp_dpsub@fd4a0000 {
> 
> display-controller@...
> 
> > +		compatible = "xlnx,zynqmp-dpsub-1.7";
> > +		reg = <0x0 0xfd4a0000 0x0 0x1000>,
> > +		      <0x0 0xfd4aa000 0x0 0x1000>,
> > +		      <0x0 0xfd4ab000 0x0 0x1000>,
> > +		      <0x0 0xfd4ac000 0x0 0x1000>;
> > +		reg-names = "dp", "blend", "av_buf", "aud";
> > +		interrupts = <0 119 4>;
> > +		interrupt-parent = <&gic>;
> > +
> > +		clock-names = "dp_apb_clk", "dp_aud_clk",
> "dp_live_video_in_clk";
> > +		clocks = <&dp_aclk>, <&clkc 17>, <&si570_1>;
> > +
> > +		phys = <&lane1 PHY_TYPE_DP 0 1 27000000>,
> > +		       <&lane0 PHY_TYPE_DP 1 1 27000000>;
> > +		phy-names = "dp-phy0", "dp-phy1";
> > +
> > +		power-domains = <&pd_dp>;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		vid-layer {
> > +			dma-names = "vid0", "vid1", "vid2";
> > +			dmas = <&xlnx_dpdma 0>,
> > +			       <&xlnx_dpdma 1>,
> > +			       <&xlnx_dpdma 2>;
> > +		};
> > +
> > +		gfx-layer {
> 
> These 2 nodes don't seem necessary. Just make "dmas" take 4 values and
> define where the gfx0 channel is located (index 0 or 3?).
> 

That is correct, as of now. But, in the follow up patch / internal development, each layer needs to be referenced by external device node (ex, external device connected to each layer). That's why there's a child node for each layer. I can still remove these nodes for now, and add when those are needed. Please let me know, otherwise, I'll keep these nodes.

> > +			dma-names = "gfx0";
> > +			dmas = <&xlnx_dpdma 3>;
> > +		};
> > +
> > +		crtc_port: port@0 {
> > +			reg = <0>;
> > +			crtc: endpoint {
> > +				remote-endpoint = <&encoder>;
> > +			};
> > +		};
> > +		port@1 {
> 
> Multiple port nodes should be under a ports node especially if you have
> other child nodes.
> 

Will fix it.

Thanks,
-hyun

> > +			reg = <1>;
> > +			encoder: endpoint {
> > +				remote-endpoint = <&crtc>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel