[PATCH v2 0/3] Power10 basic energy management

[PATCH v2 0/3] Power10 basic energy management

[Pratik Rajesh Sampat]

Changelog v1 --> v2:
1. Save-restore DAWR and DAWRX unconditionally as they are lost in
shallow idle states too
2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
correct naming terminology

Pratik Rajesh Sampat (3):
  powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
  powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
  powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable

 arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
 1 file changed, 22 insertions(+), 12 deletions(-)

-- 
2.25.4

[PATCH v2 1/3] powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above

[Pratik Rajesh Sampat]

POWER9 onwards the support for the registers HID1, HID4, HID5 has been
receded.
Although mfspr on the above registers worked in Power9, In Power10
simulator is unrecognized. Moving their assignment under the
check for machines lower than Power9

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 2dd467383a88..19d94d021357 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -73,9 +73,6 @@ static int pnv_save_sprs_for_deep_states(void)
 	 */
 	uint64_t lpcr_val	= mfspr(SPRN_LPCR);
 	uint64_t hid0_val	= mfspr(SPRN_HID0);
-	uint64_t hid1_val	= mfspr(SPRN_HID1);
-	uint64_t hid4_val	= mfspr(SPRN_HID4);
-	uint64_t hid5_val	= mfspr(SPRN_HID5);
 	uint64_t hmeer_val	= mfspr(SPRN_HMEER);
 	uint64_t msr_val = MSR_IDLE;
 	uint64_t psscr_val = pnv_deepest_stop_psscr_val;
@@ -117,6 +114,9 @@ static int pnv_save_sprs_for_deep_states(void)
 
 			/* Only p8 needs to set extra HID regiters */
 			if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+				uint64_t hid1_val = mfspr(SPRN_HID1);
+				uint64_t hid4_val = mfspr(SPRN_HID4);
+				uint64_t hid5_val = mfspr(SPRN_HID5);
 
 				rc = opal_slw_set_reg(pir, SPRN_HID1, hid1_val);
 				if (rc != 0)
-- 
2.25.4

[PATCH v2 1/3] powerpc/powernv/idle: Exclude mfspr on HID1, 4, 5 on P9 and above

[Pratik Rajesh Sampat]

POWER9 onwards the support for the registers HID1, HID4, HID5 has been
receded.
Although mfspr on the above registers worked in Power9, In Power10
simulator is unrecognized. Moving their assignment under the
check for machines lower than Power9

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 2dd467383a88..19d94d021357 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -73,9 +73,6 @@ static int pnv_save_sprs_for_deep_states(void)
 	 */
 	uint64_t lpcr_val	= mfspr(SPRN_LPCR);
 	uint64_t hid0_val	= mfspr(SPRN_HID0);
-	uint64_t hid1_val	= mfspr(SPRN_HID1);
-	uint64_t hid4_val	= mfspr(SPRN_HID4);
-	uint64_t hid5_val	= mfspr(SPRN_HID5);
 	uint64_t hmeer_val	= mfspr(SPRN_HMEER);
 	uint64_t msr_val = MSR_IDLE;
 	uint64_t psscr_val = pnv_deepest_stop_psscr_val;
@@ -117,6 +114,9 @@ static int pnv_save_sprs_for_deep_states(void)
 
 			/* Only p8 needs to set extra HID regiters */
 			if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+				uint64_t hid1_val = mfspr(SPRN_HID1);
+				uint64_t hid4_val = mfspr(SPRN_HID4);
+				uint64_t hid5_val = mfspr(SPRN_HID5);
 
 				rc = opal_slw_set_reg(pir, SPRN_HID1, hid1_val);
 				if (rc != 0)
-- 
2.25.4

[PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

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)) {
+		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

[PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10

[Pratik Rajesh Sampat]

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)) {
+		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

[PATCH v2 3/3] powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable

[Pratik Rajesh Sampat]

Replace the variable name from using "pnv_first_spr_loss_level" to
"pnv_first_fullstate_loss_level".

As pnv_first_spr_loss_level is supposed to be the earliest state that
has OPAL_PM_LOSE_FULL_CONTEXT set, however as shallow states too loose
SPR values, render an incorrect terminology.

Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index f2e2a6a4c274..d54e7ef234e3 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -48,7 +48,7 @@ static bool default_stop_found;
  * First stop state levels when SPR and TB loss can occur.
  */
 static u64 pnv_first_tb_loss_level = MAX_STOP_STATE + 1;
-static u64 pnv_first_spr_loss_level = MAX_STOP_STATE + 1;
+static u64 pnv_first_fullstate_loss_level = MAX_STOP_STATE + 1;
 
 /*
  * psscr value and mask of the deepest stop idle state.
@@ -659,7 +659,7 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 		  */
 		mmcr0		= mfspr(SPRN_MMCR0);
 	}
-	if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
+	if ((psscr & PSSCR_RL_MASK) >= pnv_first_fullstate_loss_level) {
 		sprs.lpcr	= mfspr(SPRN_LPCR);
 		sprs.hfscr	= mfspr(SPRN_HFSCR);
 		sprs.fscr	= mfspr(SPRN_FSCR);
@@ -751,7 +751,7 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
 	 * just always test PSSCR for SPR/TB state loss.
 	 */
 	pls = (psscr & PSSCR_PLS) >> PSSCR_PLS_SHIFT;
-	if (likely(pls < pnv_first_spr_loss_level)) {
+	if (likely(pls < pnv_first_fullstate_loss_level)) {
 		if (sprs_saved)
 			atomic_stop_thread_idle();
 		goto out;
@@ -1098,7 +1098,7 @@ static void __init pnv_power9_idle_init(void)
 	 * the deepest loss-less (OPAL_PM_STOP_INST_FAST) stop state.
 	 */
 	pnv_first_tb_loss_level = MAX_STOP_STATE + 1;
-	pnv_first_spr_loss_level = MAX_STOP_STATE + 1;
+	pnv_first_fullstate_loss_level = MAX_STOP_STATE + 1;
 	for (i = 0; i < nr_pnv_idle_states; i++) {
 		int err;
 		struct pnv_idle_states_t *state = &pnv_idle_states[i];
@@ -1109,8 +1109,8 @@ static void __init pnv_power9_idle_init(void)
 			pnv_first_tb_loss_level = psscr_rl;
 
 		if ((state->flags & OPAL_PM_LOSE_FULL_CONTEXT) &&
-		     (pnv_first_spr_loss_level > psscr_rl))
-			pnv_first_spr_loss_level = psscr_rl;
+		     (pnv_first_fullstate_loss_level > psscr_rl))
+			pnv_first_fullstate_loss_level = psscr_rl;
 
 		/*
 		 * The idle code does not deal with TB loss occurring
@@ -1121,8 +1121,8 @@ static void __init pnv_power9_idle_init(void)
 		 * compatibility.
 		 */
 		if ((state->flags & OPAL_PM_TIMEBASE_STOP) &&
-		     (pnv_first_spr_loss_level > psscr_rl))
-			pnv_first_spr_loss_level = psscr_rl;
+		     (pnv_first_fullstate_loss_level > psscr_rl))
+			pnv_first_fullstate_loss_level = psscr_rl;
 
 		err = validate_psscr_val_mask(&state->psscr_val,
 					      &state->psscr_mask,
@@ -1168,7 +1168,7 @@ static void __init pnv_power9_idle_init(void)
 	}
 
 	pr_info("cpuidle-powernv: First stop level that may lose SPRs = 0x%llx\n",
-		pnv_first_spr_loss_level);
+		pnv_first_fullstate_loss_level);
 
 	pr_info("cpuidle-powernv: First stop level that may lose timebase = 0x%llx\n",
 		pnv_first_tb_loss_level);
-- 
2.25.4

Re: [PATCH v2 0/3] Power10 basic energy management

[Nicholas Piggin]

Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
> Changelog v1 --> v2:
> 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
> shallow idle states too
> 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
> correct naming terminology
> 
> Pratik Rajesh Sampat (3):
>   powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>   powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>   powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
> 
>  arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>  1 file changed, 22 insertions(+), 12 deletions(-)

These look okay to me, but the CPU_FTR_ARCH_300 test for 
pnv_power9_idle_init() is actually wrong, it should be a PVR test 
because idle is not completely architected (not even shallow stop 
states, unfortunately).

It doesn't look like we support POWER10 idle correctly yet, and on older
kernels it wouldn't work even if we fixed newer, so ideally the PVR 
check would be backported as a fix in the front of the series.

Sadly, we have no OPAL idle driver yet. Hopefully we will before the
next processor shows up :P

Thanks,
Nick

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10

[Nicholas Piggin]

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

Re: [PATCH v2 1/3] powerpc/powernv/idle: Exclude mfspr on HID1, 4, 5 on P9 and above

[Nicholas Piggin]

Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
> POWER9 onwards the support for the registers HID1, HID4, HID5 has been
> receded.
> Although mfspr on the above registers worked in Power9, In Power10
> simulator is unrecognized. Moving their assignment under the
> check for machines lower than Power9

Seems like a good fix.

Thanks,
Nick

> 
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/idle.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 2dd467383a88..19d94d021357 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -73,9 +73,6 @@ static int pnv_save_sprs_for_deep_states(void)
>  	 */
>  	uint64_t lpcr_val	= mfspr(SPRN_LPCR);
>  	uint64_t hid0_val	= mfspr(SPRN_HID0);
> -	uint64_t hid1_val	= mfspr(SPRN_HID1);
> -	uint64_t hid4_val	= mfspr(SPRN_HID4);
> -	uint64_t hid5_val	= mfspr(SPRN_HID5);
>  	uint64_t hmeer_val	= mfspr(SPRN_HMEER);
>  	uint64_t msr_val = MSR_IDLE;
>  	uint64_t psscr_val = pnv_deepest_stop_psscr_val;
> @@ -117,6 +114,9 @@ static int pnv_save_sprs_for_deep_states(void)
>  
>  			/* Only p8 needs to set extra HID regiters */
>  			if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
> +				uint64_t hid1_val = mfspr(SPRN_HID1);
> +				uint64_t hid4_val = mfspr(SPRN_HID4);
> +				uint64_t hid5_val = mfspr(SPRN_HID5);
>  
>  				rc = opal_slw_set_reg(pir, SPRN_HID1, hid1_val);
>  				if (rc != 0)
> -- 
> 2.25.4
> 
> 

Re: [PATCH v2 0/3] Power10 basic energy management

[Pratik Sampat]

Thank you for your comments,

On 13/07/20 10:53 am, Nicholas Piggin wrote:
> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
>> Changelog v1 --> v2:
>> 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
>> shallow idle states too
>> 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
>> correct naming terminology
>>
>> Pratik Rajesh Sampat (3):
>>    powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>>    powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>>    powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
>>
>>   arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>>   1 file changed, 22 insertions(+), 12 deletions(-)
> These look okay to me, but the CPU_FTR_ARCH_300 test for
> pnv_power9_idle_init() is actually wrong, it should be a PVR test
> because idle is not completely architected (not even shallow stop
> states, unfortunately).
>
> It doesn't look like we support POWER10 idle correctly yet, and on older
> kernels it wouldn't work even if we fixed newer, so ideally the PVR
> check would be backported as a fix in the front of the series.
>
> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
> next processor shows up :P
>
> Thanks,
> Nick

So if I understand this correctly, in powernv/idle.c where we check for
CPU_FTR_ARCH_300, we should rather be making a pvr_version_is(PVR_POWER9)
check instead?

Of course, the P10 PVR and its relevant checks will have to be added then too.

Thanks
Pratik

  

Re: [PATCH v2 0/3] Power10 basic energy management

[Gautham R Shenoy]

On Mon, Jul 13, 2020 at 03:23:21PM +1000, Nicholas Piggin wrote:
> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
> > Changelog v1 --> v2:
> > 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
> > shallow idle states too
> > 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
> > correct naming terminology
> > 
> > Pratik Rajesh Sampat (3):
> >   powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
> >   powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
> >   powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
> > 
> >  arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> These look okay to me, but the CPU_FTR_ARCH_300 test for 
> pnv_power9_idle_init() is actually wrong, it should be a PVR test 
> because idle is not completely architected (not even shallow stop 
> states, unfortunately).
> 
> It doesn't look like we support POWER10 idle correctly yet, and on older
> kernels it wouldn't work even if we fixed newer, so ideally the PVR 
> check would be backported as a fix in the front of the series.
> 
> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
> next processor shows up :P

Abhishek posted a version recently :
https://patchwork.ozlabs.org/project/skiboot/patch/20200706043533.76539-1-huntbag@linux.vnet.ibm.com/


> 
> Thanks,
> Nick

--
Thanks and Regards
gautham.

Re: [PATCH v2 0/3] Power10 basic energy management

[Gautham R Shenoy]

On Mon, Jul 13, 2020 at 03:23:21PM +1000, Nicholas Piggin wrote:
> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
> > Changelog v1 --> v2:
> > 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
> > shallow idle states too
> > 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
> > correct naming terminology
> > 
> > Pratik Rajesh Sampat (3):
> >   powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
> >   powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
> >   powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
> > 
> >  arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> These look okay to me, but the CPU_FTR_ARCH_300 test for 
> pnv_power9_idle_init() is actually wrong, it should be a PVR test 
> because idle is not completely architected (not even shallow stop 
> states, unfortunately).
> 
> It doesn't look like we support POWER10 idle correctly yet, and on older
> kernels it wouldn't work even if we fixed newer, so ideally the PVR 
> check would be backported as a fix in the front of the series.
> 
> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
> next processor shows up :P

Abhishek posted a version recently :
https://patchwork.ozlabs.org/project/skiboot/patch/20200706043533.76539-1-huntbag@linux.vnet.ibm.com/


> 
> Thanks,
> Nick

--
Thanks and Regards
gautham.

Re: [PATCH v2 0/3] Power10 basic energy management

[Nicholas Piggin]

Excerpts from Pratik Sampat's message of July 13, 2020 8:02 pm:
> Thank you for your comments,
> 
> On 13/07/20 10:53 am, Nicholas Piggin wrote:
>> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
>>> Changelog v1 --> v2:
>>> 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
>>> shallow idle states too
>>> 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
>>> correct naming terminology
>>>
>>> Pratik Rajesh Sampat (3):
>>>    powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>>>    powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>>>    powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
>>>
>>>   arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>>>   1 file changed, 22 insertions(+), 12 deletions(-)
>> These look okay to me, but the CPU_FTR_ARCH_300 test for
>> pnv_power9_idle_init() is actually wrong, it should be a PVR test
>> because idle is not completely architected (not even shallow stop
>> states, unfortunately).
>>
>> It doesn't look like we support POWER10 idle correctly yet, and on older
>> kernels it wouldn't work even if we fixed newer, so ideally the PVR
>> check would be backported as a fix in the front of the series.
>>
>> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
>> next processor shows up :P
>>
>> Thanks,
>> Nick
> 
> So if I understand this correctly, in powernv/idle.c where we check for
> CPU_FTR_ARCH_300, we should rather be making a pvr_version_is(PVR_POWER9)
> check instead?
> 
> Of course, the P10 PVR and its relevant checks will have to be added then too.

Yes I think so, unfortunately.

Thanks,
Nick

Re: [PATCH v2 0/3] Power10 basic energy management

[Nicholas Piggin]

Excerpts from Gautham R Shenoy's message of July 13, 2020 8:48 pm:
> On Mon, Jul 13, 2020 at 03:23:21PM +1000, Nicholas Piggin wrote:
>> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
>> > Changelog v1 --> v2:
>> > 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
>> > shallow idle states too
>> > 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
>> > correct naming terminology
>> > 
>> > Pratik Rajesh Sampat (3):
>> >   powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>> >   powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>> >   powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
>> > 
>> >  arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>> >  1 file changed, 22 insertions(+), 12 deletions(-)
>> 
>> These look okay to me, but the CPU_FTR_ARCH_300 test for 
>> pnv_power9_idle_init() is actually wrong, it should be a PVR test 
>> because idle is not completely architected (not even shallow stop 
>> states, unfortunately).
>> 
>> It doesn't look like we support POWER10 idle correctly yet, and on older
>> kernels it wouldn't work even if we fixed newer, so ideally the PVR 
>> check would be backported as a fix in the front of the series.
>> 
>> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
>> next processor shows up :P
> 
> Abhishek posted a version recently :
> https://patchwork.ozlabs.org/project/skiboot/patch/20200706043533.76539-1-huntbag@linux.vnet.ibm.com/

Yep, I saw that. Still keen to get it working, just had other priorities 
in the short term. We'll need to do this OPAL v4 thing for it.

Thanks,
Nick

Re: [PATCH v2 0/3] Power10 basic energy management

[Nicholas Piggin]

Excerpts from Gautham R Shenoy's message of July 13, 2020 8:48 pm:
> On Mon, Jul 13, 2020 at 03:23:21PM +1000, Nicholas Piggin wrote:
>> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
>> > Changelog v1 --> v2:
>> > 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
>> > shallow idle states too
>> > 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
>> > correct naming terminology
>> > 
>> > Pratik Rajesh Sampat (3):
>> >   powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>> >   powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>> >   powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
>> > 
>> >  arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>> >  1 file changed, 22 insertions(+), 12 deletions(-)
>> 
>> These look okay to me, but the CPU_FTR_ARCH_300 test for 
>> pnv_power9_idle_init() is actually wrong, it should be a PVR test 
>> because idle is not completely architected (not even shallow stop 
>> states, unfortunately).
>> 
>> It doesn't look like we support POWER10 idle correctly yet, and on older
>> kernels it wouldn't work even if we fixed newer, so ideally the PVR 
>> check would be backported as a fix in the front of the series.
>> 
>> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
>> next processor shows up :P
> 
> Abhishek posted a version recently :
> https://patchwork.ozlabs.org/project/skiboot/patch/20200706043533.76539-1-huntbag@linux.vnet.ibm.com/

Yep, I saw that. Still keen to get it working, just had other priorities 
in the short term. We'll need to do this OPAL v4 thing for it.

Thanks,
Nick

Re: [PATCH v2 0/3] Power10 basic energy management

[Pratik Sampat]

On 13/07/20 10:20 pm, Nicholas Piggin wrote:
> Excerpts from Pratik Sampat's message of July 13, 2020 8:02 pm:
>> Thank you for your comments,
>>
>> On 13/07/20 10:53 am, Nicholas Piggin wrote:
>>> Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm:
>>>> Changelog v1 --> v2:
>>>> 1. Save-restore DAWR and DAWRX unconditionally as they are lost in
>>>> shallow idle states too
>>>> 2. Rename pnv_first_spr_loss_level to pnv_first_fullstate_loss_level to
>>>> correct naming terminology
>>>>
>>>> Pratik Rajesh Sampat (3):
>>>>     powerpc/powernv/idle: Exclude mfspr on HID1,4,5 on P9 and above
>>>>     powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
>>>>     powerpc/powernv/idle: Rename pnv_first_spr_loss_level variable
>>>>
>>>>    arch/powerpc/platforms/powernv/idle.c | 34 +++++++++++++++++----------
>>>>    1 file changed, 22 insertions(+), 12 deletions(-)
>>> These look okay to me, but the CPU_FTR_ARCH_300 test for
>>> pnv_power9_idle_init() is actually wrong, it should be a PVR test
>>> because idle is not completely architected (not even shallow stop
>>> states, unfortunately).
>>>
>>> It doesn't look like we support POWER10 idle correctly yet, and on older
>>> kernels it wouldn't work even if we fixed newer, so ideally the PVR
>>> check would be backported as a fix in the front of the series.
>>>
>>> Sadly, we have no OPAL idle driver yet. Hopefully we will before the
>>> next processor shows up :P
>>>
>>> Thanks,
>>> Nick
>> So if I understand this correctly, in powernv/idle.c where we check for
>> CPU_FTR_ARCH_300, we should rather be making a pvr_version_is(PVR_POWER9)
>> check instead?
>>
>> Of course, the P10 PVR and its relevant checks will have to be added then too.
> Yes I think so, unfortunately.
>
> Thanks,
> Nick

Sure, I'll add these checks in.

Thanks,
Pratik

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

[Ravi Bangoria]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

[Ravi Bangoria]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10

[Ravi Bangoria]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10

[Ravi Bangoria]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

[Michael Neuling]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

[Michael Neuling]

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

Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10

[Pratik Sampat]

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