Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae <inki.dae@samsung.com>]

Hi Javier,

2015년 11월 24일 03:38에 Javier Martinez Canillas 이(가) 쓴 글:
> Hello Inki,
> 
> On 11/23/2015 01:47 PM, Inki Dae wrote:
>> 2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>> Hello,
>>>
>>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>>> Hi Daniel,
>>>>
>>>>
>>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>>>> Hi Inki,
>>>>>
>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>>>> The boot log says,
>>>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>>>
>>>>>>>
>>>>>>> This message is a red herring for the reported issue, the message is also
>>>>>>> present when the machine boots and the display is brought correctly.
>>>>>>>
>>>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>>>
>>>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>>>> &dp {
>>>>>>>>       status = "okay";
>>>>>>>>       pinctrl-names = "default";
>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>       samsung,link-rate = <0x06>;
>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>
>>>>>>>>       ports {
>>>>>>>>               port@0 {
>>>>>>>>                       dp_out: endpoint {
>>>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>>>                       };
>>>>>>>>               };
>>>>>>>>       };
>>>>>>>> };
>>>>>>>>
>>>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>>>> &dp {
>>>>>>>>       status = "okay";
>>>>>>>>       pinctrl-names = "default";
>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>       panel = <&panel>;
>>>>>>>> };
>>>>>>>>
>>>>>>>
>>>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>>>
>>>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>>>> that points to the panel.
>>>>>>>
>>>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>>>
>>>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>>>> exynos5800-peach-pit.dts.
>>>>>>
>>>>>> From a common-sense standpoint, there is no any reason to build
>>>>>> and probe dp driver if the board doesn't use dp hardware.
>>>>>
>>>>> I agree with what you say, but unfortunately you've slightly misread
>>>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>>>> which I am writing this) has an eDP panel directly connected. The DT
>>>
>>> Thanks a lot Daniel for clarifying my comments to Inki :)
>>>
>>>>> describes both the eDP connector from FIMD and the eDP panel, except
>>>>> that there is no connection between the DT nodes.
>>>
>>> There *is* a connection between the FIMD eDP connector and the eDP panel
>>> nodes but these are connected using a phandle while the connection for
>>> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
>>> bindings (Documentation/devicetree/bindings/graph.txt).
>>>
>>> And also the connection between the eDP/LVDS bridge and the LVDS panel
>>> is using an OF graph node, so what I meant is that it would be much more
>>> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
>>> connections used the OF graph DT bindings.
>>>
>>>>
>>>> Right. I misread what Javier said. :)
>>>>
>>>>>
>>>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>>>
>>>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>>>
>>>>>> It's really not good. This would make it more complex. The best
>>>>>> solution is just to
>>>>>> remove the dt node from the device tree file.
>>>>>
>>>>> Given the above, not really. Javier's patch seems correct to me - as
>>>>> you can see, there is a panel node, and that is the panel that's
>>>>> really connected.
>>>>
>>>> Indeed. Javier's patch will correct it.
>>>>
>>>
>>> Just to be clear, my patch is not correct since the Exynos DP driver and
>>> its DT binding does not support to connect an FIMD eDP connector to an
>>> eDP panel directly using OF graph ports / endpoints (only a phandle). But
>>> is an example of how the DT will look like if we extend to support that.
>>
>> Yes, you added just a port node for the panel device and removed a panel
>> property from the device tree file so now dp driver cannot get a device node
>> object of panel node because now dp driver isn't considered for it yet.
>>
>> I think there are two ways to correct it. One is,
>> 1. Add a port node for the panel device to the device tree file.
>> 2. Add of_graph dt bindings support for getting the panel node to dp driver
>>    and remove existing of_parse_phandle function call for getting a device
>>    node object for the panel device.
>>
> 
> Exactly.
>  
>> Other is,
>> 1. Revive a panel property and remove the port node you added.
>>
> 
> Yes, this is the current code that works. Is just that is not consistent but
> I don't really mind. I just wanted to explain why the DTS was different for
> both boards but it seems that I created more confusion than anything else :)
>  
>> In addition, it seems that existing bridge of_graph dt bindings codes of now
>> dp driver should be modified like below,
>>
>> endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
>> if (endpoint) {
>>         bridge_node = of_graph_get_remote_port_parent(endpoint);
>>         if (bridge_node) {
>>                 dp->bridge = of_drm_find_bridge(bridge_node);
>>                 of_node_put(bridge_node);
>>                 if (!dp->bridge)
>>                         return -EPROBE_DEFER;
>>         } else {
>>                 DRM_ERROR("has no port node for the bridge deivce");
>>                 return -ENXIO;
>>         }
>> }
>>
>> If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
>> shouldn't be NULL.
>>
>> The former looks more reasonable to me.
>>
> 
> I'm not too familiar with the OF graph API but I agree that returning a
> -EPROBE_DEFER when of_graph_get_remote_port_parent() returns NULL seems
> like the wrong thing to do.
> 
> Now I don't know if -ENXIO is the right errno code, maybe -EINVAL (since
> means the DTS is invalid)? or maybe just omit that case as it is ommited
> if of_graph_get_next_endpoint() fails?
> 
>>>
>>> IIRC at the beginning only eDP -> panel was supported and the phandle
>>> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
>>> case was needed, then a bridge phandle was added but Ajay was asked to
>>> use OF graph instead a phandle and we ended with different approaches
>>> to connect components depending if a bridge is used or not.
>>
>> Well, wouldn't it be enough to remove the panel phandle relevant codes
>> from dp driver and add of_graph dt bindings support for the panel device
>> to the dp driver instead?
>>
> 
> The problem is that removing the panel phandle is not an option without
> breaking DT backward compatibility since now an eDP -> panel lookup by
> using a phandle is a DT ABI and old DTBs could be shipped that use it.

Right. The backward compatibility should be kept.
For this, I think we could update the dp driver like below,

	panel_node = NULL;
	
	/* This is for the backward compatibility. */
	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
	if (panel_node) {
		...
	} else {
		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
		if (endpoint) {
			panel_node = of_graph_get_remote_port_parent(endpoint);
			if (panel_node) {
				...
			} else {
				...
			}
		}
	}

	endpoint = of_graph_get_next_endpoint(dev->of_node, panel_node);
	...

With the change, we could not only follow the graph concept but also keep the backward compatibility.
Javier, do you have other opinion?

Thanks,
Inki Dae

> 
> So even if the driver and DT binding are extended to allow an eDP -> panel
> lookup using ports and endpoints, both approaches have to be kept in the
> driver and DT ABI so I don't think the complexity is worth it just for the
> sake of consistency.
> 
>> Thanks,
>> Inki Dae
>>
> 
> Best regards,
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel