Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Vikas Sajjan <vikas.sajjan@xxxxxxxxxx>]

Hi Inki Dae and Viresh,

On 8 April 2013 16:41, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:

On 8 April 2013 16:37, Vikas Sajjan <vikas.sajjan@xxxxxxxxxx> wrote:
> While migrating to common clock framework (CCF), I 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.
>
> Calling clk_prepare() for FIMD clocks fixes the issue.
>
> This patch also replaces clk_disable() with clk_unprepare() during exit, since
> clk_prepare() is called in fimd_probe().

I asked you about fixing your commit log too.. It still looks incorrect to me.

This patch doesn't have anything to do with CCF pulling clocks down, but
calling clk_prepare() before clk_enable() is must now.. that's it..
nothing more.

  what I noticed is the fimd_clock() which in turn calls clk_enable(), will only be called if the RUNTIME PM is enabled. So the current patch breaks and display won't appear, if we don't enable the RUNTIME PM. So it becomes mandatory to enable  RUNTIME PM, to FIMD to work.
I am NOT sure whether it is a good idea make FIMD work if and only if RUMTIME PM is enabled.
I guess Mr. Inki Dae can throw more light on this.
Or else make it like the earlier V1 version where clk_prepare_enable() was called in fimd_probe() itself.

> Signed-off-by: Vikas Sajjan <vikas.sajjan@xxxxxxxxxx>
> ---
> Changes since v3:
>         - added clk_prepare() in fimd_probe() and clk_unprepare() in fimd_remove()
>          as suggested by Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Changes since v2:
>         - moved clk_prepare_enable() and clk_disable_unprepare() from
>         fimd_probe() to fimd_clock() as suggested by Inki Dae <inki.dae@xxxxxxxxxxx>
> 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 |   14 ++++++++++++--
>  1 file changed, 12 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..aa22370 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -934,6 +934,16 @@ static int fimd_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       ret = clk_prepare(ctx->bus_clk);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_prepare(ctx->lcd_clk);
> +       if  (ret < 0) {
> +               clk_unprepare(ctx->bus_clk);
> +               return ret;
> +       }
> +
>         ctx->vidcon0 = pdata->vidcon0;
>         ctx->vidcon1 = pdata->vidcon1;
>         ctx->default_win = pdata->default_win;
> @@ -981,8 +991,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_unprepare(ctx->lcd_clk);
> +       clk_unprepare(ctx->bus_clk);

This looks wrong again.. You still need to call clk_disable() to make
clk enabled
count zero...

    Viresh had an suggestion, that the original code had a call clk_disable() in fimd_remove(), which is really NOT required as there is NO clk_enable() in fimd_probe() and we can right away delete clk_disable() from fimd_remove().

And also i think i should be breaking this patch into 2, 1st patch for adding clk_prepare_enable() ( if we want remove dependency on RUNTIME PM ) in fimd_probe() for CCF migration another one for idea of replacing clk_disable() with adding clk_disable_unprepare() (since we will be adding clk_prepare_enable() in probe ) in fimd_remove() .

Mr. Inki Dae any thoughts on this.

--
Thanks and Regards

 Vikas Sajjan

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