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

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

[Rahul Sharma]

From: Rahul Sharma <Rahul.Sharma@samsung.com>

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 <Rahul.Sharma@samsung.com>
---
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

Re: [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 <rahul.sharma@samsung.com> wrote:
> From: Rahul Sharma <Rahul.Sharma@samsung.com>
>
> 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 <Rahul.Sharma@samsung.com>
> ---
> 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

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

[Rahul Sharma]

On 22 May 2014 11:51, Sachin Kamat <sachin.kamat@linaro.org> 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

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

[Sachin Kamat]

On 22 May 2014 12:06, Rahul Sharma <rahul.sharma@samsung.com> wrote:
> On 22 May 2014 11:51, Sachin Kamat <sachin.kamat@linaro.org> 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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 587 bytes --]

On Thu, May 22, 2014 at 12:06:16PM +0530, Rahul Sharma wrote:
> On 22 May 2014 11:51, Sachin Kamat <sachin.kamat@linaro.org> 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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
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 in probe before accessing fimd registers

[Rahul Sharma]

On 22 May 2014 13:33, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, May 22, 2014 at 12:06:16PM +0530, Rahul Sharma wrote:
>> On 22 May 2014 11:51, Sachin Kamat <sachin.kamat@linaro.org> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>