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 <jav...@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 <dan...@fooishbar.org>: >>>>> Hi Inki, >>>>> >>>>> On 21 November 2015 at 09:38, Inki Dae <daei...@gmail.com> wrote: >>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas >>>>>> <jav...@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, > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html