[PATCHv6 1/3] ARM:dt-bindings Intel FPGA Video and Image Processing Suite

[Ong, Hean Loong]

Hi Laurent Thanks. my replies are below

On Fri, 2017-08-18 at 16:11 +0300, Laurent Pinchart wrote:
> Hi Hean Loong,
> 
> (CC'ing dri-devel again as I noticed it wasn't CC'ed anymore)
> 
> On Friday 18 Aug 2017 08:34:44 Ong, Hean Loong wrote:
> > 
> > Hi Laurent,
> > Thanks for the comments, I drafted a copy of the DT bindings based
> > on
> > your recommendations and inputs. I inserted the changes below the
> > previous comments. 
> [snip]
> 
> > 
> > Intel Video and Image Processing(VIP) Frame Buffer II bindings
> > 
> > Supported hardware: Intel FPGA SoC Arria10 and above with display
> > port
> > IP
> > 
> > The Video Frame Buffer II in Video Image Processing (VIP) suite is
> > an IP
> > core that interfaces between system memory and Avalon-ST video
> > ports. The IP
> > core can be configured to support the memory reader (from memory to
> > Avalon-
> > ST) and/or memory writer (from Avalon-ST to memory) interfaces.
> > 
> > Connections between the Frame Buffer II and other video IP cores in
> > the
> > system are modelled using the OF graph DT bindings. The Frame
> > Buffer II node
> > has up to two OF graph ports. When the memory writer interface is
> > enabled,
> > port 0 maps to the Avalon-ST Input (din) port. When the memory
> > reader
> > interface is enabled, port 1 maps to the Avalon-ST Output (dout)
> > port.
> > 
> > More information the FPGA video IP component can be acquired from
> > https://www.altera.com/content/dam/altera-www/global/en_US/pdfs\
> > /literature/ug/ug_vip.pdf
> > 
> > New bindings:
> > =============
> How are the bindings "new" ? You can omit that title.
> 

Noted.
> > 
> > Required properties:
> > ----------------------------
> > - compatible: "altr,vip-frame-buffer-2.0"
> > - reg: Physical base address and length of the framebuffer
> > controller's
> > 	registers.
> > - altr,max-width: The maximum width of the framebuffer in pixels.
> > - altr,max-height: The maximum height of the framebuffer in pixels.
> > - altr,mem-port-width = the bus width of the avalon master port 
> > 	on the frame reader
> You need to mention the ports here as they are mandatory. I would
> move the 
> second paragraph from the introduction to here. You should also refer
> to the 
> file defining the OF graph DT bindings. You can find examples in
> other DT 
> bindings.
> 
Noted
> > 
> > Example:
> > ----------------------------
> >  +-----+      +-----------+      +------------+      +-----------+
> >  |  D  |      | Frame     |      | DP/HDMI TX |      | DP/HDMI   |
> >  |  D  |----->| Buffer II |----->| Controller |----->| Connector |
> >  |  R  |      |           |      |            |      |           |
> >  +-----+      +-----------+      +------------+      +-----------+
> > 
> > framebuffer at 100000280 {
> >         compatible = "altr,vip-frame-buffer-2.0";
> >         reg = <0x00000001 0x00000280 0x00000040>;
> >         altr,max-width = <1280>;
> >         altr,max-height = <720>;
> >         altr,mem-port-width = <128>;
> > 
> >         ports {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> > 
> >                 port at 1 {
> >                         reg = <1>;
> >                         fb_output: endpoint {
> >                                 remote-endpoint =
> > <&dp_encoder_input>;
> >                         };
> >                 };
> >         };
> > };
> > 
> > If there is a need to scale the Frame Buffer II IP cores in
> > the pipeline, each node would have its own node, connected
> > through ports and endpoints. 
> > 
> > hdmi-encoder at ......... {
> >         compatible = "altr,hdmi-tx-16.0";
> This was just an example, please use the real compatible string of
> the HDMI 
> controller (and please submit DT bindings for the HDMI controller :-
> )). Please 
> also fill the reg property with values from a real example.
> 
I was trying to get overall picture to be correct before I proceed
further. We currently do not have support for HDMI therefore I would
omit this until a HDMI IP is available. The only values are available
for the Display Port.
> > 
> >         reg = <.....>;
> >         /* Other IP-specific properties here */
> > 
> >         ports {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> > 
> >                 port at 0 {
> >                         reg = <0>;
> >                         hdmi_tx_input: endpoint {
> >                                 remote-endpoint = <&fb_output>;
> >                         };
> >                 };
> > 
> >                 port at 1 {
> >                         reg = <1>;
> >                         hdmi_tx_output: endpoint {
> >                                 remote-endpoint =
> > <&hdmi_conn_input>;
> >                         };
> >                 };
> >         };
> > };
> > 
> > hdmi-connector at 0 {
> >         compatible = "hdmi-connector";
> >         type = "a";
> > 
> >         port {
> >                 hdmi_conn_input: endpoint {
> >                         remote-endpoint = <&hdmi_tx_output>;
> >                 };
> >         };
> > };