<div dir="ltr"><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/21 Tomasz Figa <span dir="ltr"><<a href="mailto:tomasz.figa@gmail.com" target="_blank">tomasz.figa@gmail.com</a>></span><br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
Hi,<br>
<div class="im"><br>
On Monday 08 of April 2013 16:41:54 Viresh Kumar wrote:<br>
> On 8 April 2013 16:37, Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org">vikas.sajjan@linaro.org</a>> wrote:<br>
> > While migrating to common clock framework (CCF), I found that the FIMD<br>
> > clocks were pulled down by the CCF.<br>
> > If CCF finds any clock(s) which has NOT been claimed by any of the<br>
> > drivers, then such clock(s) are PULLed low by CCF.<br>
> ><br>
> > Calling clk_prepare() for FIMD clocks fixes the issue.<br>
> ><br>
> > This patch also replaces clk_disable() with clk_unprepare() during<br>
> > exit, since clk_prepare() is called in fimd_probe().<br>
><br>
> I asked you about fixing your commit log too.. It still looks incorrect<br>
> to me.<br>
><br>
> This patch doesn't have anything to do with CCF pulling clocks down, but<br>
> calling clk_prepare() before clk_enable() is must now.. that's it..<br>
> nothing more.<br>
><br>
<br>
</div>I fully agree.<br>
<br>
The message should be something like:<br>
<br>
Common Clock Framework introduced the need to prepare clocks before<br>
enabling them, otherwise clk_enable() fails. This patch adds clk_prepare<br>
calls to the driver.<br>
<br>
and that's all.<br>
<br>
What you are observing as "CCF pulling clocks down" is the fact that<br>
clk_enable() fails if the clock is not prepared and so the clock is not<br>
enabled in result.<br>
<br>
Another thing is that CCF is not pulling anything down. GPIO pins can be<br>
pulled down (or up or not pulled), but clocks can be masked, gated or<br>
simply disabled - this does not imply their signal level.<br>
<div><div class="h5"><br>
> > Signed-off-by: Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org">vikas.sajjan@linaro.org</a>><br>
> > ---<br>
> ><br>
> > Changes since v3:<br>
> > - added clk_prepare() in fimd_probe() and clk_unprepare() in<br>
> > fimd_remove()><br>
> > as suggested by Viresh Kumar <<a href="mailto:viresh.kumar@linaro.org">viresh.kumar@linaro.org</a>><br>
> ><br>
> > Changes since v2:<br>
> > - moved clk_prepare_enable() and clk_disable_unprepare() from<br>
> > fimd_probe() to fimd_clock() as suggested by Inki Dae<br>
> > <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>>><br>
> > Changes since v1:<br>
> > - added error checking for clk_prepare_enable() and also<br>
> > replaced<br>
> > clk_disable() with clk_disable_unprepare() during exit.<br>
> ><br>
> > ---<br>
> ><br>
> > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 14 ++++++++++++--<br>
> > 1 file changed, 12 insertions(+), 2 deletions(-)<br>
> ><br>
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 9537761..aa22370<br>
> > 100644<br>
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > @@ -934,6 +934,16 @@ static int fimd_probe(struct platform_device<br>
> > *pdev)><br>
> > return ret;<br>
> ><br>
> > }<br>
> ><br>
> > + ret = clk_prepare(ctx->bus_clk);<br>
> > + if (ret < 0)<br>
> > + return ret;<br>
> > +<br>
> > + ret = clk_prepare(ctx->lcd_clk);<br>
> > + if (ret < 0) {<br>
> > + clk_unprepare(ctx->bus_clk);<br>
> > + return ret;<br>
> > + }<br>
> > +<br>
<br>
</div></div>Why not just simply use clk_prepare_enable() instead of all calls to<br>
clk_enable() in the driver?<br>
<br>
Same goes for s/clk_disable/clk_disable_unprepare/ .<br>
<div class="im"><br></div></blockquote><div> </div><div>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.</div>
<div> </div><div> </div><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote"><div class="im">
> ><br>
> > ctx->vidcon0 = pdata->vidcon0;<br>
> > ctx->vidcon1 = pdata->vidcon1;<br>
> > ctx->default_win = pdata->default_win;<br>
> ><br>
> > @@ -981,8 +991,8 @@ static int fimd_remove(struct platform_device<br>
> > *pdev)><br>
> > if (ctx->suspended)<br>
> ><br>
> > goto out;<br>
> ><br>
> > - clk_disable(ctx->lcd_clk);<br>
> > - clk_disable(ctx->bus_clk);<br>
> > + clk_unprepare(ctx->lcd_clk);<br>
> > + clk_unprepare(ctx->bus_clk);<br>
><br>
> This looks wrong again.. You still need to call clk_disable() to make<br>
> clk enabled<br>
> count zero...<br>
<br>
</div>Viresh is right again here.<br>
<br></blockquote><div> </div><div>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?</div>
<div> </div><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
Best regards,<br>
Tomasz<br>
<div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br></div></div>