| Message ID | 20200710052207.12003-3-psampat@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Power10 basic energy management | expand |
Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm: > Additional registers DAWR0, DAWRX0 may be lost on Power 10 for > stop levels < 4. > Therefore save the values of these SPRs before entering a "stop" > state and restore their values on wakeup. Hmm, where do you get this from? Documentation I see says DAWR is lost on POWER9 but not P10. Does idle thread even need to save DAWR, or does it get switched when going to a thread that has a watchpoint set? Thanks, Nick > > Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> > --- > arch/powerpc/platforms/powernv/idle.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > index 19d94d021357..f2e2a6a4c274 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -600,6 +600,8 @@ struct p9_sprs { > u64 iamr; > u64 amor; > u64 uamor; > + u64 dawr0; > + u64 dawrx0; > }; > > static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > sprs.iamr = mfspr(SPRN_IAMR); > sprs.amor = mfspr(SPRN_AMOR); > sprs.uamor = mfspr(SPRN_UAMOR); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + sprs.dawr0 = mfspr(SPRN_DAWR0); > + sprs.dawrx0 = mfspr(SPRN_DAWRX0); > + } > > srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ > > @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > mtspr(SPRN_IAMR, sprs.iamr); > mtspr(SPRN_AMOR, sprs.amor); > mtspr(SPRN_UAMOR, sprs.uamor); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + mtspr(SPRN_DAWR0, sprs.dawr0); > + mtspr(SPRN_DAWRX0, sprs.dawrx0); > + } > > /* > * Workaround for POWER9 DD2.0, if we lost resources, the ERAT > -- > 2.25.4 > >
Hi Pratik, On 7/10/20 10:52 AM, Pratik Rajesh Sampat wrote: > Additional registers DAWR0, DAWRX0 may be lost on Power 10 for > stop levels < 4. p10 has one more pair DAWR1/DAWRX1. Please include that as well. Ravi
Hi Nick, On 7/13/20 11:22 AM, Nicholas Piggin wrote: > Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm: >> Additional registers DAWR0, DAWRX0 may be lost on Power 10 for >> stop levels < 4. >> Therefore save the values of these SPRs before entering a "stop" >> state and restore their values on wakeup. > > Hmm, where do you get this from? Documentation I see says DAWR is lost > on POWER9 but not P10. > > Does idle thread even need to save DAWR, or does it get switched when > going to a thread that has a watchpoint set? I don't know how idle states works internally but IIUC, we need to save/restore DAWRs. This is needed when user creates per-cpu watchpoint event. Ravi
On Fri, 2020-07-10 at 10:52 +0530, Pratik Rajesh Sampat wrote: > Additional registers DAWR0, DAWRX0 may be lost on Power 10 for > stop levels < 4. > Therefore save the values of these SPRs before entering a "stop" > state and restore their values on wakeup. > > Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> > --- > arch/powerpc/platforms/powernv/idle.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/idle.c > b/arch/powerpc/platforms/powernv/idle.c > index 19d94d021357..f2e2a6a4c274 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -600,6 +600,8 @@ struct p9_sprs { > u64 iamr; > u64 amor; > u64 uamor; > + u64 dawr0; > + u64 dawrx0; > }; > > static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > sprs.iamr = mfspr(SPRN_IAMR); > sprs.amor = mfspr(SPRN_AMOR); > sprs.uamor = mfspr(SPRN_UAMOR); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { Can you add a comment here saying even though DAWR0 is ARCH_30, it's only required to be saved on 31. Otherwise this looks pretty odd. > + sprs.dawr0 = mfspr(SPRN_DAWR0); > + sprs.dawrx0 = mfspr(SPRN_DAWRX0); > + } > > srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ > > @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > mtspr(SPRN_IAMR, sprs.iamr); > mtspr(SPRN_AMOR, sprs.amor); > mtspr(SPRN_UAMOR, sprs.uamor); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + mtspr(SPRN_DAWR0, sprs.dawr0); > + mtspr(SPRN_DAWRX0, sprs.dawrx0); > + } > > /* > * Workaround for POWER9 DD2.0, if we lost resources, the ERAT
On 24/07/20 6:55 am, Michael Neuling wrote: > On Fri, 2020-07-10 at 10:52 +0530, Pratik Rajesh Sampat wrote: >> Additional registers DAWR0, DAWRX0 may be lost on Power 10 for >> stop levels < 4. >> Therefore save the values of these SPRs before entering a "stop" >> state and restore their values on wakeup. >> >> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >> --- >> arch/powerpc/platforms/powernv/idle.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/powerpc/platforms/powernv/idle.c >> b/arch/powerpc/platforms/powernv/idle.c >> index 19d94d021357..f2e2a6a4c274 100644 >> --- a/arch/powerpc/platforms/powernv/idle.c >> +++ b/arch/powerpc/platforms/powernv/idle.c >> @@ -600,6 +600,8 @@ struct p9_sprs { >> u64 iamr; >> u64 amor; >> u64 uamor; >> + u64 dawr0; >> + u64 dawrx0; >> }; >> >> static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) >> @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long >> psscr, bool mmu_on) >> sprs.iamr = mfspr(SPRN_IAMR); >> sprs.amor = mfspr(SPRN_AMOR); >> sprs.uamor = mfspr(SPRN_UAMOR); >> + if (cpu_has_feature(CPU_FTR_ARCH_31)) { You are actually viewing an old version of the patches The main point of change were based on comments from Nick Piggin, I have changed the top level function check from ARCH_300 to a P9 PVR check instead. A similar thing needs to be done for P10, however as the P10 PVR isn't exposed yet, I've shelved this particular patch. Nick's comment to check based on PVR:https://lkml.org/lkml/2020/7/13/1018 v4 of the series:https://lkml.org/lkml/2020/7/21/784 Thanks for your review, Pratik > Can you add a comment here saying even though DAWR0 is ARCH_30, it's only > required to be saved on 31. Otherwise this looks pretty odd. > >> + sprs.dawr0 = mfspr(SPRN_DAWR0); >> + sprs.dawrx0 = mfspr(SPRN_DAWRX0); >> + } >> >> srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ >> >> @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long >> psscr, bool mmu_on) >> mtspr(SPRN_IAMR, sprs.iamr); >> mtspr(SPRN_AMOR, sprs.amor); >> mtspr(SPRN_UAMOR, sprs.uamor); >> + if (cpu_has_feature(CPU_FTR_ARCH_31)) { >> + mtspr(SPRN_DAWR0, sprs.dawr0); >> + mtspr(SPRN_DAWRX0, sprs.dawrx0); >> + } >> >> /* >> * Workaround for POWER9 DD2.0, if we lost resources, the ERAT
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 19d94d021357..f2e2a6a4c274 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -600,6 +600,8 @@ struct p9_sprs { u64 iamr; u64 amor; u64 uamor; + u64 dawr0; + u64 dawrx0; }; static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) sprs.iamr = mfspr(SPRN_IAMR); sprs.amor = mfspr(SPRN_AMOR); sprs.uamor = mfspr(SPRN_UAMOR); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { + sprs.dawr0 = mfspr(SPRN_DAWR0); + sprs.dawrx0 = mfspr(SPRN_DAWRX0); + } srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) mtspr(SPRN_IAMR, sprs.iamr); mtspr(SPRN_AMOR, sprs.amor); mtspr(SPRN_UAMOR, sprs.uamor); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { + mtspr(SPRN_DAWR0, sprs.dawr0); + mtspr(SPRN_DAWRX0, sprs.dawrx0); + } /* * Workaround for POWER9 DD2.0, if we lost resources, the ERAT
Additional registers DAWR0, DAWRX0 may be lost on Power 10 for stop levels < 4. Therefore save the values of these SPRs before entering a "stop" state and restore their values on wakeup. Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> --- arch/powerpc/platforms/powernv/idle.c | 10 ++++++++++ 1 file changed, 10 insertions(+)