* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-13 23:02 ` [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path Kevin Hilman
@ 2010-09-14 14:14 ` Varadarajan, Charulatha
2010-09-14 14:41 ` Kevin Hilman
2010-09-14 16:09 ` Basak, Partha
2010-09-22 8:22 ` Kalliguddi, Hema
2 siblings, 1 reply; 18+ messages in thread
From: Varadarajan, Charulatha @ 2010-09-14 14:14 UTC (permalink / raw)
To: Kevin Hilman, linux-omap; +Cc: Basak, Partha, Tero Kristo
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, September 14, 2010 4:33 AM
> To: linux-omap@vger.kernel.org
> Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
> Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> From: Kevin Hilman <khilman@ti.com>
>
> Currently, we wait until late in the idle path where interrupts are
> disabled to do runtime-PM-like management for certain special-case
> devices like GPIO.
>
> As a prerequiste to moving GPIO to the new runtime PM framework, move
> this runtime-PM-like code out of the late idle path into new device
> idle and resume functions that can be called before interrupts are
> disabled by CPUidle and/or suspend.
>
> In addition, move all the GPIO-specific logic into the GPIO core
> instead of keeping GPIO-specific knowledge of power-states, context
> saving etc. in the PM core.
>
> Also, call the new device-idle and -resume methods from CPUidle and
> static suspend path.
>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> arch/arm/mach-omap2/pm.h | 2 +
> arch/arm/mach-omap2/pm24xx.c | 2 +-
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
> arch/arm/plat-omap/gpio.c | 57
> ++++++++++++++++++++++++--------
> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> 6 files changed, 67 insertions(+), 40 deletions(-)
<<snip>>
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 7951eef..b0467c1 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -29,6 +29,8 @@
> #include <asm/mach/irq.h>
> #include <plat/powerdomain.h>
>
> +static struct powerdomain *per_pwrdm;
> +
> /*
> * OMAP1510 GPIO registers
> */
> @@ -207,6 +209,9 @@ struct gpio_bank {
> u32 dbck_enable_mask;
> };
>
> +static void omap3_gpio_restore_context(void);
> +static void omap3_gpio_save_context(void);
> +
> #define METHOD_MPUIO 0
> #define METHOD_GPIO_1510 1
> #define METHOD_GPIO_1610 2
> @@ -1778,6 +1783,8 @@ static int __init _omap_gpio_init(void)
> }
> #endif
>
> + if (cpu_class_is_omap2())
> + per_pwrdm = pwrdm_lookup("per_pwrdm");
"per_pwrdm" is not available for OMAP24xx.
>
> #ifdef CONFIG_ARCH_OMAP15XX
> if (cpu_is_omap15xx()) {
> @@ -2074,14 +2081,22 @@ static struct sys_device omap_gpio_device = {
>
> static int workaround_enabled;
>
> -void omap2_gpio_prepare_for_idle(int power_state)
> +void omap2_gpio_prepare_for_idle(void)
> {
> - int i, c = 0;
> - int min = 0;
> + int i, c = 0, min = 0;
> + int per_next_state;
> +
> + if (!per_pwrdm)
> + return;
"per_pwrdm" is not available for OMAP24xx. Hence
this breaks the omap2_gpio_prepare_for_idle() path for OMAP2.
In OMAP2420, all gpio banks are in wakeup domain and
in OMAP2430 GPIO banks 1-4 are in wakeup domain & GPIO5 is
in core domain.
-V Charulatha
> +
> + per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> + if (per_next_state >= PWRDM_POWER_INACTIVE)
> + return;
>
> if (cpu_is_omap34xx())
> min = 1;
<<snip>>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-14 14:14 ` Varadarajan, Charulatha
@ 2010-09-14 14:41 ` Kevin Hilman
0 siblings, 0 replies; 18+ messages in thread
From: Kevin Hilman @ 2010-09-14 14:41 UTC (permalink / raw)
To: Varadarajan, Charulatha; +Cc: linux-omap, Basak, Partha, Tero Kristo
"Varadarajan, Charulatha" <charu@ti.com> writes:
[...]
>> -void omap2_gpio_prepare_for_idle(int power_state)
>> +void omap2_gpio_prepare_for_idle(void)
>> {
>> - int i, c = 0;
>> - int min = 0;
>> + int i, c = 0, min = 0;
>> + int per_next_state;
>> +
>> + if (!per_pwrdm)
>> + return;
>
> "per_pwrdm" is not available for OMAP24xx. Hence
> this breaks the omap2_gpio_prepare_for_idle() path for OMAP2.
>
> In OMAP2420, all gpio banks are in wakeup domain and
> in OMAP2430 GPIO banks 1-4 are in wakeup domain & GPIO5 is
> in core domain.
Sure, this patch is just a proof of concept that we can move the GPIO
idle management out of the interrupts disabled path.
When you adapt this approach to your conversion series, since you have
already broken these functions up to be per-bank, you can have a
per-bank powerdomain that can be configured per-SoC.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-13 23:02 ` [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path Kevin Hilman
2010-09-14 14:14 ` Varadarajan, Charulatha
@ 2010-09-14 16:09 ` Basak, Partha
2010-09-14 16:57 ` Kevin Hilman
2010-09-22 8:22 ` Kalliguddi, Hema
2 siblings, 1 reply; 18+ messages in thread
From: Basak, Partha @ 2010-09-14 16:09 UTC (permalink / raw)
To: Kevin Hilman, linux-omap; +Cc: Varadarajan, Charulatha, Tero Kristo
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, September 14, 2010 4:33 AM
> To: linux-omap@vger.kernel.org
> Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
> Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> From: Kevin Hilman <khilman@ti.com>
>
> Currently, we wait until late in the idle path where interrupts are
> disabled to do runtime-PM-like management for certain special-case
> devices like GPIO.
>
> As a prerequiste to moving GPIO to the new runtime PM framework, move
> this runtime-PM-like code out of the late idle path into new device
> idle and resume functions that can be called before interrupts are
> disabled by CPUidle and/or suspend.
>
> In addition, move all the GPIO-specific logic into the GPIO core
> instead of keeping GPIO-specific knowledge of power-states, context
> saving etc. in the PM core.
>
> Also, call the new device-idle and -resume methods from CPUidle and
> static suspend path.
>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> arch/arm/mach-omap2/pm.h | 2 +
> arch/arm/mach-omap2/pm24xx.c | 2 +-
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
> arch/arm/plat-omap/gpio.c | 57
> ++++++++++++++++++++++++--------
> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> 6 files changed, 67 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 0923b82..681d823 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
> pwrdm_set_next_pwrst(per_pd, per_next_state);
>
> select_state:
> + omap3_device_idle();
> +
> dev->last_state = new_state;
> ret = omap3_enter_idle(dev, new_state);
>
> + omap3_device_resume();
> +
In the generic cpu_idle() in process.c, interrupts are already disabled
before control comes to cpuidle_idle_call() via pm_idle()
local_irq_disable();
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
* This will eventually be removed - pm_idle
* functions should always return with IRQs
* enabled.
*/
WARN_ON(irqs_disabled());
local_irq_enable();
}
omap3_enter_idle_bm() will be called from inside cpuidle_idle_call()
via target_state->enter(dev, target_state).
So, interrupts are already disabled here.
Am I missing something?
> /* Restore original PER state if it was modified */
> if (per_next_state != per_saved_state)
> pwrdm_set_next_pwrst(per_pd, per_saved_state);
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 3de6ece..33cae4b 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -22,6 +22,8 @@ extern void omap_sram_idle(void);
> extern int omap3_can_sleep(void);
> extern int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
> extern int omap3_idle_init(void);
> +extern void omap3_device_idle(void);
> +extern void omap3_device_resume(void);
>
> struct cpuidle_params {
> u8 valid;
> diff --git a/arch/arm/mach-omap2/pm24xx.c
> b/arch/arm/mach-omap2/pm24xx.c
> index 6aeedea..7bbf0db 100644
> --- a/arch/arm/mach-omap2/pm24xx.c
> +++ b/arch/arm/mach-omap2/pm24xx.c
> @@ -106,7 +106,7 @@ static void omap2_enter_full_retention(void)
> l = omap_ctrl_readl(OMAP2_CONTROL_DEVCONF0) |
> OMAP24XX_USBSTANDBYCTRL;
> omap_ctrl_writel(l, OMAP2_CONTROL_DEVCONF0);
>
> - omap2_gpio_prepare_for_idle(PWRDM_POWER_RET);
> + omap2_gpio_prepare_for_idle();
>
> if (omap2_pm_debug) {
> omap2_pm_dump(0, 0, 0);
> diff --git a/arch/arm/mach-omap2/pm34xx.c
> b/arch/arm/mach-omap2/pm34xx.c
> index bb2ba1e..9e590d9 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -74,16 +74,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
> static struct powerdomain *core_pwrdm, *per_pwrdm;
> static struct powerdomain *cam_pwrdm;
>
> -static inline void omap3_per_save_context(void)
> -{
> - omap_gpio_save_context();
> -}
> -
> -static inline void omap3_per_restore_context(void)
> -{
> - omap_gpio_restore_context();
> -}
> -
> static void omap3_enable_io_chain(void)
> {
> int timeout = 0;
> @@ -332,6 +322,16 @@ static void restore_table_entry(void)
> restore_control_register(control_reg_value);
> }
>
> +void omap3_device_idle(void)
> +{
> + omap2_gpio_prepare_for_idle();
> +}
> +
> +void omap3_device_resume(void)
> +{
> + omap2_gpio_resume_after_idle();
> +}
> +
> void omap_sram_idle(void)
> {
> /* Variable to tell what needs to be saved and restored
> @@ -344,7 +344,7 @@ void omap_sram_idle(void)
> int mpu_next_state = PWRDM_POWER_ON;
> int per_next_state = PWRDM_POWER_ON;
> int core_next_state = PWRDM_POWER_ON;
> - int core_prev_state, per_prev_state;
> + int core_prev_state;
> u32 sdrc_pwr = 0;
>
> if (!_omap_sram_idle)
> @@ -387,12 +387,9 @@ void omap_sram_idle(void)
> }
>
> /* PER */
> - if (per_next_state < PWRDM_POWER_ON) {
> + if (per_next_state < PWRDM_POWER_ON)
> omap_uart_prepare_idle(2);
> - omap2_gpio_prepare_for_idle(per_next_state);
> - if (per_next_state == PWRDM_POWER_OFF)
> - omap3_per_save_context();
> - }
> +
>
> /* CORE */
> if (core_next_state < PWRDM_POWER_ON) {
> @@ -454,13 +451,8 @@ void omap_sram_idle(void)
> omap3_intc_resume_idle();
>
> /* PER */
> - if (per_next_state < PWRDM_POWER_ON) {
> - per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
> - omap2_gpio_resume_after_idle();
> - if (per_prev_state == PWRDM_POWER_OFF)
> - omap3_per_restore_context();
> + if (per_next_state < PWRDM_POWER_ON)
> omap_uart_resume_idle(2);
> - }
>
> /* Disable IO-PAD and IO-CHAIN wakeup */
> if (omap3_has_io_wakeup() &&
> @@ -570,6 +562,7 @@ static void omap2_pm_wakeup_on_timer(u32
> seconds, u32 milliseconds)
> static int omap3_pm_prepare(void)
> {
> disable_hlt();
> + omap3_device_idle();
> return 0;
> }
>
> @@ -637,6 +630,7 @@ static int omap3_pm_enter(suspend_state_t unused)
>
> static void omap3_pm_finish(void)
> {
> + omap3_device_resume();
> enable_hlt();
> }
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 7951eef..b0467c1 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -29,6 +29,8 @@
> #include <asm/mach/irq.h>
> #include <plat/powerdomain.h>
>
> +static struct powerdomain *per_pwrdm;
> +
> /*
> * OMAP1510 GPIO registers
> */
> @@ -207,6 +209,9 @@ struct gpio_bank {
> u32 dbck_enable_mask;
> };
>
> +static void omap3_gpio_restore_context(void);
> +static void omap3_gpio_save_context(void);
> +
> #define METHOD_MPUIO 0
> #define METHOD_GPIO_1510 1
> #define METHOD_GPIO_1610 2
> @@ -1778,6 +1783,8 @@ static int __init _omap_gpio_init(void)
> }
> #endif
>
> + if (cpu_class_is_omap2())
> + per_pwrdm = pwrdm_lookup("per_pwrdm");
>
> #ifdef CONFIG_ARCH_OMAP15XX
> if (cpu_is_omap15xx()) {
> @@ -2074,14 +2081,22 @@ static struct sys_device omap_gpio_device = {
>
> static int workaround_enabled;
>
> -void omap2_gpio_prepare_for_idle(int power_state)
> +void omap2_gpio_prepare_for_idle(void)
> {
> - int i, c = 0;
> - int min = 0;
> + int i, c = 0, min = 0;
> + int per_next_state;
> +
> + if (!per_pwrdm)
> + return;
> +
> + per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> + if (per_next_state >= PWRDM_POWER_INACTIVE)
> + return;
>
> if (cpu_is_omap34xx())
> min = 1;
>
> + workaround_enabled = 0;
> for (i = min; i < gpio_bank_count; i++) {
> struct gpio_bank *bank = &gpio_bank[i];
> u32 l1, l2;
> @@ -2089,7 +2104,7 @@ void omap2_gpio_prepare_for_idle(int
> power_state)
> if (bank->dbck_enable_mask)
> clk_disable(bank->dbck);
>
> - if (power_state > PWRDM_POWER_OFF)
> + if (per_next_state > PWRDM_POWER_OFF)
> continue;
>
> /* If going to OFF, remove triggering for all
> @@ -2135,20 +2150,35 @@ void omap2_gpio_prepare_for_idle(int
> power_state)
>
> c++;
> }
> - if (!c) {
> - workaround_enabled = 0;
> - return;
> - }
> - workaround_enabled = 1;
> + if (c)
> + workaround_enabled = 1;
> +
> + if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF))
> + omap3_gpio_save_context();
> }
>
> void omap2_gpio_resume_after_idle(void)
> {
> - int i;
> - int min = 0;
> + int i, min = 0;
> + int per_next_state;
> +
> + if (!per_pwrdm)
> + return;
> +
> + per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> + if (per_next_state >= PWRDM_POWER_INACTIVE)
> + return;
> +
> + if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF)) {
> + int per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
> +
> + if (per_prev_state == PWRDM_POWER_OFF)
> + omap3_gpio_restore_context();
> + }
>
> if (cpu_is_omap34xx())
> min = 1;
> +
> for (i = min; i < gpio_bank_count; i++) {
> struct gpio_bank *bank = &gpio_bank[i];
> u32 l, gen, gen0, gen1;
> @@ -2235,14 +2265,13 @@ void omap2_gpio_resume_after_idle(void)
> }
> }
> }
> -
> }
>
> #endif
>
> #ifdef CONFIG_ARCH_OMAP3
> /* save the registers of bank 2-6 */
> -void omap_gpio_save_context(void)
> +static void omap3_gpio_save_context(void)
> {
> int i;
>
> @@ -2275,7 +2304,7 @@ void omap_gpio_save_context(void)
> }
>
> /* restore the required registers of bank 2-6 */
> -void omap_gpio_restore_context(void)
> +static void omap3_gpio_restore_context(void)
> {
> int i;
>
> diff --git a/arch/arm/plat-omap/include/plat/gpio.h
> b/arch/arm/plat-omap/include/plat/gpio.h
> index de1c604..c81ef94 100644
> --- a/arch/arm/plat-omap/include/plat/gpio.h
> +++ b/arch/arm/plat-omap/include/plat/gpio.h
> @@ -72,12 +72,10 @@
> IH_GPIO_BASE + (nr))
>
> extern int omap_gpio_init(void); /* Call from board init only */
> -extern void omap2_gpio_prepare_for_idle(int power_state);
> +extern void omap2_gpio_prepare_for_idle(void);
> extern void omap2_gpio_resume_after_idle(void);
> extern void omap_set_gpio_debounce(int gpio, int enable);
> extern void omap_set_gpio_debounce_time(int gpio, int enable);
> -extern void omap_gpio_save_context(void);
> -extern void omap_gpio_restore_context(void);
>
> /*------------------------------------------------------------
> -------------*/
>
> /* Wrappers for "new style" GPIO calls, using the new infrastructure
> --
> 1.7.2.1
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-14 16:09 ` Basak, Partha
@ 2010-09-14 16:57 ` Kevin Hilman
2010-09-15 8:02 ` Basak, Partha
2010-09-23 12:54 ` Basak, Partha
0 siblings, 2 replies; 18+ messages in thread
From: Kevin Hilman @ 2010-09-14 16:57 UTC (permalink / raw)
To: Basak, Partha; +Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo
"Basak, Partha" <p-basak2@ti.com> writes:
>> From: Kevin Hilman <khilman@ti.com>
>>
>> Currently, we wait until late in the idle path where interrupts are
>> disabled to do runtime-PM-like management for certain special-case
>> devices like GPIO.
>>
>> As a prerequiste to moving GPIO to the new runtime PM framework, move
>> this runtime-PM-like code out of the late idle path into new device
>> idle and resume functions that can be called before interrupts are
>> disabled by CPUidle and/or suspend.
>>
>> In addition, move all the GPIO-specific logic into the GPIO core
>> instead of keeping GPIO-specific knowledge of power-states, context
>> saving etc. in the PM core.
>>
>> Also, call the new device-idle and -resume methods from CPUidle and
>> static suspend path.
>>
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> ---
>> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
>> arch/arm/mach-omap2/pm.h | 2 +
>> arch/arm/mach-omap2/pm24xx.c | 2 +-
>> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
>> arch/arm/plat-omap/gpio.c | 57
>> ++++++++++++++++++++++++--------
>> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
>> 6 files changed, 67 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> b/arch/arm/mach-omap2/cpuidle34xx.c
>> index 0923b82..681d823 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>> cpuidle_device *dev,
>> pwrdm_set_next_pwrst(per_pd, per_next_state);
>>
>> select_state:
>> + omap3_device_idle();
>> +
>> dev->last_state = new_state;
>> ret = omap3_enter_idle(dev, new_state);
>>
>> + omap3_device_resume();
>> +
> In the generic cpu_idle() in process.c, interrupts are already disabled
> before control comes to cpuidle_idle_call() via pm_idle()
> local_irq_disable();
> if (hlt_counter) {
> local_irq_enable();
> cpu_relax();
> } else {
> stop_critical_timings();
> pm_idle();
> start_critical_timings();
> /*
> * This will eventually be removed - pm_idle
> * functions should always return with IRQs
> * enabled.
> */
> WARN_ON(irqs_disabled());
> local_irq_enable();
> }
>
> omap3_enter_idle_bm() will be called from inside cpuidle_idle_call()
> via target_state->enter(dev, target_state).
> So, interrupts are already disabled here.
>
> Am I missing something?
You're right.
I knew this was the case for !CPUIDLE setup, but had thought (without
testing) that the CPUidle core had re-enabled interrupts during the
governor selection process etc.
While I investigate ways to manage this in CPUidle, the following should
be fine for now to include with $SUBJECT patch.
Kevin
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 681d823..c5cb9d0 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
goto select_state;
}
+ /*
+ * Enable IRQs during the device activity checking and idle management.
+ * IRQs are later (re)disabled when entering the actual idle function.
+ * Device idle management that is using runtime PM needs to have
+ * interrupts enabled when calling into the runtime PM core.
+ */
+ local_irq_enable();
+
cx = cpuidle_get_statedata(state);
core_next_state = cx->core_state;
k
^ permalink raw reply related [flat|nested] 18+ messages in thread* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-14 16:57 ` Kevin Hilman
@ 2010-09-15 8:02 ` Basak, Partha
2010-09-23 12:54 ` Basak, Partha
1 sibling, 0 replies; 18+ messages in thread
From: Basak, Partha @ 2010-09-15 8:02 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, September 14, 2010 10:28 PM
> To: Basak, Partha
> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> "Basak, Partha" <p-basak2@ti.com> writes:
>
> >> From: Kevin Hilman <khilman@ti.com>
> >>
> >> Currently, we wait until late in the idle path where interrupts are
> >> disabled to do runtime-PM-like management for certain special-case
> >> devices like GPIO.
> >>
> >> As a prerequiste to moving GPIO to the new runtime PM
> framework, move
> >> this runtime-PM-like code out of the late idle path into new device
> >> idle and resume functions that can be called before interrupts are
> >> disabled by CPUidle and/or suspend.
> >>
> >> In addition, move all the GPIO-specific logic into the GPIO core
> >> instead of keeping GPIO-specific knowledge of power-states, context
> >> saving etc. in the PM core.
> >>
> >> Also, call the new device-idle and -resume methods from CPUidle and
> >> static suspend path.
> >>
> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
> >> ---
> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> >> arch/arm/mach-omap2/pm.h | 2 +
> >> arch/arm/mach-omap2/pm24xx.c | 2 +-
> >> arch/arm/mach-omap2/pm34xx.c | 38
> +++++++++------------
> >> arch/arm/plat-omap/gpio.c | 57
> >> ++++++++++++++++++++++++--------
> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> >> 6 files changed, 67 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> index 0923b82..681d823 100644
> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
> >> cpuidle_device *dev,
> >> pwrdm_set_next_pwrst(per_pd, per_next_state);
> >>
> >> select_state:
> >> + omap3_device_idle();
> >> +
> >> dev->last_state = new_state;
> >> ret = omap3_enter_idle(dev, new_state);
> >>
> >> + omap3_device_resume();
> >> +
> > In the generic cpu_idle() in process.c, interrupts are
> already disabled
> > before control comes to cpuidle_idle_call() via pm_idle()
> > local_irq_disable();
> > if (hlt_counter) {
> > local_irq_enable();
> > cpu_relax();
> > } else {
> > stop_critical_timings();
> > pm_idle();
> > start_critical_timings();
> > /*
> > * This will eventually be
> removed - pm_idle
> > * functions should always
> return with IRQs
> > * enabled.
> > */
> > WARN_ON(irqs_disabled());
> > local_irq_enable();
> > }
> >
> > omap3_enter_idle_bm() will be called from inside
> cpuidle_idle_call()
> > via target_state->enter(dev, target_state).
> > So, interrupts are already disabled here.
> >
> > Am I missing something?
>
> You're right.
>
> I knew this was the case for !CPUIDLE setup, but had thought (without
> testing) that the CPUidle core had re-enabled interrupts during the
> governor selection process etc.
>
> While I investigate ways to manage this in CPUidle, the
> following should
> be fine for now to include with $SUBJECT patch.
>
> Kevin
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 681d823..c5cb9d0 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
> goto select_state;
> }
>
> + /*
> + * Enable IRQs during the device activity checking and
> idle management.
> + * IRQs are later (re)disabled when entering the actual
> idle function.
> + * Device idle management that is using runtime PM needs to have
> + * interrupts enabled when calling into the runtime PM core.
> + */
> + local_irq_enable();
> +
> cx = cpuidle_get_statedata(state);
> core_next_state = cx->core_state;
>
OK. We will base on top of this patch.
> k
>
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-14 16:57 ` Kevin Hilman
2010-09-15 8:02 ` Basak, Partha
@ 2010-09-23 12:54 ` Basak, Partha
2010-09-23 15:38 ` Kevin Hilman
1 sibling, 1 reply; 18+ messages in thread
From: Basak, Partha @ 2010-09-23 12:54 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, September 14, 2010 10:28 PM
> To: Basak, Partha
> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> "Basak, Partha" <p-basak2@ti.com> writes:
>
> >> From: Kevin Hilman <khilman@ti.com>
> >>
> >> Currently, we wait until late in the idle path where interrupts are
> >> disabled to do runtime-PM-like management for certain special-case
> >> devices like GPIO.
> >>
> >> As a prerequiste to moving GPIO to the new runtime PM
> framework, move
> >> this runtime-PM-like code out of the late idle path into new device
> >> idle and resume functions that can be called before interrupts are
> >> disabled by CPUidle and/or suspend.
> >>
> >> In addition, move all the GPIO-specific logic into the GPIO core
> >> instead of keeping GPIO-specific knowledge of power-states, context
> >> saving etc. in the PM core.
> >>
> >> Also, call the new device-idle and -resume methods from CPUidle and
> >> static suspend path.
> >>
> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
> >> ---
> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> >> arch/arm/mach-omap2/pm.h | 2 +
> >> arch/arm/mach-omap2/pm24xx.c | 2 +-
> >> arch/arm/mach-omap2/pm34xx.c | 38
> +++++++++------------
> >> arch/arm/plat-omap/gpio.c | 57
> >> ++++++++++++++++++++++++--------
> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> >> 6 files changed, 67 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> index 0923b82..681d823 100644
> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
> >> cpuidle_device *dev,
> >> pwrdm_set_next_pwrst(per_pd, per_next_state);
> >>
> >> select_state:
> >> + omap3_device_idle();
> >> +
> >> dev->last_state = new_state;
> >> ret = omap3_enter_idle(dev, new_state);
> >>
> >> + omap3_device_resume();
> >> +
> > In the generic cpu_idle() in process.c, interrupts are
> already disabled
> > before control comes to cpuidle_idle_call() via pm_idle()
> > local_irq_disable();
> > if (hlt_counter) {
> > local_irq_enable();
> > cpu_relax();
> > } else {
> > stop_critical_timings();
> > pm_idle();
> > start_critical_timings();
> > /*
> > * This will eventually be
> removed - pm_idle
> > * functions should always
> return with IRQs
> > * enabled.
> > */
> > WARN_ON(irqs_disabled());
> > local_irq_enable();
> > }
> >
> > omap3_enter_idle_bm() will be called from inside
> cpuidle_idle_call()
> > via target_state->enter(dev, target_state).
> > So, interrupts are already disabled here.
> >
> > Am I missing something?
>
> You're right.
>
> I knew this was the case for !CPUIDLE setup, but had thought (without
> testing) that the CPUidle core had re-enabled interrupts during the
> governor selection process etc.
>
> While I investigate ways to manage this in CPUidle, the
> following should
> be fine for now to include with $SUBJECT patch.
>
> Kevin
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 681d823..c5cb9d0 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
> cpuidle_device *dev,
> goto select_state;
> }
>
> + /*
> + * Enable IRQs during the device activity checking and
> idle management.
> + * IRQs are later (re)disabled when entering the actual
> idle function.
> + * Device idle management that is using runtime PM needs to have
> + * interrupts enabled when calling into the runtime PM core.
> + */
> + local_irq_enable();
After put_sync() retuns, there will be a time window where interrupts
are enabled but clocks are disabled before the interrupts are disabled again.
Accessing any register to service a device interrupt coming during this window
will lead to a crash for cases where iclk and fclks are same and we have the
iclk defined as the main_clk as well.
Same argument holds while returning from Idle. We are facing this issue for OMAP3
GPIO while trying to define the main_clk = interface clock based on your other commment.
> +
> cx = cpuidle_get_statedata(state);
> core_next_state = cx->core_state;
>
> k
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-23 12:54 ` Basak, Partha
@ 2010-09-23 15:38 ` Kevin Hilman
2010-09-23 19:57 ` Basak, Partha
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2010-09-23 15:38 UTC (permalink / raw)
To: Basak, Partha; +Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo
"Basak, Partha" <p-basak2@ti.com> writes:
>
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Tuesday, September 14, 2010 10:28 PM
>> To: Basak, Partha
>> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
>> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>> interrupts-disabled idle path
>>
>> "Basak, Partha" <p-basak2@ti.com> writes:
>>
>> >> From: Kevin Hilman <khilman@ti.com>
>> >>
>> >> Currently, we wait until late in the idle path where interrupts are
>> >> disabled to do runtime-PM-like management for certain special-case
>> >> devices like GPIO.
>> >>
>> >> As a prerequiste to moving GPIO to the new runtime PM
>> framework, move
>> >> this runtime-PM-like code out of the late idle path into new device
>> >> idle and resume functions that can be called before interrupts are
>> >> disabled by CPUidle and/or suspend.
>> >>
>> >> In addition, move all the GPIO-specific logic into the GPIO core
>> >> instead of keeping GPIO-specific knowledge of power-states, context
>> >> saving etc. in the PM core.
>> >>
>> >> Also, call the new device-idle and -resume methods from CPUidle and
>> >> static suspend path.
>> >>
>> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> >> ---
>> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
>> >> arch/arm/mach-omap2/pm.h | 2 +
>> >> arch/arm/mach-omap2/pm24xx.c | 2 +-
>> >> arch/arm/mach-omap2/pm34xx.c | 38
>> +++++++++------------
>> >> arch/arm/plat-omap/gpio.c | 57
>> >> ++++++++++++++++++++++++--------
>> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
>> >> 6 files changed, 67 insertions(+), 40 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> index 0923b82..681d823 100644
>> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>> >> cpuidle_device *dev,
>> >> pwrdm_set_next_pwrst(per_pd, per_next_state);
>> >>
>> >> select_state:
>> >> + omap3_device_idle();
>> >> +
>> >> dev->last_state = new_state;
>> >> ret = omap3_enter_idle(dev, new_state);
>> >>
>> >> + omap3_device_resume();
>> >> +
>> > In the generic cpu_idle() in process.c, interrupts are
>> already disabled
>> > before control comes to cpuidle_idle_call() via pm_idle()
>> > local_irq_disable();
>> > if (hlt_counter) {
>> > local_irq_enable();
>> > cpu_relax();
>> > } else {
>> > stop_critical_timings();
>> > pm_idle();
>> > start_critical_timings();
>> > /*
>> > * This will eventually be
>> removed - pm_idle
>> > * functions should always
>> return with IRQs
>> > * enabled.
>> > */
>> > WARN_ON(irqs_disabled());
>> > local_irq_enable();
>> > }
>> >
>> > omap3_enter_idle_bm() will be called from inside
>> cpuidle_idle_call()
>> > via target_state->enter(dev, target_state).
>> > So, interrupts are already disabled here.
>> >
>> > Am I missing something?
>>
>> You're right.
>>
>> I knew this was the case for !CPUIDLE setup, but had thought (without
>> testing) that the CPUidle core had re-enabled interrupts during the
>> governor selection process etc.
>>
>> While I investigate ways to manage this in CPUidle, the
>> following should
>> be fine for now to include with $SUBJECT patch.
>>
>> Kevin
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> b/arch/arm/mach-omap2/cpuidle34xx.c
>> index 681d823..c5cb9d0 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
>> cpuidle_device *dev,
>> goto select_state;
>> }
>>
>> + /*
>> + * Enable IRQs during the device activity checking and
>> idle management.
>> + * IRQs are later (re)disabled when entering the actual
>> idle function.
>> + * Device idle management that is using runtime PM needs to have
>> + * interrupts enabled when calling into the runtime PM core.
>> + */
>> + local_irq_enable();
>
> After put_sync() retuns, there will be a time window where interrupts
> are enabled but clocks are disabled before the interrupts are disabled again.
> Accessing any register to service a device interrupt coming during this window
> will lead to a crash for cases where iclk and fclks are same and we have the
> iclk defined as the main_clk as well.
>
> Same argument holds while returning from Idle. We are facing this issue for OMAP3
> GPIO while trying to define the main_clk = interface clock based on your other commment.
This is the same problem as has existed in static/suspend resume.
IOW, if it's possible for a device interrupt to arrive between device
suspend and actual suspend, then the device interrupt should be
disabled in the suspend hook (or runtime_suspend hook in your case.)
The catch is how to handle these interrupts if they are wakeup sources.
If these interrupt are wakeup sources, then they should not be disabled
in the [runtime_]suspend path, but rather the ISR for that device should
just do a get_sync() and continue.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-23 15:38 ` Kevin Hilman
@ 2010-09-23 19:57 ` Basak, Partha
2010-09-23 23:18 ` Kevin Hilman
0 siblings, 1 reply; 18+ messages in thread
From: Basak, Partha @ 2010-09-23 19:57 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo, Cousson, Benoit
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Thursday, September 23, 2010 9:08 PM
> To: Basak, Partha
> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> "Basak, Partha" <p-basak2@ti.com> writes:
>
> >
> >
> >> -----Original Message-----
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> Sent: Tuesday, September 14, 2010 10:28 PM
> >> To: Basak, Partha
> >> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha;
> Tero Kristo
> >> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> >> interrupts-disabled idle path
> >>
> >> "Basak, Partha" <p-basak2@ti.com> writes:
> >>
> >> >> From: Kevin Hilman <khilman@ti.com>
> >> >>
> >> >> Currently, we wait until late in the idle path where
> interrupts are
> >> >> disabled to do runtime-PM-like management for certain
> special-case
> >> >> devices like GPIO.
> >> >>
> >> >> As a prerequiste to moving GPIO to the new runtime PM
> >> framework, move
> >> >> this runtime-PM-like code out of the late idle path
> into new device
> >> >> idle and resume functions that can be called before
> interrupts are
> >> >> disabled by CPUidle and/or suspend.
> >> >>
> >> >> In addition, move all the GPIO-specific logic into the GPIO core
> >> >> instead of keeping GPIO-specific knowledge of
> power-states, context
> >> >> saving etc. in the PM core.
> >> >>
> >> >> Also, call the new device-idle and -resume methods from
> CPUidle and
> >> >> static suspend path.
> >> >>
> >> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
> >> >> ---
> >> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> >> >> arch/arm/mach-omap2/pm.h | 2 +
> >> >> arch/arm/mach-omap2/pm24xx.c | 2 +-
> >> >> arch/arm/mach-omap2/pm34xx.c | 38
> >> +++++++++------------
> >> >> arch/arm/plat-omap/gpio.c | 57
> >> >> ++++++++++++++++++++++++--------
> >> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> >> >> 6 files changed, 67 insertions(+), 40 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> index 0923b82..681d823 100644
> >> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
> >> >> cpuidle_device *dev,
> >> >> pwrdm_set_next_pwrst(per_pd, per_next_state);
> >> >>
> >> >> select_state:
> >> >> + omap3_device_idle();
> >> >> +
> >> >> dev->last_state = new_state;
> >> >> ret = omap3_enter_idle(dev, new_state);
> >> >>
> >> >> + omap3_device_resume();
> >> >> +
> >> > In the generic cpu_idle() in process.c, interrupts are
> >> already disabled
> >> > before control comes to cpuidle_idle_call() via pm_idle()
> >> > local_irq_disable();
> >> > if (hlt_counter) {
> >> > local_irq_enable();
> >> > cpu_relax();
> >> > } else {
> >> > stop_critical_timings();
> >> > pm_idle();
> >> > start_critical_timings();
> >> > /*
> >> > * This will eventually be
> >> removed - pm_idle
> >> > * functions should always
> >> return with IRQs
> >> > * enabled.
> >> > */
> >> > WARN_ON(irqs_disabled());
> >> > local_irq_enable();
> >> > }
> >> >
> >> > omap3_enter_idle_bm() will be called from inside
> >> cpuidle_idle_call()
> >> > via target_state->enter(dev, target_state).
> >> > So, interrupts are already disabled here.
> >> >
> >> > Am I missing something?
> >>
> >> You're right.
> >>
> >> I knew this was the case for !CPUIDLE setup, but had
> thought (without
> >> testing) that the CPUidle core had re-enabled interrupts during the
> >> governor selection process etc.
> >>
> >> While I investigate ways to manage this in CPUidle, the
> >> following should
> >> be fine for now to include with $SUBJECT patch.
> >>
> >> Kevin
> >>
> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> b/arch/arm/mach-omap2/cpuidle34xx.c
> >> index 681d823..c5cb9d0 100644
> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
> >> cpuidle_device *dev,
> >> goto select_state;
> >> }
> >>
> >> + /*
> >> + * Enable IRQs during the device activity checking and
> >> idle management.
> >> + * IRQs are later (re)disabled when entering the actual
> >> idle function.
> >> + * Device idle management that is using runtime PM needs to have
> >> + * interrupts enabled when calling into the runtime PM core.
> >> + */
> >> + local_irq_enable();
> >
> > After put_sync() retuns, there will be a time window where
> interrupts
> > are enabled but clocks are disabled before the interrupts
> are disabled again.
> > Accessing any register to service a device interrupt coming
> during this window
> > will lead to a crash for cases where iclk and fclks are
> same and we have the
> > iclk defined as the main_clk as well.
> >
> > Same argument holds while returning from Idle. We are
> facing this issue for OMAP3
> > GPIO while trying to define the main_clk = interface clock
> based on your other commment.
>
> This is the same problem as has existed in static/suspend resume.
>
> IOW, if it's possible for a device interrupt to arrive between device
> suspend and actual suspend, then the device interrupt should be
> disabled in the suspend hook (or runtime_suspend hook in your case.)
>
> The catch is how to handle these interrupts if they are
> wakeup sources.
Precisely.
For example, the ethernet interrupts over GPIO are causing problem.
>
> If these interrupt are wakeup sources, then they should not
> be disabled
> in the [runtime_]suspend path, but rather the ISR for that
> device should
> just do a get_sync() and continue.
We cannot do a get_sync() from ISR context, right?
For GPIOs in particular, this problem can be resolved if we do not tie up
the interface clock as the main_clk. So long the interface/slave clock
is not also a main_clk & OCPIF_SWSUP_IDLE flag is not set for a hwmod
there is no issue, as slave clocks are NOT turned on/off in _enable_clocks()
/_disable_clocks() inside hwmod fw.
Alternatively, we could have thought of removing get_sync/put_sync altogether
from the Idle path for GPIO. But though this would work fine for OMAP3/OMAP4 &
OMAP2420/2430 Wakeup domain GPIOs, but for OMAP2430 CORE domain GPIOs, since it
has a separate fclk & iclk, we would still need to cut the fclk in the Idle-path
to enable CORE transition, thereby needing a get_sync/put_sync .
If this is OK, we can consider this approach.
>
> Kevin
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-23 19:57 ` Basak, Partha
@ 2010-09-23 23:18 ` Kevin Hilman
2010-09-23 23:53 ` Kevin Hilman
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2010-09-23 23:18 UTC (permalink / raw)
To: Basak, Partha
Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo, Cousson, Benoit
"Basak, Partha" <p-basak2@ti.com> writes:
>
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Thursday, September 23, 2010 9:08 PM
>> To: Basak, Partha
>> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
>> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>> interrupts-disabled idle path
>>
>> "Basak, Partha" <p-basak2@ti.com> writes:
>>
>> >
>> >
>> >> -----Original Message-----
>> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> >> Sent: Tuesday, September 14, 2010 10:28 PM
>> >> To: Basak, Partha
>> >> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha;
>> Tero Kristo
>> >> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>> >> interrupts-disabled idle path
>> >>
>> >> "Basak, Partha" <p-basak2@ti.com> writes:
>> >>
>> >> >> From: Kevin Hilman <khilman@ti.com>
>> >> >>
>> >> >> Currently, we wait until late in the idle path where
>> interrupts are
>> >> >> disabled to do runtime-PM-like management for certain
>> special-case
>> >> >> devices like GPIO.
>> >> >>
>> >> >> As a prerequiste to moving GPIO to the new runtime PM
>> >> framework, move
>> >> >> this runtime-PM-like code out of the late idle path
>> into new device
>> >> >> idle and resume functions that can be called before
>> interrupts are
>> >> >> disabled by CPUidle and/or suspend.
>> >> >>
>> >> >> In addition, move all the GPIO-specific logic into the GPIO core
>> >> >> instead of keeping GPIO-specific knowledge of
>> power-states, context
>> >> >> saving etc. in the PM core.
>> >> >>
>> >> >> Also, call the new device-idle and -resume methods from
>> CPUidle and
>> >> >> static suspend path.
>> >> >>
>> >> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> >> >> ---
>> >> >> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
>> >> >> arch/arm/mach-omap2/pm.h | 2 +
>> >> >> arch/arm/mach-omap2/pm24xx.c | 2 +-
>> >> >> arch/arm/mach-omap2/pm34xx.c | 38
>> >> +++++++++------------
>> >> >> arch/arm/plat-omap/gpio.c | 57
>> >> >> ++++++++++++++++++++++++--------
>> >> >> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
>> >> >> 6 files changed, 67 insertions(+), 40 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> >> b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> >> index 0923b82..681d823 100644
>> >> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> >> @@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>> >> >> cpuidle_device *dev,
>> >> >> pwrdm_set_next_pwrst(per_pd, per_next_state);
>> >> >>
>> >> >> select_state:
>> >> >> + omap3_device_idle();
>> >> >> +
>> >> >> dev->last_state = new_state;
>> >> >> ret = omap3_enter_idle(dev, new_state);
>> >> >>
>> >> >> + omap3_device_resume();
>> >> >> +
>> >> > In the generic cpu_idle() in process.c, interrupts are
>> >> already disabled
>> >> > before control comes to cpuidle_idle_call() via pm_idle()
>> >> > local_irq_disable();
>> >> > if (hlt_counter) {
>> >> > local_irq_enable();
>> >> > cpu_relax();
>> >> > } else {
>> >> > stop_critical_timings();
>> >> > pm_idle();
>> >> > start_critical_timings();
>> >> > /*
>> >> > * This will eventually be
>> >> removed - pm_idle
>> >> > * functions should always
>> >> return with IRQs
>> >> > * enabled.
>> >> > */
>> >> > WARN_ON(irqs_disabled());
>> >> > local_irq_enable();
>> >> > }
>> >> >
>> >> > omap3_enter_idle_bm() will be called from inside
>> >> cpuidle_idle_call()
>> >> > via target_state->enter(dev, target_state).
>> >> > So, interrupts are already disabled here.
>> >> >
>> >> > Am I missing something?
>> >>
>> >> You're right.
>> >>
>> >> I knew this was the case for !CPUIDLE setup, but had
>> thought (without
>> >> testing) that the CPUidle core had re-enabled interrupts during the
>> >> governor selection process etc.
>> >>
>> >> While I investigate ways to manage this in CPUidle, the
>> >> following should
>> >> be fine for now to include with $SUBJECT patch.
>> >>
>> >> Kevin
>> >>
>> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> index 681d823..c5cb9d0 100644
>> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> >> @@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
>> >> cpuidle_device *dev,
>> >> goto select_state;
>> >> }
>> >>
>> >> + /*
>> >> + * Enable IRQs during the device activity checking and
>> >> idle management.
>> >> + * IRQs are later (re)disabled when entering the actual
>> >> idle function.
>> >> + * Device idle management that is using runtime PM needs to have
>> >> + * interrupts enabled when calling into the runtime PM core.
>> >> + */
>> >> + local_irq_enable();
>> >
>> > After put_sync() retuns, there will be a time window where
>> interrupts
>> > are enabled but clocks are disabled before the interrupts
>> are disabled again.
>> > Accessing any register to service a device interrupt coming
>> during this window
>> > will lead to a crash for cases where iclk and fclks are
>> same and we have the
>> > iclk defined as the main_clk as well.
>> >
>> > Same argument holds while returning from Idle. We are
>> facing this issue for OMAP3
>> > GPIO while trying to define the main_clk = interface clock
>> based on your other commment.
>>
>> This is the same problem as has existed in static/suspend resume.
>>
>> IOW, if it's possible for a device interrupt to arrive between device
>> suspend and actual suspend, then the device interrupt should be
>> disabled in the suspend hook (or runtime_suspend hook in your case.)
>>
>> The catch is how to handle these interrupts if they are
>> wakeup sources.
>
> Precisely.
> For example, the ethernet interrupts over GPIO are causing problem.
>
>>
>> If these interrupt are wakeup sources, then they should not
>> be disabled
>> in the [runtime_]suspend path, but rather the ISR for that
>> device should
>> just do a get_sync() and continue.
>
> We cannot do a get_sync() from ISR context, right?
Right, but we *should* be able to. ;)
I'm still trying to craft a good description of this problem so I can
argue better for it on linux-pm.
Until then...
A bit of a hack, but you could do a _get_noresume() (which is safe from
interrupt context) and directly call the drivers ->runtime_resume()
method, which would be the equivalent of a _get_sync(). Followed of
course by a _put() (async version, also interrupt safe) at the end of
the ISR to keep the usecount correct.
> For GPIOs in particular, this problem can be resolved if we do not tie up
> the interface clock as the main_clk. So long the interface/slave clock
> is not also a main_clk & OCPIF_SWSUP_IDLE flag is not set for a hwmod
> there is no issue, as slave clocks are NOT turned on/off in _enable_clocks()
> /_disable_clocks() inside hwmod fw.
The problem though is that without a main clock, autodeps are not
tracked which means that PER might transition independently of the
initiator (in this case, MPU) which we do not want either.
I discussed this a little more with Paul, and the root of the problem is
that the clock framework really should not disable iclks if iclk
autoidle is enabled. This is a change that is needed in the clock
framework that might have other side effects so is to late to attempt
for this merge window.
So for now, I suggest we try the above hack (well commented in the code,
of course.)
> Alternatively, we could have thought of removing get_sync/put_sync altogether
> from the Idle path for GPIO. But though this would work fine for OMAP3/OMAP4 &
> OMAP2420/2430 Wakeup domain GPIOs, but for OMAP2430 CORE domain GPIOs, since it
> has a separate fclk & iclk, we would still need to cut the fclk in the Idle-path
> to enable CORE transition, thereby needing a get_sync/put_sync .
>
> If this is OK, we can consider this approach.
This would certainly be the ideal solution.
It would be great to get rid of the need for calling GPIO core in the
idle path, but I'm not yet convinced we can do that.
For example, there is some other errata handling in my pm-gpio branch
(that I've been waiting to rework until after this hwmod conversion.) I
would want to be sure that those things can still be correctly handled.
Also How would you propose to handle 2430 CORE GPIOs?
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-23 23:18 ` Kevin Hilman
@ 2010-09-23 23:53 ` Kevin Hilman
2010-09-25 3:30 ` Basak, Partha
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2010-09-23 23:53 UTC (permalink / raw)
To: Basak, Partha
Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo, Cousson, Benoit
Kevin Hilman <khilman@deeprootsystems.com> writes:
[...]
>>
>> We cannot do a get_sync() from ISR context, right?
>
> Right, but we *should* be able to. ;)
>
> I'm still trying to craft a good description of this problem so I can
> argue better for it on linux-pm.
>
> Until then...
>
> A bit of a hack, but you could do a _get_noresume() (which is safe from
> interrupt context) and directly call the drivers ->runtime_resume()
> method, which would be the equivalent of a _get_sync(). Followed of
> course by a _put() (async version, also interrupt safe) at the end of
> the ISR to keep the usecount correct.
You probably figured this out already, but I just realized that this
won't currently work either as omap_hwmod is using mutexes, and is
safe in ISR context either. :(
What about for now just directly enabling (and re-disabling) the hwmod
clocks in the ISR using omap_hwmod_[enable|disable]_clocks()
Since this is a core driver in arch/arm/*omap*, you can directly call
the omap_hwmod API.
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-23 23:53 ` Kevin Hilman
@ 2010-09-25 3:30 ` Basak, Partha
2010-09-27 14:53 ` Kevin Hilman
0 siblings, 1 reply; 18+ messages in thread
From: Basak, Partha @ 2010-09-25 3:30 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo, Cousson, Benoit
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Friday, September 24, 2010 5:23 AM
> To: Basak, Partha
> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero
> Kristo; Cousson, Benoit
> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
> interrupts-disabled idle path
>
> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
> [...]
>
> >>
> >> We cannot do a get_sync() from ISR context, right?
> >
> > Right, but we *should* be able to. ;)
> >
> > I'm still trying to craft a good description of this
> problem so I can
> > argue better for it on linux-pm.
> >
> > Until then...
> >
> > A bit of a hack, but you could do a _get_noresume() (which
> is safe from
> > interrupt context) and directly call the drivers ->runtime_resume()
> > method, which would be the equivalent of a _get_sync(). Followed of
> > course by a _put() (async version, also interrupt safe) at
> the end of
> > the ISR to keep the usecount correct.
>
> You probably figured this out already, but I just realized that this
> won't currently work either as omap_hwmod is using mutexes, and is
> safe in ISR context either. :(
>
> What about for now just directly enabling (and re-disabling) the hwmod
> clocks in the ISR using omap_hwmod_[enable|disable]_clocks()
>
> Since this is a core driver in arch/arm/*omap*, you can directly call
> the omap_hwmod API.
>
omap_hwmod_[enable/disable]_clocks() use mutex inside & therefore cannot
be used in the ISR context
We cannot readily use the _enable_clocks/_disble_clocks directly as they are
static APIs.
But we can use the non-mutex versions, of omap_hwmod_enable/idle.
(_omap_hwmod_enable/_omap_hwmod_disable)
Do you agree?
> Kevin
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-25 3:30 ` Basak, Partha
@ 2010-09-27 14:53 ` Kevin Hilman
0 siblings, 0 replies; 18+ messages in thread
From: Kevin Hilman @ 2010-09-27 14:53 UTC (permalink / raw)
To: Basak, Partha
Cc: linux-omap, Varadarajan, Charulatha, Tero Kristo, Cousson, Benoit
"Basak, Partha" <p-basak2@ti.com> writes:
>
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Friday, September 24, 2010 5:23 AM
>> To: Basak, Partha
>> Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero
>> Kristo; Cousson, Benoit
>> Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>> interrupts-disabled idle path
>>
>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>
>> [...]
>>
>> >>
>> >> We cannot do a get_sync() from ISR context, right?
>> >
>> > Right, but we *should* be able to. ;)
>> >
>> > I'm still trying to craft a good description of this
>> problem so I can
>> > argue better for it on linux-pm.
>> >
>> > Until then...
>> >
>> > A bit of a hack, but you could do a _get_noresume() (which
>> is safe from
>> > interrupt context) and directly call the drivers ->runtime_resume()
>> > method, which would be the equivalent of a _get_sync(). Followed of
>> > course by a _put() (async version, also interrupt safe) at
>> the end of
>> > the ISR to keep the usecount correct.
>>
>> You probably figured this out already, but I just realized that this
>> won't currently work either as omap_hwmod is using mutexes, and is
>> safe in ISR context either. :(
>>
>> What about for now just directly enabling (and re-disabling) the hwmod
>> clocks in the ISR using omap_hwmod_[enable|disable]_clocks()
>>
>> Since this is a core driver in arch/arm/*omap*, you can directly call
>> the omap_hwmod API.
>>
>
> omap_hwmod_[enable/disable]_clocks() use mutex inside & therefore cannot
> be used in the ISR context
>
> We cannot readily use the _enable_clocks/_disble_clocks directly as they are
> static APIs.
>
> But we can use the non-mutex versions, of omap_hwmod_enable/idle.
> (_omap_hwmod_enable/_omap_hwmod_disable)
>
> Do you agree?
Yes, I meant to use the non-mutex versions.
Sorry,
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-13 23:02 ` [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path Kevin Hilman
2010-09-14 14:14 ` Varadarajan, Charulatha
2010-09-14 16:09 ` Basak, Partha
@ 2010-09-22 8:22 ` Kalliguddi, Hema
2010-09-22 14:24 ` Kevin Hilman
2 siblings, 1 reply; 18+ messages in thread
From: Kalliguddi, Hema @ 2010-09-22 8:22 UTC (permalink / raw)
To: Kevin Hilman, linux-omap
Cc: Varadarajan, Charulatha, Basak, Partha, Tero Kristo
Hi,
>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org
>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin Hilman
>Sent: Tuesday, September 14, 2010 4:33 AM
>To: linux-omap@vger.kernel.org
>Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
>Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>interrupts-disabled idle path
>
>From: Kevin Hilman <khilman@ti.com>
>
>Currently, we wait until late in the idle path where interrupts are
>disabled to do runtime-PM-like management for certain special-case
>devices like GPIO.
>
>As a prerequiste to moving GPIO to the new runtime PM framework, move
>this runtime-PM-like code out of the late idle path into new device
>idle and resume functions that can be called before interrupts are
>disabled by CPUidle and/or suspend.
>
>In addition, move all the GPIO-specific logic into the GPIO core
>instead of keeping GPIO-specific knowledge of power-states, context
>saving etc. in the PM core.
>
>Also, call the new device-idle and -resume methods from CPUidle and
>static suspend path.
>
>Signed-off-by: Kevin Hilman <khilman@ti.com>
>---
> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
> arch/arm/mach-omap2/pm.h | 2 +
> arch/arm/mach-omap2/pm24xx.c | 2 +-
> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
> arch/arm/plat-omap/gpio.c | 57
>++++++++++++++++++++++++--------
> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
> 6 files changed, 67 insertions(+), 40 deletions(-)
>
>diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>b/arch/arm/mach-omap2/cpuidle34xx.c
>index 0923b82..681d823 100644
>--- a/arch/arm/mach-omap2/cpuidle34xx.c
>+++ b/arch/arm/mach-omap2/cpuidle34xx.c
>@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>cpuidle_device *dev,
> pwrdm_set_next_pwrst(per_pd, per_next_state);
>
> select_state:
>+ omap3_device_idle();
>+
> dev->last_state = new_state;
> ret = omap3_enter_idle(dev, new_state);
>
>+ omap3_device_resume();
>+
> /* Restore original PER state if it was modified */
> if (per_next_state != per_saved_state)
> pwrdm_set_next_pwrst(per_pd, per_saved_state);
>diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
>index 3de6ece..33cae4b 100644
>--- a/arch/arm/mach-omap2/pm.h
>+++ b/arch/arm/mach-omap2/pm.h
>@@ -22,6 +22,8 @@ extern void omap_sram_idle(void);
> extern int omap3_can_sleep(void);
> extern int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
> extern int omap3_idle_init(void);
>+extern void omap3_device_idle(void);
>+extern void omap3_device_resume(void);
>
> struct cpuidle_params {
> u8 valid;
>diff --git a/arch/arm/mach-omap2/pm24xx.c
>b/arch/arm/mach-omap2/pm24xx.c
>index 6aeedea..7bbf0db 100644
>--- a/arch/arm/mach-omap2/pm24xx.c
>+++ b/arch/arm/mach-omap2/pm24xx.c
>@@ -106,7 +106,7 @@ static void omap2_enter_full_retention(void)
> l = omap_ctrl_readl(OMAP2_CONTROL_DEVCONF0) |
>OMAP24XX_USBSTANDBYCTRL;
> omap_ctrl_writel(l, OMAP2_CONTROL_DEVCONF0);
>
>- omap2_gpio_prepare_for_idle(PWRDM_POWER_RET);
>+ omap2_gpio_prepare_for_idle();
>
> if (omap2_pm_debug) {
> omap2_pm_dump(0, 0, 0);
>diff --git a/arch/arm/mach-omap2/pm34xx.c
>b/arch/arm/mach-omap2/pm34xx.c
>index bb2ba1e..9e590d9 100644
>--- a/arch/arm/mach-omap2/pm34xx.c
>+++ b/arch/arm/mach-omap2/pm34xx.c
>@@ -74,16 +74,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
> static struct powerdomain *core_pwrdm, *per_pwrdm;
> static struct powerdomain *cam_pwrdm;
>
>-static inline void omap3_per_save_context(void)
>-{
>- omap_gpio_save_context();
>-}
>-
>-static inline void omap3_per_restore_context(void)
>-{
>- omap_gpio_restore_context();
>-}
>-
> static void omap3_enable_io_chain(void)
> {
> int timeout = 0;
>@@ -332,6 +322,16 @@ static void restore_table_entry(void)
> restore_control_register(control_reg_value);
> }
>
>+void omap3_device_idle(void)
>+{
>+ omap2_gpio_prepare_for_idle();
>+}
>+
Is not it is a good idea to pass the new_state as the parameter for the driver calls?
It will reduce each individual drivers reading the PRCM register to findout the next state.
This might save some time in the idle loop.
>+void omap3_device_resume(void)
>+{
>+ omap2_gpio_resume_after_idle();
>+}
>+
Here we will need to pass the next_state as well as previous_state as parameters. If we agree to
pass the parameters.
~Hema
> void omap_sram_idle(void)
> {
> /* Variable to tell what needs to be saved and restored
>@@ -344,7 +344,7 @@ void omap_sram_idle(void)
> int mpu_next_state = PWRDM_POWER_ON;
> int per_next_state = PWRDM_POWER_ON;
> int core_next_state = PWRDM_POWER_ON;
>- int core_prev_state, per_prev_state;
>+ int core_prev_state;
> u32 sdrc_pwr = 0;
>
> if (!_omap_sram_idle)
>@@ -387,12 +387,9 @@ void omap_sram_idle(void)
> }
>
> /* PER */
>- if (per_next_state < PWRDM_POWER_ON) {
>+ if (per_next_state < PWRDM_POWER_ON)
> omap_uart_prepare_idle(2);
>- omap2_gpio_prepare_for_idle(per_next_state);
>- if (per_next_state == PWRDM_POWER_OFF)
>- omap3_per_save_context();
>- }
>+
>
> /* CORE */
> if (core_next_state < PWRDM_POWER_ON) {
>@@ -454,13 +451,8 @@ void omap_sram_idle(void)
> omap3_intc_resume_idle();
>
> /* PER */
>- if (per_next_state < PWRDM_POWER_ON) {
>- per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>- omap2_gpio_resume_after_idle();
>- if (per_prev_state == PWRDM_POWER_OFF)
>- omap3_per_restore_context();
>+ if (per_next_state < PWRDM_POWER_ON)
> omap_uart_resume_idle(2);
>- }
>
> /* Disable IO-PAD and IO-CHAIN wakeup */
> if (omap3_has_io_wakeup() &&
>@@ -570,6 +562,7 @@ static void omap2_pm_wakeup_on_timer(u32
>seconds, u32 milliseconds)
> static int omap3_pm_prepare(void)
> {
> disable_hlt();
>+ omap3_device_idle();
> return 0;
> }
>
>@@ -637,6 +630,7 @@ static int omap3_pm_enter(suspend_state_t unused)
>
> static void omap3_pm_finish(void)
> {
>+ omap3_device_resume();
> enable_hlt();
> }
>
>diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>index 7951eef..b0467c1 100644
>--- a/arch/arm/plat-omap/gpio.c
>+++ b/arch/arm/plat-omap/gpio.c
>@@ -29,6 +29,8 @@
> #include <asm/mach/irq.h>
> #include <plat/powerdomain.h>
>
>+static struct powerdomain *per_pwrdm;
>+
> /*
> * OMAP1510 GPIO registers
> */
>@@ -207,6 +209,9 @@ struct gpio_bank {
> u32 dbck_enable_mask;
> };
>
>+static void omap3_gpio_restore_context(void);
>+static void omap3_gpio_save_context(void);
>+
> #define METHOD_MPUIO 0
> #define METHOD_GPIO_1510 1
> #define METHOD_GPIO_1610 2
>@@ -1778,6 +1783,8 @@ static int __init _omap_gpio_init(void)
> }
> #endif
>
>+ if (cpu_class_is_omap2())
>+ per_pwrdm = pwrdm_lookup("per_pwrdm");
>
> #ifdef CONFIG_ARCH_OMAP15XX
> if (cpu_is_omap15xx()) {
>@@ -2074,14 +2081,22 @@ static struct sys_device omap_gpio_device = {
>
> static int workaround_enabled;
>
>-void omap2_gpio_prepare_for_idle(int power_state)
>+void omap2_gpio_prepare_for_idle(void)
> {
>- int i, c = 0;
>- int min = 0;
>+ int i, c = 0, min = 0;
>+ int per_next_state;
>+
>+ if (!per_pwrdm)
>+ return;
>+
>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>+ return;
>
> if (cpu_is_omap34xx())
> min = 1;
>
>+ workaround_enabled = 0;
> for (i = min; i < gpio_bank_count; i++) {
> struct gpio_bank *bank = &gpio_bank[i];
> u32 l1, l2;
>@@ -2089,7 +2104,7 @@ void omap2_gpio_prepare_for_idle(int power_state)
> if (bank->dbck_enable_mask)
> clk_disable(bank->dbck);
>
>- if (power_state > PWRDM_POWER_OFF)
>+ if (per_next_state > PWRDM_POWER_OFF)
> continue;
>
> /* If going to OFF, remove triggering for all
>@@ -2135,20 +2150,35 @@ void omap2_gpio_prepare_for_idle(int
>power_state)
>
> c++;
> }
>- if (!c) {
>- workaround_enabled = 0;
>- return;
>- }
>- workaround_enabled = 1;
>+ if (c)
>+ workaround_enabled = 1;
>+
>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF))
>+ omap3_gpio_save_context();
> }
>
> void omap2_gpio_resume_after_idle(void)
> {
>- int i;
>- int min = 0;
>+ int i, min = 0;
>+ int per_next_state;
>+
>+ if (!per_pwrdm)
>+ return;
>+
>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>+ return;
>+
>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF)) {
>+ int per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>+
>+ if (per_prev_state == PWRDM_POWER_OFF)
>+ omap3_gpio_restore_context();
>+ }
>
> if (cpu_is_omap34xx())
> min = 1;
>+
> for (i = min; i < gpio_bank_count; i++) {
> struct gpio_bank *bank = &gpio_bank[i];
> u32 l, gen, gen0, gen1;
>@@ -2235,14 +2265,13 @@ void omap2_gpio_resume_after_idle(void)
> }
> }
> }
>-
> }
>
> #endif
>
> #ifdef CONFIG_ARCH_OMAP3
> /* save the registers of bank 2-6 */
>-void omap_gpio_save_context(void)
>+static void omap3_gpio_save_context(void)
> {
> int i;
>
>@@ -2275,7 +2304,7 @@ void omap_gpio_save_context(void)
> }
>
> /* restore the required registers of bank 2-6 */
>-void omap_gpio_restore_context(void)
>+static void omap3_gpio_restore_context(void)
> {
> int i;
>
>diff --git a/arch/arm/plat-omap/include/plat/gpio.h
>b/arch/arm/plat-omap/include/plat/gpio.h
>index de1c604..c81ef94 100644
>--- a/arch/arm/plat-omap/include/plat/gpio.h
>+++ b/arch/arm/plat-omap/include/plat/gpio.h
>@@ -72,12 +72,10 @@
> IH_GPIO_BASE + (nr))
>
> extern int omap_gpio_init(void); /* Call from board init only */
>-extern void omap2_gpio_prepare_for_idle(int power_state);
>+extern void omap2_gpio_prepare_for_idle(void);
> extern void omap2_gpio_resume_after_idle(void);
> extern void omap_set_gpio_debounce(int gpio, int enable);
> extern void omap_set_gpio_debounce_time(int gpio, int enable);
>-extern void omap_gpio_save_context(void);
>-extern void omap_gpio_restore_context(void);
>
>/*-------------------------------------------------------------
>------------*/
>
> /* Wrappers for "new style" GPIO calls, using the new infrastructure
>--
>1.7.2.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe
>linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-22 8:22 ` Kalliguddi, Hema
@ 2010-09-22 14:24 ` Kevin Hilman
2010-09-22 15:09 ` Kalliguddi, Hema
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2010-09-22 14:24 UTC (permalink / raw)
To: Kalliguddi, Hema
Cc: linux-omap, Varadarajan, Charulatha, Basak, Partha, Tero Kristo
"Kalliguddi, Hema" <hemahk@ti.com> writes:
>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org
>>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin Hilman
>>Sent: Tuesday, September 14, 2010 4:33 AM
>>To: linux-omap@vger.kernel.org
>>Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
>>Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>>interrupts-disabled idle path
>>
>>From: Kevin Hilman <khilman@ti.com>
>>
>>Currently, we wait until late in the idle path where interrupts are
>>disabled to do runtime-PM-like management for certain special-case
>>devices like GPIO.
>>
>>As a prerequiste to moving GPIO to the new runtime PM framework, move
>>this runtime-PM-like code out of the late idle path into new device
>>idle and resume functions that can be called before interrupts are
>>disabled by CPUidle and/or suspend.
>>
>>In addition, move all the GPIO-specific logic into the GPIO core
>>instead of keeping GPIO-specific knowledge of power-states, context
>>saving etc. in the PM core.
>>
>>Also, call the new device-idle and -resume methods from CPUidle and
>>static suspend path.
>>
>>Signed-off-by: Kevin Hilman <khilman@ti.com>
>>---
>> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
>> arch/arm/mach-omap2/pm.h | 2 +
>> arch/arm/mach-omap2/pm24xx.c | 2 +-
>> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
>> arch/arm/plat-omap/gpio.c | 57
>>++++++++++++++++++++++++--------
>> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
>> 6 files changed, 67 insertions(+), 40 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>>b/arch/arm/mach-omap2/cpuidle34xx.c
>>index 0923b82..681d823 100644
>>--- a/arch/arm/mach-omap2/cpuidle34xx.c
>>+++ b/arch/arm/mach-omap2/cpuidle34xx.c
>>@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>>cpuidle_device *dev,
>> pwrdm_set_next_pwrst(per_pd, per_next_state);
>>
>> select_state:
>>+ omap3_device_idle();
>>+
>> dev->last_state = new_state;
>> ret = omap3_enter_idle(dev, new_state);
>>
>>+ omap3_device_resume();
>>+
>> /* Restore original PER state if it was modified */
>> if (per_next_state != per_saved_state)
>> pwrdm_set_next_pwrst(per_pd, per_saved_state);
>>diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
>>index 3de6ece..33cae4b 100644
>>--- a/arch/arm/mach-omap2/pm.h
>>+++ b/arch/arm/mach-omap2/pm.h
>>@@ -22,6 +22,8 @@ extern void omap_sram_idle(void);
>> extern int omap3_can_sleep(void);
>> extern int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
>> extern int omap3_idle_init(void);
>>+extern void omap3_device_idle(void);
>>+extern void omap3_device_resume(void);
>>
>> struct cpuidle_params {
>> u8 valid;
>>diff --git a/arch/arm/mach-omap2/pm24xx.c
>>b/arch/arm/mach-omap2/pm24xx.c
>>index 6aeedea..7bbf0db 100644
>>--- a/arch/arm/mach-omap2/pm24xx.c
>>+++ b/arch/arm/mach-omap2/pm24xx.c
>>@@ -106,7 +106,7 @@ static void omap2_enter_full_retention(void)
>> l = omap_ctrl_readl(OMAP2_CONTROL_DEVCONF0) |
>>OMAP24XX_USBSTANDBYCTRL;
>> omap_ctrl_writel(l, OMAP2_CONTROL_DEVCONF0);
>>
>>- omap2_gpio_prepare_for_idle(PWRDM_POWER_RET);
>>+ omap2_gpio_prepare_for_idle();
>>
>> if (omap2_pm_debug) {
>> omap2_pm_dump(0, 0, 0);
>>diff --git a/arch/arm/mach-omap2/pm34xx.c
>>b/arch/arm/mach-omap2/pm34xx.c
>>index bb2ba1e..9e590d9 100644
>>--- a/arch/arm/mach-omap2/pm34xx.c
>>+++ b/arch/arm/mach-omap2/pm34xx.c
>>@@ -74,16 +74,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
>> static struct powerdomain *core_pwrdm, *per_pwrdm;
>> static struct powerdomain *cam_pwrdm;
>>
>>-static inline void omap3_per_save_context(void)
>>-{
>>- omap_gpio_save_context();
>>-}
>>-
>>-static inline void omap3_per_restore_context(void)
>>-{
>>- omap_gpio_restore_context();
>>-}
>>-
>> static void omap3_enable_io_chain(void)
>> {
>> int timeout = 0;
>>@@ -332,6 +322,16 @@ static void restore_table_entry(void)
>> restore_control_register(control_reg_value);
>> }
>>
>>+void omap3_device_idle(void)
>>+{
>>+ omap2_gpio_prepare_for_idle();
>>+}
>>+
>
> Is not it is a good idea to pass the new_state as the parameter for the driver calls?
> It will reduce each individual drivers reading the PRCM register to findout the next state.
> This might save some time in the idle loop.
I chose not to pass the parameters on purpose. All of the intelligence
for device idle (including which powerdomain state to check) should live
in the driver code.
If reading the PRCM registers is becoming a problem, it will be a simple
patch to patch the powerdomain core to cache the current value of it's
next state so at least reads of next_state will be fast.
Kevin
>>+void omap3_device_resume(void)
>>+{
>>+ omap2_gpio_resume_after_idle();
>>+}
>>+
>
> Here we will need to pass the next_state as well as previous_state as parameters. If we agree to
> pass the parameters.
>
> ~Hema
>
>> void omap_sram_idle(void)
>> {
>> /* Variable to tell what needs to be saved and restored
>>@@ -344,7 +344,7 @@ void omap_sram_idle(void)
>> int mpu_next_state = PWRDM_POWER_ON;
>> int per_next_state = PWRDM_POWER_ON;
>> int core_next_state = PWRDM_POWER_ON;
>>- int core_prev_state, per_prev_state;
>>+ int core_prev_state;
>> u32 sdrc_pwr = 0;
>>
>> if (!_omap_sram_idle)
>>@@ -387,12 +387,9 @@ void omap_sram_idle(void)
>> }
>>
>> /* PER */
>>- if (per_next_state < PWRDM_POWER_ON) {
>>+ if (per_next_state < PWRDM_POWER_ON)
>> omap_uart_prepare_idle(2);
>>- omap2_gpio_prepare_for_idle(per_next_state);
>>- if (per_next_state == PWRDM_POWER_OFF)
>>- omap3_per_save_context();
>>- }
>>+
>>
>> /* CORE */
>> if (core_next_state < PWRDM_POWER_ON) {
>>@@ -454,13 +451,8 @@ void omap_sram_idle(void)
>> omap3_intc_resume_idle();
>>
>> /* PER */
>>- if (per_next_state < PWRDM_POWER_ON) {
>>- per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>>- omap2_gpio_resume_after_idle();
>>- if (per_prev_state == PWRDM_POWER_OFF)
>>- omap3_per_restore_context();
>>+ if (per_next_state < PWRDM_POWER_ON)
>> omap_uart_resume_idle(2);
>>- }
>>
>> /* Disable IO-PAD and IO-CHAIN wakeup */
>> if (omap3_has_io_wakeup() &&
>>@@ -570,6 +562,7 @@ static void omap2_pm_wakeup_on_timer(u32
>>seconds, u32 milliseconds)
>> static int omap3_pm_prepare(void)
>> {
>> disable_hlt();
>>+ omap3_device_idle();
>> return 0;
>> }
>>
>>@@ -637,6 +630,7 @@ static int omap3_pm_enter(suspend_state_t unused)
>>
>> static void omap3_pm_finish(void)
>> {
>>+ omap3_device_resume();
>> enable_hlt();
>> }
>>
>>diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>>index 7951eef..b0467c1 100644
>>--- a/arch/arm/plat-omap/gpio.c
>>+++ b/arch/arm/plat-omap/gpio.c
>>@@ -29,6 +29,8 @@
>> #include <asm/mach/irq.h>
>> #include <plat/powerdomain.h>
>>
>>+static struct powerdomain *per_pwrdm;
>>+
>> /*
>> * OMAP1510 GPIO registers
>> */
>>@@ -207,6 +209,9 @@ struct gpio_bank {
>> u32 dbck_enable_mask;
>> };
>>
>>+static void omap3_gpio_restore_context(void);
>>+static void omap3_gpio_save_context(void);
>>+
>> #define METHOD_MPUIO 0
>> #define METHOD_GPIO_1510 1
>> #define METHOD_GPIO_1610 2
>>@@ -1778,6 +1783,8 @@ static int __init _omap_gpio_init(void)
>> }
>> #endif
>>
>>+ if (cpu_class_is_omap2())
>>+ per_pwrdm = pwrdm_lookup("per_pwrdm");
>>
>> #ifdef CONFIG_ARCH_OMAP15XX
>> if (cpu_is_omap15xx()) {
>>@@ -2074,14 +2081,22 @@ static struct sys_device omap_gpio_device = {
>>
>> static int workaround_enabled;
>>
>>-void omap2_gpio_prepare_for_idle(int power_state)
>>+void omap2_gpio_prepare_for_idle(void)
>> {
>>- int i, c = 0;
>>- int min = 0;
>>+ int i, c = 0, min = 0;
>>+ int per_next_state;
>>+
>>+ if (!per_pwrdm)
>>+ return;
>>+
>>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>>+ return;
>>
>> if (cpu_is_omap34xx())
>> min = 1;
>>
>>+ workaround_enabled = 0;
>> for (i = min; i < gpio_bank_count; i++) {
>> struct gpio_bank *bank = &gpio_bank[i];
>> u32 l1, l2;
>>@@ -2089,7 +2104,7 @@ void omap2_gpio_prepare_for_idle(int power_state)
>> if (bank->dbck_enable_mask)
>> clk_disable(bank->dbck);
>>
>>- if (power_state > PWRDM_POWER_OFF)
>>+ if (per_next_state > PWRDM_POWER_OFF)
>> continue;
>>
>> /* If going to OFF, remove triggering for all
>>@@ -2135,20 +2150,35 @@ void omap2_gpio_prepare_for_idle(int
>>power_state)
>>
>> c++;
>> }
>>- if (!c) {
>>- workaround_enabled = 0;
>>- return;
>>- }
>>- workaround_enabled = 1;
>>+ if (c)
>>+ workaround_enabled = 1;
>>+
>>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF))
>>+ omap3_gpio_save_context();
>> }
>>
>> void omap2_gpio_resume_after_idle(void)
>> {
>>- int i;
>>- int min = 0;
>>+ int i, min = 0;
>>+ int per_next_state;
>>+
>>+ if (!per_pwrdm)
>>+ return;
>>+
>>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>>+ return;
>>+
>>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF)) {
>>+ int per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>>+
>>+ if (per_prev_state == PWRDM_POWER_OFF)
>>+ omap3_gpio_restore_context();
>>+ }
>>
>> if (cpu_is_omap34xx())
>> min = 1;
>>+
>> for (i = min; i < gpio_bank_count; i++) {
>> struct gpio_bank *bank = &gpio_bank[i];
>> u32 l, gen, gen0, gen1;
>>@@ -2235,14 +2265,13 @@ void omap2_gpio_resume_after_idle(void)
>> }
>> }
>> }
>>-
>> }
>>
>> #endif
>>
>> #ifdef CONFIG_ARCH_OMAP3
>> /* save the registers of bank 2-6 */
>>-void omap_gpio_save_context(void)
>>+static void omap3_gpio_save_context(void)
>> {
>> int i;
>>
>>@@ -2275,7 +2304,7 @@ void omap_gpio_save_context(void)
>> }
>>
>> /* restore the required registers of bank 2-6 */
>>-void omap_gpio_restore_context(void)
>>+static void omap3_gpio_restore_context(void)
>> {
>> int i;
>>
>>diff --git a/arch/arm/plat-omap/include/plat/gpio.h
>>b/arch/arm/plat-omap/include/plat/gpio.h
>>index de1c604..c81ef94 100644
>>--- a/arch/arm/plat-omap/include/plat/gpio.h
>>+++ b/arch/arm/plat-omap/include/plat/gpio.h
>>@@ -72,12 +72,10 @@
>> IH_GPIO_BASE + (nr))
>>
>> extern int omap_gpio_init(void); /* Call from board init only */
>>-extern void omap2_gpio_prepare_for_idle(int power_state);
>>+extern void omap2_gpio_prepare_for_idle(void);
>> extern void omap2_gpio_resume_after_idle(void);
>> extern void omap_set_gpio_debounce(int gpio, int enable);
>> extern void omap_set_gpio_debounce_time(int gpio, int enable);
>>-extern void omap_gpio_save_context(void);
>>-extern void omap_gpio_restore_context(void);
>>
>>/*-------------------------------------------------------------
>>------------*/
>>
>> /* Wrappers for "new style" GPIO calls, using the new infrastructure
>>--
>>1.7.2.1
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe
>>linux-omap" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
2010-09-22 14:24 ` Kevin Hilman
@ 2010-09-22 15:09 ` Kalliguddi, Hema
0 siblings, 0 replies; 18+ messages in thread
From: Kalliguddi, Hema @ 2010-09-22 15:09 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-omap, Varadarajan, Charulatha, Basak, Partha, Tero Kristo
>-----Original Message-----
>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: Wednesday, September 22, 2010 7:55 PM
>To: Kalliguddi, Hema
>Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha;
>Basak, Partha; Tero Kristo
>Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>interrupts-disabled idle path
>
>"Kalliguddi, Hema" <hemahk@ti.com> writes:
>
>>>-----Original Message-----
>>>From: linux-omap-owner@vger.kernel.org
>>>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin Hilman
>>>Sent: Tuesday, September 14, 2010 4:33 AM
>>>To: linux-omap@vger.kernel.org
>>>Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
>>>Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
>>>interrupts-disabled idle path
>>>
>>>From: Kevin Hilman <khilman@ti.com>
>>>
>>>Currently, we wait until late in the idle path where interrupts are
>>>disabled to do runtime-PM-like management for certain special-case
>>>devices like GPIO.
>>>
>>>As a prerequiste to moving GPIO to the new runtime PM framework, move
>>>this runtime-PM-like code out of the late idle path into new device
>>>idle and resume functions that can be called before interrupts are
>>>disabled by CPUidle and/or suspend.
>>>
>>>In addition, move all the GPIO-specific logic into the GPIO core
>>>instead of keeping GPIO-specific knowledge of power-states, context
>>>saving etc. in the PM core.
>>>
>>>Also, call the new device-idle and -resume methods from CPUidle and
>>>static suspend path.
>>>
>>>Signed-off-by: Kevin Hilman <khilman@ti.com>
>>>---
>>> arch/arm/mach-omap2/cpuidle34xx.c | 4 ++
>>> arch/arm/mach-omap2/pm.h | 2 +
>>> arch/arm/mach-omap2/pm24xx.c | 2 +-
>>> arch/arm/mach-omap2/pm34xx.c | 38 +++++++++------------
>>> arch/arm/plat-omap/gpio.c | 57
>>>++++++++++++++++++++++++--------
>>> arch/arm/plat-omap/include/plat/gpio.h | 4 +--
>>> 6 files changed, 67 insertions(+), 40 deletions(-)
>>>
>>>diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>>>b/arch/arm/mach-omap2/cpuidle34xx.c
>>>index 0923b82..681d823 100644
>>>--- a/arch/arm/mach-omap2/cpuidle34xx.c
>>>+++ b/arch/arm/mach-omap2/cpuidle34xx.c
>>>@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
>>>cpuidle_device *dev,
>>> pwrdm_set_next_pwrst(per_pd, per_next_state);
>>>
>>> select_state:
>>>+ omap3_device_idle();
>>>+
>>> dev->last_state = new_state;
>>> ret = omap3_enter_idle(dev, new_state);
>>>
>>>+ omap3_device_resume();
>>>+
>>> /* Restore original PER state if it was modified */
>>> if (per_next_state != per_saved_state)
>>> pwrdm_set_next_pwrst(per_pd, per_saved_state);
>>>diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
>>>index 3de6ece..33cae4b 100644
>>>--- a/arch/arm/mach-omap2/pm.h
>>>+++ b/arch/arm/mach-omap2/pm.h
>>>@@ -22,6 +22,8 @@ extern void omap_sram_idle(void);
>>> extern int omap3_can_sleep(void);
>>> extern int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
>>> extern int omap3_idle_init(void);
>>>+extern void omap3_device_idle(void);
>>>+extern void omap3_device_resume(void);
>>>
>>> struct cpuidle_params {
>>> u8 valid;
>>>diff --git a/arch/arm/mach-omap2/pm24xx.c
>>>b/arch/arm/mach-omap2/pm24xx.c
>>>index 6aeedea..7bbf0db 100644
>>>--- a/arch/arm/mach-omap2/pm24xx.c
>>>+++ b/arch/arm/mach-omap2/pm24xx.c
>>>@@ -106,7 +106,7 @@ static void omap2_enter_full_retention(void)
>>> l = omap_ctrl_readl(OMAP2_CONTROL_DEVCONF0) |
>>>OMAP24XX_USBSTANDBYCTRL;
>>> omap_ctrl_writel(l, OMAP2_CONTROL_DEVCONF0);
>>>
>>>- omap2_gpio_prepare_for_idle(PWRDM_POWER_RET);
>>>+ omap2_gpio_prepare_for_idle();
>>>
>>> if (omap2_pm_debug) {
>>> omap2_pm_dump(0, 0, 0);
>>>diff --git a/arch/arm/mach-omap2/pm34xx.c
>>>b/arch/arm/mach-omap2/pm34xx.c
>>>index bb2ba1e..9e590d9 100644
>>>--- a/arch/arm/mach-omap2/pm34xx.c
>>>+++ b/arch/arm/mach-omap2/pm34xx.c
>>>@@ -74,16 +74,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
>>> static struct powerdomain *core_pwrdm, *per_pwrdm;
>>> static struct powerdomain *cam_pwrdm;
>>>
>>>-static inline void omap3_per_save_context(void)
>>>-{
>>>- omap_gpio_save_context();
>>>-}
>>>-
>>>-static inline void omap3_per_restore_context(void)
>>>-{
>>>- omap_gpio_restore_context();
>>>-}
>>>-
>>> static void omap3_enable_io_chain(void)
>>> {
>>> int timeout = 0;
>>>@@ -332,6 +322,16 @@ static void restore_table_entry(void)
>>> restore_control_register(control_reg_value);
>>> }
>>>
>>>+void omap3_device_idle(void)
>>>+{
>>>+ omap2_gpio_prepare_for_idle();
>>>+}
>>>+
>>
>> Is not it is a good idea to pass the new_state as the
>parameter for the driver calls?
>> It will reduce each individual drivers reading the PRCM
>register to findout the next state.
>> This might save some time in the idle loop.
>
>I chose not to pass the parameters on purpose. All of the intelligence
>for device idle (including which powerdomain state to check)
>should live
>in the driver code.
>
OK agree.
>If reading the PRCM registers is becoming a problem, it will
>be a simple
>patch to patch the powerdomain core to cache the current value of it's
>next state so at least reads of next_state will be fast.
>
This is good idea, with this we can avoid n number of PRCM register reads.
Today it may not be a problem as there are few hooks in the idle path
But if there are more number of driver calls, then it might be useful patch.
~Hema
>Kevin
>
>>>+void omap3_device_resume(void)
>>>+{
>>>+ omap2_gpio_resume_after_idle();
>>>+}
>>>+
>>
>> Here we will need to pass the next_state as well as
>previous_state as parameters. If we agree to
>> pass the parameters.
>>
>> ~Hema
>>
>>> void omap_sram_idle(void)
>>> {
>>> /* Variable to tell what needs to be saved and restored
>>>@@ -344,7 +344,7 @@ void omap_sram_idle(void)
>>> int mpu_next_state = PWRDM_POWER_ON;
>>> int per_next_state = PWRDM_POWER_ON;
>>> int core_next_state = PWRDM_POWER_ON;
>>>- int core_prev_state, per_prev_state;
>>>+ int core_prev_state;
>>> u32 sdrc_pwr = 0;
>>>
>>> if (!_omap_sram_idle)
>>>@@ -387,12 +387,9 @@ void omap_sram_idle(void)
>>> }
>>>
>>> /* PER */
>>>- if (per_next_state < PWRDM_POWER_ON) {
>>>+ if (per_next_state < PWRDM_POWER_ON)
>>> omap_uart_prepare_idle(2);
>>>- omap2_gpio_prepare_for_idle(per_next_state);
>>>- if (per_next_state == PWRDM_POWER_OFF)
>>>- omap3_per_save_context();
>>>- }
>>>+
>>>
>>> /* CORE */
>>> if (core_next_state < PWRDM_POWER_ON) {
>>>@@ -454,13 +451,8 @@ void omap_sram_idle(void)
>>> omap3_intc_resume_idle();
>>>
>>> /* PER */
>>>- if (per_next_state < PWRDM_POWER_ON) {
>>>- per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>>>- omap2_gpio_resume_after_idle();
>>>- if (per_prev_state == PWRDM_POWER_OFF)
>>>- omap3_per_restore_context();
>>>+ if (per_next_state < PWRDM_POWER_ON)
>>> omap_uart_resume_idle(2);
>>>- }
>>>
>>> /* Disable IO-PAD and IO-CHAIN wakeup */
>>> if (omap3_has_io_wakeup() &&
>>>@@ -570,6 +562,7 @@ static void omap2_pm_wakeup_on_timer(u32
>>>seconds, u32 milliseconds)
>>> static int omap3_pm_prepare(void)
>>> {
>>> disable_hlt();
>>>+ omap3_device_idle();
>>> return 0;
>>> }
>>>
>>>@@ -637,6 +630,7 @@ static int omap3_pm_enter(suspend_state_t unused)
>>>
>>> static void omap3_pm_finish(void)
>>> {
>>>+ omap3_device_resume();
>>> enable_hlt();
>>> }
>>>
>>>diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>>>index 7951eef..b0467c1 100644
>>>--- a/arch/arm/plat-omap/gpio.c
>>>+++ b/arch/arm/plat-omap/gpio.c
>>>@@ -29,6 +29,8 @@
>>> #include <asm/mach/irq.h>
>>> #include <plat/powerdomain.h>
>>>
>>>+static struct powerdomain *per_pwrdm;
>>>+
>>> /*
>>> * OMAP1510 GPIO registers
>>> */
>>>@@ -207,6 +209,9 @@ struct gpio_bank {
>>> u32 dbck_enable_mask;
>>> };
>>>
>>>+static void omap3_gpio_restore_context(void);
>>>+static void omap3_gpio_save_context(void);
>>>+
>>> #define METHOD_MPUIO 0
>>> #define METHOD_GPIO_1510 1
>>> #define METHOD_GPIO_1610 2
>>>@@ -1778,6 +1783,8 @@ static int __init _omap_gpio_init(void)
>>> }
>>> #endif
>>>
>>>+ if (cpu_class_is_omap2())
>>>+ per_pwrdm = pwrdm_lookup("per_pwrdm");
>>>
>>> #ifdef CONFIG_ARCH_OMAP15XX
>>> if (cpu_is_omap15xx()) {
>>>@@ -2074,14 +2081,22 @@ static struct sys_device omap_gpio_device = {
>>>
>>> static int workaround_enabled;
>>>
>>>-void omap2_gpio_prepare_for_idle(int power_state)
>>>+void omap2_gpio_prepare_for_idle(void)
>>> {
>>>- int i, c = 0;
>>>- int min = 0;
>>>+ int i, c = 0, min = 0;
>>>+ int per_next_state;
>>>+
>>>+ if (!per_pwrdm)
>>>+ return;
>>>+
>>>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>>>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>>>+ return;
>>>
>>> if (cpu_is_omap34xx())
>>> min = 1;
>>>
>>>+ workaround_enabled = 0;
>>> for (i = min; i < gpio_bank_count; i++) {
>>> struct gpio_bank *bank = &gpio_bank[i];
>>> u32 l1, l2;
>>>@@ -2089,7 +2104,7 @@ void omap2_gpio_prepare_for_idle(int
>power_state)
>>> if (bank->dbck_enable_mask)
>>> clk_disable(bank->dbck);
>>>
>>>- if (power_state > PWRDM_POWER_OFF)
>>>+ if (per_next_state > PWRDM_POWER_OFF)
>>> continue;
>>>
>>> /* If going to OFF, remove triggering for all
>>>@@ -2135,20 +2150,35 @@ void omap2_gpio_prepare_for_idle(int
>>>power_state)
>>>
>>> c++;
>>> }
>>>- if (!c) {
>>>- workaround_enabled = 0;
>>>- return;
>>>- }
>>>- workaround_enabled = 1;
>>>+ if (c)
>>>+ workaround_enabled = 1;
>>>+
>>>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF))
>>>+ omap3_gpio_save_context();
>>> }
>>>
>>> void omap2_gpio_resume_after_idle(void)
>>> {
>>>- int i;
>>>- int min = 0;
>>>+ int i, min = 0;
>>>+ int per_next_state;
>>>+
>>>+ if (!per_pwrdm)
>>>+ return;
>>>+
>>>+ per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>>>+ if (per_next_state >= PWRDM_POWER_INACTIVE)
>>>+ return;
>>>+
>>>+ if (cpu_is_omap34xx() && (per_next_state == PWRDM_POWER_OFF)) {
>>>+ int per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>>>+
>>>+ if (per_prev_state == PWRDM_POWER_OFF)
>>>+ omap3_gpio_restore_context();
>>>+ }
>>>
>>> if (cpu_is_omap34xx())
>>> min = 1;
>>>+
>>> for (i = min; i < gpio_bank_count; i++) {
>>> struct gpio_bank *bank = &gpio_bank[i];
>>> u32 l, gen, gen0, gen1;
>>>@@ -2235,14 +2265,13 @@ void omap2_gpio_resume_after_idle(void)
>>> }
>>> }
>>> }
>>>-
>>> }
>>>
>>> #endif
>>>
>>> #ifdef CONFIG_ARCH_OMAP3
>>> /* save the registers of bank 2-6 */
>>>-void omap_gpio_save_context(void)
>>>+static void omap3_gpio_save_context(void)
>>> {
>>> int i;
>>>
>>>@@ -2275,7 +2304,7 @@ void omap_gpio_save_context(void)
>>> }
>>>
>>> /* restore the required registers of bank 2-6 */
>>>-void omap_gpio_restore_context(void)
>>>+static void omap3_gpio_restore_context(void)
>>> {
>>> int i;
>>>
>>>diff --git a/arch/arm/plat-omap/include/plat/gpio.h
>>>b/arch/arm/plat-omap/include/plat/gpio.h
>>>index de1c604..c81ef94 100644
>>>--- a/arch/arm/plat-omap/include/plat/gpio.h
>>>+++ b/arch/arm/plat-omap/include/plat/gpio.h
>>>@@ -72,12 +72,10 @@
>>> IH_GPIO_BASE + (nr))
>>>
>>> extern int omap_gpio_init(void); /* Call from board init only */
>>>-extern void omap2_gpio_prepare_for_idle(int power_state);
>>>+extern void omap2_gpio_prepare_for_idle(void);
>>> extern void omap2_gpio_resume_after_idle(void);
>>> extern void omap_set_gpio_debounce(int gpio, int enable);
>>> extern void omap_set_gpio_debounce_time(int gpio, int enable);
>>>-extern void omap_gpio_save_context(void);
>>>-extern void omap_gpio_restore_context(void);
>>>
>>>/*-------------------------------------------------------------
>>>------------*/
>>>
>>> /* Wrappers for "new style" GPIO calls, using the new infrastructure
>>>--
>>>1.7.2.1
>>>
>>>--
>>>To unsubscribe from this list: send the line "unsubscribe
>>>linux-omap" in
>>>the body of a message to majordomo@vger.kernel.org
>>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread