Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Michael Neuling]

> > > +#define OPAL_PM_TIMEBASE_STOP0x0002
> > > +#define OPAL_PM_LOSE_HYP_CONTEXT 0x2000
> > > +#define OPAL_PM_LOSE_FULL_CONTEXT0x4000
> > >  #define OPAL_PM_NAP_ENABLED  0x0001
> > >  #define OPAL_PM_SLEEP_ENABLED0x0002
> > >  #define OPAL_PM_WINKLE_ENABLED   0x0004
> > >  #define OPAL_PM_SLEEP_ENABLED_ER10x0008 /* with
> > > workaround */
> > > +#define OPAL_PM_STOP_INST_FAST   0x0010
> > > +#define OPAL_PM_STOP_INST_DEEP   0x0020
> > I don't see the above in skiboot yet?
> I've posted it here -
> http://patchwork.ozlabs.org/patch/617828/

FWIW, this is in now.

https://github.com/open-power/skiboot/commit/952daa69baca407383bc900911f6c40718a0e289

> > 
> > 
> > > 
> > > diff --git a/arch/powerpc/include/asm/paca.h
> > > b/arch/powerpc/include/asm/paca.h
> > > index 546540b..ae91b44 100644
> > > --- a/arch/powerpc/include/asm/paca.h
> > > +++ b/arch/powerpc/include/asm/paca.h
> > > @@ -171,6 +171,8 @@ struct paca_struct {
> > >   /* Mask to denote subcore sibling threads */
> > >   u8 subcore_sibling_mask;
> > >  #endif
> > > + /* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set
> > > */
> > > + u64 thread_psscr;
> > I'm not entirely clear on why that needs to be in the paca. Could it
> > not be global?
> > 
> While we use Requested Level (RL) field of PSSCR to request a stop
> level, other fields in the SPR like EC, ESL, TR, PSLL, MTL can be
> modified by individual threads less frequently to alter the behaviour of
> stop. So the idea was to have a per-thread variable with all (except RL)
> fields of PSSCR set appropriately. Threads at the time of entering idle,
> can modify the RL field in the variable and execute stop instruction.

But we don't do any of this currently? This is setup at init
in pnv_init_idle_states() and only the RL is changed in power_stop().

So it can still be a global.  It could just be a constant currently even.

>   .text
> > >  
> > >  /*
> > > @@ -61,8 +75,19 @@ save_sprs_to_stack:
> > >    * Note all register i.e per-core, per-subcore or per-thread 
> > > is saved
> > >    * here since any thread in the core might wake up first
> > >    */
> > > +BEGIN_FTR_SECTION
> > > + mfspr   r3,SPRN_PTCR
> > > + std r3,_PTCR(r1)
> > > + mfspr   r3,SPRN_LMRR
> > > + std r3,_LMRR(r1)
> > > + mfspr   r3,SPRN_LMSER
> > > + std r3,_LMSER(r1)
> > > + mfspr   r3,SPRN_ASDR
> > > + std r3,_ASDR(r1)
> > > +FTR_SECTION_ELSE
> > A comment here saying that SDR1 is removed in ISA 3.0 would be helpful.
> > 
> Ok.

I thought we decided we didn't need LMRR, LMSR, 

https://lkml.org/lkml/2016/6/8/1121

or ASDR isn't actually used at all yet and is only valid for some page
faults, so we don't need it here also.

> +END_MMU_FTR_SECTION_IFCLR(MMU_FTR_RADIX)
> > > +
> > > + /* Restore per thread state */
> > > +BEGIN_FTR_SECTION
> > > + bl  __restore_cpu_power9
> > > +
> > > + ld  r4,_LMRR(r1)
> > > + mtspr   SPRN_LMRR,r4
> > > + ld  r4,_LMSER(r1)
> > > + mtspr   SPRN_LMSER,r4
> > > + ld  r4,_ASDR(r1)
> > > + mtspr   SPRN_ASDR,r4
> > Should those be in __restore_cpu_power9 ?
> I was not sure how these registers will be used, but after speaking to
> Aneesh and Mikey I realized these registers will not need restoring.
> LMRR and LMSER are associated with the context and ADSR will be consumed
> before entering stop. So I'll be dropping the this hunk in next revision.

Yep.

> 
> > >   pnv_alloc_idle_core_states();
> > >  
> > > + if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
> > > + for_each_possible_cpu(i) {
> > > +
> > > + u64 psscr_init_val = PSSCR_ESL | PSSCR_EC |
> > > + PSSCR_PSLL_MASK | PSSCR_TR_MASK |
> > > + PSSCR_MTL_MASK;
> > > +
> > > + paca[i].thread_psscr = psscr_init_val;

This seems to be the only place you set this.  Why put it in the paca, why
not just make this a constant? 

Mikey

Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Michael Neuling]

> > > +#define OPAL_PM_TIMEBASE_STOP0x0002
> > > +#define OPAL_PM_LOSE_HYP_CONTEXT 0x2000
> > > +#define OPAL_PM_LOSE_FULL_CONTEXT0x4000
> > >  #define OPAL_PM_NAP_ENABLED  0x0001
> > >  #define OPAL_PM_SLEEP_ENABLED0x0002
> > >  #define OPAL_PM_WINKLE_ENABLED   0x0004
> > >  #define OPAL_PM_SLEEP_ENABLED_ER10x0008 /* with
> > > workaround */
> > > +#define OPAL_PM_STOP_INST_FAST   0x0010
> > > +#define OPAL_PM_STOP_INST_DEEP   0x0020
> > I don't see the above in skiboot yet?
> I've posted it here -
> http://patchwork.ozlabs.org/patch/617828/

FWIW, this is in now.

https://github.com/open-power/skiboot/commit/952daa69baca407383bc900911f6c40718a0e289

> > 
> > 
> > > 
> > > diff --git a/arch/powerpc/include/asm/paca.h
> > > b/arch/powerpc/include/asm/paca.h
> > > index 546540b..ae91b44 100644
> > > --- a/arch/powerpc/include/asm/paca.h
> > > +++ b/arch/powerpc/include/asm/paca.h
> > > @@ -171,6 +171,8 @@ struct paca_struct {
> > >   /* Mask to denote subcore sibling threads */
> > >   u8 subcore_sibling_mask;
> > >  #endif
> > > + /* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set
> > > */
> > > + u64 thread_psscr;
> > I'm not entirely clear on why that needs to be in the paca. Could it
> > not be global?
> > 
> While we use Requested Level (RL) field of PSSCR to request a stop
> level, other fields in the SPR like EC, ESL, TR, PSLL, MTL can be
> modified by individual threads less frequently to alter the behaviour of
> stop. So the idea was to have a per-thread variable with all (except RL)
> fields of PSSCR set appropriately. Threads at the time of entering idle,
> can modify the RL field in the variable and execute stop instruction.

But we don't do any of this currently? This is setup at init
in pnv_init_idle_states() and only the RL is changed in power_stop().

So it can still be a global.  It could just be a constant currently even.

>   .text
> > >  
> > >  /*
> > > @@ -61,8 +75,19 @@ save_sprs_to_stack:
> > >    * Note all register i.e per-core, per-subcore or per-thread 
> > > is saved
> > >    * here since any thread in the core might wake up first
> > >    */
> > > +BEGIN_FTR_SECTION
> > > + mfspr   r3,SPRN_PTCR
> > > + std r3,_PTCR(r1)
> > > + mfspr   r3,SPRN_LMRR
> > > + std r3,_LMRR(r1)
> > > + mfspr   r3,SPRN_LMSER
> > > + std r3,_LMSER(r1)
> > > + mfspr   r3,SPRN_ASDR
> > > + std r3,_ASDR(r1)
> > > +FTR_SECTION_ELSE
> > A comment here saying that SDR1 is removed in ISA 3.0 would be helpful.
> > 
> Ok.

I thought we decided we didn't need LMRR, LMSR, 

https://lkml.org/lkml/2016/6/8/1121

or ASDR isn't actually used at all yet and is only valid for some page
faults, so we don't need it here also.

> +END_MMU_FTR_SECTION_IFCLR(MMU_FTR_RADIX)
> > > +
> > > + /* Restore per thread state */
> > > +BEGIN_FTR_SECTION
> > > + bl  __restore_cpu_power9
> > > +
> > > + ld  r4,_LMRR(r1)
> > > + mtspr   SPRN_LMRR,r4
> > > + ld  r4,_LMSER(r1)
> > > + mtspr   SPRN_LMSER,r4
> > > + ld  r4,_ASDR(r1)
> > > + mtspr   SPRN_ASDR,r4
> > Should those be in __restore_cpu_power9 ?
> I was not sure how these registers will be used, but after speaking to
> Aneesh and Mikey I realized these registers will not need restoring.
> LMRR and LMSER are associated with the context and ADSR will be consumed
> before entering stop. So I'll be dropping the this hunk in next revision.

Yep.

> 
> > >   pnv_alloc_idle_core_states();
> > >  
> > > + if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
> > > + for_each_possible_cpu(i) {
> > > +
> > > + u64 psscr_init_val = PSSCR_ESL | PSSCR_EC |
> > > + PSSCR_PSLL_MASK | PSSCR_TR_MASK |
> > > + PSSCR_MTL_MASK;
> > > +
> > > + paca[i].thread_psscr = psscr_init_val;

This seems to be the only place you set this.  Why put it in the paca, why
not just make this a constant? 

Mikey

Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Shreyas B Prabhu]

Hi Michael,

On 06/15/2016 04:44 PM, Michael Ellerman wrote:
> Hi Shreyas,
> 
> Comments inline ...
> 
> On Wed, 2016-08-06 at 16:54:28 UTC, "Shreyas B. Prabhu" wrote:
>> POWER ISA v3 defines a new idle processor core mechanism. In summary,
>>  a) new instruction named stop is added. This instruction replaces
>>  instructions like nap, sleep, rvwinkle.
>>  b) new per thread SPR named Processor Stop Status and Control Register
>>  (PSSCR) is added which controls the behavior of stop instruction.
>>
>> PSSCR layout:
>> --
>> | PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
>> --
>> 0  4 41   4243   44 4854   5660
>>
>> PSSCR key fields:
>>  Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
>>  power-saving state the thread entered since stop instruction was last
>>  executed.
>>
>>  Bit 42 - Enable State Loss
>>  0 - No state is lost irrespective of other fields
>>  1 - Allows state loss
>>
>>  Bits 44:47 - Power-Saving Level Limit
>>  This limits the power-saving level that can be entered into.
>>
>>  Bits 60:63 - Requested Level
>>  Used to specify which power-saving level must be entered on executing
>>  stop instruction
> 
> That would probably be good as a comment somewhere too, maybe idle.c
> 

Ok. I'll add it there.

>> diff --git a/arch/powerpc/include/asm/cpuidle.h 
>> b/arch/powerpc/include/asm/cpuidle.h
>> index d2f99ca..3d7fc06 100644
>> --- a/arch/powerpc/include/asm/cpuidle.h
>> +++ b/arch/powerpc/include/asm/cpuidle.h
>> @@ -13,6 +13,8 @@
>>  #ifndef __ASSEMBLY__
>>  extern u32 pnv_fastsleep_workaround_at_entry[];
>>  extern u32 pnv_fastsleep_workaround_at_exit[];
>> +
>> +extern u64 pnv_first_deep_stop_state;
> 
> Should this have some safe initial value?
> 
>> diff --git a/arch/powerpc/include/asm/machdep.h 
>> b/arch/powerpc/include/asm/machdep.h
>> index 6bdcd0d..ae3b155 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -262,6 +262,7 @@ struct machdep_calls {
>>  extern void e500_idle(void);
>>  extern void power4_idle(void);
>>  extern void power7_idle(void);
>> +extern void power_stop0(void);
> 
> Can that have a better name please?

What do you have in mind?
power_arch300_idle0()?
power_arch300_stop0()?
or I can use power9_idle() here. But we will still need a better
replacement for power_stop() below.

> 
>> diff --git a/arch/powerpc/include/asm/opal-api.h 
>> b/arch/powerpc/include/asm/opal-api.h
>> index 9bb8ddf..7f3f8c6 100644
>> --- a/arch/powerpc/include/asm/opal-api.h
>> +++ b/arch/powerpc/include/asm/opal-api.h
>> @@ -162,13 +162,20 @@
>>  
>>  /* Device tree flags */
>>  
>> -/* Flags set in power-mgmt nodes in device tree if
>> - * respective idle states are supported in the platform.
>> +/*
>> + * Flags set in power-mgmt nodes in device tree describing
>> + * idle states that are supported in the platform.
>>   */
>> +
>> +#define OPAL_PM_TIMEBASE_STOP   0x0002
>> +#define OPAL_PM_LOSE_HYP_CONTEXT0x2000
>> +#define OPAL_PM_LOSE_FULL_CONTEXT   0x4000
>>  #define OPAL_PM_NAP_ENABLED 0x0001
>>  #define OPAL_PM_SLEEP_ENABLED   0x0002
>>  #define OPAL_PM_WINKLE_ENABLED  0x0004
>>  #define OPAL_PM_SLEEP_ENABLED_ER1   0x0008 /* with workaround */
>> +#define OPAL_PM_STOP_INST_FAST  0x0010
>> +#define OPAL_PM_STOP_INST_DEEP  0x0020
> 
> I don't see the above in skiboot yet?

I've posted it here -
http://patchwork.ozlabs.org/patch/617828/
> 
>> diff --git a/arch/powerpc/include/asm/paca.h 
>> b/arch/powerpc/include/asm/paca.h
>> index 546540b..ae91b44 100644
>> --- a/arch/powerpc/include/asm/paca.h
>> +++ b/arch/powerpc/include/asm/paca.h
>> @@ -171,6 +171,8 @@ struct paca_struct {
>>  /* Mask to denote subcore sibling threads */
>>  u8 subcore_sibling_mask;
>>  #endif
>> +/* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set */
>> +u64 thread_psscr;
> 
> I'm not entirely clear on why that needs to be in the paca. Could it not be 
> global?
> 

While we use Requested Level (RL) field of PSSCR to request a stop
level, other fields in the SPR like EC, ESL, TR, PSLL, MTL can be
modified by individual threads less frequently to alter the behaviour of
stop. So the idea was to have a per-thread variable with all (except RL)
fields of PSSCR set appropriately. Threads at the time of entering idle,
can modify the RL field in the variable and execute stop instruction.


>> diff --git a/arch/powerpc/include/asm/processor.h 
>> b/arch/powerpc/include/asm/processor.h
>> index 009fab1..7f92fc8 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -457,6 +457,7 @@ extern int powersave_nap;/* set if nap mode can 
>> be used in idle loop */
>>  

Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Shreyas B Prabhu]

Hi Michael,

On 06/15/2016 04:44 PM, Michael Ellerman wrote:
> Hi Shreyas,
> 
> Comments inline ...
> 
> On Wed, 2016-08-06 at 16:54:28 UTC, "Shreyas B. Prabhu" wrote:
>> POWER ISA v3 defines a new idle processor core mechanism. In summary,
>>  a) new instruction named stop is added. This instruction replaces
>>  instructions like nap, sleep, rvwinkle.
>>  b) new per thread SPR named Processor Stop Status and Control Register
>>  (PSSCR) is added which controls the behavior of stop instruction.
>>
>> PSSCR layout:
>> --
>> | PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
>> --
>> 0  4 41   4243   44 4854   5660
>>
>> PSSCR key fields:
>>  Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
>>  power-saving state the thread entered since stop instruction was last
>>  executed.
>>
>>  Bit 42 - Enable State Loss
>>  0 - No state is lost irrespective of other fields
>>  1 - Allows state loss
>>
>>  Bits 44:47 - Power-Saving Level Limit
>>  This limits the power-saving level that can be entered into.
>>
>>  Bits 60:63 - Requested Level
>>  Used to specify which power-saving level must be entered on executing
>>  stop instruction
> 
> That would probably be good as a comment somewhere too, maybe idle.c
> 

Ok. I'll add it there.

>> diff --git a/arch/powerpc/include/asm/cpuidle.h 
>> b/arch/powerpc/include/asm/cpuidle.h
>> index d2f99ca..3d7fc06 100644
>> --- a/arch/powerpc/include/asm/cpuidle.h
>> +++ b/arch/powerpc/include/asm/cpuidle.h
>> @@ -13,6 +13,8 @@
>>  #ifndef __ASSEMBLY__
>>  extern u32 pnv_fastsleep_workaround_at_entry[];
>>  extern u32 pnv_fastsleep_workaround_at_exit[];
>> +
>> +extern u64 pnv_first_deep_stop_state;
> 
> Should this have some safe initial value?
> 
>> diff --git a/arch/powerpc/include/asm/machdep.h 
>> b/arch/powerpc/include/asm/machdep.h
>> index 6bdcd0d..ae3b155 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -262,6 +262,7 @@ struct machdep_calls {
>>  extern void e500_idle(void);
>>  extern void power4_idle(void);
>>  extern void power7_idle(void);
>> +extern void power_stop0(void);
> 
> Can that have a better name please?

What do you have in mind?
power_arch300_idle0()?
power_arch300_stop0()?
or I can use power9_idle() here. But we will still need a better
replacement for power_stop() below.

> 
>> diff --git a/arch/powerpc/include/asm/opal-api.h 
>> b/arch/powerpc/include/asm/opal-api.h
>> index 9bb8ddf..7f3f8c6 100644
>> --- a/arch/powerpc/include/asm/opal-api.h
>> +++ b/arch/powerpc/include/asm/opal-api.h
>> @@ -162,13 +162,20 @@
>>  
>>  /* Device tree flags */
>>  
>> -/* Flags set in power-mgmt nodes in device tree if
>> - * respective idle states are supported in the platform.
>> +/*
>> + * Flags set in power-mgmt nodes in device tree describing
>> + * idle states that are supported in the platform.
>>   */
>> +
>> +#define OPAL_PM_TIMEBASE_STOP   0x0002
>> +#define OPAL_PM_LOSE_HYP_CONTEXT0x2000
>> +#define OPAL_PM_LOSE_FULL_CONTEXT   0x4000
>>  #define OPAL_PM_NAP_ENABLED 0x0001
>>  #define OPAL_PM_SLEEP_ENABLED   0x0002
>>  #define OPAL_PM_WINKLE_ENABLED  0x0004
>>  #define OPAL_PM_SLEEP_ENABLED_ER1   0x0008 /* with workaround */
>> +#define OPAL_PM_STOP_INST_FAST  0x0010
>> +#define OPAL_PM_STOP_INST_DEEP  0x0020
> 
> I don't see the above in skiboot yet?

I've posted it here -
http://patchwork.ozlabs.org/patch/617828/
> 
>> diff --git a/arch/powerpc/include/asm/paca.h 
>> b/arch/powerpc/include/asm/paca.h
>> index 546540b..ae91b44 100644
>> --- a/arch/powerpc/include/asm/paca.h
>> +++ b/arch/powerpc/include/asm/paca.h
>> @@ -171,6 +171,8 @@ struct paca_struct {
>>  /* Mask to denote subcore sibling threads */
>>  u8 subcore_sibling_mask;
>>  #endif
>> +/* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set */
>> +u64 thread_psscr;
> 
> I'm not entirely clear on why that needs to be in the paca. Could it not be 
> global?
> 

While we use Requested Level (RL) field of PSSCR to request a stop
level, other fields in the SPR like EC, ESL, TR, PSLL, MTL can be
modified by individual threads less frequently to alter the behaviour of
stop. So the idea was to have a per-thread variable with all (except RL)
fields of PSSCR set appropriately. Threads at the time of entering idle,
can modify the RL field in the variable and execute stop instruction.


>> diff --git a/arch/powerpc/include/asm/processor.h 
>> b/arch/powerpc/include/asm/processor.h
>> index 009fab1..7f92fc8 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -457,6 +457,7 @@ extern int powersave_nap;/* set if nap mode can 
>> be used in idle loop */
>>  

Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Michael Ellerman]

Hi Shreyas,

Comments inline ...

On Wed, 2016-08-06 at 16:54:28 UTC, "Shreyas B. Prabhu" wrote:
> POWER ISA v3 defines a new idle processor core mechanism. In summary,
>  a) new instruction named stop is added. This instruction replaces
>   instructions like nap, sleep, rvwinkle.
>  b) new per thread SPR named Processor Stop Status and Control Register
>   (PSSCR) is added which controls the behavior of stop instruction.
> 
> PSSCR layout:
> --
> | PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
> --
> 0  4 41   4243   44 4854   5660
> 
> PSSCR key fields:
>   Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
>   power-saving state the thread entered since stop instruction was last
>   executed.
> 
>   Bit 42 - Enable State Loss
>   0 - No state is lost irrespective of other fields
>   1 - Allows state loss
> 
>   Bits 44:47 - Power-Saving Level Limit
>   This limits the power-saving level that can be entered into.
> 
>   Bits 60:63 - Requested Level
>   Used to specify which power-saving level must be entered on executing
>   stop instruction

That would probably be good as a comment somewhere too, maybe idle.c

> diff --git a/arch/powerpc/include/asm/cpuidle.h 
> b/arch/powerpc/include/asm/cpuidle.h
> index d2f99ca..3d7fc06 100644
> --- a/arch/powerpc/include/asm/cpuidle.h
> +++ b/arch/powerpc/include/asm/cpuidle.h
> @@ -13,6 +13,8 @@
>  #ifndef __ASSEMBLY__
>  extern u32 pnv_fastsleep_workaround_at_entry[];
>  extern u32 pnv_fastsleep_workaround_at_exit[];
> +
> +extern u64 pnv_first_deep_stop_state;

Should this have some safe initial value?

> diff --git a/arch/powerpc/include/asm/machdep.h 
> b/arch/powerpc/include/asm/machdep.h
> index 6bdcd0d..ae3b155 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -262,6 +262,7 @@ struct machdep_calls {
>  extern void e500_idle(void);
>  extern void power4_idle(void);
>  extern void power7_idle(void);
> +extern void power_stop0(void);

Can that have a better name please?

> diff --git a/arch/powerpc/include/asm/opal-api.h 
> b/arch/powerpc/include/asm/opal-api.h
> index 9bb8ddf..7f3f8c6 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -162,13 +162,20 @@
>  
>  /* Device tree flags */
>  
> -/* Flags set in power-mgmt nodes in device tree if
> - * respective idle states are supported in the platform.
> +/*
> + * Flags set in power-mgmt nodes in device tree describing
> + * idle states that are supported in the platform.
>   */
> +
> +#define OPAL_PM_TIMEBASE_STOP0x0002
> +#define OPAL_PM_LOSE_HYP_CONTEXT 0x2000
> +#define OPAL_PM_LOSE_FULL_CONTEXT0x4000
>  #define OPAL_PM_NAP_ENABLED  0x0001
>  #define OPAL_PM_SLEEP_ENABLED0x0002
>  #define OPAL_PM_WINKLE_ENABLED   0x0004
>  #define OPAL_PM_SLEEP_ENABLED_ER10x0008 /* with workaround */
> +#define OPAL_PM_STOP_INST_FAST   0x0010
> +#define OPAL_PM_STOP_INST_DEEP   0x0020

I don't see the above in skiboot yet?

> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 546540b..ae91b44 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -171,6 +171,8 @@ struct paca_struct {
>   /* Mask to denote subcore sibling threads */
>   u8 subcore_sibling_mask;
>  #endif
> + /* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set */
> + u64 thread_psscr;

I'm not entirely clear on why that needs to be in the paca. Could it not be 
global?

> diff --git a/arch/powerpc/include/asm/processor.h 
> b/arch/powerpc/include/asm/processor.h
> index 009fab1..7f92fc8 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -457,6 +457,7 @@ 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 power_stop(unsigned long state);

Can that also have a better name?

> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index a0948f4..89a00d9 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -145,6 +145,16 @@
>  #define MSR_64BIT0
>  #endif
>  
> +/* Power Management - PSSCR Fields */
> +#define PSSCR_RL_MASK0x000F
> +#define PSSCR_MTL_MASK   0x00F0
> +#define PSSCR_TR_MASK0x0300
> +#define PSSCR_PSLL_MASK  0x000F
> +#define PSSCR_EC 0x0010
> +#define PSSCR_ESL0x0020
> +#define PSSCR_SD 0x0040

Can we get 

Re: [v6, 08/11] powerpc/powernv: Add platform support for stop instruction

[Michael Ellerman]

Hi Shreyas,

Comments inline ...

On Wed, 2016-08-06 at 16:54:28 UTC, "Shreyas B. Prabhu" wrote:
> POWER ISA v3 defines a new idle processor core mechanism. In summary,
>  a) new instruction named stop is added. This instruction replaces
>   instructions like nap, sleep, rvwinkle.
>  b) new per thread SPR named Processor Stop Status and Control Register
>   (PSSCR) is added which controls the behavior of stop instruction.
> 
> PSSCR layout:
> --
> | PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
> --
> 0  4 41   4243   44 4854   5660
> 
> PSSCR key fields:
>   Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
>   power-saving state the thread entered since stop instruction was last
>   executed.
> 
>   Bit 42 - Enable State Loss
>   0 - No state is lost irrespective of other fields
>   1 - Allows state loss
> 
>   Bits 44:47 - Power-Saving Level Limit
>   This limits the power-saving level that can be entered into.
> 
>   Bits 60:63 - Requested Level
>   Used to specify which power-saving level must be entered on executing
>   stop instruction

That would probably be good as a comment somewhere too, maybe idle.c

> diff --git a/arch/powerpc/include/asm/cpuidle.h 
> b/arch/powerpc/include/asm/cpuidle.h
> index d2f99ca..3d7fc06 100644
> --- a/arch/powerpc/include/asm/cpuidle.h
> +++ b/arch/powerpc/include/asm/cpuidle.h
> @@ -13,6 +13,8 @@
>  #ifndef __ASSEMBLY__
>  extern u32 pnv_fastsleep_workaround_at_entry[];
>  extern u32 pnv_fastsleep_workaround_at_exit[];
> +
> +extern u64 pnv_first_deep_stop_state;

Should this have some safe initial value?

> diff --git a/arch/powerpc/include/asm/machdep.h 
> b/arch/powerpc/include/asm/machdep.h
> index 6bdcd0d..ae3b155 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -262,6 +262,7 @@ struct machdep_calls {
>  extern void e500_idle(void);
>  extern void power4_idle(void);
>  extern void power7_idle(void);
> +extern void power_stop0(void);

Can that have a better name please?

> diff --git a/arch/powerpc/include/asm/opal-api.h 
> b/arch/powerpc/include/asm/opal-api.h
> index 9bb8ddf..7f3f8c6 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -162,13 +162,20 @@
>  
>  /* Device tree flags */
>  
> -/* Flags set in power-mgmt nodes in device tree if
> - * respective idle states are supported in the platform.
> +/*
> + * Flags set in power-mgmt nodes in device tree describing
> + * idle states that are supported in the platform.
>   */
> +
> +#define OPAL_PM_TIMEBASE_STOP0x0002
> +#define OPAL_PM_LOSE_HYP_CONTEXT 0x2000
> +#define OPAL_PM_LOSE_FULL_CONTEXT0x4000
>  #define OPAL_PM_NAP_ENABLED  0x0001
>  #define OPAL_PM_SLEEP_ENABLED0x0002
>  #define OPAL_PM_WINKLE_ENABLED   0x0004
>  #define OPAL_PM_SLEEP_ENABLED_ER10x0008 /* with workaround */
> +#define OPAL_PM_STOP_INST_FAST   0x0010
> +#define OPAL_PM_STOP_INST_DEEP   0x0020

I don't see the above in skiboot yet?

> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 546540b..ae91b44 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -171,6 +171,8 @@ struct paca_struct {
>   /* Mask to denote subcore sibling threads */
>   u8 subcore_sibling_mask;
>  #endif
> + /* Template for PSSCR with EC, ESL, TR, PSLL, MTL fields set */
> + u64 thread_psscr;

I'm not entirely clear on why that needs to be in the paca. Could it not be 
global?

> diff --git a/arch/powerpc/include/asm/processor.h 
> b/arch/powerpc/include/asm/processor.h
> index 009fab1..7f92fc8 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -457,6 +457,7 @@ 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 power_stop(unsigned long state);

Can that also have a better name?

> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index a0948f4..89a00d9 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -145,6 +145,16 @@
>  #define MSR_64BIT0
>  #endif
>  
> +/* Power Management - PSSCR Fields */
> +#define PSSCR_RL_MASK0x000F
> +#define PSSCR_MTL_MASK   0x00F0
> +#define PSSCR_TR_MASK0x0300
> +#define PSSCR_PSLL_MASK  0x000F
> +#define PSSCR_EC 0x0010
> +#define PSSCR_ESL0x0020
> +#define PSSCR_SD 0x0040

Can we get 

[PATCH v6 08/11] powerpc/powernv: Add platform support for stop instruction

[Shreyas B. Prabhu]

POWER ISA v3 defines a new idle processor core mechanism. In summary,
 a) new instruction named stop is added. This instruction replaces
instructions like nap, sleep, rvwinkle.
 b) new per thread SPR named Processor Stop Status and Control Register
(PSSCR) is added which controls the behavior of stop instruction.

PSSCR layout:
--
| PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
--
0  4 41   4243   44 4854   5660

PSSCR key fields:
Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
power-saving state the thread entered since stop instruction was last
executed.

Bit 42 - Enable State Loss
0 - No state is lost irrespective of other fields
1 - Allows state loss

Bits 44:47 - Power-Saving Level Limit
This limits the power-saving level that can be entered into.

Bits 60:63 - Requested Level
Used to specify which power-saving level must be entered on executing
stop instruction

This patch adds support for stop instruction and PSSCR handling.

Reviewed-by: Gautham R. Shenoy 
Signed-off-by: Shreyas B. Prabhu 
---
Changes in v6
=
 - Save/restore new P9 SPRs when using deep idle states

Changes in v4:
==
 - Added PSSCR layout to commit message
 - Improved / Fixed comments
 - Fixed whitespace error in paca.h
 - Using MAX_POSSIBLE_STOP_STATE macro instead of hardcoding 0xF as 
   max possible stop state

Changes in v3:
==
 - Instead of introducing new file idle_power_stop.S, P9 idle support
   is added to idle_power_common.S using CPU_FTR sections.
 - Fixed r4 reg clobbering in power_stop0
 - Improved comments

Changes in v2:
==
 - Using CPU_FTR_ARCH_300 bit instead of CPU_FTR_STOP_INST

 arch/powerpc/include/asm/cpuidle.h|   2 +
 arch/powerpc/include/asm/kvm_book3s_asm.h |   2 +-
 arch/powerpc/include/asm/machdep.h|   1 +
 arch/powerpc/include/asm/opal-api.h   |  11 +-
 arch/powerpc/include/asm/paca.h   |   2 +
 arch/powerpc/include/asm/ppc-opcode.h |   4 +
 arch/powerpc/include/asm/processor.h  |   1 +
 arch/powerpc/include/asm/reg.h|  14 +++
 arch/powerpc/kernel/asm-offsets.c |   2 +
 arch/powerpc/kernel/idle_power_common.S   | 175 +++---
 arch/powerpc/platforms/powernv/idle.c |  84 --
 11 files changed, 265 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/include/asm/cpuidle.h 
b/arch/powerpc/include/asm/cpuidle.h
index d2f99ca..3d7fc06 100644
--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h
@@ -13,6 +13,8 @@
 #ifndef __ASSEMBLY__
 extern u32 pnv_fastsleep_workaround_at_entry[];
 extern u32 pnv_fastsleep_workaround_at_exit[];
+
+extern u64 pnv_first_deep_stop_state;
 #endif
 
 #endif
diff --git a/arch/powerpc/include/asm/kvm_book3s_asm.h 
b/arch/powerpc/include/asm/kvm_book3s_asm.h
index 72b6225..d318d43 100644
--- a/arch/powerpc/include/asm/kvm_book3s_asm.h
+++ b/arch/powerpc/include/asm/kvm_book3s_asm.h
@@ -162,7 +162,7 @@ struct kvmppc_book3s_shadow_vcpu {
 
 /* Values for kvm_state */
 #define KVM_HWTHREAD_IN_KERNEL 0
-#define KVM_HWTHREAD_IN_NAP1
+#define KVM_HWTHREAD_IN_IDLE   1
 #define KVM_HWTHREAD_IN_KVM2
 
 #endif /* __ASM_KVM_BOOK3S_ASM_H__ */
diff --git a/arch/powerpc/include/asm/machdep.h 
b/arch/powerpc/include/asm/machdep.h
index 6bdcd0d..ae3b155 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -262,6 +262,7 @@ struct machdep_calls {
 extern void e500_idle(void);
 extern void power4_idle(void);
 extern void power7_idle(void);
+extern void power_stop0(void);
 extern void ppc6xx_idle(void);
 extern void book3e_idle(void);
 
diff --git a/arch/powerpc/include/asm/opal-api.h 
b/arch/powerpc/include/asm/opal-api.h
index 9bb8ddf..7f3f8c6 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -162,13 +162,20 @@
 
 /* Device tree flags */
 
-/* Flags set in power-mgmt nodes in device tree if
- * respective idle states are supported in the platform.
+/*
+ * Flags set in power-mgmt nodes in device tree describing
+ * idle states that are supported in the platform.
  */
+
+#define OPAL_PM_TIMEBASE_STOP  0x0002
+#define OPAL_PM_LOSE_HYP_CONTEXT   0x2000
+#define OPAL_PM_LOSE_FULL_CONTEXT  0x4000
 #define OPAL_PM_NAP_ENABLED0x0001
 #define OPAL_PM_SLEEP_ENABLED  0x0002
 #define OPAL_PM_WINKLE_ENABLED 0x0004
 #define OPAL_PM_SLEEP_ENABLED_ER1  0x0008 /* with workaround */
+#define OPAL_PM_STOP_INST_FAST 0x0010
+#define OPAL_PM_STOP_INST_DEEP 0x0020
 
 /*
  * OPAL_CONFIG_CPU_IDLE_STATE parameters
diff --git 

[PATCH v6 08/11] powerpc/powernv: Add platform support for stop instruction

[Shreyas B. Prabhu]

POWER ISA v3 defines a new idle processor core mechanism. In summary,
 a) new instruction named stop is added. This instruction replaces
instructions like nap, sleep, rvwinkle.
 b) new per thread SPR named Processor Stop Status and Control Register
(PSSCR) is added which controls the behavior of stop instruction.

PSSCR layout:
--
| PLS | /// | SD | ESL | EC | PSLL | /// | TR | MTL | RL |
--
0  4 41   4243   44 4854   5660

PSSCR key fields:
Bits 0:3  - Power-Saving Level Status. This field indicates the lowest
power-saving state the thread entered since stop instruction was last
executed.

Bit 42 - Enable State Loss
0 - No state is lost irrespective of other fields
1 - Allows state loss

Bits 44:47 - Power-Saving Level Limit
This limits the power-saving level that can be entered into.

Bits 60:63 - Requested Level
Used to specify which power-saving level must be entered on executing
stop instruction

This patch adds support for stop instruction and PSSCR handling.

Reviewed-by: Gautham R. Shenoy 
Signed-off-by: Shreyas B. Prabhu 
---
Changes in v6
=
 - Save/restore new P9 SPRs when using deep idle states

Changes in v4:
==
 - Added PSSCR layout to commit message
 - Improved / Fixed comments
 - Fixed whitespace error in paca.h
 - Using MAX_POSSIBLE_STOP_STATE macro instead of hardcoding 0xF as 
   max possible stop state

Changes in v3:
==
 - Instead of introducing new file idle_power_stop.S, P9 idle support
   is added to idle_power_common.S using CPU_FTR sections.
 - Fixed r4 reg clobbering in power_stop0
 - Improved comments

Changes in v2:
==
 - Using CPU_FTR_ARCH_300 bit instead of CPU_FTR_STOP_INST

 arch/powerpc/include/asm/cpuidle.h|   2 +
 arch/powerpc/include/asm/kvm_book3s_asm.h |   2 +-
 arch/powerpc/include/asm/machdep.h|   1 +
 arch/powerpc/include/asm/opal-api.h   |  11 +-
 arch/powerpc/include/asm/paca.h   |   2 +
 arch/powerpc/include/asm/ppc-opcode.h |   4 +
 arch/powerpc/include/asm/processor.h  |   1 +
 arch/powerpc/include/asm/reg.h|  14 +++
 arch/powerpc/kernel/asm-offsets.c |   2 +
 arch/powerpc/kernel/idle_power_common.S   | 175 +++---
 arch/powerpc/platforms/powernv/idle.c |  84 --
 11 files changed, 265 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/include/asm/cpuidle.h 
b/arch/powerpc/include/asm/cpuidle.h
index d2f99ca..3d7fc06 100644
--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h
@@ -13,6 +13,8 @@
 #ifndef __ASSEMBLY__
 extern u32 pnv_fastsleep_workaround_at_entry[];
 extern u32 pnv_fastsleep_workaround_at_exit[];
+
+extern u64 pnv_first_deep_stop_state;
 #endif
 
 #endif
diff --git a/arch/powerpc/include/asm/kvm_book3s_asm.h 
b/arch/powerpc/include/asm/kvm_book3s_asm.h
index 72b6225..d318d43 100644
--- a/arch/powerpc/include/asm/kvm_book3s_asm.h
+++ b/arch/powerpc/include/asm/kvm_book3s_asm.h
@@ -162,7 +162,7 @@ struct kvmppc_book3s_shadow_vcpu {
 
 /* Values for kvm_state */
 #define KVM_HWTHREAD_IN_KERNEL 0
-#define KVM_HWTHREAD_IN_NAP1
+#define KVM_HWTHREAD_IN_IDLE   1
 #define KVM_HWTHREAD_IN_KVM2
 
 #endif /* __ASM_KVM_BOOK3S_ASM_H__ */
diff --git a/arch/powerpc/include/asm/machdep.h 
b/arch/powerpc/include/asm/machdep.h
index 6bdcd0d..ae3b155 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -262,6 +262,7 @@ struct machdep_calls {
 extern void e500_idle(void);
 extern void power4_idle(void);
 extern void power7_idle(void);
+extern void power_stop0(void);
 extern void ppc6xx_idle(void);
 extern void book3e_idle(void);
 
diff --git a/arch/powerpc/include/asm/opal-api.h 
b/arch/powerpc/include/asm/opal-api.h
index 9bb8ddf..7f3f8c6 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -162,13 +162,20 @@
 
 /* Device tree flags */
 
-/* Flags set in power-mgmt nodes in device tree if
- * respective idle states are supported in the platform.
+/*
+ * Flags set in power-mgmt nodes in device tree describing
+ * idle states that are supported in the platform.
  */
+
+#define OPAL_PM_TIMEBASE_STOP  0x0002
+#define OPAL_PM_LOSE_HYP_CONTEXT   0x2000
+#define OPAL_PM_LOSE_FULL_CONTEXT  0x4000
 #define OPAL_PM_NAP_ENABLED0x0001
 #define OPAL_PM_SLEEP_ENABLED  0x0002
 #define OPAL_PM_WINKLE_ENABLED 0x0004
 #define OPAL_PM_SLEEP_ENABLED_ER1  0x0008 /* with workaround */
+#define OPAL_PM_STOP_INST_FAST 0x0010
+#define OPAL_PM_STOP_INST_DEEP 0x0020
 
 /*
  * OPAL_CONFIG_CPU_IDLE_STATE parameters
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index