<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/19 Vikas Sajjan <span dir="ltr"><<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Hi Inki Dae and Viresh,<br><div class="gmail_extra"><br><div class="gmail_quote"><div class="im">On 8 April 2013 16:41, Viresh Kumar <span dir="ltr"><<a href="mailto:viresh.kumar@linaro.org" target="_blank">viresh.kumar@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>On 8 April 2013 16:37, Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>> wrote:<br>
> While migrating to common clock framework (CCF), I found that the FIMD clocks<br>
> 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 exit, since<br>
> clk_prepare() is called in fimd_probe().<br>
<br>
</div>I asked you about fixing your commit log too.. It still looks incorrect 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>
<div><div><br></div></div></blockquote></div><div> 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.<br>
</div></div></div></div></blockquote><div><br></div><div>Right, this is our intention.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>
I am NOT sure whether it is a good idea make FIMD work if and only if RUMTIME PM is enabled.<br></div></div></div></div></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I guess Mr. Inki Dae can throw more light on this.<br>
Or else make it like the earlier V1 version where clk_prepare_enable() was called in fimd_probe() itself.<br>
<br></div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>
> Signed-off-by: Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>><br>
> ---<br>
> Changes since v3:<br>
> - added clk_prepare() in fimd_probe() and clk_unprepare() in fimd_remove()<br>
> as suggested by Viresh Kumar <<a href="mailto:viresh.kumar@linaro.org" target="_blank">viresh.kumar@linaro.org</a>><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 <<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>><br>
> Changes since v1:<br>
> - added error checking for clk_prepare_enable() and also replaced<br>
> clk_disable() with clk_disable_unprepare() during exit.<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 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> index 9537761..aa22370 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 *pdev)<br>
> return ret;<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>
> ctx->vidcon0 = pdata->vidcon0;<br>
> ctx->vidcon1 = pdata->vidcon1;<br>
> ctx->default_win = pdata->default_win;<br>
> @@ -981,8 +991,8 @@ static int fimd_remove(struct platform_device *pdev)<br>
> if (ctx->suspended)<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>
</div></div>This looks wrong again.. You still need to call clk_disable() to make<br>
clk enabled<br>
count zero...<br>
</blockquote></div></div></div><br></div><div class="gmail_extra"> 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().<br>
<br>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() .<br>
</div><div class="gmail_extra"><br>
Mr. Inki Dae any thoughts on this.<span class="HOEnZb"><font color="#888888"><br clear="all"></font></span></div><span class="HOEnZb"><font color="#888888"><div class="gmail_extra"><br></div></font></span></div></blockquote>
<div><br></div><div>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.<br><br>Applied. :)<br><br>Thanks,<br>
Inki Dae<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="HOEnZb"><font color="#888888"><div class="gmail_extra">-- <br>
Thanks and Regards<div> Vikas Sajjan</div>
</div></font></span></div>
<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>
<br></blockquote></div><br></div></div>