"Gautham R. Shenoy" <e...@linux.vnet.ibm.com> writes: > From: "Gautham R. Shenoy" <e...@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
