[PATCH v4] drm/exynos: prepare FIMD clocks

[Vikas Sajjan]

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.
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 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.

-- 
Thanks and Regards
 Vikas Sajjan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130419/5c2e9403/attachment-0001.html>