[PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

[PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

[Javier Martinez Canillas]

Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
->remove()") changed the driver's .remove function logic to fist do
a pm_runtime_get_sync() to make sure the device is powered before
attempting to gate the gsc clock.

But the commit also removed a pm_runtime_disable() call that leads
to an unbalanced pm_runtime_enable() error if the driver is removed
and re-probed:

exynos-gsc 13e00000.video-scaler: Unbalanced pm_runtime_enable!
exynos-gsc 13e10000.video-scaler: Unbalanced pm_runtime_enable!

Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at ->remove()")
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index cbf75b6194b4..83272f10722d 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1118,6 +1118,7 @@ static int gsc_remove(struct platform_device *pdev)
 		clk_disable_unprepare(gsc->clock[i]);
 
 	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
 	return 0;
-- 
2.7.4

[PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
CONFIG_PM is unset") removed the implicit dependency that the driver
had with CONFIG_PM, since it relied on the config option to be enabled.

In order to work with !CONFIG_PM, the GSC reset logic that happens in
the runtime resume callback had to be executed on the probe function.

The problem is that if CONFIG_PM is enabled, the power domain for the
GSC could be disabled and so an attempt to write to the GSC_SW_RESET
register leads to an unhandled fault / imprecise external abort error:

[   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[   10.186982] pgd = ed728000
[   10.190847] [00000000] *pgd=00000000
[   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
[   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   10.237134] task: ed49e400 task.stack: ed724000
[   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
[   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
[   10.256139] pc : [<bf2429e0>]    lr : [<bf240734>]    psr: 60070013
[   10.256139] sp : ed725d30  ip : 00000000  fp : 00000001
[   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
[   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : ffff8ecd  r4 : c0c03900
[   10.285664] r3 : 00000000  r2 : 00000001  r1 : ed7d8b98  r0 : ed7d8810

So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
making the reset in probe unneeded.

Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset")
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid
XU4 board fails to boot when the config option is disabled.

Best regards,
Javier

 drivers/media/platform/exynos-gsc/gsc-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 83272f10722d..42e1e09ea915 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, gsc);
 
-	gsc_hw_set_sw_reset(gsc);
-	gsc_wait_reset(gsc);
+	if (!IS_ENABLED(CONFIG_PM)) {
+		gsc_hw_set_sw_reset(gsc);
+		gsc_wait_reset(gsc);
+	}
 
 	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
-- 
2.7.4

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Marek Szyprowski]

Hi Javier,

On 2017-01-18 01:30, Javier Martinez Canillas wrote:
> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
> CONFIG_PM is unset") removed the implicit dependency that the driver
> had with CONFIG_PM, since it relied on the config option to be enabled.
>
> In order to work with !CONFIG_PM, the GSC reset logic that happens in
> the runtime resume callback had to be executed on the probe function.
>
> The problem is that if CONFIG_PM is enabled, the power domain for the
> GSC could be disabled and so an attempt to write to the GSC_SW_RESET
> register leads to an unhandled fault / imprecise external abort error:

Driver core ensures that driver's probe() is called with respective power
domain turned on, so this is not the right reason for the proposed change.

> [   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
> [   10.186982] pgd = ed728000
> [   10.190847] [00000000] *pgd=00000000
> [   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
> [   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   10.237134] task: ed49e400 task.stack: ed724000
> [   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
> [   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
> [   10.256139] pc : [<bf2429e0>]    lr : [<bf240734>]    psr: 60070013
> [   10.256139] sp : ed725d30  ip : 00000000  fp : 00000001
> [   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
> [   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : ffff8ecd  r4 : c0c03900
> [   10.285664] r3 : 00000000  r2 : 00000001  r1 : ed7d8b98  r0 : ed7d8810
>
> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
> making the reset in probe unneeded.
>
> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset")
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

Frankly, I don't get why this change is needed.

>
> ---
>
> I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid
> XU4 board fails to boot when the config option is disabled.
>
> Best regards,
> Javier
>
>   drivers/media/platform/exynos-gsc/gsc-core.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 83272f10722d..42e1e09ea915 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, gsc);
>   
> -	gsc_hw_set_sw_reset(gsc);
> -	gsc_wait_reset(gsc);
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		gsc_hw_set_sw_reset(gsc);
> +		gsc_wait_reset(gsc);
> +	}
>   
>   	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello Marek,

Thanks a lot for your feedback.

On 01/19/2017 11:17 AM, Marek Szyprowski wrote:
> Hi Javier,
> 
> On 2017-01-18 01:30, Javier Martinez Canillas wrote:
>> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
>> CONFIG_PM is unset") removed the implicit dependency that the driver
>> had with CONFIG_PM, since it relied on the config option to be enabled.
>>
>> In order to work with !CONFIG_PM, the GSC reset logic that happens in
>> the runtime resume callback had to be executed on the probe function.
>>
>> The problem is that if CONFIG_PM is enabled, the power domain for the
>> GSC could be disabled and so an attempt to write to the GSC_SW_RESET
>> register leads to an unhandled fault / imprecise external abort error:
> 
> Driver core ensures that driver's probe() is called with respective power
> domain turned on, so this is not the right reason for the proposed change.
>

Ok, I misunderstood the relationship between runtime PM and the power domains
then. I thought the power domains were only powered on when the runtime PM
framework resumed an associated device (i.e: pm_runtime_get_sync() is called).

But even if this isn't the case, shouldn't the reset in probe only be needed
if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).

>> [   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
>> [   10.186982] pgd = ed728000
>> [   10.190847] [00000000] *pgd=00000000
>> [   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
>> [   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> [   10.237134] task: ed49e400 task.stack: ed724000
>> [   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
>> [   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
>> [   10.256139] pc : [<bf2429e0>]    lr : [<bf240734>]    psr: 60070013
>> [   10.256139] sp : ed725d30  ip : 00000000  fp : 00000001
>> [   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
>> [   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : ffff8ecd  r4 : c0c03900
>> [   10.285664] r3 : 00000000  r2 : 00000001  r1 : ed7d8b98  r0 : ed7d8810
>>
>> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
>> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
>> making the reset in probe unneeded.
>>
>> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset")
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Frankly, I don't get why this change is needed.
>

Yes, it seems $SUBJECT is just papering over the real issue. There's
something really wrong with the Exynos power domains, I see that PDs
can't be disabled by the genpd framework, exynos_pd_power_off() fail:

# dmesg | grep power-domain
[    4.893318] Power domain power-domain@10044020 disable failed
[    4.893342] Power domain power-domain@10044120 disable failed
[    4.893711] Power domain power-domain@10044000 disable failed
[   12.690052] Power domain power-domain@10044000 disable failed
[   12.703963] Power domain power-domain@10044000 disable failed

So PD are kept on even when unused / attached devices are suspended.
Only the mfc_pd (power-domain@10044060) is correctly turned off.

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on              
    /devices/platform/soc/14450000.mixer                active
    /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on              
power-domain@10044060           off-0           
    /devices/platform/soc/11000000.codec                suspended
power-domain@10044020           on              
power-domain@10044000           on              
    /devices/platform/soc/13e00000.video-scaler         suspended
    /devices/platform/soc/13e10000.video-scaler         suspended

Also when removing the exynos_gsc driver, I get the same error:

# rmmod s5p_mfc
[  106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec
# rmmod exynos_gsc
[  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
[  227.015116] pgd = ed5dc000
[  227.017213] [00048e14] *pgd=b17c6835
[  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
...
[  227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
[  227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4)
[  227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218)

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on              
    /devices/platform/soc/14450000.mixer                active
    /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on              
power-domain@10044060           off-0           
power-domain@10044020           on              
power-domain@10044000           on              
    /devices/platform/soc/13e00000.video-scaler         suspended
    /devices/platform/soc/13e10000.video-scaler         resuming

This seems to be caused by some needed clocks to access the power domains
to be gated, since I don't get these erros when passing clk_ignore_unused
as parameter in the kernel command line.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello Marek,

On 01/19/2017 11:56 AM, Javier Martinez Canillas wrote:
> On 01/19/2017 11:17 AM, Marek Szyprowski wrote:

[snip]

> 
> Also when removing the exynos_gsc driver, I get the same error:
> 
> # rmmod s5p_mfc
> [  106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec
> # rmmod exynos_gsc
> [  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
> [  227.015116] pgd = ed5dc000
> [  227.017213] [00048e14] *pgd=b17c6835
> [  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
> ...
> [  227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
> [  227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4)
> [  227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218)
> 
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>     /device                                             runtime status
> ----------------------------------------------------------------------
> power-domain@100440C0           on              
>     /devices/platform/soc/14450000.mixer                active
>     /devices/platform/soc/14530000.hdmi                 active
> power-domain@10044120           on              
> power-domain@10044060           off-0           
> power-domain@10044020           on              
> power-domain@10044000           on              
>     /devices/platform/soc/13e00000.video-scaler         suspended
>     /devices/platform/soc/13e10000.video-scaler         resuming
> 
> This seems to be caused by some needed clocks to access the power domains
> to be gated, since I don't get these erros when passing clk_ignore_unused
> as parameter in the kernel command line.
>

I found the issue. The problem was that Exynos5422 needs not only the
CLK_ACLK_ 300_GSCL but also CLK_ACLK432_SCALER to be ungated in order
to access the GSCL IP block.

The Exynos5422 manual shows in Figure 7-14 that ACLK_432_SCLAER is needed
for the internal buses.

Exynos5420 only needs CLK_ACLK_ 300_GSCL to be ungated.

With following hack, the issue goes away for the gsc_pd in the Odroid XU4:

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 8c8b495cbf0d..9876ec28b94c 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -586,7 +586,7 @@ static const struct samsung_gate_clock exynos5800_gate_clks[] __initconst = {
 	GATE(CLK_ACLK550_CAM, "aclk550_cam", "mout_user_aclk550_cam",
 				GATE_BUS_TOP, 24, 0, 0),
 	GATE(CLK_ACLK432_SCALER, "aclk432_scaler", "mout_user_aclk432_scaler",
-				GATE_BUS_TOP, 27, 0, 0),
+				GATE_BUS_TOP, 27, CLK_IS_CRITICAL, 0),
 };

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on              
    /devices/platform/soc/14450000.mixer                active
    /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on              
power-domain@10044060           off-0           
    /devices/platform/soc/11000000.codec                suspended
power-domain@10044020           on              
power-domain@10044000           off-0           
    /devices/platform/soc/13e00000.video-scaler         suspended
    /devices/platform/soc/13e10000.video-scaler         suspended

# rmmod s5p_mfc
[   82.885227] s5p-mfc 11000000.codec: Removing 11000000.codec
# rmmod exynos_gsc

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
power-domain@100440C0           on              
    /devices/platform/soc/14450000.mixer                active
    /devices/platform/soc/14530000.hdmi                 active
power-domain@10044120           on              
power-domain@10044060           off-0           
power-domain@10044020           on              
power-domain@10044000           off-0
 
I'll post a proper patch for the exynos5800.dtsi, to override the
clocks in the gsc_pd device node.

I also see that the two power domains that fail to be disabled msc_pd
(power-domain@10044120) and isp_pd (power-domain@10044020) don't have
clocks defined in the exynos54xx.dtsi. I'll add clocks for those too.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Marek Szyprowski]

Hi Javier,

On 2017-01-19 18:51, Javier Martinez Canillas wrote:
> On 01/19/2017 11:56 AM, Javier Martinez Canillas wrote:
>> On 01/19/2017 11:17 AM, Marek Szyprowski wrote:
> [snip]
>
>> Also when removing the exynos_gsc driver, I get the same error:
>>
>> # rmmod s5p_mfc
>> [  106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec
>> # rmmod exynos_gsc
>> [  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
>> [  227.015116] pgd = ed5dc000
>> [  227.017213] [00048e14] *pgd=b17c6835
>> [  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
>> ...
>> [  227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
>> [  227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4)
>> [  227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218)
>>
>> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>> domain                          status          slaves
>>      /device                                             runtime status
>> ----------------------------------------------------------------------
>> power-domain@100440C0           on
>>      /devices/platform/soc/14450000.mixer                active
>>      /devices/platform/soc/14530000.hdmi                 active
>> power-domain@10044120           on
>> power-domain@10044060           off-0
>> power-domain@10044020           on
>> power-domain@10044000           on
>>      /devices/platform/soc/13e00000.video-scaler         suspended
>>      /devices/platform/soc/13e10000.video-scaler         resuming
>>
>> This seems to be caused by some needed clocks to access the power domains
>> to be gated, since I don't get these erros when passing clk_ignore_unused
>> as parameter in the kernel command line.
>>
> I found the issue. The problem was that Exynos5422 needs not only the
> CLK_ACLK_ 300_GSCL but also CLK_ACLK432_SCALER to be ungated in order
> to access the GSCL IP block.
>
> The Exynos5422 manual shows in Figure 7-14 that ACLK_432_SCLAER is needed
> for the internal buses.
>
> Exynos5420 only needs CLK_ACLK_ 300_GSCL to be ungated.
>
> With following hack, the issue goes away for the gsc_pd in the Odroid XU4:
>
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 8c8b495cbf0d..9876ec28b94c 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -586,7 +586,7 @@ static const struct samsung_gate_clock exynos5800_gate_clks[] __initconst = {
>   	GATE(CLK_ACLK550_CAM, "aclk550_cam", "mout_user_aclk550_cam",
>   				GATE_BUS_TOP, 24, 0, 0),
>   	GATE(CLK_ACLK432_SCALER, "aclk432_scaler", "mout_user_aclk432_scaler",
> -				GATE_BUS_TOP, 27, 0, 0),
> +				GATE_BUS_TOP, 27, CLK_IS_CRITICAL, 0),
>   };
>
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>      /device                                             runtime status
> ----------------------------------------------------------------------
> power-domain@100440C0           on
>      /devices/platform/soc/14450000.mixer                active
>      /devices/platform/soc/14530000.hdmi                 active
> power-domain@10044120           on
> power-domain@10044060           off-0
>      /devices/platform/soc/11000000.codec                suspended
> power-domain@10044020           on
> power-domain@10044000           off-0
>      /devices/platform/soc/13e00000.video-scaler         suspended
>      /devices/platform/soc/13e10000.video-scaler         suspended
>
> # rmmod s5p_mfc
> [   82.885227] s5p-mfc 11000000.codec: Removing 11000000.codec
> # rmmod exynos_gsc
>
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>      /device                                             runtime status
> ----------------------------------------------------------------------
> power-domain@100440C0           on
>      /devices/platform/soc/14450000.mixer                active
>      /devices/platform/soc/14530000.hdmi                 active
> power-domain@10044120           on
> power-domain@10044060           off-0
> power-domain@10044020           on
> power-domain@10044000           off-0
>   
> I'll post a proper patch for the exynos5800.dtsi, to override the
> clocks in the gsc_pd device node.
>
> I also see that the two power domains that fail to be disabled msc_pd
> (power-domain@10044120) and isp_pd (power-domain@10044020) don't have
> clocks defined in the exynos54xx.dtsi. I'll add clocks for those too.

Please send this patch instead of adding more clocks to the power domains.
This way we will avoid adding more dependencies to userspace (DT ABI).
Likely those clocks are also needed to access any device in that power
domains.

Later, once the runtime PM for clocks get merged, a patch for exynos542x
clocks driver can be made to replace IS_CRITICAL with proper runtime
handling.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello Marek,

On 01/20/2017 05:37 AM, Marek Szyprowski wrote:

[snip]

>>   I'll post a proper patch for the exynos5800.dtsi, to override the
>> clocks in the gsc_pd device node.
>>
>> I also see that the two power domains that fail to be disabled msc_pd
>> (power-domain@10044120) and isp_pd (power-domain@10044020) don't have
>> clocks defined in the exynos54xx.dtsi. I'll add clocks for those too.
> 
> Please send this patch instead of adding more clocks to the power domains.
> This way we will avoid adding more dependencies to userspace (DT ABI).
> Likely those clocks are also needed to access any device in that power
> domains.
> 
> Later, once the runtime PM for clocks get merged, a patch for exynos542x
> clocks driver can be made to replace IS_CRITICAL with proper runtime
> handling.
> 

Ok, I thought that we didn't want to mark more clocks as CLK_IGNORE_UNUSED
or CLK_IS_CRITICAL but I agree this can be done as a temporary workaround
until a proper runtime PM based solution gets merged.

> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello,

On 01/20/2017 07:06 AM, Javier Martinez Canillas wrote:
> Hello Marek,
> 
> On 01/20/2017 05:37 AM, Marek Szyprowski wrote:
> 
> [snip]
>>
>> Please send this patch instead of adding more clocks to the power domains.
>> This way we will avoid adding more dependencies to userspace (DT ABI).
>> Likely those clocks are also needed to access any device in that power
>> domains.
>>
>> Later, once the runtime PM for clocks get merged, a patch for exynos542x
>> clocks driver can be made to replace IS_CRITICAL with proper runtime
>> handling.
>>
> 
> Ok, I thought that we didn't want to mark more clocks as CLK_IGNORE_UNUSED
> or CLK_IS_CRITICAL but I agree this can be done as a temporary workaround
> until a proper runtime PM based solution gets merged.
> 

I posted following patch [0] for clk-exynos5420, so $SUBJECT can be dropped.

[0]: https://patchwork.kernel.org/patch/9527957/

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Marek Szyprowski]

Hi Javier,

On 2017-01-19 15:56, Javier Martinez Canillas wrote:
> Thanks a lot for your feedback.
>
> On 01/19/2017 11:17 AM, Marek Szyprowski wrote:
>> On 2017-01-18 01:30, Javier Martinez Canillas wrote:
>>> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
>>> CONFIG_PM is unset") removed the implicit dependency that the driver
>>> had with CONFIG_PM, since it relied on the config option to be enabled.
>>>
>>> In order to work with !CONFIG_PM, the GSC reset logic that happens in
>>> the runtime resume callback had to be executed on the probe function.
>>>
>>> The problem is that if CONFIG_PM is enabled, the power domain for the
>>> GSC could be disabled and so an attempt to write to the GSC_SW_RESET
>>> register leads to an unhandled fault / imprecise external abort error:
>> Driver core ensures that driver's probe() is called with respective power
>> domain turned on, so this is not the right reason for the proposed change.
> Ok, I misunderstood the relationship between runtime PM and the power domains
> then. I thought the power domains were only powered on when the runtime PM
> framework resumed an associated device (i.e: pm_runtime_get_sync() is called).

Power domains are implemented transparently for the drivers. Even when 
driver
doesn't support runtime pm, but its device is in the power domain, the 
core will
ensure that the domain will be turned on all the time the driver is 
bound to the
device.

> But even if this isn't the case, shouldn't the reset in probe only be needed
> if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).

This looks like an over-engineering. I don't like polluting driver code with
conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to 
reset
the device in driver probe, especially just in case the device was left in
some partially configured/working state by bootloader or previous kernel run
(if started from kexec). Adding this conditional code to avoid some issues
with power domain or clocks configuration also suggests that one should
instead solve the problem elsewhere. Driver should be able to access device
registers in its probe() in any case without the additional hacks.

>>> [   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
>>> [   10.186982] pgd = ed728000
>>> [   10.190847] [00000000] *pgd=00000000
>>> [   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
>>> [   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>> [   10.237134] task: ed49e400 task.stack: ed724000
>>> [   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
>>> [   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
>>> [   10.256139] pc : [<bf2429e0>]    lr : [<bf240734>]    psr: 60070013
>>> [   10.256139] sp : ed725d30  ip : 00000000  fp : 00000001
>>> [   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
>>> [   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : ffff8ecd  r4 : c0c03900
>>> [   10.285664] r3 : 00000000  r2 : 00000001  r1 : ed7d8b98  r0 : ed7d8810
>>>
>>> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
>>> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
>>> making the reset in probe unneeded.
>>>
>>> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM is unset")
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> Frankly, I don't get why this change is needed.
>>
> Yes, it seems $SUBJECT is just papering over the real issue. There's
> something really wrong with the Exynos power domains, I see that PDs
> can't be disabled by the genpd framework, exynos_pd_power_off() fail:
>
> # dmesg | grep power-domain
> [    4.893318] Power domain power-domain@10044020 disable failed
> [    4.893342] Power domain power-domain@10044120 disable failed
> [    4.893711] Power domain power-domain@10044000 disable failed
> [   12.690052] Power domain power-domain@10044000 disable failed
> [   12.703963] Power domain power-domain@10044000 disable failed
>
> So PD are kept on even when unused / attached devices are suspended.
> Only the mfc_pd (power-domain@10044060) is correctly turned off.
>
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>      /device                                             runtime status
> ----------------------------------------------------------------------
> power-domain@100440C0           on
>      /devices/platform/soc/14450000.mixer                active
>      /devices/platform/soc/14530000.hdmi                 active
> power-domain@10044120           on
> power-domain@10044060           off-0
>      /devices/platform/soc/11000000.codec                suspended
> power-domain@10044020           on
> power-domain@10044000           on
>      /devices/platform/soc/13e00000.video-scaler         suspended
>      /devices/platform/soc/13e10000.video-scaler         suspended
>
> Also when removing the exynos_gsc driver, I get the same error:
>
> # rmmod s5p_mfc
> [  106.405972] s5p-mfc 11000000.codec: Removing 11000000.codec
> # rmmod exynos_gsc
> [  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
> [  227.015116] pgd = ed5dc000
> [  227.017213] [00048e14] *pgd=b17c6835
> [  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
> ...
> [  227.241585] [<bf2429bc>] (gsc_wait_reset [exynos_gsc]) from [<bf24009c>] (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
> [  227.252331] [<bf24009c>] (gsc_runtime_resume [exynos_gsc]) from [<c042e488>] (genpd_runtime_resume+0x120/0x1d4)
> [  227.262294] [<c042e488>] (genpd_runtime_resume) from [<c04241c0>] (__rpm_callback+0xc8/0x218)
>
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          slaves
>      /device                                             runtime status
> ----------------------------------------------------------------------
> power-domain@100440C0           on
>      /devices/platform/soc/14450000.mixer                active
>      /devices/platform/soc/14530000.hdmi                 active
> power-domain@10044120           on
> power-domain@10044060           off-0
> power-domain@10044020           on
> power-domain@10044000           on
>      /devices/platform/soc/13e00000.video-scaler         suspended
>      /devices/platform/soc/13e10000.video-scaler         resuming
>
> This seems to be caused by some needed clocks to access the power domains
> to be gated, since I don't get these erros when passing clk_ignore_unused
> as parameter in the kernel command line.

I think that those issues were fixes by the following patch:
https://patchwork.kernel.org/patch/9484607/
It still didn't reach mainline, but I hope it will go as a fix to v4.10.

Please test if this solves your issue. Please not that adding more clocks
to the power domain drivers will solve only the problem with turning domain
on/off, but the are more cases where those clocks should be turned on (like
IOMMU integration), so marking them as critical solves that problem too.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello Marek,

On 01/20/2017 05:08 AM, Marek Szyprowski wrote:
> Hi Javier,
> 

[snip]

>> Ok, I misunderstood the relationship between runtime PM and the power domains
>> then. I thought the power domains were only powered on when the runtime PM
>> framework resumed an associated device (i.e: pm_runtime_get_sync() is called).
> 
> Power domains are implemented transparently for the drivers. Even when driver
> doesn't support runtime pm, but its device is in the power domain, the core will
> ensure that the domain will be turned on all the time the driver is bound to the
> device.
>

I got it now, thank a lot for your explanation.
 
>> But even if this isn't the case, shouldn't the reset in probe only be needed
>> if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).
> 
> This looks like an over-engineering. I don't like polluting driver code with
> conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to reset
> the device in driver probe, especially just in case the device was left in
> some partially configured/working state by bootloader or previous kernel run
> (if started from kexec). Adding this conditional code to avoid some issues
> with power domain or clocks configuration also suggests that one should
> instead solve the problem elsewhere. Driver should be able to access device
> registers in its probe() in any case without the additional hacks.
>

Fair enough, I already posted the patch but I'll ask it to be dropped.

[snip]

>>
>> This seems to be caused by some needed clocks to access the power domains
>> to be gated, since I don't get these erros when passing clk_ignore_unused
>> as parameter in the kernel command line.
> 
> I think that those issues were fixes by the following patch:
> https://patchwork.kernel.org/patch/9484607/
> It still didn't reach mainline, but I hope it will go as a fix to v4.10.
>

That won't be enough since the CLK_ACLK432_SCALER needs to be ungated for
Exynos5422/5800 as mentioned on another mail in this thread.

> Please test if this solves your issue. Please not that adding more clocks
> to the power domain drivers will solve only the problem with turning domain
> on/off, but the are more cases where those clocks should be turned on (like
> IOMMU integration), so marking them as critical solves that problem too.
>

Ok, I understand your point.
 
> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

[Javier Martinez Canillas]

Hello Marek,

On 01/20/2017 05:08 AM, Marek Szyprowski wrote:

[snip]

>>
>> This seems to be caused by some needed clocks to access the power domains
>> to be gated, since I don't get these erros when passing clk_ignore_unused
>> as parameter in the kernel command line.
> 
> I think that those issues were fixes by the following patch:
> https://patchwork.kernel.org/patch/9484607/
> It still didn't reach mainline, but I hope it will go as a fix to v4.10.
> 

Argh, I missed on a first read that the patch you mentioned already marks
CLK_ACLK432_SCALER as CLK_IS_CRITICAL. So please also ignore my clk patch.

Sorry for the noise and the mess with these patches. Thought I also tested
on linux-next to see if the issue was solved there, but it seems I didn't.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

[Marek Szyprowski]

Hi Javier,

On 2017-01-18 01:30, Javier Martinez Canillas wrote:
> Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
> ->remove()") changed the driver's .remove function logic to fist do
> a pm_runtime_get_sync() to make sure the device is powered before
> attempting to gate the gsc clock.
>
> But the commit also removed a pm_runtime_disable() call that leads
> to an unbalanced pm_runtime_enable() error if the driver is removed
> and re-probed:
>
> exynos-gsc 13e00000.video-scaler: Unbalanced pm_runtime_enable!
> exynos-gsc 13e10000.video-scaler: Unbalanced pm_runtime_enable!
>
> Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at ->remove()")
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

I must have mixed something during the rebase of the Ulf's patch, because
the original one kept pm_runtime_disable in the right place:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317678.html

I'm really sorry.

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>
>   drivers/media/platform/exynos-gsc/gsc-core.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index cbf75b6194b4..83272f10722d 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1118,6 +1118,7 @@ static int gsc_remove(struct platform_device *pdev)
>   		clk_disable_unprepare(gsc->clock[i]);
>   
>   	pm_runtime_put_noidle(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>   
>   	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>   	return 0;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

[Javier Martinez Canillas]

Hello Marek,

On 01/19/2017 11:12 AM, Marek Szyprowski wrote:
> Hi Javier,
> 
> On 2017-01-18 01:30, Javier Martinez Canillas wrote:
>> Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
>> ->remove()") changed the driver's .remove function logic to fist do
>> a pm_runtime_get_sync() to make sure the device is powered before
>> attempting to gate the gsc clock.
>>
>> But the commit also removed a pm_runtime_disable() call that leads
>> to an unbalanced pm_runtime_enable() error if the driver is removed
>> and re-probed:
>>
>> exynos-gsc 13e00000.video-scaler: Unbalanced pm_runtime_enable!
>> exynos-gsc 13e10000.video-scaler: Unbalanced pm_runtime_enable!
>>
>> Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at ->remove()")
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> I must have mixed something during the rebase of the Ulf's patch, because
> the original one kept pm_runtime_disable in the right place:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317678.html
> 
> I'm really sorry.
> 

Ah, I see. No worries.

> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 

Thanks a lot for your review.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America