Re: [PATCH v4 3/4] powernv: Pass PSSCR value and mask to power9_idle_stop

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

Hi Balbir,


Thanks for reviewing the patch. Please find my comments inline.

On Wed, Dec 14, 2016 at 11:16:26AM +1100, Balbir Singh wrote:
[..snip..]
> >  
> >  /*
> > - * r3 - requested stop state
> > + * r3 - PSSCR value corresponding to the requested stop state.
> >   */
> >  power_enter_stop:
> >  #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> > @@ -274,9 +272,19 @@ power_enter_stop:
> >  	stb	r4,HSTATE_HWTHREAD_STATE(r13)
> >  #endif
> >  /*
> > + * Check if we are executing the lite variant with ESL=EC=0
> > + */
> > +	andis.   r4, r3, PSSCR_EC_ESL_MASK_SHIFTED
> 
> r4 = psscr & (PSSCR_EC | PSSCR_ESL)
> 
> > +	andi.    r3, r3, PSSCR_RL_MASK   /* r3 = requested stop state */
> 
> r3 = psscr & RL_MASK (requested mask). 
> 
> Why do we do and andis. followed by andi. and a compdi below?

Do you mean why are we not using the CR0 value instead of using cmpdi
again ? Hmm.. The subsequent code expect r3 to contain only the RL
value. So, how about the following?

andi.  r4, r3, PSSCR_RL_MASK;
andis. r3, r3, PSSCR_EC_ESL_MASK_SHIFTED;
mr     r3, r4;
bne    1f;

> 
> > +	cmpdi	 r4, 0
> 
> r4 == 0 implies we either both PSSCR_EC|ESL are cleared.

> I am not sure if our checks for EC are well defined/implemented.
> Should we worry about EC at all at this point?

Yes, we need to check the value of EC. Because if EC == 0, that
implies that the hardware will wake up from the stop instruction at
the subsequent instruction which we need to handle. This behaviour is
only available from POWER9 onwards, since on POWER8, the wakeup from
nap,sleep and winkle were always at 0x100. Hence the existing code
assumes that all the wakeups are at 0x100, which is what this patch
modifies.


> 
> > +	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)
> >  	cmpd	r3,r4
> >  	bge	2f
> > @@ -353,16 +361,17 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
> >  	ld	r3,ORIG_GPR3(r1);	/* Restore original r3 */	\
> >  20:	nop;
> >  
> > -
> 
> Spurious change?

There were two empty lines for no particular reason. So got rid of one
of them.

> 
> >  /*
> > - * 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
> > +	andc    r5, 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 */
> >  /*

[..snip..]

> > @@ -253,9 +259,11 @@ static void power9_idle(void)
> >  u64 pnv_first_deep_stop_state = MAX_STOP_STATE;
> >  
> >  /*
> > - * Deepest stop idle state. Used when a cpu is offlined
> > + * psscr value and mask of the deepest stop idle state.
> > + * Used when a cpu is offlined.
> >   */
> > -u64 pnv_deepest_stop_state;
> > +u64 pnv_deepest_stop_psscr_val;
> > +u64 pnv_deepest_stop_psscr_mask;
> >  
> >  /*
> >   * Power ISA 3.0 idle initialization.
> > @@ -302,28 +310,58 @@ static int __init pnv_arch300_idle_init(struct device_node *np, u32 *flags,
> >  					int dt_idle_states)
> 
> In some cases we say power9 and arch300 in others, not related to
> >  this patchset, but just a generic comment

Will see if I can make this consistent.


[..snip..]

> > @@ -333,12 +371,27 @@ static int __init pnv_arch300_idle_init(struct device_node *np, u32 *flags,
> >  		     (pnv_first_deep_stop_state > psscr_rl))
> >  			pnv_first_deep_stop_state = psscr_rl;
> >  
> > -		if (pnv_deepest_stop_state < psscr_rl)
> > -			pnv_deepest_stop_state = psscr_rl;
> > -	}
> > +		if (max_residency_ns < residency_ns[i]) {
> > +			max_residency_ns = residency_ns[i];
> > +			pnv_deepest_stop_psscr_val =
> > +				compute_psscr_val(psscr_val[i], psscr_mask[i]);
> > +			pnv_deepest_stop_psscr_mask =
> > +				compute_psscr_mask(psscr_mask[i]);
> > +		}
> >  
> 
> Does it make sense to have them sorted and then use the last value
> from the array?

Yes, if the firmware can be relied upon to do this, we can obtain the
deepest_stop_psscr_val and the mask in constant time.

However, this init function is called only once during the boot, and
we are anyway iterating over all the idle states to find the first
deep stop state and the default stop state. So the optimization for
deepest_stop_psscr_val and mask may not gain us much.

> 
> 
> Balbir Singh
> 

--
Thanks and Regards
gautham.