Hi Javier,
2015-11-24 22:19 GMT+09:00 Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>:
> 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@xxxxxxxxxxxxxxx>:
>>>>> Hello,
>>>>>
>>>>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>>
>>>>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@xxxxxxxxxxxxx>:
>>>>>>> Hi Inki,
>>>>>>>
>>>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@xxxxxxxxx> wrote:
>>>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>:
>>>>>>>>> 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?
>>
>
> 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 a lot. I will post the patch set soon CCing you. :)
Thanks,
Inki Dae
>
>> Thanks,
>> Inki Dae
>>
>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
