2013/4/21 Tomasz Figa <tomasz.figa@xxxxxxxxx>
Hi,
I fully agree.
On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
>
The message should be something like:
Common Clock Framework introduced the need to prepare clocks before
enabling them, otherwise clk_enable() fails. This patch adds clk_prepare
calls to the driver.
and that's all.
What you are observing as "CCF pulling clocks down" is the fact that
clk_enable() fails if the clock is not prepared and so the clock is not
enabled in result.
Another thing is that CCF is not pulling anything down. GPIO pins can be
pulled down (or up or not pulled), but clocks can be masked, gated or
simply disabled - this does not imply their signal level.
Why not just simply use clk_prepare_enable() instead of all calls to
> > 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;
> > + }
> > +
clk_enable() in the driver?
Same goes for s/clk_disable/clk_disable_unprepare/ .
I agree with you. Using clk_prepare_enable() is more clear. Actually I had already commented on this. Please see the patch v2. But this way also looks good to me.
> >Viresh is right again here.
> > 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...
Ok, you two guys say together this looks wrong so I'd like to take more checking. I thought that clk->clk_enable is 1 at here and it would be 0 by pm_runtimg_put_sync(). Is there any my missing point?
Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
