linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Gautham R Shenoy <ego@linux.vnet.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
	Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>,
	"Shreyas B. Prabhu" <shreyasbp@gmail.com>,
	Michael Neuling <mikey@neuling.org>,
	Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Subject: Re: [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop
Date: Fri, 23 Sep 2016 19:18:52 +0530	[thread overview]
Message-ID: <20160923134852.GA22259@in.ibm.com> (raw)
In-Reply-To: <871t0a98tq.fsf@concordia.ellerman.id.au>

Hi Michael,

On Fri, Sep 23, 2016 at 09:03:45PM +1000, Michael Ellerman wrote:
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> 
> > From: "Gautham R. Shenoy" <ego@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?

It is supposed to be a new stop state, whose behaviour is different
from the existing stop states in terms of where it wakes up from stop.

You are right, it has 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?

It does save less power compared to the corresponding variant where
ESL=EC=1.

> 
> > 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.

Let me rework the patch to use the cpu-idle-state-psscr property which
is currently set to 0 in the firmware and
cpu-idle-state-psscr-mask which is set to 0xF for all the stop
states.

I agree, we can do without the hardcoding of the mask in the
kernel.

> 
> > @@ -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?

Because we are only setting the Requested Level field which is the
bottom 4 bits. Everything else is taken from the hardcoded mask, which
is not incorrect, but not a very flexible design.

Thanks for the comments Michael. I will come back with a cleaner
patch.

> 
> cheers
> 

--
Thanks and Regards
gautham.

      reply	other threads:[~2016-09-23 13:49 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-16  9:47 [PATCH 0/2] powernv: Implement lite variant of stop with ESL=EC=0 Gautham R. Shenoy
2016-09-16  9:47 ` [PATCH 1/2] powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro Gautham R. Shenoy
2016-09-16  9:47 ` [PATCH 2/2] powernv:idle:Implement lite variant of power_enter_stop Gautham R. Shenoy
2016-09-20  5:54   ` Balbir Singh
2016-09-20  8:32     ` Gautham R Shenoy
2016-09-23 11:03   ` Michael Ellerman
2016-09-23 13:48     ` Gautham R Shenoy [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160923134852.GA22259@in.ibm.com \
    --to=ego@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mikey@neuling.org \
    --cc=mpe@ellerman.id.au \
    --cc=shilpa.bhat@linux.vnet.ibm.com \
    --cc=shreyasbp@gmail.com \
    --cc=svaidy@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).