[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Javier Martinez Canillas]

This reverts commit 2d2c9a8d0a4f90e298315d2f4a282d8bd5d45e5c
("ARM: dts: add display power domain for exynos5250").

The mentioned commit added a domain definition for the DISP1
power domain and references to it in the appropriate devices
but this change breaks the display in at least the Exynos5250
based Snow and Spring Chromebooks.

On these machines, the boot-loader enables the DISP1 domain and
before the mentioned commit, the kernel didn't know about it so
the power domain remained always enabled.

But after that commit when the exynos-dp probe is deferred,
the DISP1 domain is powered off and on again but the exynos-dp
driver fails to configure the video showing the following error:

exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok
exynos-dp 145b0000.dp-controller: unable to config video

The same issue happens when the display is turned off and on
again using DPMS.

So, it seems the DISP1 power domain definition is not complete
since the display works with the initialization made by the boot
loader but it does not work when the power domain is enabled by
the kernel.

Having the definition in the DTS makes the power domain to be
powered on when needed and powered off when not needed which is
better in terms of power consumption but for now is safer to just
revert the commit to avoid adding a regression in some machines.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5250.dtsi | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 77f656eb8e6b..482fa97c6b3a 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -105,12 +105,6 @@
 		#power-domain-cells = <0>;
 	};
 
-	pd_disp1: disp1-power-domain at 100440A0 {
-		compatible = "samsung,exynos4210-pd";
-		reg = <0x100440A0 0x20>;
-		#power-domain-cells = <0>;
-	};
-
 	clock: clock-controller at 10010000 {
 		compatible = "samsung,exynos5250-clock";
 		reg = <0x10010000 0x30000>;
@@ -752,7 +746,6 @@
 	hdmi: hdmi {
 		compatible = "samsung,exynos4212-hdmi";
 		reg = <0x14530000 0x70000>;
-		power-domains = <&pd_disp1>;
 		interrupts = <0 95 0>;
 		clocks = <&clock CLK_HDMI>, <&clock CLK_SCLK_HDMI>,
 			 <&clock CLK_SCLK_PIXEL>, <&clock CLK_SCLK_HDMIPHY>,
@@ -765,7 +758,6 @@
 	mixer {
 		compatible = "samsung,exynos5250-mixer";
 		reg = <0x14450000 0x10000>;
-		power-domains = <&pd_disp1>;
 		interrupts = <0 94 0>;
 		clocks = <&clock CLK_MIXER>, <&clock CLK_HDMI>,
 			 <&clock CLK_SCLK_HDMI>;
@@ -779,7 +771,6 @@
 	};
 
 	dp: dp-controller at 145B0000 {
-		power-domains = <&pd_disp1>;
 		clocks = <&clock CLK_DP>;
 		clock-names = "dp";
 		phys = <&dp_phy>;
@@ -787,7 +778,6 @@
 	};
 
 	fimd: fimd at 14400000 {
-		power-domains = <&pd_disp1>;
 		clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>;
 		clock-names = "sclk_fimd", "fimd";
 	};
-- 
2.1.4

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Krzysztof Kozlowski]

2015-03-23 11:49 GMT+01:00 Javier Martinez Canillas
<javier.martinez@collabora.co.uk>:
> This reverts commit 2d2c9a8d0a4f90e298315d2f4a282d8bd5d45e5c
> ("ARM: dts: add display power domain for exynos5250").
>
> The mentioned commit added a domain definition for the DISP1
> power domain and references to it in the appropriate devices
> but this change breaks the display in at least the Exynos5250
> based Snow and Spring Chromebooks.
>
> On these machines, the boot-loader enables the DISP1 domain and
> before the mentioned commit, the kernel didn't know about it so
> the power domain remained always enabled.
>
> But after that commit when the exynos-dp probe is deferred,
> the DISP1 domain is powered off and on again but the exynos-dp
> driver fails to configure the video showing the following error:
>
> exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok
> exynos-dp 145b0000.dp-controller: unable to config video
>
> The same issue happens when the display is turned off and on
> again using DPMS.
>
> So, it seems the DISP1 power domain definition is not complete
> since the display works with the initialization made by the boot
> loader but it does not work when the power domain is enabled by
> the kernel.
>
> Having the definition in the DTS makes the power domain to be
> powered on when needed and powered off when not needed which is
> better in terms of power consumption but for now is safer to just
> revert the commit to avoid adding a regression in some machines.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

I looked at the DP and FIMD drivers and with great help of Andrzej
Hajda found the issue: the FIMD driver does not enable DP clock
(DP_MIE_CLKCON register). The process looks like:
1. Bootloader sets the DP_MIE_CLKCON to proper value.
2. FIMD is probed, DP power domain is on.
3. FIMD is deffered, DP power domain is turned off.
4. The FIMD registers are reset so DP clock is turned off.
5. DP driver is probed, DP power domain is turned on but clock is not enabled.

I'll do some more testing and sent a patch for it till end of Monday
so I think we should not revert this commit.

Best regards,
Krzysztof

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Javier Martinez Canillas]

Hello Krzysztof,

On 03/27/2015 03:29 PM, Krzysztof Kozlowski wrote:
> 2015-03-23 11:49 GMT+01:00 Javier Martinez Canillas
> 
> I looked at the DP and FIMD drivers and with great help of Andrzej
> Hajda found the issue: the FIMD driver does not enable DP clock
> (DP_MIE_CLKCON register). The process looks like:
> 1. Bootloader sets the DP_MIE_CLKCON to proper value.
> 2. FIMD is probed, DP power domain is on.
> 3. FIMD is deffered, DP power domain is turned off.
> 4. The FIMD registers are reset so DP clock is turned off.
> 5. DP driver is probed, DP power domain is turned on but clock is not enabled.
>

Great! I knew it was some setup missing that was made by the boot-loader.

> I'll do some more testing and sent a patch for it till end of Monday
> so I think we should not revert this commit.
>

Agree, I of course preferred to fix the actual cause instead of relying
on the boot-loader to do the initialization but I wasn't able to figure
it out so I posted this patch to revert the commit in the meantime.

> Best regards,
> Krzysztof
> 

Best regards,
Javier

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Kevin Hilman]

Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:

> This reverts commit 2d2c9a8d0a4f90e298315d2f4a282d8bd5d45e5c
> ("ARM: dts: add display power domain for exynos5250").
>
> The mentioned commit added a domain definition for the DISP1
> power domain and references to it in the appropriate devices
> but this change breaks the display in at least the Exynos5250
> based Snow and Spring Chromebooks.
>
> On these machines, the boot-loader enables the DISP1 domain and
> before the mentioned commit, the kernel didn't know about it so
> the power domain remained always enabled.
>
> But after that commit when the exynos-dp probe is deferred,
> the DISP1 domain is powered off and on again but the exynos-dp
> driver fails to configure the video showing the following error:
>
> exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok
> exynos-dp 145b0000.dp-controller: unable to config video
>
> The same issue happens when the display is turned off and on
> again using DPMS.
>
> So, it seems the DISP1 power domain definition is not complete
> since the display works with the initialization made by the boot
> loader but it does not work when the power domain is enabled by
> the kernel.
>
> Having the definition in the DTS makes the power domain to be
> powered on when needed and powered off when not needed which is
> better in terms of power consumption but for now is safer to just
> revert the commit to avoid adding a regression in some machines.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Tested-by: Kevin Hilman <khilman@linaro.org>

FWIW, this patch fixes the boot panics when using MMC rootfs on
exynos5800-peach-pi with current linux-next that have been happening
for awhile.

For several months now, DRM/display related stuff is very routinely
breaking basic booting on exynos5, which gives the rather strong
impression that the DRM stuff is not tested well enough to be merged.

Kevin

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Sjoerd Simons]

On Wed, 2015-04-29 at 10:40 -0700, Kevin Hilman wrote:
> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:

> Tested-by: Kevin Hilman <khilman@linaro.org>
> 
> FWIW, this patch fixes the boot panics when using MMC rootfs on
> exynos5800-peach-pi with current linux-next that have been happening
> for awhile.

That seems unlikely as this patch changes things in the exynos5250.dtsi,
which isn't used by exynos5800-peach-pi.?

> For several months now, DRM/display related stuff is very routinely
> breaking basic booting on exynos5, which gives the rather strong
> impression that the DRM stuff is not tested well enough to be merged.

Unfortunately it seems to have been that way for ages, but only has
started being visible since Javier turned on the various exynos DRM
drivers on in exynos_defconfig.. So now we're actually seeing bugs
rather then just having broken code sit around :/

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Javier Martinez Canillas]

Hello,

On 04/29/2015 09:45 PM, Sjoerd Simons wrote:
> On Wed, 2015-04-29 at 10:40 -0700, Kevin Hilman wrote:
>> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:
> 
>> Tested-by: Kevin Hilman <khilman@linaro.org>
>> 
>> FWIW, this patch fixes the boot panics when using MMC rootfs on
>> exynos5800-peach-pi with current linux-next that have been happening
>> for awhile.
> 
> That seems unlikely as this patch changes things in the exynos5250.dtsi,
> which isn't used by exynos5800-peach-pi.?
>

Indeed, it will be surprising if $subject changes any behavior on
Exynos5420/5422/5800 machines.

FWIW, this patch is not needed anymore since the right fix is Krzysztof's
"drm/exynos: Enable DP clock to fix display on Exynos5250 and other" patch
which is already in linux-next.

The regression on Exynos5420/5422/5800 that has been for a while is fixed
by "ARM: dts: Make DP a consumer of DISP1 power domain on Exynos5420" [0]
which has been posted many weeks ago.
 
>> For several months now, DRM/display related stuff is very routinely
>> breaking basic booting on exynos5, which gives the rather strong
>> impression that the DRM stuff is not tested well enough to be merged.
> 
> Unfortunately it seems to have been that way for ages, but only has
> started being visible since Javier turned on the various exynos DRM
> drivers on in exynos_defconfig.. So now we're actually seeing bugs
> rather then just having broken code sit around :/
>

Agreed.

Best regards,
Javier

[0]: https://lkml.org/lkml/2015/4/12/153

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Kevin Hilman]

Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:

> Hello,
>
> On 04/29/2015 09:45 PM, Sjoerd Simons wrote:
>> On Wed, 2015-04-29 at 10:40 -0700, Kevin Hilman wrote:
>>> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:
>> 
>>> Tested-by: Kevin Hilman <khilman@linaro.org>
>>> 
>>> FWIW, this patch fixes the boot panics when using MMC rootfs on
>>> exynos5800-peach-pi with current linux-next that have been happening
>>> for awhile.
>> 
>> That seems unlikely as this patch changes things in the exynos5250.dtsi,
>> which isn't used by exynos5800-peach-pi.?
>>
>
> Indeed, it will be surprising if $subject changes any behavior on
> Exynos5420/5422/5800 machines.
>
> FWIW, this patch is not needed anymore since the right fix is Krzysztof's
> "drm/exynos: Enable DP clock to fix display on Exynos5250 and other" patch
> which is already in linux-next.
>
> The regression on Exynos5420/5422/5800 that has been for a while is fixed
> by "ARM: dts: Make DP a consumer of DISP1 power domain on Exynos5420" [0]
> which has been posted many weeks ago.

Thanks for clarifying.

I'm not sure how this worked for me.  Seems like sometimes, even with
the imprecise aborts it does actually finish booting to a shell, so I
must've gotten successful boot and thought it was because of this
revert.

Kevin

[PATCH 1/1] Revert "ARM: dts: add display power domain for exynos5250"

[Javier Martinez Canillas]

Hello Kevin,

On 04/30/2015 04:59 PM, Kevin Hilman wrote:
> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:
>>
>> The regression on Exynos5420/5422/5800 that has been for a while is fixed
>> by "ARM: dts: Make DP a consumer of DISP1 power domain on Exynos5420" [0]
>> which has been posted many weeks ago.
> 
> Thanks for clarifying.
> 
> I'm not sure how this worked for me.  Seems like sometimes, even with
> the imprecise aborts it does actually finish booting to a shell, so I
> must've gotten successful boot and thought it was because of this
> revert.

That is correct, the imprecise external abort error most of the times
lead to a kernel panic but sometimes it doesn't and the the system finish
booting to user-space even after the error.

I've pinged Kukjin again yesterday to pick the fix and he said that will
do it this weekend [0]. So hopefully exynos5420 machines will boot again
in linux-next soon after being broken for weeks.

> 
> Kevin
>

Best regards,
Javier

[0]: https://lkml.org/lkml/2015/4/29/781