Re: [PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Gautham R Shenoy]

Hi Stewart,

On Wed, Oct 12, 2016 at 04:35:35PM +1100, Stewart Smith wrote:
> 
> What if we just treat the 0xF state from firmware as special and set it
> to DEFAULT_PSSCR_MASK in that case? That deals with old skiboot, new
> kernel, and sets a pretty small special case that's easy to track into
> the future as something we should watch out for.

Yes, that will work. I will use this approach in the next version.

> 
> Additionally, if we make skiboot set sane values in ~DEFAULT_PSSCR_MASK
> for valid fields in PSSCR on boot/(also kexec?), then
> we should end up in a situation where everything works with everything
> (even if you don't get the best power saving). Specifically, new
> skiboot, old kernel... but it looks like there's nothing currently
> missing there

No we're not missing much there. From the code of the old kernel, I
see that if we pass the psscr_val instead of just the RL to
power9_idle_stop(), we end up saving all the SPRs in
power_enter_stop() even if the psscr_val didn't correspond to a deep
idle state.

But that should be ok.

> 
> Should this patch also have Fixes: 3005c597ba4 and CC to stable?

Yes, thanks for pointing it out. I shall mark the next version to
stable.

> 
> -- 
> Stewart Smith
> OPAL Architect, IBM.

--
Thanks and Regards
gautham.

Re: [PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Stewart Smith]

Gautham R Shenoy  writes:
> On Tue, Oct 04, 2016 at 10:33:27PM +1100, Balbir Singh wrote:
>> 
>> 
>> On 04/10/16 21:32, Michael Ellerman wrote:
>> > "Gautham R. Shenoy"  writes:
>> > 
>> >> From: "Gautham R. Shenoy" 
>> >>
>> >> The power9_idle_stop method currently takes only the requested stop
>> >> level as a parameter and picks up the rest of the PSSCR bits from a
>> >> hand-coded macro. This is not a very flexible design, especially when
>> >> the firmware has the capability to communicate the psscr value and the
>> >> mask associated with a particular stop state via device tree.
>> >>
>> >> This patch modifies the power9_idle_stop API to take as parameters the
>> >> PSSCR value and the PSSCR mask corresponding to the stop state that
>> >> needs to be set. These PSSCR value and mask are respectively obtained
>> >> by parsing the "ibm,cpu-idle-state-psscr" and
>> >> "ibm,cpu-idle-state-psscr-mask" fields from the device tree.
>> >>
>> >> In addition to this, the patch adds support for handling stop states
>> >> for which ESL and EC bits in the PSSCR are zero. As per the
>> >> architecture, a wakeup from these stop states resumes execution from
>> >> the subsequent instruction as opposed to waking up at the System
>> >> Vector.
>> > 
>> > That looks good.
>> > 
>> >> This patch depends on the following skiboot patch that exports the
>> >> PSSCR values and the mask for all the stop states:
>> >> https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html
>> > 
>> > But we can't depend on a skiboot patch. The kernel has to cope with
>> > running on an old skiboot.
>>
>
> Hmm.. We can still do that. The older skiboot only provides the RL
> field of the PSSCR value for each stop state and the corresponding
> PSSCR mask is set to 0xF in the older skiboot for all the stop states.
>
> We can insist that the future skiboot sets the ESL, EC, PSLL, TR, MTL
> and the the RL fields of the PSSCR for any exported stop state. This
> should be reflected in the psscr_mask of that stop state.  Thus, the
> psscr_mask of any stop state proposed in the future will have:
> (PSSCR_ESL_MASK | PSCCR_EC_MASK | PSCCR_PSLL_MASK | PSSCR_TR_MASK |
> PSSCR_MTL_MASK | PSSCR_RL_MASK) bits set in the skiboot.
>
> To handle the older firmware, we can do something like the following
> during the discovery of the stop states to mimic the behaviour present
> in the 4.8 kernel running on older firmware.
>
> === drivers/cpuidle/cpuidle-powernv.c ===
> /*
>  * By default we set the ESL and EC bits in the PSSCR.
>  * The MTL and PSLL are set to the maximum value possible as per the
>  * ISA, i.e 15.
>  * The Transition Rate is set to the Maximum value 3.
>  */
> #define DEFAULT_PSSCR_VAL  PSSCR_ESL_MASK |   \
>  PSCCR_EC_MASK | PSCCR_PSLL_MASK |\
>  PSSCR_TR_MASK | PSSCR_MTL_MASK
>
> #define DEFAULT_PSSCR_MASK PSSCR_ESL_MASK |   \
>  PSCCR_EC_MASK | PSCCR_PSLL_MASK |\
>  PSSCR_TR_MASK | PSSCR_MTL_MASK | \
>  PSSCR_RL_MASK
>
>
> static int powernv_add_idle_states(void)
> {
>   .
>   .
>   .
>   for (i = 0; i < dt_idle_states; i++) {
>   u64 val, mask;
>   .   
>   .
>   .
>   val = (DEFAULT_PSSCR_VAL & ~psscr_mask[i]) | psscr_val[i];
>   mask = DEFAULT_PSSCR_MASK | psscr_mask[i];
>   stop_psscr_table[nr_idle_states].val = val;
>   stop_psscr_table[nr_idle_states].mask = mask;
>   }
> } 
> 
>
>
> Is this approach ok ?

What if we just treat the 0xF state from firmware as special and set it
to DEFAULT_PSSCR_MASK in that case? That deals with old skiboot, new
kernel, and sets a pretty small special case that's easy to track into
the future as something we should watch out for.

Additionally, if we make skiboot set sane values in ~DEFAULT_PSSCR_MASK
for valid fields in PSSCR on boot/(also kexec?), then
we should end up in a situation where everything works with everything
(even if you don't get the best power saving). Specifically, new
skiboot, old kernel... but it looks like there's nothing currently
missing there

Should this patch also have Fixes: 3005c597ba4 and CC to stable?

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Gautham R Shenoy]

Hi Balbir, Michael,

On Tue, Oct 04, 2016 at 10:33:27PM +1100, Balbir Singh wrote:
> 
> 
> On 04/10/16 21:32, Michael Ellerman wrote:
> > "Gautham R. Shenoy"  writes:
> > 
> >> From: "Gautham R. Shenoy" 
> >>
> >> The power9_idle_stop method currently takes only the requested stop
> >> level as a parameter and picks up the rest of the PSSCR bits from a
> >> hand-coded macro. This is not a very flexible design, especially when
> >> the firmware has the capability to communicate the psscr value and the
> >> mask associated with a particular stop state via device tree.
> >>
> >> This patch modifies the power9_idle_stop API to take as parameters the
> >> PSSCR value and the PSSCR mask corresponding to the stop state that
> >> needs to be set. These PSSCR value and mask are respectively obtained
> >> by parsing the "ibm,cpu-idle-state-psscr" and
> >> "ibm,cpu-idle-state-psscr-mask" fields from the device tree.
> >>
> >> In addition to this, the patch adds support for handling stop states
> >> for which ESL and EC bits in the PSSCR are zero. As per the
> >> architecture, a wakeup from these stop states resumes execution from
> >> the subsequent instruction as opposed to waking up at the System
> >> Vector.
> > 
> > That looks good.
> > 
> >> This patch depends on the following skiboot patch that exports the
> >> PSSCR values and the mask for all the stop states:
> >> https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html
> > 
> > But we can't depend on a skiboot patch. The kernel has to cope with
> > running on an old skiboot.
>

Hmm.. We can still do that. The older skiboot only provides the RL
field of the PSSCR value for each stop state and the corresponding
PSSCR mask is set to 0xF in the older skiboot for all the stop states.

We can insist that the future skiboot sets the ESL, EC, PSLL, TR, MTL
and the the RL fields of the PSSCR for any exported stop state. This
should be reflected in the psscr_mask of that stop state.  Thus, the
psscr_mask of any stop state proposed in the future will have:
(PSSCR_ESL_MASK | PSCCR_EC_MASK | PSCCR_PSLL_MASK | PSSCR_TR_MASK |
PSSCR_MTL_MASK | PSSCR_RL_MASK) bits set in the skiboot.

To handle the older firmware, we can do something like the following
during the discovery of the stop states to mimic the behaviour present
in the 4.8 kernel running on older firmware.

=== drivers/cpuidle/cpuidle-powernv.c ===
/*
 * By default we set the ESL and EC bits in the PSSCR.
 * The MTL and PSLL are set to the maximum value possible as per the
 * ISA, i.e 15.
 * The Transition Rate is set to the Maximum value 3.
 */
#define DEFAULT_PSSCR_VAL  PSSCR_ESL_MASK | \
   PSCCR_EC_MASK | PSCCR_PSLL_MASK |\
   PSSCR_TR_MASK | PSSCR_MTL_MASK

#define DEFAULT_PSSCR_MASK PSSCR_ESL_MASK | \
   PSCCR_EC_MASK | PSCCR_PSLL_MASK |\
   PSSCR_TR_MASK | PSSCR_MTL_MASK | \
   PSSCR_RL_MASK


static int powernv_add_idle_states(void)
{
.
.
.
for (i = 0; i < dt_idle_states; i++) {
u64 val, mask;
.   
.
.
val = (DEFAULT_PSSCR_VAL & ~psscr_mask[i]) | psscr_val[i];
mask = DEFAULT_PSSCR_MASK | psscr_mask[i];
stop_psscr_table[nr_idle_states].val = val;
stop_psscr_table[nr_idle_states].mask = mask;
}
}   



Is this approach ok ?


> Hi, Michael
> 
> I think with an older skiboot the flags don't get exported and the
> new cpuidle (stop state) does not get discovered. I don't think there
> is any breakage. Gautham am I missing something?

In the second verison of the patch, I am not adding any new flags in
the skiboot, but explicitly providing the psscr_value and psscr_mask
for each stop state.  Thus if you see the v2 of the skiboot patch, the
lite versions of stop0,stop1 and stop2 are defined with no additional
flags.

The second version of the kernel patch wouldn't then work with the
older skiboot since the older skiboot exports only the RL field of the
psscr_val while the new kernel patch expects all the relavant bits of
the psscr_val be set. This would result in a breakage.


> 
> Balbir Singh.
> 

--
Thanks and Regards
gautham.

Re: [PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Balbir Singh]

On 04/10/16 21:32, Michael Ellerman wrote:
> "Gautham R. Shenoy"  writes:
> 
>> From: "Gautham R. Shenoy" 
>>
>> The power9_idle_stop method currently takes only the requested stop
>> level as a parameter and picks up the rest of the PSSCR bits from a
>> hand-coded macro. This is not a very flexible design, especially when
>> the firmware has the capability to communicate the psscr value and the
>> mask associated with a particular stop state via device tree.
>>
>> This patch modifies the power9_idle_stop API to take as parameters the
>> PSSCR value and the PSSCR mask corresponding to the stop state that
>> needs to be set. These PSSCR value and mask are respectively obtained
>> by parsing the "ibm,cpu-idle-state-psscr" and
>> "ibm,cpu-idle-state-psscr-mask" fields from the device tree.
>>
>> In addition to this, the patch adds support for handling stop states
>> for which ESL and EC bits in the PSSCR are zero. As per the
>> architecture, a wakeup from these stop states resumes execution from
>> the subsequent instruction as opposed to waking up at the System
>> Vector.
> 
> That looks good.
> 
>> This patch depends on the following skiboot patch that exports the
>> PSSCR values and the mask for all the stop states:
>> https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html
> 
> But we can't depend on a skiboot patch. The kernel has to cope with
> running on an old skiboot.

Hi, Michael

I think with an older skiboot the flags don't get exported and the
new cpuidle (stop state) does not get discovered. I don't think there
is any breakage. Gautham am I missing something?

Balbir Singh.

Re: [PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Michael Ellerman]

"Gautham R. Shenoy"  writes:

> From: "Gautham R. Shenoy" 
>
> The power9_idle_stop method currently takes only the requested stop
> level as a parameter and picks up the rest of the PSSCR bits from a
> hand-coded macro. This is not a very flexible design, especially when
> the firmware has the capability to communicate the psscr value and the
> mask associated with a particular stop state via device tree.
>
> This patch modifies the power9_idle_stop API to take as parameters the
> PSSCR value and the PSSCR mask corresponding to the stop state that
> needs to be set. These PSSCR value and mask are respectively obtained
> by parsing the "ibm,cpu-idle-state-psscr" and
> "ibm,cpu-idle-state-psscr-mask" fields from the device tree.
>
> In addition to this, the patch adds support for handling stop states
> for which ESL and EC bits in the PSSCR are zero. As per the
> architecture, a wakeup from these stop states resumes execution from
> the subsequent instruction as opposed to waking up at the System
> Vector.

That looks good.

> This patch depends on the following skiboot patch that exports the
> PSSCR values and the mask for all the stop states:
> https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html

But we can't depend on a skiboot patch. The kernel has to cope with
running on an old skiboot.

I realise this is for Power9, and we haven't shipped any of them yet.
But skiboot has had Power9 support since May, so I don't think we want
to declare that all of those skiboot versions are unsupported because of
this.

So is there any way we can make this work on skiboot versions that don't
have the patch above?

cheers

[PATCH 2/2] powernv: Pass PSSCR value and mask to power9_idle_stop

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" 

The power9_idle_stop method currently takes only the requested stop
level as a parameter and picks up the rest of the PSSCR bits from a
hand-coded macro. This is not a very flexible design, especially when
the firmware has the capability to communicate the psscr value and the
mask associated with a particular stop state via device tree.

This patch modifies the power9_idle_stop API to take as parameters the
PSSCR value and the PSSCR mask corresponding to the stop state that
needs to be set. These PSSCR value and mask are respectively obtained
by parsing the "ibm,cpu-idle-state-psscr" and
"ibm,cpu-idle-state-psscr-mask" fields from the device tree.

In addition to this, the patch adds support for handling stop states
for which ESL and EC bits in the PSSCR are zero. As per the
architecture, a wakeup from these stop states resumes execution from
the subsequent instruction as opposed to waking up at the System
Vector.

This patch depends on the following skiboot patch that exports the
PSSCR values and the mask for all the stop states:
https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html

Signed-off-by: Gautham R. Shenoy 
---
 arch/powerpc/include/asm/processor.h |  3 +-
 arch/powerpc/kernel/idle_book3s.S| 31 -
 arch/powerpc/platforms/powernv/idle.c| 76 +++-
 arch/powerpc/platforms/powernv/powernv.h |  3 +-
 arch/powerpc/platforms/powernv/smp.c |  7 +--
 drivers/cpuidle/cpuidle-powernv.c| 30 ++---
 6 files changed, 115 insertions(+), 35 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index 68e3bf5..4ead497 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_psscr_val,
+   unsigned long stop_psscr_mask);
 
 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..9f77cd4 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -40,9 +40,7 @@
 #define _WORC  GPR11
 #define _PTCR  GPR12
 
-#define PSSCR_HV_TEMPLATE  PSSCR_ESL | PSSCR_EC | \
-   PSSCR_PSLL_MASK | PSSCR_TR_MASK | \
-   PSSCR_MTL_MASK
+#define PSSCR_EC_ESL_MASK_SHIFTED  (PSSCR_EC | PSSCR_ESL) >> 16
 
.text
 
@@ -247,13 +245,23 @@ enter_winkle:
IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE)
 
 /*
- * r3 - requested stop state
+ * r3 - PSSCR value corresponding to the requested stop state.
  */
 power_enter_stop:
 /*
+ * Check if we are executing the lite variant with ESL=EC=0
+ */
+   andis.   r4, r3, PSSCR_EC_ESL_MASK_SHIFTED
+   andi.r3, r3, PSSCR_RL_MASK   /* r3 = requested stop state */
+   cmpdir4, 0
+   bne  1f
+   IDLE_STATE_ENTER_SEQ(PPC_STOP)
+   li  r3,0  /* Since we didn't lose state, return 0 */
+   b   pnv_wakeup_noloss
+/*
  * Check if the requested state is a deep idle state.
  */
-   LOAD_REG_ADDRBASE(r5,pnv_first_deep_stop_state)
+1: LOAD_REG_ADDRBASE(r5,pnv_first_deep_stop_state)
ld  r4,ADDROFF(pnv_first_deep_stop_state)(r5)
cmpdr3,r4
bge 2f
@@ -330,16 +338,17 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);  
\
ld  r3,ORIG_GPR3(r1);   /* Restore original r3 */   \
 20:nop;
 
-
 /*
- * r3 - requested stop state
+ * r3 - The PSSCR value corresponding to the stop state.
+ * r4 - The PSSCR mask corrresonding to the stop state.
  */
 _GLOBAL(power9_idle_stop)
-   LOAD_REG_IMMEDIATE(r4, PSSCR_HV_TEMPLATE)
-   or  r4,r4,r3
-   mtspr   SPRN_PSSCR, r4
-   li  r4, 1
+   mfspr   r5, SPRN_PSSCR
+   andcr5, r5, r4
+   or  r3, r3, r5
+   mtspr   SPRN_PSSCR, r3
LOAD_REG_ADDR(r5,power_enter_stop)
+   li  r4, 1
b   pnv_powersave_common
/* No return */
 /*
diff --git a/arch/powerpc/platforms/powernv/idle.c 
b/arch/powerpc/platforms/powernv/idle.c
index 479c256..575f2aa 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -237,15 +237,21 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
show_fastsleep_workaround_applyonce,
store_fastsleep_workaround_applyonce);
 
+/*
+ * The default stop state that will be used by ppc_md.power_save
+ * function on platforms that support