diff mbox

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

Message ID 1515117959-18068-7-git-send-email-hyun.kwon@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hyun Kwon Jan. 5, 2018, 2:05 a.m. UTC
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

Comments

Rob Herring (Arm) Jan. 9, 2018, 4:07 a.m. UTC | #1
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.

> +
> +- 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?

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

> +
> +- 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?).

> +			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.

> +			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
Hyun Kwon Jan. 11, 2018, 2:06 a.m. UTC | #2
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
diff mbox

Patch

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).
+
+- 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.
+
+- 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 {
+		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 {
+			dma-names = "gfx0";
+			dmas = <&xlnx_dpdma 3>;
+		};
+
+		crtc_port: port@0 {
+			reg = <0>;
+			crtc: endpoint {
+				remote-endpoint = <&encoder>;
+			};
+		};
+		port@1 {
+			reg = <1>;
+			encoder: endpoint {
+				remote-endpoint = <&crtc>;
+			};
+		};
+	};
+};
+