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@xxxxxxxxxx> > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
