[PATCH v2 0/7] drm/exynos: add pm runtime support
Javier Martinez Canillas
javier at osg.samsung.com
Tue Nov 24 05:19:11 PST 2015
Hello Inki,
On 11/23/2015 11:28 PM, Inki Dae wrote:
> 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 at 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 at fooishbar.org>:
>>>>>> Hi Inki,
>>>>>>
>>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki at gmail.com> wrote:
>>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier at 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 at 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 at 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?
>
Assuming you can make a distinction if the endpoint is a panel or a bridge,
then yes, I agree with the idea of the patch. Please feel free to cc me if
you post such a patch and I'll gladly test it on my Exynos5800 Peach Pi.
> Thanks,
> Inki Dae
>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
More information about the dri-devel
mailing list