| Message ID | 1517967400-16993-2-git-send-email-hyun.kwon@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +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. > +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. > + 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>; > + }; > +};
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 >
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 +----------------------------------- + +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. + +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"; + 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>; + }; +}; +