diff mbox

[v2,2/2] dt-bindings: drm/bridge: Document Cadence DSI bridge bindings

Message ID 1496405096-18275-2-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON June 2, 2017, 12:04 p.m. UTC
Document the bindings used for the Cadence DSI bridge.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt

Comments

Archit Taneja June 3, 2017, 6:13 p.m. UTC | #1
Hi,

On 06/02/2017 05:34 PM, Boris Brezillon wrote:
> Document the bindings used for the Cadence DSI bridge.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> new file mode 100644
> index 000000000000..770c5c5b1e93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> @@ -0,0 +1,55 @@
> +Cadence DSI bridge
> +==================
> +
> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.

Is this a separate chip, or an IP integrated into SoCs? If it's the 
latter, I don't think DPI on the its input side is the right term to 
use. Maybe RGB would be more appropriate here.

> +
> +Required properties:
> +- compatible: should be set to "cdns,dsi".

Would it be better to take a dw-hdmi like approach here? I.e, the
binding should be specific to the SoC that integrates this DSI
bridge?

> +- reg: physical base address and length of the controller's registers.
> +- interrupts: interrupt line connected to the DSI bridge.
> +- clocks: DSI bridge clocks.
> +- clock-names: must contain "pclk" and "sysclk".
> +- phys: phandle link to the MIPI D-PHY controller.
> +- phy-names: must contain "dphy".
> +- #address-cells: must be set to 1.
> +- #size-cells: must be set to 0.
> +
> +Required subnodes:
> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> +  Currently contains a single input port at address 0 representing the DPI
> +  input. Other ports will be added later to support the SDI inputs.
> +  Port 0 should be connected to a DPI encoder output.

The output of the DSI bridge may be another bridge, which could be i2c
controlled. In that case, it won't be a child of the DSI bridge. For
such scenarios, we might want to define an output port for the bridge.

Thanks,
Archit

> +- one subnode per DSI device connected on the DSI bus. Each DSI device should
> +  contain a reg property encoding its address on the bus.
> +
> +Example:
> +
> +	dsi0: dsi@fd0c0000 {
> +		compatible = "cdns,dsi";
> +		reg = <0x0 0xfd0c0000 0x0 0x1000>;
> +		clocks = <&pclk>, <&sysclk>;
> +		clock-names = "pclk", "hclk";
> +		interrupts = <1>;
> +		phys = <&dphy1>;
> +		phy-names = "dphy";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				dsi0_dpi_input: endpoint {
> +					remote-endpoint = <&xxx_dpi_output>;
> +				};
> +			};
> +		};
> +
> +		panel: dsi-dev@0 {
> +			compatible = "<vendor,panel>";
> +			reg = <0>;
> +		};
> +	};
> +
>
Boris BREZILLON June 6, 2017, 9:35 a.m. UTC | #2
On Sat, 3 Jun 2017 23:43:17 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi,
> 
> On 06/02/2017 05:34 PM, Boris Brezillon wrote:
> > Document the bindings used for the Cadence DSI bridge.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > new file mode 100644
> > index 000000000000..770c5c5b1e93
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > @@ -0,0 +1,55 @@
> > +Cadence DSI bridge
> > +==================
> > +
> > +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.  
> 
> Is this a separate chip, or an IP integrated into SoCs?

It's supposed to be integrated into SoCs.

> If it's the 
> latter, I don't think DPI on the its input side is the right term to 
> use. Maybe RGB would be more appropriate here.

Well, the datasheet explicitly mentions DPI, and you can also send
pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
appropriate, but if you really want me to use RGB I can change that.

BTW, can you detail why DPI is not appropriate for internal parallel
busses. I don't understand why it makes a difference when the bus is exposed
through external pins.

> 
> > +
> > +Required properties:
> > +- compatible: should be set to "cdns,dsi".  
> 
> Would it be better to take a dw-hdmi like approach here? I.e, the
> binding should be specific to the SoC that integrates this DSI
> bridge?

Hm, I was considering something slightly different: adding new compat
strings to the DSI bridge driver and keep the interface to the display
controller drivers as simple as possible to avoid duplicating the glue
used to bind the component in all display controller drivers embedding
this bridge.

Note that right now we don't have any SoC embedding this IP. It has
been tested on an FPGA with a very basic display controller (designed
for testing purpose only). By exposing this IP as a simple bridge, we
can easily attach it to any kind of display controller, and if we ever
need to use the component framework, then it should be pretty easy to
add support for this feature as a follow-up patch.

> 
> > +- reg: physical base address and length of the controller's registers.
> > +- interrupts: interrupt line connected to the DSI bridge.
> > +- clocks: DSI bridge clocks.
> > +- clock-names: must contain "pclk" and "sysclk".
> > +- phys: phandle link to the MIPI D-PHY controller.
> > +- phy-names: must contain "dphy".
> > +- #address-cells: must be set to 1.
> > +- #size-cells: must be set to 0.
> > +
> > +Required subnodes:
> > +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> > +  Currently contains a single input port at address 0 representing the DPI
> > +  input. Other ports will be added later to support the SDI inputs.
> > +  Port 0 should be connected to a DPI encoder output.  
> 
> The output of the DSI bridge may be another bridge, which could be i2c
> controlled. In that case, it won't be a child of the DSI bridge. For
> such scenarios, we might want to define an output port for the bridge.

Hm, okay. IIRC, this is something you mentioned when I asked how to
describe input/output ports for a DSI bridge a while ago.

I'm still not sure how the links between input and output endpoint are
supposed to be described.

For example, if you take the case where you have the DSI device
directly described under the DSI host controller, should I create
another node for this output port? Something like the following?

	dsi@xxx {
		#address-cells = <1>;
		#size-cells = <0>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;
			dpi_in: port@0 {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;

				endpoint@0 {
					remote-endpoint = <&dpi_out>;
				};
			};

			dsi_out0: port@1 {
				reg = <1>;
				#address-cells = <1>;
				#size-cells = <0>;

				dsi_out0: endpoint@0 {
					remote-endpoint = <&dsi_panel0_in>;
				};
			};

			dsi_out0: port@2 {
				reg = <2>;
				#address-cells = <1>;
				#size-cells = <0>;

				dsi_out1: endpoint@0 {
					remote-endpoint = <&dsi_panel1_in>;
				};
			};
		};

		panel@0 {
			compatible = "...";
			reg = <0>;
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				#address-cells = <1>;
		                #size-cells = <0>;
				reg = <0>;

				dsi_panel0_in: endpoint@0 {
					remote-endpoint = <&dsi_out0>;
				};
			};
		};

		panel@1 {
			compatible = "...";
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				#address-cells = <1>;
		                #size-cells = <0>;
				reg = <0>;

				dsi_panel1_in: endpoint@0 {
					remote-endpoint = <&dsi_out1>;
				};
			};
		};
	};

Thanks for the review,

Boris
Tomi Valkeinen June 6, 2017, 12:30 p.m. UTC | #3
On 02/06/17 15:04, Boris Brezillon wrote:
> Document the bindings used for the Cadence DSI bridge.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> new file mode 100644
> index 000000000000..770c5c5b1e93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> @@ -0,0 +1,55 @@
> +Cadence DSI bridge
> +==================
> +
> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> +
> +Required properties:
> +- compatible: should be set to "cdns,dsi".
> +- reg: physical base address and length of the controller's registers.
> +- interrupts: interrupt line connected to the DSI bridge.
> +- clocks: DSI bridge clocks.
> +- clock-names: must contain "pclk" and "sysclk".

clock-names doesn't match the example below.

> +- phys: phandle link to the MIPI D-PHY controller.
> +- phy-names: must contain "dphy".
> +- #address-cells: must be set to 1.
> +- #size-cells: must be set to 0.
> +
> +Required subnodes:
> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> +  Currently contains a single input port at address 0 representing the DPI
> +  input. Other ports will be added later to support the SDI inputs.

Typo with "SDI".

 Tomi
Boris BREZILLON June 6, 2017, 12:37 p.m. UTC | #4
Hi Tomi,

On Tue, 6 Jun 2017 15:30:26 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 02/06/17 15:04, Boris Brezillon wrote:
> > Document the bindings used for the Cadence DSI bridge.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > new file mode 100644
> > index 000000000000..770c5c5b1e93
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> > @@ -0,0 +1,55 @@
> > +Cadence DSI bridge
> > +==================
> > +
> > +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> > +
> > +Required properties:
> > +- compatible: should be set to "cdns,dsi".
> > +- reg: physical base address and length of the controller's registers.
> > +- interrupts: interrupt line connected to the DSI bridge.
> > +- clocks: DSI bridge clocks.
> > +- clock-names: must contain "pclk" and "sysclk".  
> 
> clock-names doesn't match the example below.

Indeed. I'll fix the example.

> 
> > +- phys: phandle link to the MIPI D-PHY controller.
> > +- phy-names: must contain "dphy".
> > +- #address-cells: must be set to 1.
> > +- #size-cells: must be set to 0.
> > +
> > +Required subnodes:
> > +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> > +  Currently contains a single input port at address 0 representing the DPI
> > +  input. Other ports will be added later to support the SDI inputs.  
> 
> Typo with "SDI".

No, the 2nd and 3rd input ports are really called SDI. Here
is the datasheet description:

"
SDI: 
Serial Display Interface - this is the name of the block that is built
to interface the Display application processor to the DSI link. This is
a proprietary interface.
"

Thanks,

Boris
Tomi Valkeinen June 6, 2017, 12:40 p.m. UTC | #5
On 06/06/17 12:35, Boris Brezillon wrote:
> On Sat, 3 Jun 2017 23:43:17 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
> 
>> Hi,
>>
>> On 06/02/2017 05:34 PM, Boris Brezillon wrote:
>>> Document the bindings used for the Cadence DSI bridge.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> ---
>>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> new file mode 100644
>>> index 000000000000..770c5c5b1e93
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> @@ -0,0 +1,55 @@
>>> +Cadence DSI bridge
>>> +==================
>>> +
>>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.  
>>
>> Is this a separate chip, or an IP integrated into SoCs?
> 
> It's supposed to be integrated into SoCs.
> 
>> If it's the 
>> latter, I don't think DPI on the its input side is the right term to 
>> use. Maybe RGB would be more appropriate here.
> 
> Well, the datasheet explicitly mentions DPI, and you can also send
> pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
> appropriate, but if you really want me to use RGB I can change that.
> 
> BTW, can you detail why DPI is not appropriate for internal parallel
> busses. I don't understand why it makes a difference when the bus is exposed
> through external pins.

I think MIPI DPI is fine, if it is indeed MIPI DPI. But mot all parallel
video busses are MIPI DPI.

>>> +Required subnodes:
>>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
>>> +  Currently contains a single input port at address 0 representing the DPI
>>> +  input. Other ports will be added later to support the SDI inputs.
>>> +  Port 0 should be connected to a DPI encoder output.  
>>
>> The output of the DSI bridge may be another bridge, which could be i2c
>> controlled. In that case, it won't be a child of the DSI bridge. For
>> such scenarios, we might want to define an output port for the bridge.
> 
> Hm, okay. IIRC, this is something you mentioned when I asked how to
> describe input/output ports for a DSI bridge a while ago.
> 
> I'm still not sure how the links between input and output endpoint are
> supposed to be described.
> 
> For example, if you take the case where you have the DSI device
> directly described under the DSI host controller, should I create
> another node for this output port? Something like the following?
> 
> 	dsi@xxx {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			dpi_in: port@0 {
> 				reg = <0>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				endpoint@0 {
> 					remote-endpoint = <&dpi_out>;
> 				};
> 			};
> 
> 			dsi_out0: port@1 {
> 				reg = <1>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				dsi_out0: endpoint@0 {
> 					remote-endpoint = <&dsi_panel0_in>;
> 				};
> 			};
> 
> 			dsi_out0: port@2 {
> 				reg = <2>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				dsi_out1: endpoint@0 {
> 					remote-endpoint = <&dsi_panel1_in>;
> 				};
> 			};
> 		};
> 
> 		panel@0 {
> 			compatible = "...";
> 			reg = <0>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				#address-cells = <1>;
> 		                #size-cells = <0>;
> 				reg = <0>;
> 
> 				dsi_panel0_in: endpoint@0 {
> 					remote-endpoint = <&dsi_out0>;
> 				};
> 			};
> 		};
> 
> 		panel@1 {
> 			compatible = "...";
> 			reg = <1>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				#address-cells = <1>;
> 		                #size-cells = <0>;
> 				reg = <0>;
> 
> 				dsi_panel1_in: endpoint@0 {
> 					remote-endpoint = <&dsi_out1>;
> 				};
> 			};
> 		};
> 	};
> 

The ports & endpoints describe the video path, and the node child-parent
relationship describe the control path. And "port" is a physical
connector of some sort, and endpoint is a virtual channel or such.

So, you can have DSI peripherals which are either children of the DSI
bridge, and can be controlled with DSI commands. Or, you can have, say,
i2c peripherals, defined under an i2c node, which just take the video
stream from the DSI bridge. Both would have similar ports & endpoints,
but the DT nodes would be located under different parents.

Also, you can't have two output ports unless the DSI bridge has actually
multiple output pins. If the two panels are connected to the same DSI
pins, and the DSI virtual channel is used to direct the output to the
correct panel, then these should be endpoints.

 Tomi
Boris BREZILLON June 6, 2017, 12:48 p.m. UTC | #6
On Tue, 6 Jun 2017 15:40:25 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 06/06/17 12:35, Boris Brezillon wrote:
> > On Sat, 3 Jun 2017 23:43:17 +0530
> > Archit Taneja <architt@codeaurora.org> wrote:
> >   
> >> Hi,
> >>
> >> On 06/02/2017 05:34 PM, Boris Brezillon wrote:  
> >>> Document the bindings used for the Cadence DSI bridge.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>> ---
> >>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >>>  1 file changed, 55 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> new file mode 100644
> >>> index 000000000000..770c5c5b1e93
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> @@ -0,0 +1,55 @@
> >>> +Cadence DSI bridge
> >>> +==================
> >>> +
> >>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.    
> >>
> >> Is this a separate chip, or an IP integrated into SoCs?  
> > 
> > It's supposed to be integrated into SoCs.
> >   
> >> If it's the 
> >> latter, I don't think DPI on the its input side is the right term to 
> >> use. Maybe RGB would be more appropriate here.  
> > 
> > Well, the datasheet explicitly mentions DPI, and you can also send
> > pixels in YUV422 and YUV420 format on this bus, so I don't think RGB is
> > appropriate, but if you really want me to use RGB I can change that.
> > 
> > BTW, can you detail why DPI is not appropriate for internal parallel
> > busses. I don't understand why it makes a difference when the bus is exposed
> > through external pins.  
> 
> I think MIPI DPI is fine, if it is indeed MIPI DPI. But mot all parallel
> video busses are MIPI DPI.
> 
> >>> +Required subnodes:
> >>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> >>> +  Currently contains a single input port at address 0 representing the DPI
> >>> +  input. Other ports will be added later to support the SDI inputs.
> >>> +  Port 0 should be connected to a DPI encoder output.    
> >>
> >> The output of the DSI bridge may be another bridge, which could be i2c
> >> controlled. In that case, it won't be a child of the DSI bridge. For
> >> such scenarios, we might want to define an output port for the bridge.  
> > 
> > Hm, okay. IIRC, this is something you mentioned when I asked how to
> > describe input/output ports for a DSI bridge a while ago.
> > 
> > I'm still not sure how the links between input and output endpoint are
> > supposed to be described.
> > 
> > For example, if you take the case where you have the DSI device
> > directly described under the DSI host controller, should I create
> > another node for this output port? Something like the following?
> > 
> > 	dsi@xxx {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		ports {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			dpi_in: port@0 {
> > 				reg = <0>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				endpoint@0 {
> > 					remote-endpoint = <&dpi_out>;
> > 				};
> > 			};
> > 
> > 			dsi_out0: port@1 {
> > 				reg = <1>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				dsi_out0: endpoint@0 {
> > 					remote-endpoint = <&dsi_panel0_in>;
> > 				};
> > 			};
> > 
> > 			dsi_out0: port@2 {
> > 				reg = <2>;
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				dsi_out1: endpoint@0 {
> > 					remote-endpoint = <&dsi_panel1_in>;
> > 				};
> > 			};
> > 		};
> > 
> > 		panel@0 {
> > 			compatible = "...";
> > 			reg = <0>;
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			port@0 {
> > 				#address-cells = <1>;
> > 		                #size-cells = <0>;
> > 				reg = <0>;
> > 
> > 				dsi_panel0_in: endpoint@0 {
> > 					remote-endpoint = <&dsi_out0>;
> > 				};
> > 			};
> > 		};
> > 
> > 		panel@1 {
> > 			compatible = "...";
> > 			reg = <1>;
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			port@0 {
> > 				#address-cells = <1>;
> > 		                #size-cells = <0>;
> > 				reg = <0>;
> > 
> > 				dsi_panel1_in: endpoint@0 {
> > 					remote-endpoint = <&dsi_out1>;
> > 				};
> > 			};
> > 		};
> > 	};
> >   
> 
> The ports & endpoints describe the video path, and the node child-parent
> relationship describe the control path. And "port" is a physical
> connector of some sort, and endpoint is a virtual channel or such.
> 
> So, you can have DSI peripherals which are either children of the DSI
> bridge, and can be controlled with DSI commands. Or, you can have, say,
> i2c peripherals, defined under an i2c node, which just take the video
> stream from the DSI bridge. Both would have similar ports & endpoints,
> but the DT nodes would be located under different parents.
> 
> Also, you can't have two output ports unless the DSI bridge has actually
> multiple output pins. If the two panels are connected to the same DSI
> pins, and the DSI virtual channel is used to direct the output to the
> correct panel, then these should be endpoints.

Okay. Thanks for the clarification. Can you confirm that this version
is correct?

 	dsi@xxx {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
			dpi_in: port@0 {
 				reg = <0>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
 				endpoint@0 {
 					remote-endpoint = <&dpi_out>;
 				};
 			};
 
 			dsi_out: port@1 {
 				reg = <1>;
				#address-cells = <1>;
 				#size-cells = <0>;
 
 				dsi_out_vc0: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_panel0_in>;
				};

 				dsi_out_vc1: endpoint@1 {
					reg = <1>;
 					remote-endpoint = <&dsi_panel1_in>;
 				};
 			};
 		};
 
 		panel@0 {
 			compatible = "...";
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
 			port@0 {
 				#address-cells = <1>;
 		                #size-cells = <0>;
 				reg = <0>;
 
 				dsi_panel0_in: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_out_vc0>;
 				};
 			};
 		};
 
 		panel@1 {
 			compatible = "...";
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
 			port@0 {
 				#address-cells = <1>;
 		                #size-cells = <0>;
 				reg = <0>;
 
 				dsi_panel1_in: endpoint@0 {
					reg = <0>;
 					remote-endpoint = <&dsi_out_vc1>;
 				};
 			};
 		};
 	};
Tomi Valkeinen June 6, 2017, 12:58 p.m. UTC | #7
On 06/06/17 15:48, Boris Brezillon wrote:

> Okay. Thanks for the clarification. Can you confirm that this version
> is correct?
> 
>  	dsi@xxx {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
>  		ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> 			dpi_in: port@0 {
>  				reg = <0>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				endpoint@0 {
>  					remote-endpoint = <&dpi_out>;
>  				};
>  			};
>  
>  			dsi_out: port@1 {
>  				reg = <1>;
> 				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				dsi_out_vc0: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_panel0_in>;
> 				};
> 
>  				dsi_out_vc1: endpoint@1 {
> 					reg = <1>;
>  					remote-endpoint = <&dsi_panel1_in>;
>  				};
>  			};
>  		};
>  
>  		panel@0 {
>  			compatible = "...";
>  			reg = <0>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
>  			port@0 {
>  				#address-cells = <1>;
>  		                #size-cells = <0>;
>  				reg = <0>;
>  
>  				dsi_panel0_in: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_out_vc0>;
>  				};
>  			};
>  		};
>  
>  		panel@1 {
>  			compatible = "...";
>  			reg = <1>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
>  			port@0 {
>  				#address-cells = <1>;
>  		                #size-cells = <0>;
>  				reg = <0>;
>  
>  				dsi_panel1_in: endpoint@0 {
> 					reg = <0>;
>  					remote-endpoint = <&dsi_out_vc1>;
>  				};
>  			};
>  		};
>  	};
> 

Looks correct to me. I think it can be a bit shorter though:

- You don't need #address-cells and #size-cells for all. I think those
are inherited from the parent.
- If there's just one port and one endpoint, you can leave the 'reg'
out, as it's considered to be 0 by default.

So for the panel, you can have just:

port {
	dsi_panel1_in: endpoint {
		remote-endpoint = <&dsi_out_vc1>;
	};
};
Tomi Valkeinen June 6, 2017, 1:01 p.m. UTC | #8
On 06/06/17 15:37, Boris Brezillon wrote:
> Hi Tomi,
> 
> On Tue, 6 Jun 2017 15:30:26 +0300
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
>> On 02/06/17 15:04, Boris Brezillon wrote:
>>> Document the bindings used for the Cadence DSI bridge.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> ---
>>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> new file mode 100644
>>> index 000000000000..770c5c5b1e93
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
>>> @@ -0,0 +1,55 @@
>>> +Cadence DSI bridge
>>> +==================
>>> +
>>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
>>> +
>>> +Required properties:
>>> +- compatible: should be set to "cdns,dsi".
>>> +- reg: physical base address and length of the controller's registers.
>>> +- interrupts: interrupt line connected to the DSI bridge.
>>> +- clocks: DSI bridge clocks.
>>> +- clock-names: must contain "pclk" and "sysclk".  
>>
>> clock-names doesn't match the example below.
> 
> Indeed. I'll fix the example.
> 
>>
>>> +- phys: phandle link to the MIPI D-PHY controller.
>>> +- phy-names: must contain "dphy".
>>> +- #address-cells: must be set to 1.
>>> +- #size-cells: must be set to 0.
>>> +
>>> +Required subnodes:
>>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
>>> +  Currently contains a single input port at address 0 representing the DPI
>>> +  input. Other ports will be added later to support the SDI inputs.  
>>
>> Typo with "SDI".
> 
> No, the 2nd and 3rd input ports are really called SDI. Here
> is the datasheet description:
> 
> "
> SDI: 
> Serial Display Interface - this is the name of the block that is built
> to interface the Display application processor to the DSI link. This is
> a proprietary interface.
> "

Ok. Well, I think that's a bit pointless comment in the binding doc,
it'll only confuse =). Describe what the current binding is, not what
might be added later (but that can be mentioned in the commit desc if
you want).

 Tomi
Boris BREZILLON June 6, 2017, 1:06 p.m. UTC | #9
On Tue, 6 Jun 2017 16:01:45 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 06/06/17 15:37, Boris Brezillon wrote:
> > Hi Tomi,
> > 
> > On Tue, 6 Jun 2017 15:30:26 +0300
> > Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >   
> >> On 02/06/17 15:04, Boris Brezillon wrote:  
> >>> Document the bindings used for the Cadence DSI bridge.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>> ---
> >>>  .../bindings/display/bridge/cdns,dsi.txt           | 55 ++++++++++++++++++++++
> >>>  1 file changed, 55 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> new file mode 100644
> >>> index 000000000000..770c5c5b1e93
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> >>> @@ -0,0 +1,55 @@
> >>> +Cadence DSI bridge
> >>> +==================
> >>> +
> >>> +The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
> >>> +
> >>> +Required properties:
> >>> +- compatible: should be set to "cdns,dsi".
> >>> +- reg: physical base address and length of the controller's registers.
> >>> +- interrupts: interrupt line connected to the DSI bridge.
> >>> +- clocks: DSI bridge clocks.
> >>> +- clock-names: must contain "pclk" and "sysclk".    
> >>
> >> clock-names doesn't match the example below.  
> > 
> > Indeed. I'll fix the example.
> >   
> >>  
> >>> +- phys: phandle link to the MIPI D-PHY controller.
> >>> +- phy-names: must contain "dphy".
> >>> +- #address-cells: must be set to 1.
> >>> +- #size-cells: must be set to 0.
> >>> +
> >>> +Required subnodes:
> >>> +- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
> >>> +  Currently contains a single input port at address 0 representing the DPI
> >>> +  input. Other ports will be added later to support the SDI inputs.    
> >>
> >> Typo with "SDI".  
> > 
> > No, the 2nd and 3rd input ports are really called SDI. Here
> > is the datasheet description:
> > 
> > "
> > SDI: 
> > Serial Display Interface - this is the name of the block that is built
> > to interface the Display application processor to the DSI link. This is
> > a proprietary interface.
> > "  
> 
> Ok. Well, I think that's a bit pointless comment in the binding doc,
> it'll only confuse =). Describe what the current binding is, not what
> might be added later (but that can be mentioned in the commit desc if
> you want).

OK. Will do that.
Andrzej Hajda June 13, 2017, 9:02 a.m. UTC | #10
Hi,

Just spotted this thread.

On 06.06.2017 14:58, Tomi Valkeinen wrote:
> On 06/06/17 15:48, Boris Brezillon wrote:
>
>> Okay. Thanks for the clarification. Can you confirm that this version
>> is correct?
>>
>>  	dsi@xxx {
>>  		#address-cells = <1>;
>>  		#size-cells = <0>;
>>  
>>  		ports {
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>  				reg = <0>;
>>  				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  
>>  				endpoint@0 {
>>  					remote-endpoint = <&dpi_out>;
>>  				};
>>  			};
>>  
>>  			dsi_out: port@1 {
>>  				reg = <1>;
>> 				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  
>>  				dsi_out_vc0: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_panel0_in>;
>> 				};
>>
>>  				dsi_out_vc1: endpoint@1 {
>> 					reg = <1>;
>>  					remote-endpoint = <&dsi_panel1_in>;
>>  				};
>>  			};
>>  		};
>>  
>>  		panel@0 {
>>  			compatible = "...";
>>  			reg = <0>;
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  
>>  			port@0 {
>>  				#address-cells = <1>;
>>  		                #size-cells = <0>;
>>  				reg = <0>;
>>  
>>  				dsi_panel0_in: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_out_vc0>;
>>  				};
>>  			};
>>  		};
>>  
>>  		panel@1 {
>>  			compatible = "...";
>>  			reg = <1>;
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  
>>  			port@0 {
>>  				#address-cells = <1>;
>>  		                #size-cells = <0>;
>>  				reg = <0>;
>>  
>>  				dsi_panel1_in: endpoint@0 {
>> 					reg = <0>;
>>  					remote-endpoint = <&dsi_out_vc1>;
>>  				};
>>  			};
>>  		};
>>  	};
>>
> Looks correct to me. I think it can be a bit shorter though:
>
> - You don't need #address-cells and #size-cells for all. I think those
> are inherited from the parent.
> - If there's just one port and one endpoint, you can leave the 'reg'
> out, as it's considered to be 0 by default.
>
> So for the panel, you can have just:
>
> port {
> 	dsi_panel1_in: endpoint {
> 		remote-endpoint = <&dsi_out_vc1>;
> 	};
> };

In case DSI bus is used to both control and sending video signal you can
skip video links from dsi-host to dsi-child, so nodes can look like:


 	dsi@xxx {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
			dpi_in: port@0 {
 				reg = <0>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
 				endpoint@0 {
 					remote-endpoint = <&dpi_out>;
 				};
 			};
 
 		};
 
 		panel@0 {
 			compatible = "...";
 			reg = <0>;
 		};
 
 		panel@1 {
 			compatible = "...";
 			reg = <1>;
 		};
 	};



Regards
Andrzej
Archit Taneja June 14, 2017, 3:44 a.m. UTC | #11
On 06/06/2017 06:28 PM, Tomi Valkeinen wrote:
> On 06/06/17 15:48, Boris Brezillon wrote:
> 
>> Okay. Thanks for the clarification. Can you confirm that this version
>> is correct?
>>
>>   	dsi@xxx {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   
>>   		ports {
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>   				reg = <0>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				endpoint@0 {
>>   					remote-endpoint = <&dpi_out>;
>>   				};
>>   			};
>>   
>>   			dsi_out: port@1 {
>>   				reg = <1>;
>> 				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				dsi_out_vc0: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_panel0_in>;
>> 				};
>>
>>   				dsi_out_vc1: endpoint@1 {
>> 					reg = <1>;
>>   					remote-endpoint = <&dsi_panel1_in>;
>>   				};
>>   			};
>>   		};
>>   
>>   		panel@0 {
>>   			compatible = "...";
>>   			reg = <0>;
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>>   
>>   			port@0 {
>>   				#address-cells = <1>;
>>   		                #size-cells = <0>;
>>   				reg = <0>;
>>   
>>   				dsi_panel0_in: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_out_vc0>;
>>   				};
>>   			};
>>   		};
>>   
>>   		panel@1 {
>>   			compatible = "...";
>>   			reg = <1>;
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>>   
>>   			port@0 {
>>   				#address-cells = <1>;
>>   		                #size-cells = <0>;
>>   				reg = <0>;
>>   
>>   				dsi_panel1_in: endpoint@0 {
>> 					reg = <0>;
>>   					remote-endpoint = <&dsi_out_vc1>;
>>   				};
>>   			};
>>   		};
>>   	};
>>
> 
> Looks correct to me. I think it can be a bit shorter though:
> 
> - You don't need #address-cells and #size-cells for all. I think those
> are inherited from the parent.
> - If there's just one port and one endpoint, you can leave the 'reg'
> out, as it's considered to be 0 by default.
> 
> So for the panel, you can have just:
> 
> port {
> 	dsi_panel1_in: endpoint {
> 		remote-endpoint = <&dsi_out_vc1>;
> 	};
> };

Looks good to me too.

Thanks,
Archit
Boris BREZILLON June 19, 2017, 10:12 a.m. UTC | #12
On Tue, 13 Jun 2017 11:02:47 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> Hi,
> 
> Just spotted this thread.
> 
> On 06.06.2017 14:58, Tomi Valkeinen wrote:
> > On 06/06/17 15:48, Boris Brezillon wrote:
> >  
> >> Okay. Thanks for the clarification. Can you confirm that this version
> >> is correct?
> >>
> >>  	dsi@xxx {
> >>  		#address-cells = <1>;
> >>  		#size-cells = <0>;
> >>  
> >>  		ports {
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >> 			dpi_in: port@0 {
> >>  				reg = <0>;
> >>  				#address-cells = <1>;
> >>  				#size-cells = <0>;
> >>  
> >>  				endpoint@0 {
> >>  					remote-endpoint = <&dpi_out>;
> >>  				};
> >>  			};
> >>  
> >>  			dsi_out: port@1 {
> >>  				reg = <1>;
> >> 				#address-cells = <1>;
> >>  				#size-cells = <0>;
> >>  
> >>  				dsi_out_vc0: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_panel0_in>;
> >> 				};
> >>
> >>  				dsi_out_vc1: endpoint@1 {
> >> 					reg = <1>;
> >>  					remote-endpoint = <&dsi_panel1_in>;
> >>  				};
> >>  			};
> >>  		};
> >>  
> >>  		panel@0 {
> >>  			compatible = "...";
> >>  			reg = <0>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >>  
> >>  			port@0 {
> >>  				#address-cells = <1>;
> >>  		                #size-cells = <0>;
> >>  				reg = <0>;
> >>  
> >>  				dsi_panel0_in: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_out_vc0>;
> >>  				};
> >>  			};
> >>  		};
> >>  
> >>  		panel@1 {
> >>  			compatible = "...";
> >>  			reg = <1>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <0>;
> >>  
> >>  			port@0 {
> >>  				#address-cells = <1>;
> >>  		                #size-cells = <0>;
> >>  				reg = <0>;
> >>  
> >>  				dsi_panel1_in: endpoint@0 {
> >> 					reg = <0>;
> >>  					remote-endpoint = <&dsi_out_vc1>;
> >>  				};
> >>  			};
> >>  		};
> >>  	};
> >>  
> > Looks correct to me. I think it can be a bit shorter though:
> >
> > - You don't need #address-cells and #size-cells for all. I think those
> > are inherited from the parent.
> > - If there's just one port and one endpoint, you can leave the 'reg'
> > out, as it's considered to be 0 by default.
> >
> > So for the panel, you can have just:
> >
> > port {
> > 	dsi_panel1_in: endpoint {
> > 		remote-endpoint = <&dsi_out_vc1>;
> > 	};
> > };  
> 
> In case DSI bus is used to both control and sending video signal you can
> skip video links from dsi-host to dsi-child, so nodes can look like:
> 
> 
>  	dsi@xxx {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
>  		ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> 			dpi_in: port@0 {
>  				reg = <0>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
>  				endpoint@0 {
>  					remote-endpoint = <&dpi_out>;
>  				};
>  			};
>  
>  		};
>  
>  		panel@0 {
>  			compatible = "...";
>  			reg = <0>;
>  		};
>  
>  		panel@1 {
>  			compatible = "...";
>  			reg = <1>;
>  		};
>  	};
> 

Does that mean I should make port1 (AKA DSI ouput port) optional?
IMHO, it's clearer when these links are explicitly described in the DT,
but maybe there are good reasons to keep it implicit for the "control
through DSI" case.

Tomi, Archit, any opinion on this?

Regards,

Boris
Archit Taneja June 20, 2017, 6:56 a.m. UTC | #13
On 06/19/2017 03:42 PM, Boris Brezillon wrote:
> On Tue, 13 Jun 2017 11:02:47 +0200
> Andrzej Hajda <a.hajda@samsung.com> wrote:
> 
>> Hi,
>>
>> Just spotted this thread.
>>
>> On 06.06.2017 14:58, Tomi Valkeinen wrote:
>>> On 06/06/17 15:48, Boris Brezillon wrote:
>>>   
>>>> Okay. Thanks for the clarification. Can you confirm that this version
>>>> is correct?
>>>>
>>>>   	dsi@xxx {
>>>>   		#address-cells = <1>;
>>>>   		#size-cells = <0>;
>>>>   
>>>>   		ports {
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>> 			dpi_in: port@0 {
>>>>   				reg = <0>;
>>>>   				#address-cells = <1>;
>>>>   				#size-cells = <0>;
>>>>   
>>>>   				endpoint@0 {
>>>>   					remote-endpoint = <&dpi_out>;
>>>>   				};
>>>>   			};
>>>>   
>>>>   			dsi_out: port@1 {
>>>>   				reg = <1>;
>>>> 				#address-cells = <1>;
>>>>   				#size-cells = <0>;
>>>>   
>>>>   				dsi_out_vc0: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_panel0_in>;
>>>> 				};
>>>>
>>>>   				dsi_out_vc1: endpoint@1 {
>>>> 					reg = <1>;
>>>>   					remote-endpoint = <&dsi_panel1_in>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   
>>>>   		panel@0 {
>>>>   			compatible = "...";
>>>>   			reg = <0>;
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>>   
>>>>   			port@0 {
>>>>   				#address-cells = <1>;
>>>>   		                #size-cells = <0>;
>>>>   				reg = <0>;
>>>>   
>>>>   				dsi_panel0_in: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_out_vc0>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   
>>>>   		panel@1 {
>>>>   			compatible = "...";
>>>>   			reg = <1>;
>>>>   			#address-cells = <1>;
>>>>   			#size-cells = <0>;
>>>>   
>>>>   			port@0 {
>>>>   				#address-cells = <1>;
>>>>   		                #size-cells = <0>;
>>>>   				reg = <0>;
>>>>   
>>>>   				dsi_panel1_in: endpoint@0 {
>>>> 					reg = <0>;
>>>>   					remote-endpoint = <&dsi_out_vc1>;
>>>>   				};
>>>>   			};
>>>>   		};
>>>>   	};
>>>>   
>>> Looks correct to me. I think it can be a bit shorter though:
>>>
>>> - You don't need #address-cells and #size-cells for all. I think those
>>> are inherited from the parent.
>>> - If there's just one port and one endpoint, you can leave the 'reg'
>>> out, as it's considered to be 0 by default.
>>>
>>> So for the panel, you can have just:
>>>
>>> port {
>>> 	dsi_panel1_in: endpoint {
>>> 		remote-endpoint = <&dsi_out_vc1>;
>>> 	};
>>> };
>>
>> In case DSI bus is used to both control and sending video signal you can
>> skip video links from dsi-host to dsi-child, so nodes can look like:
>>
>>
>>   	dsi@xxx {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   
>>   		ports {
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>> 			dpi_in: port@0 {
>>   				reg = <0>;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>>   				endpoint@0 {
>>   					remote-endpoint = <&dpi_out>;
>>   				};
>>   			};
>>   
>>   		};
>>   
>>   		panel@0 {
>>   			compatible = "...";
>>   			reg = <0>;
>>   		};
>>   
>>   		panel@1 {
>>   			compatible = "...";
>>   			reg = <1>;
>>   		};
>>   	};
>>
> 
> Does that mean I should make port1 (AKA DSI ouput port) optional?
> IMHO, it's clearer when these links are explicitly described in the DT,
> but maybe there are good reasons to keep it implicit for the "control
> through DSI" case.
> 
> Tomi, Archit, any opinion on this?

I guess there isn't any harm in having the links explicitly described. It's
just that those ports won't be used by the driver in the "control through DSI"
case.

For the MSM DSI host bindings, we actually keep the DSI 'data-lanes' param in the
DSI output port, so it's mandatory even if the panel/bridge is controlled via
the host DSI bus.

Andrzej,

Are there any reasons why keeping the host-to-child links in the "control through
DSI" case could be harmful?

Archit

> 
> Regards,
> 
> Boris
>
Andrzej Hajda June 20, 2017, 7:22 a.m. UTC | #14
On 20.06.2017 08:56, Archit Taneja wrote:
>
> On 06/19/2017 03:42 PM, Boris Brezillon wrote:
>> On Tue, 13 Jun 2017 11:02:47 +0200
>> Andrzej Hajda <a.hajda@samsung.com> wrote:
>>
>>> Hi,
>>>
>>> Just spotted this thread.
>>>
>>> On 06.06.2017 14:58, Tomi Valkeinen wrote:
>>>> On 06/06/17 15:48, Boris Brezillon wrote:
>>>>   
>>>>> Okay. Thanks for the clarification. Can you confirm that this version
>>>>> is correct?
>>>>>
>>>>>   	dsi@xxx {
>>>>>   		#address-cells = <1>;
>>>>>   		#size-cells = <0>;
>>>>>   
>>>>>   		ports {
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>> 			dpi_in: port@0 {
>>>>>   				reg = <0>;
>>>>>   				#address-cells = <1>;
>>>>>   				#size-cells = <0>;
>>>>>   
>>>>>   				endpoint@0 {
>>>>>   					remote-endpoint = <&dpi_out>;
>>>>>   				};
>>>>>   			};
>>>>>   
>>>>>   			dsi_out: port@1 {
>>>>>   				reg = <1>;
>>>>> 				#address-cells = <1>;
>>>>>   				#size-cells = <0>;
>>>>>   
>>>>>   				dsi_out_vc0: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_panel0_in>;
>>>>> 				};
>>>>>
>>>>>   				dsi_out_vc1: endpoint@1 {
>>>>> 					reg = <1>;
>>>>>   					remote-endpoint = <&dsi_panel1_in>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   
>>>>>   		panel@0 {
>>>>>   			compatible = "...";
>>>>>   			reg = <0>;
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>>   
>>>>>   			port@0 {
>>>>>   				#address-cells = <1>;
>>>>>   		                #size-cells = <0>;
>>>>>   				reg = <0>;
>>>>>   
>>>>>   				dsi_panel0_in: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_out_vc0>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   
>>>>>   		panel@1 {
>>>>>   			compatible = "...";
>>>>>   			reg = <1>;
>>>>>   			#address-cells = <1>;
>>>>>   			#size-cells = <0>;
>>>>>   
>>>>>   			port@0 {
>>>>>   				#address-cells = <1>;
>>>>>   		                #size-cells = <0>;
>>>>>   				reg = <0>;
>>>>>   
>>>>>   				dsi_panel1_in: endpoint@0 {
>>>>> 					reg = <0>;
>>>>>   					remote-endpoint = <&dsi_out_vc1>;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>   	};
>>>>>   
>>>> Looks correct to me. I think it can be a bit shorter though:
>>>>
>>>> - You don't need #address-cells and #size-cells for all. I think those
>>>> are inherited from the parent.
>>>> - If there's just one port and one endpoint, you can leave the 'reg'
>>>> out, as it's considered to be 0 by default.
>>>>
>>>> So for the panel, you can have just:
>>>>
>>>> port {
>>>> 	dsi_panel1_in: endpoint {
>>>> 		remote-endpoint = <&dsi_out_vc1>;
>>>> 	};
>>>> };
>>> In case DSI bus is used to both control and sending video signal you can
>>> skip video links from dsi-host to dsi-child, so nodes can look like:
>>>
>>>
>>>   	dsi@xxx {
>>>   		#address-cells = <1>;
>>>   		#size-cells = <0>;
>>>   
>>>   		ports {
>>>   			#address-cells = <1>;
>>>   			#size-cells = <0>;
>>> 			dpi_in: port@0 {
>>>   				reg = <0>;
>>>   				#address-cells = <1>;
>>>   				#size-cells = <0>;
>>>   
>>>   				endpoint@0 {
>>>   					remote-endpoint = <&dpi_out>;
>>>   				};
>>>   			};
>>>   
>>>   		};
>>>   
>>>   		panel@0 {
>>>   			compatible = "...";
>>>   			reg = <0>;
>>>   		};
>>>   
>>>   		panel@1 {
>>>   			compatible = "...";
>>>   			reg = <1>;
>>>   		};
>>>   	};
>>>
>> Does that mean I should make port1 (AKA DSI ouput port) optional?
>> IMHO, it's clearer when these links are explicitly described in the DT,
>> but maybe there are good reasons to keep it implicit for the "control
>> through DSI" case.
>>
>> Tomi, Archit, any opinion on this?
> I guess there isn't any harm in having the links explicitly described. It's
> just that those ports won't be used by the driver in the "control through DSI"
> case.
>
> For the MSM DSI host bindings, we actually keep the DSI 'data-lanes' param in the
> DSI output port, so it's mandatory even if the panel/bridge is controlled via
> the host DSI bus.
>
> Andrzej,
>
> Are there any reasons why keeping the host-to-child links in the "control through
> DSI" case could be harmful?

They are redundant - DSI bus already describes the 'link', so there are
classical potential issues connected with redundancy:
- which info should be parsed by the driver,
- what to do if links provide different information than DSI bus.
And as I understand current device-tree policy is to avoid them in such
case, see [1].

[1]: http://marc.info/?l=dri-devel&m=148354108702517&w=2

Regards
Andrzej


>
> Archit
>
>> Regards,
>>
>> Boris
>>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
new file mode 100644
index 000000000000..770c5c5b1e93
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
@@ -0,0 +1,55 @@ 
+Cadence DSI bridge
+==================
+
+The Cadence DSI bridge is a DPI to DSI bridge supporting up to 4 DSI lanes.
+
+Required properties:
+- compatible: should be set to "cdns,dsi".
+- reg: physical base address and length of the controller's registers.
+- interrupts: interrupt line connected to the DSI bridge.
+- clocks: DSI bridge clocks.
+- clock-names: must contain "pclk" and "sysclk".
+- phys: phandle link to the MIPI D-PHY controller.
+- phy-names: must contain "dphy".
+- #address-cells: must be set to 1.
+- #size-cells: must be set to 0.
+
+Required subnodes:
+- ports: Ports as described in Documentation/devicetree/bindings/graph.txt.
+  Currently contains a single input port at address 0 representing the DPI
+  input. Other ports will be added later to support the SDI inputs.
+  Port 0 should be connected to a DPI encoder output.
+- one subnode per DSI device connected on the DSI bus. Each DSI device should
+  contain a reg property encoding its address on the bus.
+
+Example:
+
+	dsi0: dsi@fd0c0000 {
+		compatible = "cdns,dsi";
+		reg = <0x0 0xfd0c0000 0x0 0x1000>;
+		clocks = <&pclk>, <&sysclk>;
+		clock-names = "pclk", "hclk";
+		interrupts = <1>;
+		phys = <&dphy1>;
+		phy-names = "dphy";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				dsi0_dpi_input: endpoint {
+					remote-endpoint = <&xxx_dpi_output>;
+				};
+			};
+		};
+
+		panel: dsi-dev@0 {
+			compatible = "<vendor,panel>";
+			reg = <0>;
+		};
+	};
+