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

[Vikas Sajjan <vikas.sajjan@linaro.org>]

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

On Apr 20, 2013 8:56 PM, "Inki Dae" <inki.dae@samsung.com> wrote:
>
>
>
>
> 2013/4/19 Vikas Sajjan <vikas.sajjan@linaro.org>
>>
>> Hi Inki Dae and Viresh,
>>
>> On 8 April 2013 16:41, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>
>>> On 8 April 2013 16:37, Vikas Sajjan <vikas.sajjan@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@linaro.org>
>>> > ---
>>> > Changes since v3:
>>> >         - added clk_prepare() in fimd_probe() and clk_unprepare() in
fimd_remove()
>>> >          as suggested by Viresh Kumar <viresh.kumar@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@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 Mr. Dae

> Thanks,
> Inki Dae
>
>
>>
>> --
>> Thanks and Regards
>>  Vikas Sajjan
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>

[-- Attachment #1.2: Type: text/html, Size: 7218 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