Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop

[Michael Ellerman]

"Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx> writes:

> From: "Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx>
>
> 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