Messages in this thread | | | Subject | Re: [RFC 1/3] Interface for an idle-stop dependency structure | From | Pratik Sampat <> | Date | Sun, 12 Apr 2020 17:48:43 +0530 |
| |
Hello Gautham
On 08/04/20 4:21 pm, Gautham R Shenoy wrote: > Hi Pratik, > > On Wed, Mar 04, 2020 at 09:31:21PM +0530, Pratik Rajesh Sampat wrote: >> Design patch to introduce the idea of having a dependency structure for >> idle-stop. The structure encapsulates the following: >> 1. Bitmask for version of idle-stop >> 2. Bitmask for propterties like ENABLE/DISABLE >> 3. Function pointer which helps handle how the stop must be invoked >> >> The commit lays a foundation for other idle-stop versions to be added >> and handled cleanly based on their specified requirments. >> Currently it handles the existing "idle-stop" version by setting the >> discovery bits and the function pointer. > So, if this patch is applied, and we are running with an OPAL that > doesn't publish the "idle-stop" dt-cpu-feature, then the goal is to > not enable any stop states. Is this correct ? > Yes, all states will be disabled with no power saving.
>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com> >> --- >> arch/powerpc/include/asm/processor.h | 17 +++++++++++++++++ >> arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +++++ >> arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++---- >> drivers/cpuidle/cpuidle-powernv.c | 3 ++- >> 4 files changed, 37 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h >> index eedcbfb9a6ff..da59f01a5c09 100644 >> --- a/arch/powerpc/include/asm/processor.h >> +++ b/arch/powerpc/include/asm/processor.h >> @@ -429,6 +429,23 @@ extern void power4_idle_nap(void); >> extern unsigned long cpuidle_disable; >> enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF}; >> >> +#define STOP_ENABLE 0x00000001 >> + >> +#define STOP_VERSION_P9 0x1 >> + >> +/* >> + * Classify the dependencies of the stop states >> + * @idle_stop: function handler to handle the quirk stop version >> + * @cpuidle_prop: Signify support for stop states through kernel and/or firmware >> + * @stop_version: Classify quirk versions for stop states >> + */ >> +typedef struct { >> + unsigned long (*idle_stop)(unsigned long, bool); >> + uint8_t cpuidle_prop; >> + uint8_t stop_version; > Why do we need both cpuidle_prop and stop_version ?
The idea is that each stop_version has house multitude of overlapping properties. So the idea is to give a clean distinction. However, I can see now that the versioning and properties could be embedded in a single bitmask
>> @@ -657,6 +659,9 @@ static void __init cpufeatures_setup_start(u32 isa) >> } >> } >> >> +stop_deps_t stop_dep = {NULL, 0x0, 0x0}; >> +EXPORT_SYMBOL(stop_dep); >> + >> static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f) >> { >> const struct dt_cpu_feature_match *m; >> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c >> index 78599bca66c2..c32cdc37acf4 100644 >> --- a/arch/powerpc/platforms/powernv/idle.c >> +++ b/arch/powerpc/platforms/powernv/idle.c >> @@ -812,7 +812,7 @@ static unsigned long power9_offline_stop(unsigned long psscr) >> >> #ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE >> __ppc64_runlatch_off(); >> - srr1 = power9_idle_stop(psscr, true); >> + srr1 = stop_dep.idle_stop(psscr, true); >> __ppc64_runlatch_on(); >> #else >> /* >> @@ -828,7 +828,7 @@ static unsigned long power9_offline_stop(unsigned long psscr) >> local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_IDLE; >> >> __ppc64_runlatch_off(); >> - srr1 = power9_idle_stop(psscr, false); >> + srr1 = stop_dep.idle_stop(psscr, true); >> __ppc64_runlatch_on(); >> >> local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_KERNEL; >> @@ -856,7 +856,7 @@ void power9_idle_type(unsigned long stop_psscr_val, >> psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val; >> >> __ppc64_runlatch_off(); >> - srr1 = power9_idle_stop(psscr, true); >> + srr1 = stop_dep.idle_stop(psscr, true); >> __ppc64_runlatch_on(); >> > There is one other place in arch/powerpc/kvm/book3s_hv_rmhandlers.S > where call isa300_idle_stop_mayloss (this is kvm_nap_sequence). > > So, if stop states are not supported, then, KVM subsystem should know > about it. Some KVM configurations depend on putting the secondary > threads of the core offline into an idle state whose wakeup is from > 0x100 vector. Your patch doesn't address that part. > Sure, I'll make sure to address it there too.
> >> goto out; >> + switch(stop_dep.stop_version) { >> + case STOP_VERSION_P9: >> + stop_dep.idle_stop = power9_idle_stop; >> + break; >> + default: >> + stop_dep.idle_stop = NULL; > You should add a pr_warn() here that stop state isn't supported > because the kernel doesn't know about the version. > Sure
Thanks Pratik
| |