| Message ID | 564F4DF5.2010301@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>: > Hello Inki, > > On 11/20/2015 08:13 AM, Inki Dae wrote: >> >> >> 2015? 11? 20? 19:59? Inki Dae ?(?) ? ?: >>> Hi Javier, >>> >>> 2015? 11? 20? 00:51? Javier Martinez Canillas ?(?) ? ?: >>>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote: >>>>>>> >>>>>> >>>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi >>>>>> Chromebook (tested myself) and seems to be the cause of other Exynos >>>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail >>>>>> >>>>>> [snip] >>>>>> >>>>>>> drm/exynos: add pm_runtime to Mixer >>>>>>> drm/exynos: add pm_runtime to FIMD >>>>>> >>>>>> I had to revert these patches in order to get the machine in a bootable >>>>>> state again, the sha1 hash for these patches in next-20151119 are: >>>>>> >>>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD >>>> >>>> On a closer look, only reverting the FIMD patch is enough >>>> to make at least the Exynos5800 Peach Pi to boot again. >>> >>> Thanks for report. >>> >>> I assume that the issue is because above patch removed 'suspended' variable >>> for checking the suspend status in runtime so I revived it. >>> >>> I'm not sure that the change could resolve the issue. Could you test it >>> with the change again? I have no Exynos5800 Peach Pi board. :( >>> >>> For this, I pushed it to below exynos-drm/for-next branch, >>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c >>> >>> If the issue is resolved by the change then I will modify other patches for >>> DECON series. And if really so, there may be a corner case we missed. >> >> Oops, I found out one error at the boot log, >> http://storage.kernelci.org/next/next-20151120/arm-multi_v7_defconfig+CONFIG_LKDTM=y/lab-collabora/boot-exynos5800-peach-pi.html >> >> 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. > 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. Thanks, Inki Dae > > diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts > index 7b018e451880..9c6fd7314ee0 100644 > --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts > +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts > @@ -122,6 +122,12 @@ > compatible = "auo,b133htn01"; > power-supply = <&tps65090_fet6>; > backlight = <&backlight>; > + > + port { > + panel_in: endpoint { > + remote-endpoint = <&dp_out>; > + }; > + }; > }; > > mmc1_pwrseq: mmc1_pwrseq { > @@ -148,7 +154,14 @@ > samsung,link-rate = <0x0a>; > samsung,lane-count = <2>; > samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; > - panel = <&panel>; > + > + ports { > + port@0 { > + dp_out: endpoint { > + remote-endpoint = <&panel_in>; > + }; > + }; > + }; > }; > > &fimd { > > Best regards, > -- > Javier Martinez Canillas > Open Source Group > Samsung Research America > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 describes both the eDP connector from FIMD and the eDP panel, except that there is no connection between the DT nodes. >> 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. >> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts >> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts >> @@ -122,6 +122,12 @@ >> compatible = "auo,b133htn01"; >> power-supply = <&tps65090_fet6>; >> backlight = <&backlight>; >> + >> + port { >> + panel_in: endpoint { >> + remote-endpoint = <&dp_out>; >> + }; >> + }; >> }; >> >> mmc1_pwrseq: mmc1_pwrseq { >> @@ -148,7 +154,14 @@ >> samsung,link-rate = <0x0a>; >> samsung,lane-count = <2>; >> samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; >> - panel = <&panel>; >> + >> + ports { >> + port@0 { >> + dp_out: endpoint { >> + remote-endpoint = <&panel_in>; >> + }; >> + }; >> + }; >> }; Cheers, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > describes both the eDP connector from FIMD and the eDP panel, except > that there is no connection between the DT nodes. 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. Thanks, Inki Dae > >>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts >>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts >>> @@ -122,6 +122,12 @@ >>> compatible = "auo,b133htn01"; >>> power-supply = <&tps65090_fet6>; >>> backlight = <&backlight>; >>> + >>> + port { >>> + panel_in: endpoint { >>> + remote-endpoint = <&dp_out>; >>> + }; >>> + }; >>> }; >>> >>> mmc1_pwrseq: mmc1_pwrseq { >>> @@ -148,7 +154,14 @@ >>> samsung,link-rate = <0x0a>; >>> samsung,lane-count = <2>; >>> samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; >>> - panel = <&panel>; >>> + >>> + ports { >>> + port@0 { >>> + dp_out: endpoint { >>> + remote-endpoint = <&panel_in>; >>> + }; >>> + }; >>> + }; >>> }; > > Cheers, > Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. > Thanks, > Inki Dae > >> >>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts >>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts >>>> @@ -122,6 +122,12 @@ >>>> compatible = "auo,b133htn01"; >>>> power-supply = <&tps65090_fet6>; >>>> backlight = <&backlight>; >>>> + >>>> + port { >>>> + panel_in: endpoint { >>>> + remote-endpoint = <&dp_out>; >>>> + }; >>>> + }; >>>> }; >>>> >>>> mmc1_pwrseq: mmc1_pwrseq { >>>> @@ -148,7 +154,14 @@ >>>> samsung,link-rate = <0x0a>; >>>> samsung,lane-count = <2>; >>>> samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; >>>> - panel = <&panel>; >>>> + >>>> + ports { >>>> + port@0 { >>>> + dp_out: endpoint { >>>> + remote-endpoint = <&panel_in>; >>>> + }; >>>> + }; >>>> + }; >>>> }; >> >> Cheers, >> Daniel > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards,
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. Other is, 1. Revive a panel property and remove the port node you added. 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. > > 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? Thanks, Inki Dae > >> Thanks, >> Inki Dae >> >>> >>>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts >>>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts >>>>> @@ -122,6 +122,12 @@ >>>>> compatible = "auo,b133htn01"; >>>>> power-supply = <&tps65090_fet6>; >>>>> backlight = <&backlight>; >>>>> + >>>>> + port { >>>>> + panel_in: endpoint { >>>>> + remote-endpoint = <&dp_out>; >>>>> + }; >>>>> + }; >>>>> }; >>>>> >>>>> mmc1_pwrseq: mmc1_pwrseq { >>>>> @@ -148,7 +154,14 @@ >>>>> samsung,link-rate = <0x0a>; >>>>> samsung,lane-count = <2>; >>>>> samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; >>>>> - panel = <&panel>; >>>>> + >>>>> + ports { >>>>> + port@0 { >>>>> + dp_out: endpoint { >>>>> + remote-endpoint = <&panel_in>; >>>>> + }; >>>>> + }; >>>>> + }; >>>>> }; >>> >>> Cheers, >>> Daniel >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > Best regards, > -- > Javier Martinez Canillas > Open Source Group > Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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,
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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? > 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,
Hi Javier, 2015-11-24 22:19 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>: > 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@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? >> > > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts index 7b018e451880..9c6fd7314ee0 100644 --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts @@ -122,6 +122,12 @@ compatible = "auo,b133htn01"; power-supply = <&tps65090_fet6>; backlight = <&backlight>; + + port { + panel_in: endpoint { + remote-endpoint = <&dp_out>; + }; + }; }; mmc1_pwrseq: mmc1_pwrseq { @@ -148,7 +154,14 @@ samsung,link-rate = <0x0a>; samsung,lane-count = <2>; samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>; - panel = <&panel>; + + ports { + port@0 { + dp_out: endpoint { + remote-endpoint = <&panel_in>; + }; + }; + }; }; &fimd {