* [PATCH 0/2] powernv: Implement lite variant of stop with ESL=EC=0
@ 2016-09-16 9:47 Gautham R. Shenoy
2016-09-16 9:47 ` [PATCH 1/2] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro Gautham R. Shenoy
2016-09-16 9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy
0 siblings, 2 replies; 7+ messages in thread
From: Gautham R. Shenoy @ 2016-09-16 9:47 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: Vaidyanathan Srinivasan, Michael Ellerman, Shreyas B. Prabhu,
Michael Neuling, Shilpasri G Bhat, Gautham R. Shenoy
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Hi,
The Power ISA v3.0 allows us to execute the "stop" instruction with
ESL and EC of the PSSCR set to 0. This will ensure no loss of state,
and the wakeup from the stop will happen at an instruction following
the executed stop instruction.
This patchset adds support to run stop with ESL=EC=0 based on
a flag set for the corresponding stop state in the device tree.
The first patch renames the IDLE_STATE_ENTER_SEQ macro to
IDLE_STATE_ENTER_SEQ_NORET since the current users of this
macro expect the wakeup from stop to happen at the
System Reset vector. It reuses the name IDLE_STATE_ENTER_SEQ to a
variant where the wakeup from stop happens at the next instruction.
The second patch creates adds a new function (i.e, a lite variant)
that will execute a stop instruction with ESL=EC=0 and handle wakeup
at the subsequent instruction. A particular stop state is wired to
this new function if the device tree entry for that stop state has
OPAL_PM_WAKEUP_AT_NEXT_INST [1] flag set.
[1] : The corresponding patch in skiboot that defines
OPAL_PM_WAKEUP_AT_NEXT_INST and enables it in the device tree
can be found here:
https://lists.ozlabs.org/pipermail/skiboot/2016-September/004805.html
Gautham R. Shenoy (2):
powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro
powernv:idle:Implement lite variant of power_enter_stop
arch/powerpc/include/asm/cpuidle.h | 5 ++++-
arch/powerpc/include/asm/opal-api.h | 1 +
arch/powerpc/include/asm/processor.h | 3 ++-
arch/powerpc/kernel/exceptions-64s.S | 6 +++---
arch/powerpc/kernel/idle_book3s.S | 38 +++++++++++++++++++++++++++--------
arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++---
arch/powerpc/platforms/powernv/smp.c | 2 +-
drivers/cpuidle/cpuidle-powernv.c | 24 ++++++++++++++++++++--
8 files changed, 77 insertions(+), 19 deletions(-)
--
1.9.4
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro 2016-09-16 9:47 [PATCH 0/2] powernv: Implement lite variant of stop with ESL=EC=0 Gautham R. Shenoy @ 2016-09-16 9:47 ` Gautham R. Shenoy 2016-09-16 9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy 1 sibling, 0 replies; 7+ messages in thread From: Gautham R. Shenoy @ 2016-09-16 9:47 UTC (permalink / raw) To: linuxppc-dev, linux-kernel Cc: Vaidyanathan Srinivasan, Michael Ellerman, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat, Gautham R. Shenoy From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> Currently all the low-power idle states are expected to wake up at reset vector 0x100. Which is why the macro IDLE_STATE_ENTER_SEQ that puts the CPU to an idle state and never returns. On ISA_300, when the ESL and EC bits in the PSSCR are zero, the CPU is expected to wake up at the next instruction of the idle instruction. This patch adds a new macro named IDLE_STATE_ENTER_SEQ_NORET for the no-return variant and reuses the name IDLE_STATE_ENTER_SEQ for a variant that allows resuming operation at the instruction next to the idle-instruction. Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> --- arch/powerpc/include/asm/cpuidle.h | 5 ++++- arch/powerpc/kernel/exceptions-64s.S | 6 +++--- arch/powerpc/kernel/idle_book3s.S | 10 +++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h index 01b8a13..9fd23f6 100644 --- a/arch/powerpc/include/asm/cpuidle.h +++ b/arch/powerpc/include/asm/cpuidle.h @@ -21,7 +21,7 @@ extern u64 pnv_first_deep_stop_state; /* Idle state entry routines */ #ifdef CONFIG_PPC_P7_NAP -#define IDLE_STATE_ENTER_SEQ(IDLE_INST) \ +#define IDLE_STATE_ENTER_SEQ(IDLE_INST) \ /* Magic NAP/SLEEP/WINKLE mode enter sequence */ \ std r0,0(r1); \ ptesync; \ @@ -29,6 +29,9 @@ extern u64 pnv_first_deep_stop_state; 1: cmp cr0,r0,r0; \ bne 1b; \ IDLE_INST; \ + +#define IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST) \ + IDLE_STATE_ENTER_SEQ(IDLE_INST) \ b . #endif /* CONFIG_PPC_P7_NAP */ diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index bffec73..238307d 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -1301,12 +1301,12 @@ machine_check_handle_early: lbz r3,PACA_THREAD_IDLE_STATE(r13) cmpwi r3,PNV_THREAD_NAP bgt 10f - IDLE_STATE_ENTER_SEQ(PPC_NAP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_NAP) /* No return */ 10: cmpwi r3,PNV_THREAD_SLEEP bgt 2f - IDLE_STATE_ENTER_SEQ(PPC_SLEEP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP) /* No return */ 2: @@ -1320,7 +1320,7 @@ machine_check_handle_early: */ ori r13,r13,1 SET_PACA(r13) - IDLE_STATE_ENTER_SEQ(PPC_WINKLE) + IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE) /* No return */ 4: #endif diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index bd739fe..32d666b 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -188,7 +188,7 @@ pnv_enter_arch207_idle_mode: stb r3,PACA_THREAD_IDLE_STATE(r13) cmpwi cr3,r3,PNV_THREAD_SLEEP bge cr3,2f - IDLE_STATE_ENTER_SEQ(PPC_NAP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_NAP) /* No return */ 2: /* Sleep or winkle */ @@ -222,7 +222,7 @@ pnv_fastsleep_workaround_at_entry: common_enter: /* common code for all the threads entering sleep or winkle */ bgt cr3,enter_winkle - IDLE_STATE_ENTER_SEQ(PPC_SLEEP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP) fastsleep_workaround_at_entry: ori r15,r15,PNV_CORE_IDLE_LOCK_BIT @@ -244,7 +244,7 @@ fastsleep_workaround_at_entry: enter_winkle: bl save_sprs_to_stack - IDLE_STATE_ENTER_SEQ(PPC_WINKLE) + IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE) /* * r3 - requested stop state @@ -257,7 +257,7 @@ power_enter_stop: ld r4,ADDROFF(pnv_first_deep_stop_state)(r5) cmpd r3,r4 bge 2f - IDLE_STATE_ENTER_SEQ(PPC_STOP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_STOP) 2: /* * Entering deep idle state. @@ -279,7 +279,7 @@ lwarx_loop_stop: bl save_sprs_to_stack - IDLE_STATE_ENTER_SEQ(PPC_STOP) + IDLE_STATE_ENTER_SEQ_NORET(PPC_STOP) _GLOBAL(power7_idle) /* Now check if user or arch enabled NAP mode */ -- 1.9.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop 2016-09-16 9:47 [PATCH 0/2] powernv: Implement lite variant of stop with ESL=EC=0 Gautham R. Shenoy 2016-09-16 9:47 ` [PATCH 1/2] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro Gautham R. Shenoy @ 2016-09-16 9:47 ` Gautham R. Shenoy 2016-09-20 5:54 ` Balbir Singh 2016-09-23 11:03 ` Michael Ellerman 1 sibling, 2 replies; 7+ messages in thread From: Gautham R. Shenoy @ 2016-09-16 9:47 UTC (permalink / raw) To: linuxppc-dev, linux-kernel Cc: Vaidyanathan Srinivasan, Michael Ellerman, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat, Gautham R. Shenoy From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> This patch adds a function named power_enter_stop_lite() that can execute a stop instruction when ESL and EC bits are set to zero in the PSSCR. The function handles the wake-up from idle at the instruction immediately after the stop instruction. If the flag OPAL_PM_WAKEUP_AT_NEXT_INST[1] is set in the device tree for a stop state, then use the lite variant for that particular stop state. [1] : The corresponding patch in skiboot that defines OPAL_PM_WAKEUP_AT_NEXT_INST and enables it in the device tree can be found here: https://lists.ozlabs.org/pipermail/skiboot/2016-September/004805.html Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> --- arch/powerpc/include/asm/opal-api.h | 1 + arch/powerpc/include/asm/processor.h | 3 ++- arch/powerpc/kernel/idle_book3s.S | 28 +++++++++++++++++++++++++--- arch/powerpc/platforms/powernv/idle.c | 17 ++++++++++++++--- arch/powerpc/platforms/powernv/smp.c | 2 +- drivers/cpuidle/cpuidle-powernv.c | 24 ++++++++++++++++++++++-- 6 files changed, 65 insertions(+), 10 deletions(-) diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h index 0e2e57b..6e5741e 100644 --- a/arch/powerpc/include/asm/opal-api.h +++ b/arch/powerpc/include/asm/opal-api.h @@ -179,6 +179,7 @@ #define OPAL_PM_TIMEBASE_STOP 0x00000002 #define OPAL_PM_LOSE_HYP_CONTEXT 0x00002000 #define OPAL_PM_LOSE_FULL_CONTEXT 0x00004000 +#define OPAL_PM_WAKEUP_AT_NEXT_INST 0x00008000 #define OPAL_PM_NAP_ENABLED 0x00010000 #define OPAL_PM_SLEEP_ENABLED 0x00020000 #define OPAL_PM_WINKLE_ENABLED 0x00040000 diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h index 68e3bf5..e0549a0 100644 --- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h @@ -460,7 +460,8 @@ extern int powersave_nap; /* set if nap mode can be used in idle loop */ extern unsigned long power7_nap(int check_irq); extern unsigned long power7_sleep(void); extern unsigned long power7_winkle(void); -extern unsigned long power9_idle_stop(unsigned long stop_level); +extern unsigned long power9_idle_stop(unsigned long stop_level, + unsigned long exec_lite); extern void flush_instruction_cache(void); extern void hard_reset_now(void); diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index 32d666b..47ee106 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -43,6 +43,8 @@ #define PSSCR_HV_TEMPLATE PSSCR_ESL | PSSCR_EC | \ PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ PSSCR_MTL_MASK +#define PSSCR_HV_TEMPLATE_LITE PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ + PSSCR_MTL_MASK .text @@ -246,6 +248,20 @@ enter_winkle: IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE) + +/* + * power_enter_stop_lite : This will resume the wake up from + * idle at the subsequent instruction. + * + * Caller should set ESL=EC=0 in PSSCR before calling + * this function. + * + */ +power_enter_stop_lite: + IDLE_STATE_ENTER_SEQ(PPC_STOP) +7: li r3,0 /* Since we didn't lose state, return 0 */ + b pnv_wakeup_noloss + /* * r3 - requested stop state */ @@ -333,13 +349,19 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ /* * r3 - requested stop state + * r4 - Indicates if the lite variant with ESL=EC=0 should be executed. */ _GLOBAL(power9_idle_stop) - LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) - or r4,r4,r3 + cmpdi r4, 1 + bne 4f + LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE_LITE) + LOAD_REG_ADDR(r5,power_enter_stop_lite) + b 5f +4: LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) + LOAD_REG_ADDR(r5,power_enter_stop) +5: or r4,r4,r3 mtspr SPRN_PSSCR, r4 li r4, 1 - LOAD_REG_ADDR(r5,power_enter_stop) b pnv_powersave_common /* No return */ /* diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 479c256..c3d3fed 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -244,8 +244,15 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600, static void power9_idle(void) { /* Requesting stop state 0 */ - power9_idle_stop(0); + power9_idle_stop(0, 0); } + +static void power9_idle_lite(void) +{ + /* Requesting stop state 0 with ESL=EC=0 */ + power9_idle_stop(0, 1); +} + /* * First deep stop state. Used to figure out when to save/restore * hypervisor context. @@ -414,8 +421,12 @@ static int __init pnv_init_idle_states(void) if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED) ppc_md.power_save = power7_idle; - else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) - ppc_md.power_save = power9_idle; + else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) { + if (supported_cpuidle_states & OPAL_PM_WAKEUP_AT_NEXT_INST) + ppc_md.power_save = power9_idle_lite; + else + ppc_md.power_save = power9_idle; + } out: return 0; diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c index c789258..6587b96 100644 --- a/arch/powerpc/platforms/powernv/smp.c +++ b/arch/powerpc/platforms/powernv/smp.c @@ -183,7 +183,7 @@ static void pnv_smp_cpu_kill_self(void) ppc64_runlatch_off(); if (cpu_has_feature(CPU_FTR_ARCH_300)) - srr1 = power9_idle_stop(pnv_deepest_stop_state); + srr1 = power9_idle_stop(pnv_deepest_stop_state, 0); else if (idle_states & OPAL_PM_WINKLE_ENABLED) srr1 = power7_winkle(); else if ((idle_states & OPAL_PM_SLEEP_ENABLED) || diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index f7ca891..7021ddf 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -102,7 +102,21 @@ static int stop_loop(struct cpuidle_device *dev, int index) { ppc64_runlatch_off(); - power9_idle_stop(stop_psscr_table[index]); + power9_idle_stop(stop_psscr_table[index], 0); + ppc64_runlatch_on(); + return index; +} + +/* + * This function calls stop instruction with + * ESL and EC bits set to 0. + */ +static int stop_lite_loop(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + ppc64_runlatch_off(); + power9_idle_stop(stop_psscr_table[index], 1); ppc64_runlatch_on(); return index; } @@ -273,7 +287,13 @@ static int powernv_add_idle_states(void) names[i], CPUIDLE_NAME_LEN); powernv_states[nr_idle_states].flags = 0; - powernv_states[nr_idle_states].enter = stop_loop; + if (flags[i] & OPAL_PM_WAKEUP_AT_NEXT_INST) { + powernv_states[nr_idle_states].enter = + stop_lite_loop; + } else { + powernv_states[nr_idle_states].enter = + stop_loop; + } stop_psscr_table[nr_idle_states] = psscr_val[i]; } -- 1.9.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop 2016-09-16 9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy @ 2016-09-20 5:54 ` Balbir Singh 2016-09-20 8:32 ` Gautham R Shenoy 2016-09-23 11:03 ` Michael Ellerman 1 sibling, 1 reply; 7+ messages in thread From: Balbir Singh @ 2016-09-20 5:54 UTC (permalink / raw) To: Gautham R. Shenoy, linuxppc-dev, linux-kernel Cc: Vaidyanathan Srinivasan, Michael Ellerman, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat On 16/09/16 19:47, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > This patch adds a function named power_enter_stop_lite() that can > execute a stop instruction when ESL and EC bits are set to zero in the > PSSCR. The function handles the wake-up from idle at the instruction > immediately after the stop instruction. > > If the flag OPAL_PM_WAKEUP_AT_NEXT_INST[1] is set in the device tree > for a stop state, then use the lite variant for that particular stop > state. > > [1] : The corresponding patch in skiboot that defines > OPAL_PM_WAKEUP_AT_NEXT_INST and enables it in the device tree > can be found here: > https://lists.ozlabs.org/pipermail/skiboot/2016-September/004805.html > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/opal-api.h | 1 + > arch/powerpc/include/asm/processor.h | 3 ++- > arch/powerpc/kernel/idle_book3s.S | 28 +++++++++++++++++++++++++--- > arch/powerpc/platforms/powernv/idle.c | 17 ++++++++++++++--- > arch/powerpc/platforms/powernv/smp.c | 2 +- > drivers/cpuidle/cpuidle-powernv.c | 24 ++++++++++++++++++++++-- > 6 files changed, 65 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h > index 0e2e57b..6e5741e 100644 > --- a/arch/powerpc/include/asm/opal-api.h > +++ b/arch/powerpc/include/asm/opal-api.h > @@ -179,6 +179,7 @@ > #define OPAL_PM_TIMEBASE_STOP 0x00000002 > #define OPAL_PM_LOSE_HYP_CONTEXT 0x00002000 > #define OPAL_PM_LOSE_FULL_CONTEXT 0x00004000 > +#define OPAL_PM_WAKEUP_AT_NEXT_INST 0x00008000 > #define OPAL_PM_NAP_ENABLED 0x00010000 > #define OPAL_PM_SLEEP_ENABLED 0x00020000 > #define OPAL_PM_WINKLE_ENABLED 0x00040000 > diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h > index 68e3bf5..e0549a0 100644 > --- a/arch/powerpc/include/asm/processor.h > +++ b/arch/powerpc/include/asm/processor.h > @@ -460,7 +460,8 @@ extern int powersave_nap; /* set if nap mode can be used in idle loop */ > extern unsigned long power7_nap(int check_irq); > extern unsigned long power7_sleep(void); > extern unsigned long power7_winkle(void); > -extern unsigned long power9_idle_stop(unsigned long stop_level); > +extern unsigned long power9_idle_stop(unsigned long stop_level, > + unsigned long exec_lite); > > extern void flush_instruction_cache(void); > extern void hard_reset_now(void); > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > index 32d666b..47ee106 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -43,6 +43,8 @@ > #define PSSCR_HV_TEMPLATE PSSCR_ESL | PSSCR_EC | \ > PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > PSSCR_MTL_MASK > +#define PSSCR_HV_TEMPLATE_LITE PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > + PSSCR_MTL_MASK > > .text > > @@ -246,6 +248,20 @@ enter_winkle: > > IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE) > > + > +/* > + * power_enter_stop_lite : This will resume the wake up from > + * idle at the subsequent instruction. > + * > + * Caller should set ESL=EC=0 in PSSCR before calling > + * this function. > + * > + */ > +power_enter_stop_lite: > + IDLE_STATE_ENTER_SEQ(PPC_STOP) > +7: li r3,0 /* Since we didn't lose state, return 0 */ > + b pnv_wakeup_noloss > + > /* > * r3 - requested stop state > */ > @@ -333,13 +349,19 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ > > /* > * r3 - requested stop state > + * r4 - Indicates if the lite variant with ESL=EC=0 should be executed. > */ > _GLOBAL(power9_idle_stop) > - LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > - or r4,r4,r3 > + cmpdi r4, 1 > + bne 4f > + LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE_LITE) > + LOAD_REG_ADDR(r5,power_enter_stop_lite) > + b 5f > +4: LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > + LOAD_REG_ADDR(r5,power_enter_stop) > +5: or r4,r4,r3 > mtspr SPRN_PSSCR, r4 > li r4, 1 > - LOAD_REG_ADDR(r5,power_enter_stop) > b pnv_powersave_common > /* No return */ > /* > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > index 479c256..c3d3fed 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -244,8 +244,15 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600, > static void power9_idle(void) > { > /* Requesting stop state 0 */ > - power9_idle_stop(0); > + power9_idle_stop(0, 0); > } > + > +static void power9_idle_lite(void) > +{ > + /* Requesting stop state 0 with ESL=EC=0 */ > + power9_idle_stop(0, 1); > +} > + > /* > * First deep stop state. Used to figure out when to save/restore > * hypervisor context. > @@ -414,8 +421,12 @@ static int __init pnv_init_idle_states(void) > > if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED) > ppc_md.power_save = power7_idle; > - else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) > - ppc_md.power_save = power9_idle; > + else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) { > + if (supported_cpuidle_states & OPAL_PM_WAKEUP_AT_NEXT_INST) > + ppc_md.power_save = power9_idle_lite; > + else > + ppc_md.power_save = power9_idle; > + } If I am reading this correctly we decide at boot time whether we support wakeup at next instruction and make that the default sleep state. I am a little surprised that these are exclusive. I was expecting power9_idle_lite to be one of the states to go into before power9_idle Balbir Singh. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop 2016-09-20 5:54 ` Balbir Singh @ 2016-09-20 8:32 ` Gautham R Shenoy 0 siblings, 0 replies; 7+ messages in thread From: Gautham R Shenoy @ 2016-09-20 8:32 UTC (permalink / raw) To: Balbir Singh Cc: Gautham R. Shenoy, linuxppc-dev, linux-kernel, Vaidyanathan Srinivasan, Michael Ellerman, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat Hi Balbir, On Tue, Sep 20, 2016 at 03:54:43PM +1000, Balbir Singh wrote: > > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > > index 479c256..c3d3fed 100644 > > --- a/arch/powerpc/platforms/powernv/idle.c > > +++ b/arch/powerpc/platforms/powernv/idle.c > > @@ -244,8 +244,15 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600, > > static void power9_idle(void) > > { > > /* Requesting stop state 0 */ > > - power9_idle_stop(0); > > + power9_idle_stop(0, 0); > > } > > + > > +static void power9_idle_lite(void) > > +{ > > + /* Requesting stop state 0 with ESL=EC=0 */ > > + power9_idle_stop(0, 1); > > +} > > + > > /* > > * First deep stop state. Used to figure out when to save/restore > > * hypervisor context. > > @@ -414,8 +421,12 @@ static int __init pnv_init_idle_states(void) > > > > if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED) > > ppc_md.power_save = power7_idle; > > - else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) > > - ppc_md.power_save = power9_idle; > > + else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) { > > + if (supported_cpuidle_states & OPAL_PM_WAKEUP_AT_NEXT_INST) > > + ppc_md.power_save = power9_idle_lite; > > + else > > + ppc_md.power_save = power9_idle; > > + } > > If I am reading this correctly we decide at boot time whether we support > wakeup at next instruction and make that the default sleep state. > I am a little surprised that these are exclusive. I was expecting > power9_idle_lite to be one of the states to go into before > power9_idle At boot time, we initialize ppc_md.power_save to power9_idle/power9_idle_lite which ends up being the default idle function in the absence of the cpuidle subsystem. When cpuidle is available, idle code will call cpuidle governors which will determine the appropriate idle state that can be entered into. Each of these idle states has an associated callback function. In case of the idle-states without OPAL_PM_STOP_INST_FAST associated with them, the callback is stop_loop() and when the flag is set, the callback function is stop_lite_loop(). So when cpuidle is present, these states are not exclusive. Note that both power9_idle() and power9_idle_lite() call stop0. Just that the former executes stop0 with ESL=EC=1 and latter with ESL=EC=0. That said, you're right that in the absence of the cpuidle governor, if the lite variant of stop is advertised by the firmware through the device-tree, we end up picking the liter version of stop0 as the default idle state. Do you suggest that we retain power9_idle which calls stop0 with ESL=EC=1 ? > Balbir Singh. > -- Thanks and Regards gautham. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop 2016-09-16 9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy 2016-09-20 5:54 ` Balbir Singh @ 2016-09-23 11:03 ` Michael Ellerman 2016-09-23 13:48 ` Gautham R Shenoy 1 sibling, 1 reply; 7+ messages in thread From: Michael Ellerman @ 2016-09-23 11:03 UTC (permalink / raw) To: Gautham R. Shenoy, linuxppc-dev, linux-kernel Cc: Vaidyanathan Srinivasan, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat, Gautham R. Shenoy "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > This patch adds a function named power_enter_stop_lite() that can > execute a stop instruction when ESL and EC bits are set to zero in the > PSSCR. The function handles the wake-up from idle at the instruction > immediately after the stop instruction. > > If the flag OPAL_PM_WAKEUP_AT_NEXT_INST[1] is set in the device tree > for a stop state, then use the lite variant for that particular stop > state. Hi Gautham, It seems to me that this would be cleaner if it was modelled as a new stop state? Surely it must have different power saving characteristics to the existing state? > [1] : The corresponding patch in skiboot that defines > OPAL_PM_WAKEUP_AT_NEXT_INST and enables it in the device tree > can be found here: > https://lists.ozlabs.org/pipermail/skiboot/2016-September/004805.html Which says "This will reduce the exit latency of the idle state", and yet it doesn't change any of the latency/residency values? If all it does is reduce the exit latency, then shouldn't we always use it? Presumably it also saves less power? > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > index 32d666b..47ee106 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -43,6 +43,8 @@ > #define PSSCR_HV_TEMPLATE PSSCR_ESL | PSSCR_EC | \ > PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > PSSCR_MTL_MASK > +#define PSSCR_HV_TEMPLATE_LITE PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > + PSSCR_MTL_MASK Why do we have these at all? Firmware tells us the PSSCR values to use in the "ibm,cpu-idle-state-psscr" property. If we just used what firmware gave us then we wouldn't need the above, or the juggling below. > @@ -333,13 +349,19 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ > > /* > * r3 - requested stop state > + * r4 - Indicates if the lite variant with ESL=EC=0 should be executed. > */ > _GLOBAL(power9_idle_stop) > - LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > - or r4,r4,r3 > + cmpdi r4, 1 > + bne 4f > + LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE_LITE) > + LOAD_REG_ADDR(r5,power_enter_stop_lite) > + b 5f > +4: LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > + LOAD_REG_ADDR(r5,power_enter_stop) > +5: or r4,r4,r3 > mtspr SPRN_PSSCR, r4 > li r4, 1 > - LOAD_REG_ADDR(r5,power_enter_stop) > b pnv_powersave_common > /* No return */ > /* > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > index 479c256..c3d3fed 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -244,8 +244,15 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600, > static void power9_idle(void) > { > /* Requesting stop state 0 */ > - power9_idle_stop(0); > } That seems like the root of the problem, why aren't we passing a PSSCR value here? I also don't see us using the psscr-mask property anywhere. Why isn't that a bug? cheers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop 2016-09-23 11:03 ` Michael Ellerman @ 2016-09-23 13:48 ` Gautham R Shenoy 0 siblings, 0 replies; 7+ messages in thread From: Gautham R Shenoy @ 2016-09-23 13:48 UTC (permalink / raw) To: Michael Ellerman Cc: Gautham R. Shenoy, linuxppc-dev, linux-kernel, Vaidyanathan Srinivasan, Shreyas B. Prabhu, Michael Neuling, Shilpasri G Bhat Hi Michael, On Fri, Sep 23, 2016 at 09:03:45PM +1000, Michael Ellerman wrote: > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes: > > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > This patch adds a function named power_enter_stop_lite() that can > > execute a stop instruction when ESL and EC bits are set to zero in the > > PSSCR. The function handles the wake-up from idle at the instruction > > immediately after the stop instruction. > > > > If the flag OPAL_PM_WAKEUP_AT_NEXT_INST[1] is set in the device tree > > for a stop state, then use the lite variant for that particular stop > > state. > > Hi Gautham, > > It seems to me that this would be cleaner if it was modelled as a new > stop state? Surely it must have different power saving characteristics > to the existing state? It is supposed to be a new stop state, whose behaviour is different from the existing stop states in terms of where it wakes up from stop. You are right, it has different power saving characteristics to the existing state. > > > [1] : The corresponding patch in skiboot that defines > > OPAL_PM_WAKEUP_AT_NEXT_INST and enables it in the device tree > > can be found here: > > https://lists.ozlabs.org/pipermail/skiboot/2016-September/004805.html > > Which says "This will reduce the exit latency of the idle state", and > yet it doesn't change any of the latency/residency values? > > If all it does is reduce the exit latency, then shouldn't we always use > it? Presumably it also saves less power? It does save less power compared to the corresponding variant where ESL=EC=1. > > > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > > index 32d666b..47ee106 100644 > > --- a/arch/powerpc/kernel/idle_book3s.S > > +++ b/arch/powerpc/kernel/idle_book3s.S > > @@ -43,6 +43,8 @@ > > #define PSSCR_HV_TEMPLATE PSSCR_ESL | PSSCR_EC | \ > > PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > > PSSCR_MTL_MASK > > +#define PSSCR_HV_TEMPLATE_LITE PSSCR_PSLL_MASK | PSSCR_TR_MASK | \ > > + PSSCR_MTL_MASK > > Why do we have these at all? Firmware tells us the PSSCR values to use > in the "ibm,cpu-idle-state-psscr" property. > > If we just used what firmware gave us then we wouldn't need the above, > or the juggling below. Let me rework the patch to use the cpu-idle-state-psscr property which is currently set to 0 in the firmware and cpu-idle-state-psscr-mask which is set to 0xF for all the stop states. I agree, we can do without the hardcoding of the mask in the kernel. > > > @@ -333,13 +349,19 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ > > > > /* > > * r3 - requested stop state > > + * r4 - Indicates if the lite variant with ESL=EC=0 should be executed. > > */ > > _GLOBAL(power9_idle_stop) > > - LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > > - or r4,r4,r3 > > + cmpdi r4, 1 > > + bne 4f > > + LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE_LITE) > > + LOAD_REG_ADDR(r5,power_enter_stop_lite) > > + b 5f > > +4: LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE) > > + LOAD_REG_ADDR(r5,power_enter_stop) > > +5: or r4,r4,r3 > > mtspr SPRN_PSSCR, r4 > > li r4, 1 > > - LOAD_REG_ADDR(r5,power_enter_stop) > > b pnv_powersave_common > > /* No return */ > > /* > > > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > > index 479c256..c3d3fed 100644 > > --- a/arch/powerpc/platforms/powernv/idle.c > > +++ b/arch/powerpc/platforms/powernv/idle.c > > @@ -244,8 +244,15 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600, > > static void power9_idle(void) > > { > > /* Requesting stop state 0 */ > > - power9_idle_stop(0); > > } > > That seems like the root of the problem, why aren't we passing a PSSCR > value here? > > I also don't see us using the psscr-mask property anywhere. Why isn't > that a bug? Because we are only setting the Requested Level field which is the bottom 4 bits. Everything else is taken from the hardcoded mask, which is not incorrect, but not a very flexible design. Thanks for the comments Michael. I will come back with a cleaner patch. > > cheers > -- Thanks and Regards gautham. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-23 13:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-16 9:47 [PATCH 0/2] powernv: Implement lite variant of stop with ESL=EC=0 Gautham R. Shenoy 2016-09-16 9:47 ` [PATCH 1/2] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro Gautham R. Shenoy 2016-09-16 9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy 2016-09-20 5:54 ` Balbir Singh 2016-09-20 8:32 ` Gautham R Shenoy 2016-09-23 11:03 ` Michael Ellerman 2016-09-23 13:48 ` Gautham R Shenoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).