[PATCH v4] drm/exynos: prepare FIMD clocks

[Inki Dae]

2013/4/19 Vikas Sajjan <vikas.sajjan at linaro.org>

> Hi Inki Dae and Viresh,
>
> On 8 April 2013 16:41, Viresh Kumar <viresh.kumar at linaro.org> wrote:
>
>> On 8 April 2013 16:37, Vikas Sajjan <vikas.sajjan at linaro.org> 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.
>

Right, this is our intention.

I am NOT sure whether it is a good idea make FIMD work if and only if
> RUMTIME PM is enabled.
>

Actually, fimd driver had used not only runtime pm interface but also
clk_enable() at fimd_probe(). But this had induced the reference count pair
issue to clock. The issue was that the clock takes two references with
runtime pm. One is by clk_enable and another is by pm_runtime_get_sync().
So we are forcing only using runtime pm interface.


> 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 at linaro.org>
>> > ---
>> > Changes since v3:
>> >         - added clk_prepare() in fimd_probe() and clk_unprepare() in
>> fimd_remove()
>> >          as suggested by Viresh Kumar <viresh.kumar at linaro.org>
>> > Changes since v2:
>> >         - moved clk_prepare_enable() and clk_disable_unprepare() from
>> >         fimd_probe() to fimd_clock() as suggested by Inki Dae <
>> inki.dae at samsung.com>
>> > 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.
>
>
Sorry for being late. I think clk_prepare/unprepare are nothing to do yet
in case of Exynos but those might be used for in the future so your patch
looks good to me as is.

Applied. :)

Thanks,
Inki Dae



> --
> Thanks and Regards
>  Vikas Sajjan
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130421/fa92e05e/attachment.html>