Re: [PATCH v5 2/5] dt-bindings: display: xlnx: Add ZynqMP DP subsystem bindings

From: Hyun Kwon <hyun.kwon@xilinx.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Michal Simek <michal.simek@xilinx.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Rob Herring <robh+dt@kernel.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v5 2/5] dt-bindings: display: xlnx: Add ZynqMP DP subsystem bindings
Date: Thu, 22 Feb 2018 18:27:50 -0800	[thread overview]
Message-ID: <20180223022749.GB9544@smtp.xilinx.com> (raw)
In-Reply-To: <2731219.EK2Kz0loy1@avalon>

Hi Laurent,

On Thu, 2018-02-22 at 06:23:38 -0800, Laurent Pinchart wrote:
> Hi Hyun,
> 
> Thank you for the patch.
> 
> On Wednesday, 7 February 2018 03:36:37 EET Hyun Kwon wrote:
> > This add a dt binding for ZynqMP DP subsystem.
> > 
> > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> > v4
> > - Specify phy related descriptions
> > - Specify dma related descriptions
> > - Remove ports
> > - Remove child nodes for layers
> > - Update the example accordingly
> > v2
> > - Group multiple ports under 'ports'
> > - Replace linux specific terms with generic hardware descriptions
> > ---
> > ---
> >  .../bindings/display/xlnx/xlnx,zynqmp-dpsub.txt    | 67 +++++++++++++++++++
> >  1 file changed, 67 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..f4a2e6d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.txt
> > @@ -0,0 +1,67 @@
> > +Xilinx ZynqMP DisplayPort subsystem
> > +-----------------------------------
> 
> A short description of the hardware would be useful. You can also link to the 
> documentation. I have found v1.0, v2.0 and v2.1 of the PG199 document, which 
> seem to correspond to the "dp" register map (and confusingly documented as the 
> "DisplayPort TX Subsystem"), but no document that describes the full 
> DisplayPort subsystem as defined by these bindings.

Please refer to chapter 33 in
https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
Not sure if it has enough details, but only public documentation that I know of.

> 
> > +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. The number of lanes is configurable
> > +  between 1 and 2. The number of phandles should be 1 or 2.
> > +- phy-names: The identifier strings. "dp-phy" followed by index, 0 or 1.
> > +  For single lane, only "dp-phy0" is required. For dual lane, both
> > "dp-phy0"
> > +  and "dp-phy1" are required where "dp-phy0" is the primary lane.
> > +
> > +- power-domains: phandle for the corresponding power domain
> > +
> > +- dmas: phandles for DMA channels as defined in
> > +  Documentation/devicetree/bindings/dma/dma.txt.
> > +- dma-names: The identifier strings are required. "gfx0" for graphics layer
> > +  dma channel. "vid" followed by index (0 - 2) for video layer dma
> > channels.
> > +
> > +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.
> 
> DT bindings should describe the hardware, not the OS software. You should not 
> mention drivers.
> 
> Furthermore the above paragraph doesn't seem very clear to me, and the example 
> doesn't include any child node, so I'm left wondering what you meant.

I left some room for future and downstream, but I agree. I can remove this from
this set.

> 
> > +Example:
> > +	zynqmp-display-subsystem@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";
> 
> Without seeing the documentation it's a bit hard to tell, but it looks to me 
> like this "subsystem" aggregates four separate IP cores that can be used 
> individually. The "dp" registers in particular make me think that the 
> DisplayPort transmitter is a separate IP core corresponding to PG199. If 
> that's the case, I think the IP cores should be modeled as individual DT 
> nodes, and connected through the OF graph bindings.
> 

The PG199 is for the soft IP core, but this is for the hardened IP on
ZynqMP SoC. Please refer to documentation above. It's a single IP with
multiple register partitions, one for each block, as described here, and those
blocks are hard-wired in the IP.

Thanks,
-hyun

> > +		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>, <&lane0>;
> > +		phy-names = "dp-phy0", "dp-phy1";
> > +
> > +		power-domains = <&pd_dp>;
> > +
> > +		dma-names = "vid0", "vid1", "vid2", "gfx0";
> > +		dmas = <&xlnx_dpdma 0>,
> > +		       <&xlnx_dpdma 1>,
> > +		       <&xlnx_dpdma 2>,
> > +		       <&xlnx_dpdma 3>;
> > +	};
> > +};
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel