[PATCH v4] drm/exynos: prepare FIMD clocks

[PATCH v4] drm/exynos: prepare FIMD clocks

[Vikas Sajjan]

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

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);
 
 	pm_runtime_set_suspended(dev);
 	pm_runtime_put_sync(dev);
-- 
1.7.9.5

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

[Viresh Kumar]

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.

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

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

[Vikas Sajjan]

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

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

-- 
Thanks and Regards
 Vikas Sajjan

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

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

[Inki Dae]

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

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,
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: 7575 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

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

[Vikas Sajjan]

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

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

[Viresh Kumar]

On 20 April 2013 20:56, Inki Dae <inki.dae@samsung.com> wrote:
> 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. :)

And you think the comments i gave were incorrect then? Why?

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

[Tomasz Figa]

Hi,

On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> 

I fully agree.

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.

> > 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;
> > +       }
> > +

Why not just simply use clk_prepare_enable() instead of all calls to 
clk_enable() in the driver?

Same goes for s/clk_disable/clk_disable_unprepare/ .

> > 
> >         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 is right again here.

Best regards,
Tomasz

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

[Tomasz Figa]

On Sunday 21 of April 2013 13:23:10 Viresh Kumar wrote:
> On 20 April 2013 20:56, Inki Dae <inki.dae@samsung.com> wrote:
> > 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. :)
> 

Hmm. Now, after searching for this thread in dri-devel archives, I'm 
wondering why I haven't received some of messages from this thread through 
linux-samsung-soc mailing list...

I believe linux-samsung-soc list exists to collect all threads related to 
Samsung SoCs for people that don't want to subscribe to lists like dri-
devel, on which there is a lot of threads irrelevant to them, with the 
risk of missing the important ones.

Please always make sure that any discussion about Samsung SoCs (patches in 
particular) is going through linux-samsung-soc as well.

Thanks in advance.

Best regards,
Tomasz

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

[Inki Dae]

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

2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>

> Hi,
>
> On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> >
>
> I fully agree.
>
> 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.
>
> > > 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;
> > > +       }
> > > +
>
> Why not just simply use clk_prepare_enable() instead of all calls to
> 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.



> > >
> > >         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 is right again here.
>
>
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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

[Inki Dae]

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

2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>

> On Sunday 21 of April 2013 13:23:10 Viresh Kumar wrote:
> > On 20 April 2013 20:56, Inki Dae <inki.dae@samsung.com> wrote:
> > > 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. :)
> >
>
> Hmm. Now, after searching for this thread in dri-devel archives, I'm
> wondering why I haven't received some of messages from this thread through
> linux-samsung-soc mailing list...
>
> I believe linux-samsung-soc list exists to collect all threads related to
> Samsung SoCs for people that don't want to subscribe to lists like dri-
> devel, on which there is a lot of threads irrelevant to them, with the
> risk of missing the important ones.
>
> Please always make sure that any discussion about Samsung SoCs (patches in
> particular) is going through linux-samsung-soc as well.
>
>
Thanks for your advice. As you said, some people might not want to
subscribe to some mainling lists they don't want. And I think that the main
mailing list on this patch is dri-devel so you must receive this email
thread if you subscribed to the dri-devel. Anyway it would be best to share
this all mailing lists included in this email thread but if so, I have no
doubt to receive email bumb. :(

Thanks,
Inki Dae


> Thanks in advance.
>
> Best regards,
> Tomasz
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

[Tomasz Figa]

Hi Inki,

On Sunday 21 of April 2013 23:05:45 Inki Dae wrote:
> 2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>
> 
> > On Sunday 21 of April 2013 13:23:10 Viresh Kumar wrote:
> > > On 20 April 2013 20:56, Inki Dae <inki.dae@samsung.com> wrote:
> > > > 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. :)
> > 
> > Hmm. Now, after searching for this thread in dri-devel archives, I'm
> > wondering why I haven't received some of messages from this thread
> > through linux-samsung-soc mailing list...
> > 
> > I believe linux-samsung-soc list exists to collect all threads related
> > to Samsung SoCs for people that don't want to subscribe to lists like
> > dri- devel, on which there is a lot of threads irrelevant to them,
> > with the risk of missing the important ones.
> > 
> > Please always make sure that any discussion about Samsung SoCs
> > (patches in particular) is going through linux-samsung-soc as well.
> 
> Thanks for your advice. As you said, some people might not want to
> subscribe to some mainling lists they don't want. And I think that the
> main mailing list on this patch is dri-devel so you must receive this
> email thread if you subscribed to the dri-devel.

I agree that dri-devel is the target mailing list for DRM patches, but 
AFAIK all threads related to Samsung SoCs should be sent to linux-samsung-
soc as well.

For example, I don't subscribe dri-devel, but I do linux-samsung-soc, 
because all I want to follow is all the works related to Samsung SoCs.
Remaining threads on dri-devel are outside of my competencies.

> Anyway it would be
> best to share this all mailing lists included in this email thread but
> if so, I have no doubt to receive email bumb. :(

Hmm, you don't have to subscribe to a mailing list to post to it.

Actually I'm wondering if the fact that your previous messages did not get 
to the linux-samsung-soc list was not caused by presence of HTML part in 
your messages, which is strongly discouraged on all mailing lists and 
actually blocked on vger.kernel.org where linux-samsung-soc is hosted.

Best regards,
Tomasz

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

[Tomasz Figa]

Hi Inki,

On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
> 2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>
> 
> > Hi,
> > 
> > On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> > 
> > I fully agree.
> > 
> > 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.
> > 
> > > > 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;
> > > > +       }
> > > > +
> > 
> > Why not just simply use clk_prepare_enable() instead of all calls to
> > 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.

Well, both versions are technically correct and will have the same effect 
for Exynos SoC clocks, since only enable/disable ops change hardware 
state.

However if we look at general meaning of those generic ops, the clock will 
remain prepared for all the time the driver is loaded, even if the device 
is runtime suspended. Again on Exynos SoCs this won't have any effect, but 
I think we should respect general Common Clock Framework semantics anyway.

> > > >         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 is right again here.
> 
> 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?

You're reasoning is correct, but only assuming that runtime PM is enabled. 
When it is disabled, pm_runtime_put_sync() is a no-op.

Well, after digging into the exynos_drm_fimd driver a bit more, it seems 
like its power management code needs a serious rework, because I was able 
to find more problems:

1) fimd_activate() does not get called at all if CONFIG_PM_RUNTIME is not 
enabled (except in system-wide suspend callbacks, but this is irrelevant 
to this point) - this means that the hardware is not properly initialized 
without CONFIG_PM_RUNTIME - at least clocks does not get enabled.

2) pm_runtime_set_suspended() can be used only when runtime PM is disabled 
for the device (i.e. by calling pm_runtime_disable() or not calling 
pm_runtime_enable() at all) - when runtime PM is enabled it is basically a 
no-op returning -EAGAIN error.

So here's my proposed solution:

1) call fimd_activate() and pm_runtime_set_active() explicitly in 
fimd_probe(), before calling pm_runtime_enable():

 	mutex_init(&ctx->lock);
 
 	platform_set_drvdata(pdev, ctx);
+
+	fimd_activate(ctx, true);
 
+	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);

This would power up the device even if CONFIG_PM_RUNTIME is not enabled. 
Note that pm_runtime_get_sync() after marking the device as active with 
pm_runtime_set_active() won't result in calling fimd_runtime_resume(), 
because the device is considered already resumed.

2) in fimd_remove():

+	pm_runtime_disable(dev);
+
 	if (ctx->suspended)
-		goto out;
+		return 0;
 
-	clk_disable(ctx->lcd_clk);
-	clk_disable(ctx->bus_clk);
+	fimd_activate(ctx, false);
 
+	pm_runtime_put_noidle(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_sync(dev);
-
-out:
-	pm_runtime_disable(dev);

First, pm_runtime_disable() will prevent any further runtime PM operations 
that could change ctx->suspended state. Then, if ctx->suspended is true, 
there is no need to suspend anything and we can leave. Otherwise, we power 
down the hardware manually - which will work with both CONFIG_PM_RUNTIME 
enabled and disabled, and then mark the hardware as suspended and free 
remaining reference in runtime PM core. Note that pm_runtime_put_noidle 
just decreases the reference counter and nothing else.

3) after those two changes, all that remains is to fix compliance with 
Common Clock Framework, in other words:

s/clk_enable/clk_prepare_enable/

and

s/clk_disable/clk_disable_unprepare/
 
Best regards,
Tomasz

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

[Inki Dae]

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

Hi Tomasz,

CCing Mr. Ham


2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>

> Hi Inki,
>
> On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
> > 2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>
> >
> > > Hi,
> > >
> > > On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> > >
> > > I fully agree.
> > >
> > > 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.
> > >
> > > > > 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;
> > > > > +       }
> > > > > +
> > >
> > > Why not just simply use clk_prepare_enable() instead of all calls to
> > > 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.
>
> Well, both versions are technically correct and will have the same effect
> for Exynos SoC clocks, since only enable/disable ops change hardware
> state.
>
> However if we look at general meaning of those generic ops, the clock will
> remain prepared for all the time the driver is loaded, even if the device
>

Right, so I said previous one is more clear. I gonna revert current one and
then merge previous one(v3)



> is runtime suspended. Again on Exynos SoCs this won't have any effect, but
> I think we should respect general Common Clock Framework semantics anyway.
>
> > > > >         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 is right again here.
> >
> > 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?
>
> You're reasoning is correct, but only assuming that runtime PM is enabled.
> When it is disabled, pm_runtime_put_sync() is a no-op.
>
> Well, after digging into the exynos_drm_fimd driver a bit more, it seems
> like its power management code needs a serious rework, because I was able
> to find more problems:
>
> 1) fimd_activate() does not get called at all if CONFIG_PM_RUNTIME is not
> enabled (except in system-wide suspend callbacks, but this is irrelevant
> to this point) - this means that the hardware is not properly initialized
> without CONFIG_PM_RUNTIME - at least clocks does not get enabled.
>
> 2) pm_runtime_set_suspended() can be used only when runtime PM is disabled
> for the device (i.e. by calling pm_runtime_disable() or not calling
> pm_runtime_enable() at all) - when runtime PM is enabled it is basically a
> no-op returning -EAGAIN error.
>
> So here's my proposed solution:
>
> 1) call fimd_activate() and pm_runtime_set_active() explicitly in
> fimd_probe(), before calling pm_runtime_enable():
>
>         mutex_init(&ctx->lock);
>
>         platform_set_drvdata(pdev, ctx);
> +
> +       fimd_activate(ctx, true);
>
> +       pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         pm_runtime_get_sync(dev);
>
> This would power up the device even if CONFIG_PM_RUNTIME is not enabled.
> Note that pm_runtime_get_sync() after marking the device as active with
> pm_runtime_set_active() won't result in calling fimd_runtime_resume(),
> because the device is considered already resumed.
>
> 2) in fimd_remove():
>
> +       pm_runtime_disable(dev);
> +
>         if (ctx->suspended)
> -               goto out;
> +               return 0;
>
> -       clk_disable(ctx->lcd_clk);
> -       clk_disable(ctx->bus_clk);
> +       fimd_activate(ctx, false);
>
> +       pm_runtime_put_noidle(dev);
>         pm_runtime_set_suspended(dev);
> -       pm_runtime_put_sync(dev);
> -
> -out:
> -       pm_runtime_disable(dev);
>
> First, pm_runtime_disable() will prevent any further runtime PM operations
> that could change ctx->suspended state. Then, if ctx->suspended is true,
> there is no need to suspend anything and we can leave. Otherwise, we power
> down the hardware manually - which will work with both CONFIG_PM_RUNTIME
> enabled and disabled, and then mark the hardware as suspended and free
> remaining reference in runtime PM core. Note that pm_runtime_put_noidle
> just decreases the reference counter and nothing else.
>
> 3) after those two changes, all that remains is to fix compliance with
> Common Clock Framework, in other words:
>
> s/clk_enable/clk_prepare_enable/
>
> and
>
> s/clk_disable/clk_disable_unprepare/
>
>
Also looks good to me. But what if power domain was disabled without pm
runtime? In this case, you must enable the power domain at machine code or
bootloader somewhere. This way would not only need some hard codes to turn
the power domain on but also not manage power management fully. This is
same as only the use of pm runtime interface(needing some hard codes
without pm runtime) so I don't prefer to add clk_enable/disable to fimd
probe(). I quite tend to force only the use of pm runtime as possible. So
please add the hard codes to machine code or bootloader like you did for
power domain if you want to use drm fimd without pm runtime.

Thanks,
Inki Dae


> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

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

[Viresh Kumar]

On 21 April 2013 20:13, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 3) after those two changes, all that remains is to fix compliance with
> Common Clock Framework, in other words:
>
> s/clk_enable/clk_prepare_enable/
>
> and
>
> s/clk_disable/clk_disable_unprepare/

We don't have to call  clk_{un}prepare() everytime for your platform as
you aren't doing anything in it. So just call them once at probe/remove and
call clk_enable/disable everywhere else.

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

[Tomasz Figa]

On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
> > > 2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>
> > >
> > > > Hi,
> > > >
> > > > On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> > > >
> > > > I fully agree.
> > > >
> > > > 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.
> > > >
> > > > > > 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;
> > > > > > +       }
> > > > > > +
> > > >
> > > > Why not just simply use clk_prepare_enable() instead of all calls to
> > > > 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.
> > 
> > 
> > Well, both versions are technically correct and will have the same effect
> > for Exynos SoC clocks, since only enable/disable ops change hardware
> > state.
> > 
> > However if we look at general meaning of those generic ops, the clock will
> > remain prepared for all the time the driver is loaded, even if the device
> > 
> > 
> > 
> > Right, so I said previous one is more clear. I gonna revert current one 
and then merge previous one(v3)
> > 
> > 
> >  
> > is runtime suspended. Again on Exynos SoCs this won't have any effect, but
> > I think we should respect general Common Clock Framework semantics anyway.
> > 
> > 
> > > > > >         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 is right again here.
> > >
> > > 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?
> > 
> > 
> > You're reasoning is correct, but only assuming that runtime PM is enabled.
> > When it is disabled, pm_runtime_put_sync() is a no-op.
> > 
> > Well, after digging into the exynos_drm_fimd driver a bit more, it seems
> > like its power management code needs a serious rework, because I was able
> > to find more problems:
> > 
> > 1) fimd_activate() does not get called at all if CONFIG_PM_RUNTIME is not
> > enabled (except in system-wide suspend callbacks, but this is irrelevant
> > to this point) - this means that the hardware is not properly initialized
> > without CONFIG_PM_RUNTIME - at least clocks does not get enabled.
> > 
> > 2) pm_runtime_set_suspended() can be used only when runtime PM is disabled
> > for the device (i.e. by calling pm_runtime_disable() or not calling
> > pm_runtime_enable() at all) - when runtime PM is enabled it is basically a
> > no-op returning -EAGAIN error.
> > 
> > So here's my proposed solution:
> > 
> > 1) call fimd_activate() and pm_runtime_set_active() explicitly in
> > fimd_probe(), before calling pm_runtime_enable():
> > 
> >         mutex_init(&ctx->lock);
> > 
> >         platform_set_drvdata(pdev, ctx);
> > +
> > +       fimd_activate(ctx, true);
> > 
> > +       pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> >         pm_runtime_get_sync(dev);
> > 
> > This would power up the device even if CONFIG_PM_RUNTIME is not enabled.
> > Note that pm_runtime_get_sync() after marking the device as active with
> > pm_runtime_set_active() won't result in calling fimd_runtime_resume(),
> > because the device is considered already resumed.
> > 
> > 2) in fimd_remove():
> > 
> > +       pm_runtime_disable(dev);
> > +
> >         if (ctx->suspended)
> > -               goto out;
> > +               return 0;
> > 
> > 
> > -       clk_disable(ctx->lcd_clk);
> > -       clk_disable(ctx->bus_clk);
> > 
> > +       fimd_activate(ctx, false);
> > 
> > +       pm_runtime_put_noidle(dev);
> >         pm_runtime_set_suspended(dev);
> > -       pm_runtime_put_sync(dev);
> > -
> > -out:
> > -       pm_runtime_disable(dev);
> > 
> > First, pm_runtime_disable() will prevent any further runtime PM operations
> > that could change ctx->suspended state. Then, if ctx->suspended is true,
> > there is no need to suspend anything and we can leave. Otherwise, we power
> > down the hardware manually - which will work with both CONFIG_PM_RUNTIME
> > enabled and disabled, and then mark the hardware as suspended and free
> > remaining reference in runtime PM core. Note that pm_runtime_put_noidle
> > just decreases the reference counter and nothing else.
> > 
> > 3) after those two changes, all that remains is to fix compliance with
> > Common Clock Framework, in other words:
> > 
> > s/clk_enable/clk_prepare_enable/
> > 
> > and
> > 
> > s/clk_disable/clk_disable_unprepare/
> 
> 
> Also looks good to me. But what if power domain was disabled without pm
> runtime? In this case, you must enable the power domain at machine code or
> bootloader somewhere. This way would not only need some hard codes to turn
> the power domain on but also not manage power management fully. This is same
> as only the use of pm runtime interface(needing some hard codes without pm
> runtime) so I don't prefer to add clk_enable/disable to fimd probe(). I quite
> tend to force only the use of pm runtime as possible. So please add the hard
> codes to machine code or bootloader like you did for power domain if you
> want to use drm fimd without pm runtime.

That's not how the runtime PM, clock subsystems work:

1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be kept 
powered on all the time.

2) Common Clock Framework will always gate all clocks that have zero 
enable_count. Note that CCF support for Exynos is already merged for 3.10 and 
it will be the only available clock support method for Exynos.

AFAIK, drivers must work correctly in both cases, with CONFIG_PM_RUNTIME 
enabled and disabled.

Best regards,
Tomasz

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

[Tomasz Figa]

On Monday 22 of April 2013 10:44:00 Viresh Kumar wrote:
> On 21 April 2013 20:13, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > 3) after those two changes, all that remains is to fix compliance with
> > Common Clock Framework, in other words:
> > 
> > s/clk_enable/clk_prepare_enable/
> > 
> > and
> > 
> > s/clk_disable/clk_disable_unprepare/
> 
> We don't have to call  clk_{un}prepare() everytime for your platform as
> you aren't doing anything in it. So just call them once at probe/remove and
> call clk_enable/disable everywhere else.

Can you assure that in future SoCs, on which this driver will be used, this 
assumption will still hold true or even that in current Exynos driver this 
behavior won't be changed?

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

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

[Inki Dae]

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

2013/4/22 Tomasz Figa <t.figa@samsung.com>

> On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
> > > > 2013/4/21 Tomasz Figa <tomasz.figa@gmail.com>
> > > >
> > > > > Hi,
> > > > >
> > > > > On Monday 08 of April 2013 16:41:54 Viresh Kumar 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.
> > > > >
> > > > > I fully agree.
> > > > >
> > > > > 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.
> > > > >
> > > > > > > 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;
> > > > > > > +       }
> > > > > > > +
> > > > >
> > > > > Why not just simply use clk_prepare_enable() instead of all calls
> to
> > > > > 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.
> > >
> > >
> > > Well, both versions are technically correct and will have the same
> effect
> > > for Exynos SoC clocks, since only enable/disable ops change hardware
> > > state.
> > >
> > > However if we look at general meaning of those generic ops, the clock
> will
> > > remain prepared for all the time the driver is loaded, even if the
> device
> > >
> > >
> > >
> > > Right, so I said previous one is more clear. I gonna revert current one
> and then merge previous one(v3)
> > >
> > >
> > >
> > > is runtime suspended. Again on Exynos SoCs this won't have any effect,
> but
> > > I think we should respect general Common Clock Framework semantics
> anyway.
> > >
> > >
> > > > > > >         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 is right again here.
> > > >
> > > > 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?
> > >
> > >
> > > You're reasoning is correct, but only assuming that runtime PM is
> enabled.
> > > When it is disabled, pm_runtime_put_sync() is a no-op.
> > >
> > > Well, after digging into the exynos_drm_fimd driver a bit more, it
> seems
> > > like its power management code needs a serious rework, because I was
> able
> > > to find more problems:
> > >
> > > 1) fimd_activate() does not get called at all if CONFIG_PM_RUNTIME is
> not
> > > enabled (except in system-wide suspend callbacks, but this is
> irrelevant
> > > to this point) - this means that the hardware is not properly
> initialized
> > > without CONFIG_PM_RUNTIME - at least clocks does not get enabled.
> > >
> > > 2) pm_runtime_set_suspended() can be used only when runtime PM is
> disabled
> > > for the device (i.e. by calling pm_runtime_disable() or not calling
> > > pm_runtime_enable() at all) - when runtime PM is enabled it is
> basically a
> > > no-op returning -EAGAIN error.
> > >
> > > So here's my proposed solution:
> > >
> > > 1) call fimd_activate() and pm_runtime_set_active() explicitly in
> > > fimd_probe(), before calling pm_runtime_enable():
> > >
> > >         mutex_init(&ctx->lock);
> > >
> > >         platform_set_drvdata(pdev, ctx);
> > > +
> > > +       fimd_activate(ctx, true);
> > >
> > > +       pm_runtime_set_active(dev);
> > >         pm_runtime_enable(dev);
> > >         pm_runtime_get_sync(dev);
> > >
> > > This would power up the device even if CONFIG_PM_RUNTIME is not
> enabled.
> > > Note that pm_runtime_get_sync() after marking the device as active with
> > > pm_runtime_set_active() won't result in calling fimd_runtime_resume(),
> > > because the device is considered already resumed.
> > >
> > > 2) in fimd_remove():
> > >
> > > +       pm_runtime_disable(dev);
> > > +
> > >         if (ctx->suspended)
> > > -               goto out;
> > > +               return 0;
> > >
> > >
> > > -       clk_disable(ctx->lcd_clk);
> > > -       clk_disable(ctx->bus_clk);
> > >
> > > +       fimd_activate(ctx, false);
> > >
> > > +       pm_runtime_put_noidle(dev);
> > >         pm_runtime_set_suspended(dev);
> > > -       pm_runtime_put_sync(dev);
> > > -
> > > -out:
> > > -       pm_runtime_disable(dev);
> > >
> > > First, pm_runtime_disable() will prevent any further runtime PM
> operations
> > > that could change ctx->suspended state. Then, if ctx->suspended is
> true,
> > > there is no need to suspend anything and we can leave. Otherwise, we
> power
> > > down the hardware manually - which will work with both
> CONFIG_PM_RUNTIME
> > > enabled and disabled, and then mark the hardware as suspended and free
> > > remaining reference in runtime PM core. Note that pm_runtime_put_noidle
> > > just decreases the reference counter and nothing else.
> > >
> > > 3) after those two changes, all that remains is to fix compliance with
> > > Common Clock Framework, in other words:
> > >
> > > s/clk_enable/clk_prepare_enable/
> > >
> > > and
> > >
> > > s/clk_disable/clk_disable_unprepare/
> >
> >
> > Also looks good to me. But what if power domain was disabled without pm
> > runtime? In this case, you must enable the power domain at machine code
> or
> > bootloader somewhere. This way would not only need some hard codes to
> turn
> > the power domain on but also not manage power management fully. This is
> same
> > as only the use of pm runtime interface(needing some hard codes without
> pm
> > runtime) so I don't prefer to add clk_enable/disable to fimd probe(). I
> quite
> > tend to force only the use of pm runtime as possible. So please add the
> hard
> > codes to machine code or bootloader like you did for power domain if you
> > want to use drm fimd without pm runtime.
>
> That's not how the runtime PM, clock subsystems work:
>
> 1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be kept
> powered on all the time.
>
> 2) Common Clock Framework will always gate all clocks that have zero
> enable_count. Note that CCF support for Exynos is already merged for 3.10
> and
> it will be the only available clock support method for Exynos.
>
> AFAIK, drivers must work correctly in both cases, with CONFIG_PM_RUNTIME
> enabled and disabled.
>
>
Then is the driver worked correctly if the power domain to this device was
disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?  I
think, in this case, the device wouldn't be worked correctly because the
power of the device remains off. So you must enable the power domain
somewhere. What is the difference between these two cases?



> Best regards,
> Tomasz
>
>

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

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

[Sylwester Nawrocki]

On 04/22/2013 11:56 AM, Tomasz Figa wrote:
> On Monday 22 of April 2013 10:44:00 Viresh Kumar wrote:
>> On 21 April 2013 20:13, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> 3) after those two changes, all that remains is to fix compliance with
>>> Common Clock Framework, in other words:
>>>
>>> s/clk_enable/clk_prepare_enable/
>>>
>>> and
>>>
>>> s/clk_disable/clk_disable_unprepare/
>>
>> We don't have to call  clk_{un}prepare() everytime for your platform as
>> you aren't doing anything in it. So just call them once at probe/remove and
>> call clk_enable/disable everywhere else.

Yes, I agree with that. Additionally clk_(un)prepare must not be called in
atomic context, so some drivers will have to work like this anyway.
Or the clocks could be prepared/unprepared in the device open/close file op
for instance.

> Can you assure that in future SoCs, on which this driver will be used, this 
> assumption will still hold true or even that in current Exynos driver this 
> behavior won't be changed?

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

[Sylwester Nawrocki]

On 04/22/2013 12:03 PM, Inki Dae wrote:
>     > Also looks good to me. But what if power domain was disabled without pm
>     > runtime? In this case, you must enable the power domain at machine code or
>     > bootloader somewhere. This way would not only need some hard codes to turn
>     > the power domain on but also not manage power management fully. This is same
>     > as only the use of pm runtime interface(needing some hard codes without pm
>     > runtime) so I don't prefer to add clk_enable/disable to fimd probe(). I quite
>     > tend to force only the use of pm runtime as possible. So please add the hard
>     > codes to machine code or bootloader like you did for power domain if you
>     > want to use drm fimd without pm runtime.
> 
>     That's not how the runtime PM, clock subsystems work:
> 
>     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be kept
>     powered on all the time.
> 
>     2) Common Clock Framework will always gate all clocks that have zero
>     enable_count. Note that CCF support for Exynos is already merged for 3.10 and
>     it will be the only available clock support method for Exynos.
> 
>     AFAIK, drivers must work correctly in both cases, with CONFIG_PM_RUNTIME
>     enabled and disabled.
> 
> 
> Then is the driver worked correctly if the power domain to this device was
> disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?  I
> think, in this case, the device wouldn't be worked correctly because the power
> of the device remains off. So you must enable the power domain somewhere. What
> is the difference between these two cases?

How about making the driver dependant on PM_RUNTIME and making it always
use pm_runtime_* API, regardless if the platform actually implements runtime
PM or not ? Is there any issue in using the Runtime PM core always, rather
than coding any workarounds in drivers when PM_RUNTIME is disabled ?

Thanks,
Sylwester

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

[Inki Dae]

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

2013/4/22 Sylwester Nawrocki <s.nawrocki@samsung.com>

> On 04/22/2013 12:03 PM, Inki Dae wrote:
> >     > Also looks good to me. But what if power domain was disabled
> without pm
> >     > runtime? In this case, you must enable the power domain at machine
> code or
> >     > bootloader somewhere. This way would not only need some hard codes
> to turn
> >     > the power domain on but also not manage power management fully.
> This is same
> >     > as only the use of pm runtime interface(needing some hard codes
> without pm
> >     > runtime) so I don't prefer to add clk_enable/disable to fimd
> probe(). I quite
> >     > tend to force only the use of pm runtime as possible. So please
> add the hard
> >     > codes to machine code or bootloader like you did for power domain
> if you
> >     > want to use drm fimd without pm runtime.
> >
> >     That's not how the runtime PM, clock subsystems work:
> >
> >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be
> kept
> >     powered on all the time.
> >
> >     2) Common Clock Framework will always gate all clocks that have zero
> >     enable_count. Note that CCF support for Exynos is already merged for
> 3.10 and
> >     it will be the only available clock support method for Exynos.
> >
> >     AFAIK, drivers must work correctly in both cases, with
> CONFIG_PM_RUNTIME
> >     enabled and disabled.
> >
> >
> > Then is the driver worked correctly if the power domain to this device
> was
> > disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?
>  I
> > think, in this case, the device wouldn't be worked correctly because the
> power
> > of the device remains off. So you must enable the power domain
> somewhere. What
> > is the difference between these two cases?
>
> How about making the driver dependant on PM_RUNTIME and making it always
> use pm_runtime_* API, regardless if the platform actually implements
> runtime
> PM or not ? Is there any issue in using the Runtime PM core always, rather
> than coding any workarounds in drivers when PM_RUNTIME is disabled ?
>
>
That's what I want~~ :)




> Thanks,
> Sylwester
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

[Viresh Kumar]

On 22 April 2013 15:26, Tomasz Figa <t.figa@samsung.com> wrote:
> Can you assure that in future SoCs, on which this driver will be used, this
> assumption will still hold true or even that in current Exynos driver this
> behavior won't be changed?

Probably yes.. Registers for enabling/disabling these clocks should always
be on AMBA bus and not on SPI/I2C, i.e. on-soc... and so this will hold
true.

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

[Tomasz Figa]

On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> On 04/22/2013 12:03 PM, Inki Dae wrote:
> >     > Also looks good to me. But what if power domain was disabled without
> >     > pm
> >     > runtime? In this case, you must enable the power domain at machine
> >     > code or
> >     > bootloader somewhere. This way would not only need some hard codes
> >     > to turn
> >     > the power domain on but also not manage power management fully. This
> >     > is same as only the use of pm runtime interface(needing some hard
> >     > codes without pm runtime) so I don't prefer to add
> >     > clk_enable/disable to fimd probe(). I quite tend to force only the
> >     > use of pm runtime as possible. So please add the hard codes to
> >     > machine code or bootloader like you did for power domain if you
> >     > want to use drm fimd without pm runtime.
> >     
> >     That's not how the runtime PM, clock subsystems work:
> >     
> >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be
> >     kept
> >     powered on all the time.
> >     
> >     2) Common Clock Framework will always gate all clocks that have zero
> >     enable_count. Note that CCF support for Exynos is already merged for
> >     3.10 and it will be the only available clock support method for
> >     Exynos.
> >     
> >     AFAIK, drivers must work correctly in both cases, with
> >     CONFIG_PM_RUNTIME
> >     enabled and disabled.
> > 
> > Then is the driver worked correctly if the power domain to this device was
> > disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?  I
> > think, in this case, the device wouldn't be worked correctly because the
> > power of the device remains off. So you must enable the power domain
> > somewhere. What is the difference between these two cases?
> 
> How about making the driver dependant on PM_RUNTIME and making it always
> use pm_runtime_* API, regardless if the platform actually implements runtime
> PM or not ? Is there any issue in using the Runtime PM core always, rather
> than coding any workarounds in drivers when PM_RUNTIME is disabled ?

I don't think this is a good idea. This would mean that any user that from 
some reasons don't want to use PM_RUNTIME, would not be able to use the driver 
anymore.

Rafael, Kevin, do you have any opinion on this?

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

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

[Rafael J. Wysocki]

On Monday, April 22, 2013 12:37:36 PM Tomasz Figa wrote:
> On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> > On 04/22/2013 12:03 PM, Inki Dae wrote:
> > >     > Also looks good to me. But what if power domain was disabled without
> > >     > pm
> > >     > runtime? In this case, you must enable the power domain at machine
> > >     > code or
> > >     > bootloader somewhere. This way would not only need some hard codes
> > >     > to turn
> > >     > the power domain on but also not manage power management fully. This
> > >     > is same as only the use of pm runtime interface(needing some hard
> > >     > codes without pm runtime) so I don't prefer to add
> > >     > clk_enable/disable to fimd probe(). I quite tend to force only the
> > >     > use of pm runtime as possible. So please add the hard codes to
> > >     > machine code or bootloader like you did for power domain if you
> > >     > want to use drm fimd without pm runtime.
> > >     
> > >     That's not how the runtime PM, clock subsystems work:
> > >     
> > >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be
> > >     kept
> > >     powered on all the time.
> > >     
> > >     2) Common Clock Framework will always gate all clocks that have zero
> > >     enable_count. Note that CCF support for Exynos is already merged for
> > >     3.10 and it will be the only available clock support method for
> > >     Exynos.
> > >     
> > >     AFAIK, drivers must work correctly in both cases, with
> > >     CONFIG_PM_RUNTIME
> > >     enabled and disabled.
> > > 
> > > Then is the driver worked correctly if the power domain to this device was
> > > disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?  I
> > > think, in this case, the device wouldn't be worked correctly because the
> > > power of the device remains off. So you must enable the power domain
> > > somewhere. What is the difference between these two cases?
> > 
> > How about making the driver dependant on PM_RUNTIME and making it always
> > use pm_runtime_* API, regardless if the platform actually implements runtime
> > PM or not ? Is there any issue in using the Runtime PM core always, rather
> > than coding any workarounds in drivers when PM_RUNTIME is disabled ?
> 
> I don't think this is a good idea. This would mean that any user that from 
> some reasons don't want to use PM_RUNTIME, would not be able to use the driver 
> anymore.
> 
> Rafael, Kevin, do you have any opinion on this?

I agree.

Drivers should work for CONFIG_PM_RUNTIME unset too and static inline stubs for
all runtime PM helpers are available in that case.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Inki Dae]

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

2013/4/22 Tomasz Figa <t.figa@samsung.com>

> On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> > On 04/22/2013 12:03 PM, Inki Dae wrote:
> > >     > Also looks good to me. But what if power domain was disabled
> without
> > >     > pm
> > >     > runtime? In this case, you must enable the power domain at
> machine
> > >     > code or
> > >     > bootloader somewhere. This way would not only need some hard
> codes
> > >     > to turn
> > >     > the power domain on but also not manage power management fully.
> This
> > >     > is same as only the use of pm runtime interface(needing some hard
> > >     > codes without pm runtime) so I don't prefer to add
> > >     > clk_enable/disable to fimd probe(). I quite tend to force only
> the
> > >     > use of pm runtime as possible. So please add the hard codes to
> > >     > machine code or bootloader like you did for power domain if you
> > >     > want to use drm fimd without pm runtime.
> > >
> > >     That's not how the runtime PM, clock subsystems work:
> > >
> > >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must
> be
> > >     kept
> > >     powered on all the time.
> > >
> > >     2) Common Clock Framework will always gate all clocks that have
> zero
> > >     enable_count. Note that CCF support for Exynos is already merged
> for
> > >     3.10 and it will be the only available clock support method for
> > >     Exynos.
> > >
> > >     AFAIK, drivers must work correctly in both cases, with
> > >     CONFIG_PM_RUNTIME
> > >     enabled and disabled.
> > >
> > > Then is the driver worked correctly if the power domain to this device
> was
> > > disabled at bootloader without CONFIG_PM_RUNTIME and with
> clk_enable()?  I
> > > think, in this case, the device wouldn't be worked correctly because
> the
> > > power of the device remains off. So you must enable the power domain
> > > somewhere. What is the difference between these two cases?
> >
> > How about making the driver dependant on PM_RUNTIME and making it always
> > use pm_runtime_* API, regardless if the platform actually implements
> runtime
> > PM or not ? Is there any issue in using the Runtime PM core always,
> rather
> > than coding any workarounds in drivers when PM_RUNTIME is disabled ?
>
> I don't think this is a good idea. This would mean that any user that from
> some reasons don't want to use PM_RUNTIME, would not be able to use the
> driver
> anymore.
>
>
Again. There is any case that the driver isn't worked correctly without
CONFIG_PM_RUNTIME and with clk_enable(). Could you guarantee the driver to
be worked correctly only adding clk_enable() to probe() without
CONFIG_PM_RUNTIME? as I said before, what if the power domain to the device
was disabled?


> Rafael, Kevin, do you have any opinion on this?
>
> Best regards,
> --
> Tomasz Figa
> Samsung Poland R&D Center
> SW Solution Development, Kernel and System Framework
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

[Inki Dae]

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

2013/4/22 Rafael J. Wysocki <rjw@sisk.pl>

> On Monday, April 22, 2013 12:37:36 PM Tomasz Figa wrote:
> > On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> > > On 04/22/2013 12:03 PM, Inki Dae wrote:
> > > >     > Also looks good to me. But what if power domain was disabled
> without
> > > >     > pm
> > > >     > runtime? In this case, you must enable the power domain at
> machine
> > > >     > code or
> > > >     > bootloader somewhere. This way would not only need some hard
> codes
> > > >     > to turn
> > > >     > the power domain on but also not manage power management
> fully. This
> > > >     > is same as only the use of pm runtime interface(needing some
> hard
> > > >     > codes without pm runtime) so I don't prefer to add
> > > >     > clk_enable/disable to fimd probe(). I quite tend to force only
> the
> > > >     > use of pm runtime as possible. So please add the hard codes to
> > > >     > machine code or bootloader like you did for power domain if you
> > > >     > want to use drm fimd without pm runtime.
> > > >
> > > >     That's not how the runtime PM, clock subsystems work:
> > > >
> > > >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware
> must be
> > > >     kept
> > > >     powered on all the time.
> > > >
> > > >     2) Common Clock Framework will always gate all clocks that have
> zero
> > > >     enable_count. Note that CCF support for Exynos is already merged
> for
> > > >     3.10 and it will be the only available clock support method for
> > > >     Exynos.
> > > >
> > > >     AFAIK, drivers must work correctly in both cases, with
> > > >     CONFIG_PM_RUNTIME
> > > >     enabled and disabled.
> > > >
> > > > Then is the driver worked correctly if the power domain to this
> device was
> > > > disabled at bootloader without CONFIG_PM_RUNTIME and with
> clk_enable()?  I
> > > > think, in this case, the device wouldn't be worked correctly because
> the
> > > > power of the device remains off. So you must enable the power domain
> > > > somewhere. What is the difference between these two cases?
> > >
> > > How about making the driver dependant on PM_RUNTIME and making it
> always
> > > use pm_runtime_* API, regardless if the platform actually implements
> runtime
> > > PM or not ? Is there any issue in using the Runtime PM core always,
> rather
> > > than coding any workarounds in drivers when PM_RUNTIME is disabled ?
> >
> > I don't think this is a good idea. This would mean that any user that
> from
> > some reasons don't want to use PM_RUNTIME, would not be able to use the
> driver
> > anymore.
> >
> > Rafael, Kevin, do you have any opinion on this?
>
> I agree.
>
> Drivers should work for CONFIG_PM_RUNTIME unset too and static inline
> stubs for
> all runtime PM helpers are available in that case.
>
>
Hi Rafael,

The embedded system, at least Exynos SoC case, has the power domain device
and this device could be enabled only by pm runtime interface. So the
device couldn't be worked correctly without turning the power domain on
only calling clk_enable(). In this case, the power domain must be enabled
at machine code or bootloader. And the machine without CONFIG_PM_RUNTIME
would assume that their own drivers always are enabled so the devices would
be worked correctly. Is there any my missing point?

Thanks,
Inki Dae

Thanks,
> Rafael
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

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

[Tomasz Figa]

On Monday 22 of April 2013 12:05:49 Sylwester Nawrocki wrote:
> On 04/22/2013 11:56 AM, Tomasz Figa wrote:
> > On Monday 22 of April 2013 10:44:00 Viresh Kumar wrote:
> >> On 21 April 2013 20:13, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >>> 3) after those two changes, all that remains is to fix compliance with
> >>> Common Clock Framework, in other words:
> >>> 
> >>> s/clk_enable/clk_prepare_enable/
> >>> 
> >>> and
> >>> 
> >>> s/clk_disable/clk_disable_unprepare/
> >> 
> >> We don't have to call  clk_{un}prepare() everytime for your platform as
> >> you aren't doing anything in it. So just call them once at probe/remove
> >> and
> >> call clk_enable/disable everywhere else.
> 
> Yes, I agree with that. Additionally clk_(un)prepare must not be called in
> atomic context, so some drivers will have to work like this anyway.
> Or the clocks could be prepared/unprepared in the device open/close file op
> for instance.

Well, I don't think drivers should make any assumptions how particular clk ops 
are implemented on particular platform.

Instead, generic semantics of Common Clock Framework should be obeyed, which 
AFAIK are:
1) Each clock must be prepared before enabling.
2) clk_prepare() can not be called from atomic contexts.
3) clk_prepare_enable() can be used instead of clk_prepare() + clk_enable() 
when the driver does not need to enable the clock from atomic context.

Since the Exynos DRM FIMD driver does not need to do call any clock operations 
in atomic contexts, the approach keeping the clock handling as simple as 
possible would be to just replace all clk_{enable,disable} with 
clk_{prepare_enable,disable_unprepare}, as I suggested.

CCing Mike, the maintainer of Common Clock Framework, since he's the right 
person to pass any judgements when it is about clocks.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

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

[myungjoo.ham]

2013/4/22 Inki Dae
> 2013/4/22 Rafael J. Wysocki <rjw@sisk.pl>
> > On Monday, April 22, 2013 12:37:36 PM Tomasz Figa wrote:
> > > On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> > > > On 04/22/2013 12:03 PM, Inki Dae wrote:
> > > > >     > Also looks good to me. But what if power domain was disabled
without
> > > > >     > pm
> > > > >     > runtime? In this case, you must enable the power domain at
machine
> > > > >     > code or
> > > > >     > bootloader somewhere. This way would not only need some hard
codes
> > > > >     > to turn
> > > > >     > the power domain on but also not manage power management
fully. This
> > > > >     > is same as only the use of pm runtime interface(needing some
hard
> > > > >     > codes without pm runtime) so I don't prefer to add
> > > > >     > clk_enable/disable to fimd probe(). I quite tend to force
only the
> > > > >     > use of pm runtime as possible. So please add the hard codes
to
> > > > >     > machine code or bootloader like you did for power domain if
you
> > > > >     > want to use drm fimd without pm runtime.
> > > > >
> > > > >     That's not how the runtime PM, clock subsystems work:
> > > > >
> > > > >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware
must be
> > > > >     kept
> > > > >     powered on all the time.
> > > > >
> > > > >     2) Common Clock Framework will always gate all clocks that
have zero
> > > > >     enable_count. Note that CCF support for Exynos is already
merged for
> > > > >     3.10 and it will be the only available clock support method
for
> > > > >     Exynos.
> > > > >
> > > > >     AFAIK, drivers must work correctly in both cases, with
> > > > >     CONFIG_PM_RUNTIME
> > > > >     enabled and disabled.
> > > > >
> > > > > Then is the driver worked correctly if the power domain to this
device was
> > > > > disabled at bootloader without CONFIG_PM_RUNTIME and with
clk_enable()?  I
> > > > > think, in this case, the device wouldn't be worked correctly
because the
> > > > > power of the device remains off. So you must enable the power
domain
> > > > > somewhere. What is the difference between these two cases?
> > > >
> > > > How about making the driver dependant on PM_RUNTIME and making it
always
> > > > use pm_runtime_* API, regardless if the platform actually implements
runtime
> > > > PM or not ? Is there any issue in using the Runtime PM core always,
rather
> > > > than coding any workarounds in drivers when PM_RUNTIME is disabled ?
> > >
> > > I don't think this is a good idea. This would mean that any user that
from
> > > some reasons don't want to use PM_RUNTIME, would not be able to use
the driver
> > > anymore.
> > >
> > > Rafael, Kevin, do you have any opinion on this?
> > I agree.
> > 
> > Drivers should work for CONFIG_PM_RUNTIME unset too and static inline
stubs for
> > all runtime PM helpers are available in that case.
> > 
> Hi Rafael,
> The embedded system, at least Exynos SoC case, has the power domain device
and this device could be enabled only by pm runtime interface. So the device
couldn't be worked correctly without turning the power domain on only
calling clk_enable(). In this case, the power domain must be enabled at
machine code or bootloader. And the machine without CONFIG_PM_RUNTIME would
assume that their own drivers always are enabled so the devices would be
worked correctly. Is there any my missing point?


- Power domain: not controlled if !CONFIG_PM_RUNTIME. Thus, we may
assume that every power domain is kept ON from boot time if
!CONFIG_PM_RUNTIME.
If power domain is kept OFF from boot time (machine init code or bootloader)
with !CONFIG_PM_RUNTIME, then it's simple a mistake at BSP writer.

- Yes, the clock is still controlled while !CONFIG_PM_RUNTIME.

My opinion is also to let probe do clk-enables though I don't want it
to have #ifdef or "clk_enable()" in the probe function.
Thus, implementing "power_on()"-like function in the driver and let probe()
and
runtime_pm_get callback call it seems appropriate to me.
(that "fimd_active(ctx, true)" is "power-on" to itself, right?)


Cheers,
MyungJoo

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

[Inki Dae]

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

2013/4/23 myungjoo.ham <myungjoo.ham@samsung.com>

> 2013/4/22 Inki Dae
> > 2013/4/22 Rafael J. Wysocki <rjw@sisk.pl>
> > > On Monday, April 22, 2013 12:37:36 PM Tomasz Figa wrote:
> > > > On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
> > > > > On 04/22/2013 12:03 PM, Inki Dae wrote:
> > > > > >     > Also looks good to me. But what if power domain was
> disabled
> without
> > > > > >     > pm
> > > > > >     > runtime? In this case, you must enable the power domain at
> machine
> > > > > >     > code or
> > > > > >     > bootloader somewhere. This way would not only need some
> hard
> codes
> > > > > >     > to turn
> > > > > >     > the power domain on but also not manage power management
> fully. This
> > > > > >     > is same as only the use of pm runtime interface(needing
> some
> hard
> > > > > >     > codes without pm runtime) so I don't prefer to add
> > > > > >     > clk_enable/disable to fimd probe(). I quite tend to force
> only the
> > > > > >     > use of pm runtime as possible. So please add the hard codes
> to
> > > > > >     > machine code or bootloader like you did for power domain if
> you
> > > > > >     > want to use drm fimd without pm runtime.
> > > > > >
> > > > > >     That's not how the runtime PM, clock subsystems work:
> > > > > >
> > > > > >     1) When CONFIG_PM_RUNTIME is disabled, all the used hardware
> must be
> > > > > >     kept
> > > > > >     powered on all the time.
> > > > > >
> > > > > >     2) Common Clock Framework will always gate all clocks that
> have zero
> > > > > >     enable_count. Note that CCF support for Exynos is already
> merged for
> > > > > >     3.10 and it will be the only available clock support method
> for
> > > > > >     Exynos.
> > > > > >
> > > > > >     AFAIK, drivers must work correctly in both cases, with
> > > > > >     CONFIG_PM_RUNTIME
> > > > > >     enabled and disabled.
> > > > > >
> > > > > > Then is the driver worked correctly if the power domain to this
> device was
> > > > > > disabled at bootloader without CONFIG_PM_RUNTIME and with
> clk_enable()?  I
> > > > > > think, in this case, the device wouldn't be worked correctly
> because the
> > > > > > power of the device remains off. So you must enable the power
> domain
> > > > > > somewhere. What is the difference between these two cases?
> > > > >
> > > > > How about making the driver dependant on PM_RUNTIME and making it
> always
> > > > > use pm_runtime_* API, regardless if the platform actually
> implements
> runtime
> > > > > PM or not ? Is there any issue in using the Runtime PM core always,
> rather
> > > > > than coding any workarounds in drivers when PM_RUNTIME is disabled
> ?
> > > >
> > > > I don't think this is a good idea. This would mean that any user that
> from
> > > > some reasons don't want to use PM_RUNTIME, would not be able to use
> the driver
> > > > anymore.
> > > >
> > > > Rafael, Kevin, do you have any opinion on this?
> > > I agree.
> > >
> > > Drivers should work for CONFIG_PM_RUNTIME unset too and static inline
> stubs for
> > > all runtime PM helpers are available in that case.
> > >
> > Hi Rafael,
> > The embedded system, at least Exynos SoC case, has the power domain
> device
> and this device could be enabled only by pm runtime interface. So the
> device
> couldn't be worked correctly without turning the power domain on only
> calling clk_enable(). In this case, the power domain must be enabled at
> machine code or bootloader. And the machine without CONFIG_PM_RUNTIME would
> assume that their own drivers always are enabled so the devices would be
> worked correctly. Is there any my missing point?
>
>
> - Power domain: not controlled if !CONFIG_PM_RUNTIME. Thus, we may
> assume that every power domain is kept ON from boot time if
> !CONFIG_PM_RUNTIME.
> If power domain is kept OFF from boot time (machine init code or
> bootloader)
> with !CONFIG_PM_RUNTIME, then it's simple a mistake at BSP writer.
>
> - Yes, the clock is still controlled while !CONFIG_PM_RUNTIME.
>
> My opinion is also to let probe do clk-enables though I don't want it
> to have #ifdef or "clk_enable()" in the probe function.
> Thus, implementing "power_on()"-like function in the driver and let probe()
> and
> runtime_pm_get callback call it seems appropriate to me.
> (that "fimd_active(ctx, true)" is "power-on" to itself, right?)
>
>
I thought that it should assume the power domain and relevant clocks are
enabled without CONFIG_PM_RUNTIME. So could anyone please tell me about
that? If only the power domain , I think Tomasz's proposal is good solution.

Thanks,
Inki Dae


>
> Cheers,
> MyungJoo
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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