| Message ID | 20200710052207.12003-1-psampat@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Power10 basic energy management | expand |
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
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
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.
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
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
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