Re: [RFC v2 2/2] dt-bindings: mipi-dsi: Add dual-channel DSI related info

["Heiko Stübner" <heiko@sntech.de>]

Hi Archit,

Am Mittwoch, 6. Juni 2018, 12:21:16 CEST schrieb Archit Taneja:
> On Wednesday 06 June 2018 02:00 PM, Heiko Stübner wrote:
> > Am Mittwoch, 6. Juni 2018, 07:59:29 CEST schrieb Archit Taneja:
> >> On Monday 04 June 2018 05:47 PM, Heiko Stuebner wrote:
> >>> Am Donnerstag, 18. Januar 2018, 05:53:55 CEST schrieb Archit Taneja:
> >>>> Add binding info for peripherals that support dual-channel DSI. Add
> >>>> corresponding optional bindings for DSI host controllers that may
> >>>> be configured in this mode. Add an example of an I2C controlled
> >>>> device operating in dual-channel DSI mode.
> >>>> 
> >>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >>> 
> >>> Looks like a great solution for that problem, so
> >>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> >>> 
> >>> As I'm looking into that for my rk3399-scarlet device right now and
> >>> couldn't find this patchset in the kernel yet, is it planned to
> >>> merge or refresh these binding changes or were problems encountered.
> >>> 
> >>> At least an Ack/Review from Rob seems to be missing.
> >> 
> >> I forgot about these patches. Rob had reviewed the first one in
> >> the set the second one still needed an Ack. I'll post a v3
> >> that adds the Reviewed-bys and fixes a small typo.
> > 
> > very nice ... because it looks like yesterday I managed to make the
> > Rockchip dsi work in dual mode following this.
> > 
> > But one question came up, do you really want two input ports on the panel
> > side? I.e. hardware-wise, I guess the panel will have one 8-lane or so
> > input thatonly gets split up on the soc side onto 2 dsi controllers?
> 
> I think all dual DSI panels actually have 2 DSI controllers/parsers
> within them, one on each port. The MIPI DSI spec doesn't support 8
> lanes. Also, the pixels coming out of the host are distributed among
> the lanes differently than what would have been the case with a
> 'theoretical' 8 lane receiver.
> 
> Other than that, some dual DSI panels only accept DSI commands on the
> 'master' port, where as others expect the same command to be sent across
> both the ports.
> 
> Therefore, I think it's better to represent dual DSI panels having 2
> DSI input ports.
> 
> Your DT looks good to me.

Hmm, that doesn't match up then ;-) ... as my dt uses 2 endpoints
in one port for the dsi-links.

The issue I see with using ports and not endpoints for dual-dsi links
is with distinguishing between input and output ports.

For a panel that's easy, as you every port will be an input port and if
you have 2, it's supposed dual-dsi. But for example I guess there might
exist some dual-dsi-to-something bridges, where you would end up
with say 3 (or even more) ports ... two dual-dsi inputs and 1 or more
outputs.


While the following argument might not be 100% valid from a dt-purity
standpoint implementing this might get hairy for _any_ operating system,
as you will need each panel/bridge to tell what the ports are used for.

I.e. in my endpoint based mapping, right now I have this nice and generic
WIP function to parse the of_graph and get the master+slave nodes:

https://github.com/mmind/linux-rockchip/blob/tmp/rk3399-gru-bob-scarlet/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#L697
[0]

So I guess my proposal would be to have one port for inputs
and one port for outputs for dsi peripherals, with possibly
multiple endpoints in each.


Heiko


[0] github seems to have reliability problems, so for reference my
parsing function:

static int dw_mipi_dsi_is_dual(struct dw_mipi_dsi_rockchip *dsi,
			struct device_node **master, struct device_node **slave)
{
	struct device_node *local_ep, *remote_port, *ep;
	struct device_node *ctrls[2] = { NULL, NULL };
	int num = 0, ret = 0, idx;

	/* get local panel endpoint of the dsi controller */
	local_ep = of_graph_get_endpoint_by_regs(dsi->dev->of_node, 1, 0);
	if (!local_ep) {
		DRM_DEV_ERROR(dsi->dev, "couldn't find local panel endpoint\n");
		return -ENXIO;
	}

	/* get panel port */
	remote_port = of_graph_get_remote_port_parent(local_ep);
	of_node_put(local_ep);
	if (!remote_port) {
		DRM_DEV_ERROR(dsi->dev, "couldn't find panel port\n");
		return -ENXIO;
	}

	/* check other endpoints */
	for_each_endpoint_of_node(remote_port, ep) {
		struct device_node *np = of_graph_get_remote_port_parent(ep);

		if (!np)
			continue;

		idx = of_property_read_bool(np, "clock-master");

		/*
		 * Either master or slave already defined, drop refcnt
		 * but catch errors only after the full loop.
		 */
		if (ctrls[idx])
			of_node_put(np);
		else
			ctrls[idx] = np;

		num++;
	}
	of_node_put(remote_port);

	if (num > 2) {
		DRM_DEV_ERROR(dsi->dev, "too many dsi devices linked\n");
		ret = -EINVAL;
		goto cleanup;
	}

	/* nothing to do */
	if (num < 1) {
		ret = 0;
		goto cleanup;
	}

	if (!ctrls[1]) {
		DRM_DEV_ERROR(dsi->dev, "no master defined in dual-dsi\n");
		ret = -ENODEV;
		goto cleanup;
	}

	if (!ctrls[0]) {
		DRM_DEV_ERROR(dsi->dev, "no slave defined in dual-dsi\n");
		ret = -ENODEV;
		goto cleanup;
	}

	*master = ctrls[1];
	*slave = ctrls[0];

	return 1;

cleanup:
	for (idx = 0; idx < 2; idx++)
		if (ctrls[idx])
			of_node_put(ctrls[idx]);
	return ret;
}

> > So right now I'm operating with a devicetree like
> > 
> > &mipi_dsi {
> > 
> >          status = "okay";
> >          clock-master;
> >          
> >          ports {
> >          
> >                  mipi_out: port@1 {
> >                  
> >                          reg = <1>;
> >                          
> >                          mipi_out_panel: endpoint {
> >                          
> >                                  remote-endpoint = <&mipi_in_panel>;
> >                          
> >                          };
> >                  
> >                  };
> >          
> >          };
> >          
> >          mipi_panel: panel@0 {
> > 		  
> > 		  compatible = "innolux,p097pfg";
> > 		  
> >                  reg = <0>;
> >                  backlight = <&backlight>;
> >                  enable-gpios = <&gpio4 25 GPIO_ACTIVE_HIGH>;
> >                  pinctrl-names = "default";
> >                  pinctrl-0 = <&display_rst_l>;
> >                  
> >                  port {
> >                  
> >                          #address-cells = <1>;
> >                          #size-cells = <0>;
> >                          
> >                          mipi_in_panel: endpoint@0 {
> >                          
> >                                  reg = <0>;
> >                                  remote-endpoint = <&mipi_out_panel>;
> >                          
> >                          };
> >                          
> >                          mipi1_in_panel: endpoint@1 {
> >                          
> >                                  reg = <1>;
> >                                  remote-endpoint = <&mipi1_out_panel>;
> >                          
> >                          };
> >                  
> >                  };
> >          
> >          };
> > 
> > };
> > 
> > &mipi_dsi1 {
> > 
> >          status = "okay";
> >          
> >          ports {
> >          
> >                  mipi1_out: port@1 {
> >                  
> >                          reg = <1>;
> >                          
> >                          mipi1_out_panel: endpoint {
> >                          
> >                                  remote-endpoint = <&mipi1_in_panel>;
> >                          
> >                          };
> >                  
> >                  };
> >          
> >          };
> > 
> > };
> > 
> > 
> > I guess it is a matter of preference on what reflects the hardware
> > best, so maybe that's Robs call?
> > 
> > 
> > Heiko
> > 
> >>>> ---
> >>>> v2:
> >>>> - Specify that clock-master is a boolean property.
> >>>> - Drop/add unit-address and #*-cells where applicable.
> >>>> 
> >>>>    .../devicetree/bindings/display/mipi-dsi-bus.txt   | 80
> >>>>    ++++++++++++++++++++++ 1 file changed, 80 insertions(+)
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>> b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt index
> >>>> 94fb72cb916f..7a3abbedb3fa 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>> +++ b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>> 
> >>>> @@ -29,6 +29,13 @@ Required properties:
> >>>>    - #size-cells: Should be 0. There are cases where it makes sense to
> >>>>    use
> >>>>    a
> >>>>    
> >>>>      different value here. See below.
> >>>> 
> >>>> +Optional properties:
> >>>> +- clock-master: boolean. Should be enabled if the host is being used
> >>>> in
> >>>> +  conjunction with another DSI host to drive the same peripheral.
> >>>> Hardware
> >>>> +  supporting such a configuration generally requires the data on both
> >>>> the busses +  to be driven by the same clock. Only the DSI host
> >>>> instance
> >>>> controlling this +  clock should contain this property.
> >>>> +
> >>>> 
> >>>>    DSI peripheral
> >>>>    ==============
> >>>> 
> >>>> @@ -62,6 +69,16 @@ primary control bus, but are also connected to a DSI
> >>>> bus (mostly for the data>>
> >>>> 
> >>>>    path). Connections between such peripherals and a DSI host can be
> >>>>    represented using the graph bindings [1], [2].
> >>>> 
> >>>> +Peripherals that support dual channel DSI
> >>>> +-----------------------------------------
> >>>> +
> >>>> +Peripherals with higher bandwidth requirements can be connected to 2
> >>>> DSI
> >>>> +busses. Each DSI bus/channel drives some portion of the pixel data
> >>>> (generally +left/right half of each line of the display, or even/odd
> >>>> lines of the display). +The graph bindings should be used to represent
> >>>> the multiple DSI busses that are +connected to this peripheral. Each
> >>>> DSI
> >>>> host's output endpoint can be linked to +an input endpoint of the DSI
> >>>> peripheral.
> >>>> +
> >>>> 
> >>>>    [1] Documentation/devicetree/bindings/graph.txt
> >>>>    [2] Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> 
> >>>> @@ -71,6 +88,8 @@ Examples
> >>>> 
> >>>>      with different virtual channel configurations.
> >>>>    
> >>>>    - (4) is an example of a peripheral on a I2C control bus connected
> >>>>    with
> >>>>    to
> >>>>    
> >>>>      a DSI host using of-graph bindings.
> >>>> 
> >>>> +- (5) is an example of 2 DSI hosts driving a dual-channel DSI
> >>>> peripheral,
> >>>> +  which uses I2C as its primary control bus.
> >>>> 
> >>>>    1)
> >>>>    
> >>>>    	dsi-host {
> >>>> 
> >>>> @@ -153,3 +172,64 @@ Examples
> >>>> 
> >>>>    			};
> >>>>    		
> >>>>    		};
> >>>>    	
> >>>>    	};
> >>>> 
> >>>> +
> >>>> +5)
> >>>> +	i2c-host {
> >>>> +		dsi-bridge@35 {
> >>>> +			compatible = "...";
> >>>> +			reg = <0x35>;
> >>>> +
> >>>> +			ports {
> >>>> +				#address-cells = <1>;
> >>>> +				#size-cells = <0>;
> >>>> +
> >>>> +				port@0 {
> >>>> +					reg = <0>;
> >>>> +					dsi0_in: endpoint {
> >>>> +						remote-endpoint = <&dsi0_out>;
> >>>> +					};
> >>>> +				};
> >>>> +
> >>>> +				port@1 {
> >>>> +					reg = <1>;
> >>>> +					dsi1_in: endpoint {
> >>>> +						remote-endpoint = <&dsi1_out>;
> >>>> +					};
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +
> >>>> +	dsi0-host {
> >>>> +		...
> >>>> +
> >>>> +		/*
> >>>> +		 * this DSI instance drives the clock for both the host
> >>>> +		 * controllers
> >>>> +		 */
> >>>> +		clock-master;
> >>>> +
> >>>> +		ports {
> >>>> +			...
> >>>> +
> >>>> +			port {
> >>>> +				dsi0_out: endpoint {
> >>>> +					remote-endpoint = <&dsi0_in>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +
> >>>> +	dsi1-host {
> >>>> +		...
> >>>> +
> >>>> +		ports {
> >>>> +			...
> >>>> +
> >>>> +			port {
> >>>> +				dsi1_out: endpoint {
> >>>> +					remote-endpoint = <&dsi1_in>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>> 
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
> >>> in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel