[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Rahul Sharma]

On 22 May 2014 13:33, Thierry Reding  wrote:
> On Thu, May 22, 2014 at 12:06:16PM +0530, Rahul Sharma wrote:
>> On 22 May 2014 11:51, Sachin Kamat  wrote:
>> > Hi Rahul,
>> [snip]
>> >>
>> >> +   clk_prepare_enable(ctx->bus_clk);
>> >
>> > Probably a check for its success?
>> >
>> >> +   clk_prepare_enable(ctx->lcd_clk);
>> >
>>
>> Generally we don't check this in any of the driver. It will be
>> quite unnecessary.
>
> Then those drivers are all buggy. There's a reason why this function
> returns an int rather than void. Just because you've never seen it fail
> doesn't mean it can't.

Okay... I don't mind putting extra checks. V3 is coming :).

Best Regards,
Rahul Sharma

>
> Thierry
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Sachin Kamat]

On 22 May 2014 12:06, Rahul Sharma  wrote:
> On 22 May 2014 11:51, Sachin Kamat  wrote:
>> Hi Rahul,
> [snip]
>>>
>>> +   clk_prepare_enable(ctx->bus_clk);
>>
>> Probably a check for its success?
>>
>>> +   clk_prepare_enable(ctx->lcd_clk);
>>
>
> Generally we don't check this in any of the driver. It will be
> quite unnecessary.

However in your case, since you mentioned if the clock is not enabled, it
will hang the system when fimd probe tries to read the register, this check
would ensure it doesn't happen (hang) even if clk_prepare_enable fails for
whatever reasons.

-- 
With warm regards,
Sachin

[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Rahul Sharma]

On 22 May 2014 11:51, Sachin Kamat  wrote:
> Hi Rahul,
[snip]
>>
>> +   clk_prepare_enable(ctx->bus_clk);
>
> Probably a check for its success?
>
>> +   clk_prepare_enable(ctx->lcd_clk);
>

Generally we don't check this in any of the driver. It will be
quite unnecessary.

Regards,
Rahul Sharma

> ditto.
>
> --
> With warm regards,
> Sachin

[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Sachin Kamat]

Hi Rahul,

On 22 May 2014 10:46, Rahul Sharma  wrote:
> From: Rahul Sharma 
>
> Fimd probe is accessing fimd Registers without enabling the fimd
> gate clocks. If FIMD clocks are kept disabled in Uboot or disbaled
> during kernel boottime, the system hangs during boottime.
>
> This issue got surfaced when verifying with sysmmu enabled. Probe of
> fimd Sysmmu enables the master clock before accessing sysmmu regs and
> then disables. Later fimd probe tries to read the register without
> enabling the clock which is wrong and hangs the system.
>
> Signed-off-by: Rahul Sharma 
> ---
> Rebased on exynos-drm-next branch.
>
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 173ee97..a79ba0a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -891,9 +891,15 @@ static int fimd_bind(struct device *dev, struct device 
> *master, void *data)
> if (ctx->display)
> exynos_drm_create_enc_conn(drm_dev, ctx->display);
>
> +   clk_prepare_enable(ctx->bus_clk);

Probably a check for its success?

> +   clk_prepare_enable(ctx->lcd_clk);

ditto.

-- 
With warm regards,
Sachin

[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Rahul Sharma]

From: Rahul Sharma 

Fimd probe is accessing fimd Registers without enabling the fimd
gate clocks. If FIMD clocks are kept disabled in Uboot or disbaled
during kernel boottime, the system hangs during boottime.

This issue got surfaced when verifying with sysmmu enabled. Probe of
fimd Sysmmu enables the master clock before accessing sysmmu regs and
then disables. Later fimd probe tries to read the register without
enabling the clock which is wrong and hangs the system.

Signed-off-by: Rahul Sharma 
---
Rebased on exynos-drm-next branch.

 drivers/gpu/drm/exynos/exynos_drm_fimd.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 173ee97..a79ba0a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -891,9 +891,15 @@ static int fimd_bind(struct device *dev, struct device 
*master, void *data)
if (ctx->display)
exynos_drm_create_enc_conn(drm_dev, ctx->display);

+   clk_prepare_enable(ctx->bus_clk);
+   clk_prepare_enable(ctx->lcd_clk);
+
for (win = 0; win < WINDOWS_NR; win++)
fimd_clear_win(ctx, win);

+   clk_disable_unprepare(ctx->lcd_clk);
+   clk_disable_unprepare(ctx->bus_clk);
+
return 0;

 }
-- 
1.7.9.5

[PATCH V2] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Thierry Reding]

On Thu, May 22, 2014 at 12:06:16PM +0530, Rahul Sharma wrote:
> On 22 May 2014 11:51, Sachin Kamat  wrote:
> > Hi Rahul,
> [snip]
> >>
> >> +   clk_prepare_enable(ctx->bus_clk);
> >
> > Probably a check for its success?
> >
> >> +   clk_prepare_enable(ctx->lcd_clk);
> >
> 
> Generally we don't check this in any of the driver. It will be
> quite unnecessary.

Then those drivers are all buggy. There's a reason why this function
returns an int rather than void. Just because you've never seen it fail
doesn't mean it can't.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH v2] drm/exynos: enable FIMD clocks

[Inki Dae]

2013/3/20 Vikas Sajjan :
> While migrating to common clock framework (CCF), found that the FIMD clocks
> were pulled down by the CCF.
> If CCF finds any clock(s) which has NOT been claimed by any of the
> drivers, then such clock(s) are PULLed low by CCF.
>
> By calling clk_prepare_enable() for FIMD clocks fixes the issue.
>
> this patch also replaces clk_disable() with clk_disable_unprepare()
> during exit.
>
> Signed-off-by: Vikas Sajjan 
> ---
> Changes since v1:
> - added error checking for clk_prepare_enable() and also replaced
> clk_disable() with clk_disable_unprepare() during exit.
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..014d750 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
> return ret;
> }
>
> +   ret = clk_prepare_enable(ctx->lcd_clk);
> +   if (ret) {
> +   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
> +   return ret;
> +   }
> +
> +   ret = clk_prepare_enable(ctx->bus_clk);
> +   if (ret) {
> +   clk_disable_unprepare(ctx->lcd_clk);
> +   dev_err(dev, "failed to enable 'fimd' clock\n");
> +   return ret;
> +   }
> +

Please remove the above two clk_prepare_enable function calls and use
them in fimd_clock() instead of clk_enable/disable(). When probed,
fimd clock will be enabled by runtime pm.

Thanks,
Inki Dae

> ctx->vidcon0 = pdata->vidcon0;
> ctx->vidcon1 = pdata->vidcon1;
> ctx->default_win = pdata->default_win;
> @@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
> if (ctx->suspended)
> goto out;
>
> -   clk_disable(ctx->lcd_clk);
> -   clk_disable(ctx->bus_clk);
> +   clk_disable_unprepare(ctx->lcd_clk);
> +   clk_disable_unprepare(ctx->bus_clk);
>
> pm_runtime_set_suspended(dev);
> pm_runtime_put_sync(dev);
> --
> 1.7.9.5
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/exynos: enable FIMD clocks

[Vikas Sajjan]

Hi,

On 27 March 2013 15:53, Inki Dae  wrote:
> 2013/3/20 Vikas Sajjan :
>> While migrating to common clock framework (CCF), found that the FIMD clocks
>> were pulled down by the CCF.
>> If CCF finds any clock(s) which has NOT been claimed by any of the
>> drivers, then such clock(s) are PULLed low by CCF.
>>
>> By calling clk_prepare_enable() for FIMD clocks fixes the issue.
>>
>> this patch also replaces clk_disable() with clk_disable_unprepare()
>> during exit.
>>
>> Signed-off-by: Vikas Sajjan 
>> ---
>> Changes since v1:
>> - added error checking for clk_prepare_enable() and also replaced
>> clk_disable() with clk_disable_unprepare() during exit.
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 9537761..014d750 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> +   ret = clk_prepare_enable(ctx->lcd_clk);
>> +   if (ret) {
>> +   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
>> +   return ret;
>> +   }
>> +
>> +   ret = clk_prepare_enable(ctx->bus_clk);
>> +   if (ret) {
>> +   clk_disable_unprepare(ctx->lcd_clk);
>> +   dev_err(dev, "failed to enable 'fimd' clock\n");
>> +   return ret;
>> +   }
>> +
>
> Please remove the above two clk_prepare_enable function calls and use
> them in fimd_clock() instead of clk_enable/disable(). When probed,
> fimd clock will be enabled by runtime pm.
>

Sure, will modify and resend.

> Thanks,
> Inki Dae
>
>> ctx->vidcon0 = pdata->vidcon0;
>> ctx->vidcon1 = pdata->vidcon1;
>> ctx->default_win = pdata->default_win;
>> @@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
>> if (ctx->suspended)
>> goto out;
>>
>> -   clk_disable(ctx->lcd_clk);
>> -   clk_disable(ctx->bus_clk);
>> +   clk_disable_unprepare(ctx->lcd_clk);
>> +   clk_disable_unprepare(ctx->bus_clk);
>>
>> pm_runtime_set_suspended(dev);
>> pm_runtime_put_sync(dev);
>> --
>> 1.7.9.5
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Thanks and Regards
 Vikas Sajjan

Re: [PATCH v2] drm/exynos: enable FIMD clocks

[Vikas Sajjan]

Hi,

On 27 March 2013 15:53, Inki Dae  wrote:
> 2013/3/20 Vikas Sajjan :
>> While migrating to common clock framework (CCF), found that the FIMD clocks
>> were pulled down by the CCF.
>> If CCF finds any clock(s) which has NOT been claimed by any of the
>> drivers, then such clock(s) are PULLed low by CCF.
>>
>> By calling clk_prepare_enable() for FIMD clocks fixes the issue.
>>
>> this patch also replaces clk_disable() with clk_disable_unprepare()
>> during exit.
>>
>> Signed-off-by: Vikas Sajjan 
>> ---
>> Changes since v1:
>> - added error checking for clk_prepare_enable() and also replaced
>> clk_disable() with clk_disable_unprepare() during exit.
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 9537761..014d750 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> +   ret = clk_prepare_enable(ctx->lcd_clk);
>> +   if (ret) {
>> +   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
>> +   return ret;
>> +   }
>> +
>> +   ret = clk_prepare_enable(ctx->bus_clk);
>> +   if (ret) {
>> +   clk_disable_unprepare(ctx->lcd_clk);
>> +   dev_err(dev, "failed to enable 'fimd' clock\n");
>> +   return ret;
>> +   }
>> +
>
> Please remove the above two clk_prepare_enable function calls and use
> them in fimd_clock() instead of clk_enable/disable(). When probed,
> fimd clock will be enabled by runtime pm.
>

Sure, will modify and resend.

> Thanks,
> Inki Dae
>
>> ctx->vidcon0 = pdata->vidcon0;
>> ctx->vidcon1 = pdata->vidcon1;
>> ctx->default_win = pdata->default_win;
>> @@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
>> if (ctx->suspended)
>> goto out;
>>
>> -   clk_disable(ctx->lcd_clk);
>> -   clk_disable(ctx->bus_clk);
>> +   clk_disable_unprepare(ctx->lcd_clk);
>> +   clk_disable_unprepare(ctx->bus_clk);
>>
>> pm_runtime_set_suspended(dev);
>> pm_runtime_put_sync(dev);
>> --
>> 1.7.9.5
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Thanks and Regards
 Vikas Sajjan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/exynos: enable FIMD clocks

[Inki Dae]

2013/3/20 Vikas Sajjan :
> While migrating to common clock framework (CCF), found that the FIMD clocks
> were pulled down by the CCF.
> If CCF finds any clock(s) which has NOT been claimed by any of the
> drivers, then such clock(s) are PULLed low by CCF.
>
> By calling clk_prepare_enable() for FIMD clocks fixes the issue.
>
> this patch also replaces clk_disable() with clk_disable_unprepare()
> during exit.
>
> Signed-off-by: Vikas Sajjan 
> ---
> Changes since v1:
> - added error checking for clk_prepare_enable() and also replaced
> clk_disable() with clk_disable_unprepare() during exit.
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..014d750 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
> return ret;
> }
>
> +   ret = clk_prepare_enable(ctx->lcd_clk);
> +   if (ret) {
> +   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
> +   return ret;
> +   }
> +
> +   ret = clk_prepare_enable(ctx->bus_clk);
> +   if (ret) {
> +   clk_disable_unprepare(ctx->lcd_clk);
> +   dev_err(dev, "failed to enable 'fimd' clock\n");
> +   return ret;
> +   }
> +

Please remove the above two clk_prepare_enable function calls and use
them in fimd_clock() instead of clk_enable/disable(). When probed,
fimd clock will be enabled by runtime pm.

Thanks,
Inki Dae

> ctx->vidcon0 = pdata->vidcon0;
> ctx->vidcon1 = pdata->vidcon1;
> ctx->default_win = pdata->default_win;
> @@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
> if (ctx->suspended)
> goto out;
>
> -   clk_disable(ctx->lcd_clk);
> -   clk_disable(ctx->bus_clk);
> +   clk_disable_unprepare(ctx->lcd_clk);
> +   clk_disable_unprepare(ctx->bus_clk);
>
> pm_runtime_set_suspended(dev);
> pm_runtime_put_sync(dev);
> --
> 1.7.9.5
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/exynos: enable FIMD clocks

[Vikas Sajjan]

While migrating to common clock framework (CCF), found that the FIMD clocks
were pulled down by the CCF.
If CCF finds any clock(s) which has NOT been claimed by any of the
drivers, then such clock(s) are PULLed low by CCF.

By calling clk_prepare_enable() for FIMD clocks fixes the issue.

this patch also replaces clk_disable() with clk_disable_unprepare()
during exit.

Signed-off-by: Vikas Sajjan 
---
Changes since v1:
- added error checking for clk_prepare_enable() and also replaced 
clk_disable() with clk_disable_unprepare() during exit.
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 9537761..014d750 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
return ret;
}
 
+   ret = clk_prepare_enable(ctx->lcd_clk);
+   if (ret) {
+   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
+   return ret;
+   }
+
+   ret = clk_prepare_enable(ctx->bus_clk);
+   if (ret) {
+   clk_disable_unprepare(ctx->lcd_clk);
+   dev_err(dev, "failed to enable 'fimd' clock\n");
+   return ret;
+   }
+
ctx->vidcon0 = pdata->vidcon0;
ctx->vidcon1 = pdata->vidcon1;
ctx->default_win = pdata->default_win;
@@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
if (ctx->suspended)
goto out;
 
-   clk_disable(ctx->lcd_clk);
-   clk_disable(ctx->bus_clk);
+   clk_disable_unprepare(ctx->lcd_clk);
+   clk_disable_unprepare(ctx->bus_clk);
 
pm_runtime_set_suspended(dev);
pm_runtime_put_sync(dev);
-- 
1.7.9.5

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/exynos: enable FIMD clocks

[Vikas Sajjan]

While migrating to common clock framework (CCF), found that the FIMD clocks
were pulled down by the CCF.
If CCF finds any clock(s) which has NOT been claimed by any of the
drivers, then such clock(s) are PULLed low by CCF.

By calling clk_prepare_enable() for FIMD clocks fixes the issue.

this patch also replaces clk_disable() with clk_disable_unprepare()
during exit.

Signed-off-by: Vikas Sajjan 
---
Changes since v1:
- added error checking for clk_prepare_enable() and also replaced 
clk_disable() with clk_disable_unprepare() during exit.
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 9537761..014d750 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -934,6 +934,19 @@ static int fimd_probe(struct platform_device *pdev)
return ret;
}

+   ret = clk_prepare_enable(ctx->lcd_clk);
+   if (ret) {
+   dev_err(dev, "failed to enable 'sclk_fimd' clock\n");
+   return ret;
+   }
+
+   ret = clk_prepare_enable(ctx->bus_clk);
+   if (ret) {
+   clk_disable_unprepare(ctx->lcd_clk);
+   dev_err(dev, "failed to enable 'fimd' clock\n");
+   return ret;
+   }
+
ctx->vidcon0 = pdata->vidcon0;
ctx->vidcon1 = pdata->vidcon1;
ctx->default_win = pdata->default_win;
@@ -981,8 +994,8 @@ static int fimd_remove(struct platform_device *pdev)
if (ctx->suspended)
goto out;

-   clk_disable(ctx->lcd_clk);
-   clk_disable(ctx->bus_clk);
+   clk_disable_unprepare(ctx->lcd_clk);
+   clk_disable_unprepare(ctx->bus_clk);

pm_runtime_set_suspended(dev);
pm_runtime_put_sync(dev);
-- 
1.7.9.5