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);
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...
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
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
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
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:
- 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.
- 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:
- 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.
- 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.
- 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
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:
- 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.
- 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:
- 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.
- 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.
- 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
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:
- 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.
- 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:
- 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.
- 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.
- 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:
- When CONFIG_PM_RUNTIME is disabled, all the used hardware must be kept
powered on all the time.
- 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
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
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 disabledwithout pm
> runtime? In this case, you must enable the power domain at machinecode or
> bootloader somewhere. This way would not only need some hard codesto 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 codeswithout pm
> runtime) so I don't prefer to add clk_enable/disable to fimdprobe(). I quite
> tend to force only the use of pm runtime as possible. So pleaseadd the hard
> codes to machine code or bootloader like you did for power domainif 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 bekept
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 for3.10 and
it will be the only available clock support method for Exynos. AFAIK, drivers must work correctly in both cases, withCONFIG_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
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,
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
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 disabledwithout
> pm > runtime? In this case, you must enable the power domain atmachine
> code or > bootloader somewhere. This way would not only need some hardcodes
> to turn > the power domain on but also not manage power managementfully. This
> is same as only the use of pm runtime interface(needing somehard
> codes without pm runtime) so I don't prefer to add > clk_enable/disable to fimd probe(). I quite tend to force onlythe
> 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 hardwaremust be
kept powered on all the time. 2) Common Clock Framework will always gate all clocks that havezero
enable_count. Note that CCF support for Exynos is already mergedfor
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
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
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 wasdisabled without
> pm > runtime? In this case, you must enable the power domain atmachine
> code or > bootloader somewhere. This way would not only need somehard codes
> to turn > the power domain on but also not manage power managementfully. This
> is same as only the use of pm runtime interface(needingsome hard
> codes without pm runtime) so I don't prefer to add > clk_enable/disable to fimd probe(). I quite tend to forceonly the
> use of pm runtime as possible. So please add the hard codesto
> machine code or bootloader like you did for power domain ifyou
> 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 hardwaremust be
kept powered on all the time. 2) Common Clock Framework will always gate all clocks thathave zero
enable_count. Note that CCF support for Exynos is alreadymerged for
3.10 and it will be the only available clock support methodfor
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
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 disabledwithout
> pm > runtime? In this case, you must enable the power domain atmachine
> code or > bootloader somewhere. This way would not only need some hardcodes
> 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 onlythe
> 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 mustbe
kept powered on all the time. 2) Common Clock Framework will always gate all clocks that havezero
enable_count. Note that CCF support for Exynos is already mergedfor
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
On 21 April 2013 20:13, Tomasz Figa tomasz.figa@gmail.com wrote:
- 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.
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:
- 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,
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:
- 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?
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:
- 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,
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.
linaro-kernel@lists.linaro.org
-
Inki Dae -
Inki Dae -
myungjoo.ham -
Rafael J. Wysocki -
Sylwester Nawrocki -
Tomasz Figa -
Tomasz Figa -
Vikas Sajjan -
Viresh Kumar