Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
* DebBarma, Tarun Kanti tarun.ka...@ti.com [110304 16:20]: Now, coming back to our present requirement where we initialize Only the system timer early and is skipped later, here is the plan: (1) Have a separate class in hwmod database with unique name system_timer (2) Initialize just this one during early init Let's keep the system timer code completely separate. For the system timer we really just need to implement one function to reprogram one timer. What you've implemented is pretty much done for the rest of the timers and allows us to turn them into a regular device driver eventually. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
-Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Tuesday, March 08, 2011 5:40 AM To: DebBarma, Tarun Kanti Cc: Hilman, Kevin; linux-omap@vger.kernel.org; Basak, Partha Subject: Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support * DebBarma, Tarun Kanti tarun.ka...@ti.com [110304 16:20]: Now, coming back to our present requirement where we initialize Only the system timer early and is skipped later, here is the plan: (1) Have a separate class in hwmod database with unique name system_timer (2) Initialize just this one during early init Let's keep the system timer code completely separate. For the system timer we really just need to implement one function to reprogram one timer. What you've implemented is pretty much done for the rest of the timers and allows us to turn them into a regular device driver eventually. YES. So, this proposal of mine can be ignored now. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
-Original Message-
From: Hilman, Kevin
Sent: Friday, March 04, 2011 6:53 AM
To: DebBarma, Tarun Kanti
Cc: linux-omap@vger.kernel.org; Basak, Partha
Subject: Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
Add pm_runtime support to dmtimer. Since dmtimer is used during
early boot before pm_runtime is initialized completely there are
provisions to enable/disable clocks directly in the code during
early boot.
I'm still not crazy about the duplicate logic (both early normal) in
all the enable/disable functions.
As I've suggested in the past, why not just do a clk_get, clk_enable in
when the early timers are initialized, then do a clk_disable, clk_put()
as soon as the normal device is ready and PM runtime is enabled.
That will greatly simplify the code and eliminate the unnecessary checks
for -is_early_device which will always be false except for in early
boot (when these functions are not likely to be called anyways.)
I will do the cleanup.
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
[p-bas...@ti.com: added pm_runtime logic in probe()]
Signed-off-by: Partha Basak p-bas...@ti.com
Reviewed-by: Varadarajan, Charulatha ch...@ti.com
Acked-by: Cousson, Benoit b-cous...@ti.com
---
arch/arm/plat-omap/dmtimer.c | 63
--
1 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 53f5205..fcac422 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -39,6 +39,7 @@
#include linux/delay.h
#include linux/io.h
#include linux/slab.h
+#include linux/pm_runtime.h
#include linux/err.h
#include linux/platform_device.h
#include plat/dmtimer.h
@@ -211,13 +212,16 @@ static void omap_dm_timer_prepare(struct
omap_dm_timer *timer)
{
struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
- timer-fclk = clk_get(timer-pdev-dev, fck);
- if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer-fclk))) {
- dev_err(timer-pdev-dev, : No fclk handle.\n);
- return;
+ if (!pdata-is_omap16xx) {
Is this 'is_omap16xx' really needed here? If the clk_get fails, set
timer-fclk to NULL, and any other users of fclk should just check the
validity of timer-fclk.
OK.
+ timer-fclk = clk_get(timer-pdev-dev, fck);
+ if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer-fclk))) {
+ dev_err(timer-pdev-dev, : No fclk handle.\n);
+ return;
+ }
}
- omap_dm_timer_enable(timer);
+ if (!pdata-is_omap16xx)
+ omap_dm_timer_enable(timer);
I don't think this if is needed, as runtime PM calls will essentially be
nops on OMAP1 anyways.
Right, I will remove the check.
if (pdata-dm_timer_reset)
pdata-dm_timer_reset(timer);
@@ -292,10 +296,22 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
+
if (timer-enabled)
return;
- clk_enable(timer-fclk);
+ if (unlikely(pdata-is_early_init)) {
+ clk_enable(timer-fclk);
+ timer-enabled = 1;
+ return;
+ }
+
+ if (pm_runtime_get_sync(timer-pdev-dev) 0) {
+ dev_err(timer-pdev-dev, %s: pm_runtime_get_sync() FAILED\n,
+ __func__);
+ return;
+ }
timer-enabled = 1;
}
Taking my above suggestion, this can be simplified to:
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(timer-pdev-dev);
}
Note that with runtime PM, the -enabled flag is no longer needed
either, as you can simply check pm_runtime_suspended(dev) instead.
Sounds good!
@@ -303,10 +319,22 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
void omap_dm_timer_disable(struct omap_dm_timer *timer)
{
+ struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
+
if (!timer-enabled)
return;
- clk_disable(timer-fclk);
+ if (unlikely(pdata-is_early_init)) {
+ clk_disable(timer-fclk);
+ timer-enabled = 0;
+ return;
+ }
+
+ if (pm_runtime_put_sync(timer-pdev-dev) 0) {
+ dev_err(timer-pdev-dev, %s: pm_runtime_put_sync() FAILED\n,
+ __func__);
+ return;
+ }
timer-enabled = 0;
}
Likewise, this becomes
void omap_dm_timer_disable(struct omap_dm_timer *timer)
{
pm_runtime_put_sync(timer-pdev-dev);
}
Yes.
@@ -425,10 +453,12 @@ int omap_dm_timer_set_source(struct omap_dm_timer
*timer, int source)
if (source 0 || source = 3)
return -EINVAL;
- omap_dm_timer_disable(timer);
+ if (!pdata-is_omap16xx)
drop the if
Yes
Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
* Kevin Hilman khil...@ti.com [110303 17:22]: Tarun Kanti DebBarma tarun.ka...@ti.com writes: Add pm_runtime support to dmtimer. Since dmtimer is used during early boot before pm_runtime is initialized completely there are provisions to enable/disable clocks directly in the code during early boot. I'm still not crazy about the duplicate logic (both early normal) in all the enable/disable functions. As I've suggested in the past, why not just do a clk_get, clk_enable in when the early timers are initialized, then do a clk_disable, clk_put() as soon as the normal device is ready and PM runtime is enabled. Even better would be to have separate handling for the system timer with minimal dependencies to anything. That will greatly simplify the code and eliminate the unnecessary checks for -is_early_device which will always be false except for in early boot (when these functions are not likely to be called anyways.) And please note that only the system timer needs to be initialized early. We might as well treat the system timer separately to avoid these issues. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
-Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Friday, March 04, 2011 10:59 PM To: Hilman, Kevin Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; Basak, Partha Subject: Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support * Kevin Hilman khil...@ti.com [110303 17:22]: Tarun Kanti DebBarma tarun.ka...@ti.com writes: Add pm_runtime support to dmtimer. Since dmtimer is used during early boot before pm_runtime is initialized completely there are provisions to enable/disable clocks directly in the code during early boot. I'm still not crazy about the duplicate logic (both early normal) in all the enable/disable functions. As I've suggested in the past, why not just do a clk_get, clk_enable in when the early timers are initialized, then do a clk_disable, clk_put() as soon as the normal device is ready and PM runtime is enabled. Even better would be to have separate handling for the system timer with minimal dependencies to anything. That will greatly simplify the code and eliminate the unnecessary checks for -is_early_device which will always be false except for in early boot (when these functions are not likely to be called anyways.) And please note that only the system timer needs to be initialized early. We might as well treat the system timer separately to avoid these issues. Yes, this is applicable normally for the system timer only. But as I said earlier, we are giving flexibility whereby any one of the GPTs Can be system timer. -- Tarun -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
* DebBarma, Tarun Kanti tarun.ka...@ti.com [110304 10:55]: -Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Friday, March 04, 2011 10:59 PM To: Hilman, Kevin Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; Basak, Partha Subject: Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support * Kevin Hilman khil...@ti.com [110303 17:22]: Tarun Kanti DebBarma tarun.ka...@ti.com writes: Add pm_runtime support to dmtimer. Since dmtimer is used during early boot before pm_runtime is initialized completely there are provisions to enable/disable clocks directly in the code during early boot. I'm still not crazy about the duplicate logic (both early normal) in all the enable/disable functions. As I've suggested in the past, why not just do a clk_get, clk_enable in when the early timers are initialized, then do a clk_disable, clk_put() as soon as the normal device is ready and PM runtime is enabled. Even better would be to have separate handling for the system timer with minimal dependencies to anything. That will greatly simplify the code and eliminate the unnecessary checks for -is_early_device which will always be false except for in early boot (when these functions are not likely to be called anyways.) And please note that only the system timer needs to be initialized early. We might as well treat the system timer separately to avoid these issues. Yes, this is applicable normally for the system timer only. But as I said earlier, we are giving flexibility whereby any one of the GPTs Can be system timer. Any one of them can be used, but no need to register the others this early. Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
Tony, Kevin, [...] * Kevin Hilman khil...@ti.com [110303 17:22]: Tarun Kanti DebBarma tarun.ka...@ti.com writes: Add pm_runtime support to dmtimer. Since dmtimer is used during early boot before pm_runtime is initialized completely there are provisions to enable/disable clocks directly in the code during early boot. I'm still not crazy about the duplicate logic (both early normal) in all the enable/disable functions. As I've suggested in the past, why not just do a clk_get, clk_enable in when the early timers are initialized, then do a clk_disable, clk_put() as soon as the normal device is ready and PM runtime is enabled. Even better would be to have separate handling for the system timer with minimal dependencies to anything. That will greatly simplify the code and eliminate the unnecessary checks for -is_early_device which will always be false except for in early boot (when these functions are not likely to be called anyways.) And please note that only the system timer needs to be initialized early. We might as well treat the system timer separately to avoid these issues. Yes, this is applicable normally for the system timer only. But as I said earlier, we are giving flexibility whereby any one of the GPTs Can be system timer. Any one of them can be used, but no need to register the others this early. In that case we have to associate probably a dev attribute to system timer. Do you have alternate proposal? Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
* DebBarma, Tarun Kanti tarun.ka...@ti.com [110304 11:50]: In that case we have to associate probably a dev attribute to system timer. Do you have alternate proposal? No need for that. Keep the system timer code to the minimum so it does not have any unnecessary dependencies. That code needs to be running early and efficient, while the other timers should really be a device driver. You can have shared timer read/write functions that don't depend on dev. And you can just tag the pysical timer used for system timer as reserved. Then you can initialize the other timers later on and skip the system timer. For the other timers you can have dev oriented timer read/write wrapper functions for the other timers. Cheers, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
-Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Saturday, March 05, 2011 5:32 AM To: DebBarma, Tarun Kanti Cc: Hilman, Kevin; linux-omap@vger.kernel.org; Basak, Partha Subject: Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support * DebBarma, Tarun Kanti tarun.ka...@ti.com [110304 11:50]: In that case we have to associate probably a dev attribute to system timer. Do you have alternate proposal? No need for that. Keep the system timer code to the minimum so it does not have any unnecessary dependencies. That code needs to be running early and efficient, while the other timers should really be a device driver. You can have shared timer read/write functions that don't depend on dev. And you can just tag the pysical timer used for system timer as reserved. Then you can initialize the other timers later on and skip the system timer. For the other timers you can have dev oriented timer read/write wrapper functions for the other timers. There is a single timer list created during driver probe. It can not be done separately for system timer except through Common build and probe. If we try other way it would get complicated I believe with the current design because clockevent management code in timer-gp.c calls common exported APIs. Now, coming back to our present requirement where we initialize Only the system timer early and is skipped later, here is the plan: (1) Have a separate class in hwmod database with unique name system_timer (2) Initialize just this one during early init Cheers, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 7/8] OMAP: dmtimer: pm_runtime support
Tarun Kanti DebBarma tarun.ka...@ti.com writes:
Add pm_runtime support to dmtimer. Since dmtimer is used during
early boot before pm_runtime is initialized completely there are
provisions to enable/disable clocks directly in the code during
early boot.
I'm still not crazy about the duplicate logic (both early normal) in
all the enable/disable functions.
As I've suggested in the past, why not just do a clk_get, clk_enable in
when the early timers are initialized, then do a clk_disable, clk_put()
as soon as the normal device is ready and PM runtime is enabled.
That will greatly simplify the code and eliminate the unnecessary checks
for -is_early_device which will always be false except for in early
boot (when these functions are not likely to be called anyways.)
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
[p-bas...@ti.com: added pm_runtime logic in probe()]
Signed-off-by: Partha Basak p-bas...@ti.com
Reviewed-by: Varadarajan, Charulatha ch...@ti.com
Acked-by: Cousson, Benoit b-cous...@ti.com
---
arch/arm/plat-omap/dmtimer.c | 63
--
1 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 53f5205..fcac422 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -39,6 +39,7 @@
#include linux/delay.h
#include linux/io.h
#include linux/slab.h
+#include linux/pm_runtime.h
#include linux/err.h
#include linux/platform_device.h
#include plat/dmtimer.h
@@ -211,13 +212,16 @@ static void omap_dm_timer_prepare(struct omap_dm_timer
*timer)
{
struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
- timer-fclk = clk_get(timer-pdev-dev, fck);
- if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer-fclk))) {
- dev_err(timer-pdev-dev, : No fclk handle.\n);
- return;
+ if (!pdata-is_omap16xx) {
Is this 'is_omap16xx' really needed here? If the clk_get fails, set
timer-fclk to NULL, and any other users of fclk should just check the
validity of timer-fclk.
+ timer-fclk = clk_get(timer-pdev-dev, fck);
+ if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer-fclk))) {
+ dev_err(timer-pdev-dev, : No fclk handle.\n);
+ return;
+ }
}
- omap_dm_timer_enable(timer);
+ if (!pdata-is_omap16xx)
+ omap_dm_timer_enable(timer);
I don't think this if is needed, as runtime PM calls will essentially be
nops on OMAP1 anyways.
if (pdata-dm_timer_reset)
pdata-dm_timer_reset(timer);
@@ -292,10 +296,22 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
+
if (timer-enabled)
return;
- clk_enable(timer-fclk);
+ if (unlikely(pdata-is_early_init)) {
+ clk_enable(timer-fclk);
+ timer-enabled = 1;
+ return;
+ }
+
+ if (pm_runtime_get_sync(timer-pdev-dev) 0) {
+ dev_err(timer-pdev-dev, %s: pm_runtime_get_sync() FAILED\n,
+ __func__);
+ return;
+ }
timer-enabled = 1;
}
Taking my above suggestion, this can be simplified to:
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(timer-pdev-dev);
}
Note that with runtime PM, the -enabled flag is no longer needed
either, as you can simply check pm_runtime_suspended(dev) instead.
@@ -303,10 +319,22 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
void omap_dm_timer_disable(struct omap_dm_timer *timer)
{
+ struct dmtimer_platform_data *pdata = timer-pdev-dev.platform_data;
+
if (!timer-enabled)
return;
- clk_disable(timer-fclk);
+ if (unlikely(pdata-is_early_init)) {
+ clk_disable(timer-fclk);
+ timer-enabled = 0;
+ return;
+ }
+
+ if (pm_runtime_put_sync(timer-pdev-dev) 0) {
+ dev_err(timer-pdev-dev, %s: pm_runtime_put_sync() FAILED\n,
+ __func__);
+ return;
+ }
timer-enabled = 0;
}
Likewise, this becomes
void omap_dm_timer_disable(struct omap_dm_timer *timer)
{
pm_runtime_put_sync(timer-pdev-dev);
}
@@ -425,10 +453,12 @@ int omap_dm_timer_set_source(struct omap_dm_timer
*timer, int source)
if (source 0 || source = 3)
return -EINVAL;
- omap_dm_timer_disable(timer);
+ if (!pdata-is_omap16xx)
drop the if
+ omap_dm_timer_disable(timer);
/* change the timer clock source */
ret = pdata-set_timer_src(timer-pdev, source);
- omap_dm_timer_enable(timer);
+ if (!pdata-is_omap16xx)
and this one,
and with that, there are no users of is_omap16xx in this patch.
Kevin
+ omap_dm_timer_enable(timer);
