* intel_idle and turbostat patches supporting Haswell @ 2013-02-01 4:11 Len Brown 2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown 0 siblings, 1 reply; 11+ messages in thread From: Len Brown @ 2013-02-01 4:11 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel Here are some pathces I have queued in my tree for the next release to support Haswell. Please let me know if you see issues with any of them. thanks! -Len ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-01 4:11 intel_idle and turbostat patches supporting Haswell Len Brown @ 2013-02-01 4:11 ` Len Brown 2013-02-01 4:11 ` [PATCH 2/4] tools/power turbostat: support HSW Len Brown ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Len Brown @ 2013-02-01 4:11 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, Len Brown From: Len Brown <len.brown@intel.com> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2 (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields) observed that the MWAIT flags for Cn on every processor to date were the same, and created get_driver_data() to supply them. Unfortunately, that assumption is false, going forward. So here we restore the MWAIT flags to the cpuidle_state table. However, instead restoring the old "driver_data" field, we put the flags into the existing "flags" field, where they probalby should have lived all along. This patch does not change any operation. This patch removes 1 of the 3 users of cpuidle_state_usage.driver_data. Perhaps some day we'll get rid of the other 2. Signed-off-by: Len Brown <len.brown@intel.com> --- drivers/idle/intel_idle.c | 75 ++++++++++++++++------------------------------- 1 file changed, 26 insertions(+), 49 deletions(-) diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 2df9414..8331753 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -109,6 +109,16 @@ static struct cpuidle_state *cpuidle_state_table; #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 /* + * MWAIT takes an 8-bit "hint" in EAX "suggesting" + * the C-state (top nibble) and sub-state (bottom nibble) + * 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc. + * + * We store the hint at the top of our "flags" for each state. + */ +#define flags_2_MWAIT_EAX(flags) (((flags) >> 24) & 0xFF) +#define MWAIT_EAX_2_flags(eax) ((eax & 0xFF) << 24) + +/* * States are indexed by the cstate number, * which is also the index into the MWAIT hint array. * Thus C0 is a dummy. @@ -118,21 +128,21 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C1 */ .name = "C1-NHM", .desc = "MWAIT 0x00", - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, .exit_latency = 3, .target_residency = 6, .enter = &intel_idle }, { /* MWAIT C2 */ .name = "C3-NHM", .desc = "MWAIT 0x10", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 20, .target_residency = 80, .enter = &intel_idle }, { /* MWAIT C3 */ .name = "C6-NHM", .desc = "MWAIT 0x20", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x20) |CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 200, .target_residency = 800, .enter = &intel_idle }, @@ -143,28 +153,28 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C1 */ .name = "C1-SNB", .desc = "MWAIT 0x00", - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, .exit_latency = 1, .target_residency = 1, .enter = &intel_idle }, { /* MWAIT C2 */ .name = "C3-SNB", .desc = "MWAIT 0x10", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 80, .target_residency = 211, .enter = &intel_idle }, { /* MWAIT C3 */ .name = "C6-SNB", .desc = "MWAIT 0x20", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 104, .target_residency = 345, .enter = &intel_idle }, { /* MWAIT C4 */ .name = "C7-SNB", .desc = "MWAIT 0x30", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 109, .target_residency = 345, .enter = &intel_idle }, @@ -175,28 +185,28 @@ static struct cpuidle_state ivb_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C1 */ .name = "C1-IVB", .desc = "MWAIT 0x00", - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, .exit_latency = 1, .target_residency = 1, .enter = &intel_idle }, { /* MWAIT C2 */ .name = "C3-IVB", .desc = "MWAIT 0x10", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 59, .target_residency = 156, .enter = &intel_idle }, { /* MWAIT C3 */ .name = "C6-IVB", .desc = "MWAIT 0x20", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 80, .target_residency = 300, .enter = &intel_idle }, { /* MWAIT C4 */ .name = "C7-IVB", .desc = "MWAIT 0x30", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 87, .target_residency = 300, .enter = &intel_idle }, @@ -207,14 +217,14 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C1 */ .name = "C1-ATM", .desc = "MWAIT 0x00", - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, .exit_latency = 1, .target_residency = 4, .enter = &intel_idle }, { /* MWAIT C2 */ .name = "C2-ATM", .desc = "MWAIT 0x10", - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID, .exit_latency = 20, .target_residency = 80, .enter = &intel_idle }, @@ -222,7 +232,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C4 */ .name = "C4-ATM", .desc = "MWAIT 0x30", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 100, .target_residency = 400, .enter = &intel_idle }, @@ -230,41 +240,12 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C6 */ .name = "C6-ATM", .desc = "MWAIT 0x52", - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .flags = MWAIT_EAX_2_flags(0x52) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 140, .target_residency = 560, .enter = &intel_idle }, }; -static long get_driver_data(int cstate) -{ - int driver_data; - switch (cstate) { - - case 1: /* MWAIT C1 */ - driver_data = 0x00; - break; - case 2: /* MWAIT C2 */ - driver_data = 0x10; - break; - case 3: /* MWAIT C3 */ - driver_data = 0x20; - break; - case 4: /* MWAIT C4 */ - driver_data = 0x30; - break; - case 5: /* MWAIT C5 */ - driver_data = 0x40; - break; - case 6: /* MWAIT C6 */ - driver_data = 0x52; - break; - default: - driver_data = 0x00; - } - return driver_data; -} - /** * intel_idle * @dev: cpuidle_device @@ -278,8 +259,7 @@ static int intel_idle(struct cpuidle_device *dev, { unsigned long ecx = 1; /* break on interrupt flag */ struct cpuidle_state *state = &drv->states[index]; - struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; - unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage); + unsigned long eax = flags_2_MWAIT_EAX(state->flags); unsigned int cstate; int cpu = smp_processor_id(); @@ -558,9 +538,6 @@ static int intel_idle_cpu_init(int cpu) if (cpuidle_state_table[cstate].enter == NULL) continue; - dev->states_usage[dev->state_count].driver_data = - (void *)get_driver_data(cstate); - dev->state_count += 1; } -- 1.8.1.2.422.g08c0e7f ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] tools/power turbostat: support HSW 2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown @ 2013-02-01 4:11 ` Len Brown 2013-02-01 4:11 ` [PATCH 3/4] tools/power turbostat: decode MSR_IA32_POWER_CTL Len Brown ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Len Brown @ 2013-02-01 4:11 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, Len Brown From: Len Brown <len.brown@intel.com> This patch enables turbostat to run properly on the next-generation Intel(R) Microarchitecture, code named Haswell (HSW). HSW supports the BCLK and counters found in SNB. Signed-off-by: Len Brown <len.brown@intel.com> --- tools/power/x86/turbostat/turbostat.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c index ce6d460..b326878 100644 --- a/tools/power/x86/turbostat/turbostat.c +++ b/tools/power/x86/turbostat/turbostat.c @@ -1397,6 +1397,9 @@ int has_nehalem_turbo_ratio_limit(unsigned int family, unsigned int model) case 0x2D: /* SNB Xeon */ case 0x3A: /* IVB */ case 0x3E: /* IVB Xeon */ + case 0x3C: /* HSW */ + case 0x3F: /* HSW */ + case 0x45: /* HSW */ return 1; case 0x2E: /* Nehalem-EX Xeon - Beckton */ case 0x2F: /* Westmere-EX Xeon - Eagleton */ @@ -1488,6 +1491,9 @@ void rapl_probe(unsigned int family, unsigned int model) switch (model) { case 0x2A: case 0x3A: + case 0x3C: /* HSW */ + case 0x3F: /* HSW */ + case 0x45: /* HSW */ do_rapl = RAPL_PKG | RAPL_CORES | RAPL_GFX; break; case 0x2D: @@ -1724,6 +1730,9 @@ int is_snb(unsigned int family, unsigned int model) case 0x2D: case 0x3A: /* IVB */ case 0x3E: /* IVB Xeon */ + case 0x3C: /* HSW */ + case 0x3F: /* HSW */ + case 0x45: /* HSW */ return 1; } return 0; @@ -2248,7 +2257,7 @@ int main(int argc, char **argv) cmdline(argc, argv); if (verbose) - fprintf(stderr, "turbostat v3.0 November 23, 2012" + fprintf(stderr, "turbostat v3.1 January 8, 2013" " - Len Brown <lenb@kernel.org>\n"); turbostat_init(); -- 1.8.1.2.422.g08c0e7f ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] tools/power turbostat: decode MSR_IA32_POWER_CTL 2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown 2013-02-01 4:11 ` [PATCH 2/4] tools/power turbostat: support HSW Len Brown @ 2013-02-01 4:11 ` Len Brown 2013-02-01 4:11 ` [PATCH 4/4] intel_idle: initial HSW support Len Brown 2013-02-01 8:44 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Daniel Lezcano 3 siblings, 0 replies; 11+ messages in thread From: Len Brown @ 2013-02-01 4:11 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, Len Brown From: Len Brown <len.brown@intel.com> When verbose is enabled, print the C1E-Enable bit in MSR_IA32_POWER_CTL. also delete some redundant tests on the verbose variable. Signed-off-by: Len Brown <len.brown@intel.com> --- arch/x86/include/uapi/asm/msr-index.h | 2 ++ tools/power/x86/turbostat/turbostat.c | 13 +++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h index 433a59f..7bdaf7c 100644 --- a/arch/x86/include/uapi/asm/msr-index.h +++ b/arch/x86/include/uapi/asm/msr-index.h @@ -103,6 +103,8 @@ #define DEBUGCTLMSR_BTS_OFF_USR (1UL << 10) #define DEBUGCTLMSR_FREEZE_LBRS_ON_PMI (1UL << 11) +#define MSR_IA32_POWER_CTL 0x000001fc + #define MSR_IA32_MC0_CTL 0x00000400 #define MSR_IA32_MC0_STATUS 0x00000401 #define MSR_IA32_MC0_ADDR 0x00000402 diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c index b326878..75f64e0 100644 --- a/tools/power/x86/turbostat/turbostat.c +++ b/tools/power/x86/turbostat/turbostat.c @@ -908,8 +908,7 @@ void print_verbose_header(void) get_msr(0, MSR_NHM_PLATFORM_INFO, &msr); - if (verbose) - fprintf(stderr, "cpu0: MSR_NHM_PLATFORM_INFO: 0x%08llx\n", msr); + fprintf(stderr, "cpu0: MSR_NHM_PLATFORM_INFO: 0x%08llx\n", msr); ratio = (msr >> 40) & 0xFF; fprintf(stderr, "%d * %.0f = %.0f MHz max efficiency\n", @@ -919,13 +918,16 @@ void print_verbose_header(void) fprintf(stderr, "%d * %.0f = %.0f MHz TSC frequency\n", ratio, bclk, ratio * bclk); + get_msr(0, MSR_IA32_POWER_CTL, &msr); + fprintf(stderr, "cpu0: MSR_IA32_POWER_CTL: 0x%08llx (C1E: %sabled)\n", + msr, msr & 0x2 ? "EN" : "DIS"); + if (!do_ivt_turbo_ratio_limit) goto print_nhm_turbo_ratio_limits; get_msr(0, MSR_IVT_TURBO_RATIO_LIMIT, &msr); - if (verbose) - fprintf(stderr, "cpu0: MSR_IVT_TURBO_RATIO_LIMIT: 0x%08llx\n", msr); + fprintf(stderr, "cpu0: MSR_IVT_TURBO_RATIO_LIMIT: 0x%08llx\n", msr); ratio = (msr >> 56) & 0xFF; if (ratio) @@ -1016,8 +1018,7 @@ print_nhm_turbo_ratio_limits: get_msr(0, MSR_NHM_TURBO_RATIO_LIMIT, &msr); - if (verbose) - fprintf(stderr, "cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x%08llx\n", msr); + fprintf(stderr, "cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x%08llx\n", msr); ratio = (msr >> 56) & 0xFF; if (ratio) -- 1.8.1.2.422.g08c0e7f ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] intel_idle: initial HSW support 2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown 2013-02-01 4:11 ` [PATCH 2/4] tools/power turbostat: support HSW Len Brown 2013-02-01 4:11 ` [PATCH 3/4] tools/power turbostat: decode MSR_IA32_POWER_CTL Len Brown @ 2013-02-01 4:11 ` Len Brown 2013-02-01 8:44 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Daniel Lezcano 3 siblings, 0 replies; 11+ messages in thread From: Len Brown @ 2013-02-01 4:11 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, Len Brown From: Len Brown <len.brown@intel.com> This patch enables intel_idle to run on the next-generation Intel(R) Microarchitecture code named Haswell. Signed-off-by: Len Brown <len.brown@intel.com> --- drivers/idle/intel_idle.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 8331753..85d50a1 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -212,6 +212,38 @@ static struct cpuidle_state ivb_cstates[MWAIT_MAX_NUM_CSTATES] = { .enter = &intel_idle }, }; +static struct cpuidle_state hsw_cstates[MWAIT_MAX_NUM_CSTATES] = { + { /* MWAIT C0 */ }, + { /* MWAIT C1 */ + .name = "C1-HSW", + .desc = "MWAIT 0x00", + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, + .exit_latency = 2, + .target_residency = 2, + .enter = &intel_idle }, + { /* MWAIT C2 */ + .name = "C3-HSW", + .desc = "MWAIT 0x10", + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .exit_latency = 39, + .target_residency = 117, + .enter = &intel_idle }, + { /* MWAIT C3 */ + .name = "C6-HSW", + .desc = "MWAIT 0x20", + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .exit_latency = 55, + .target_residency = 165, + .enter = &intel_idle }, + { /* MWAIT C4 */ + .name = "C7-HSW", + .desc = "MWAIT 0x30", + .flags = MWAIT_EAX_2_flags(0x32) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, + .exit_latency = 79, + .target_residency = 237, + .enter = &intel_idle }, +}; + static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { { /* MWAIT C0 */ }, { /* MWAIT C1 */ @@ -365,6 +397,10 @@ static const struct idle_cpu idle_cpu_ivb = { .state_table = ivb_cstates, }; +static const struct idle_cpu idle_cpu_hsw = { + .state_table = hsw_cstates, +}; + #define ICPU(model, cpu) \ { X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT, (unsigned long)&cpu } @@ -382,6 +418,9 @@ static const struct x86_cpu_id intel_idle_ids[] = { ICPU(0x2d, idle_cpu_snb), ICPU(0x3a, idle_cpu_ivb), ICPU(0x3e, idle_cpu_ivb), + ICPU(0x3c, idle_cpu_hsw), + ICPU(0x3f, idle_cpu_hsw), + ICPU(0x45, idle_cpu_hsw), {} }; MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids); -- 1.8.1.2.422.g08c0e7f ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown ` (2 preceding siblings ...) 2013-02-01 4:11 ` [PATCH 4/4] intel_idle: initial HSW support Len Brown @ 2013-02-01 8:44 ` Daniel Lezcano 2013-02-01 18:40 ` Len Brown 3 siblings, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2013-02-01 8:44 UTC (permalink / raw) To: Len Brown; +Cc: linux-pm, linux-kernel, Len Brown On 02/01/2013 05:11 AM, Len Brown wrote: > From: Len Brown <len.brown@intel.com> > > The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2 > (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields) > observed that the MWAIT flags for Cn on every processor to date were the > same, and created get_driver_data() to supply them. > > Unfortunately, that assumption is false, going forward. > So here we restore the MWAIT flags to the cpuidle_state table. > However, instead restoring the old "driver_data" field, > we put the flags into the existing "flags" field, > where they probalby should have lived all along. Removing the driver_data is a good thing but I am not sure it is the case by moving the MWAIT flags to the cpuidle_state's flags field. We are mixing arch specific flags with a generic code. This is prone to errors because new flags could appear for the cpuidle core code and could collide with the arch specific flags. Wouldn't make sense to add a private field in the struct cpuidle_state structure to let the driver/arch specific to store whatever is needed ? struct cpuidle_state { ... unsigned long private; ... } > This patch does not change any operation. > > This patch removes 1 of the 3 users of cpuidle_state_usage.driver_data. > Perhaps some day we'll get rid of the other 2. +1 :) > Signed-off-by: Len Brown <len.brown@intel.com> > --- > drivers/idle/intel_idle.c | 75 ++++++++++++++++------------------------------- > 1 file changed, 26 insertions(+), 49 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index 2df9414..8331753 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -109,6 +109,16 @@ static struct cpuidle_state *cpuidle_state_table; > #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 > > /* > + * MWAIT takes an 8-bit "hint" in EAX "suggesting" > + * the C-state (top nibble) and sub-state (bottom nibble) > + * 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc. > + * > + * We store the hint at the top of our "flags" for each state. > + */ > +#define flags_2_MWAIT_EAX(flags) (((flags) >> 24) & 0xFF) > +#define MWAIT_EAX_2_flags(eax) ((eax & 0xFF) << 24) > + > +/* > * States are indexed by the cstate number, > * which is also the index into the MWAIT hint array. > * Thus C0 is a dummy. > @@ -118,21 +128,21 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C1 */ > .name = "C1-NHM", > .desc = "MWAIT 0x00", > - .flags = CPUIDLE_FLAG_TIME_VALID, > + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, > .exit_latency = 3, > .target_residency = 6, > .enter = &intel_idle }, > { /* MWAIT C2 */ > .name = "C3-NHM", > .desc = "MWAIT 0x10", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 20, > .target_residency = 80, > .enter = &intel_idle }, > { /* MWAIT C3 */ > .name = "C6-NHM", > .desc = "MWAIT 0x20", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x20) |CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 200, > .target_residency = 800, > .enter = &intel_idle }, > @@ -143,28 +153,28 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C1 */ > .name = "C1-SNB", > .desc = "MWAIT 0x00", > - .flags = CPUIDLE_FLAG_TIME_VALID, > + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, > .exit_latency = 1, > .target_residency = 1, > .enter = &intel_idle }, > { /* MWAIT C2 */ > .name = "C3-SNB", > .desc = "MWAIT 0x10", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 80, > .target_residency = 211, > .enter = &intel_idle }, > { /* MWAIT C3 */ > .name = "C6-SNB", > .desc = "MWAIT 0x20", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 104, > .target_residency = 345, > .enter = &intel_idle }, > { /* MWAIT C4 */ > .name = "C7-SNB", > .desc = "MWAIT 0x30", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 109, > .target_residency = 345, > .enter = &intel_idle }, > @@ -175,28 +185,28 @@ static struct cpuidle_state ivb_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C1 */ > .name = "C1-IVB", > .desc = "MWAIT 0x00", > - .flags = CPUIDLE_FLAG_TIME_VALID, > + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, > .exit_latency = 1, > .target_residency = 1, > .enter = &intel_idle }, > { /* MWAIT C2 */ > .name = "C3-IVB", > .desc = "MWAIT 0x10", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 59, > .target_residency = 156, > .enter = &intel_idle }, > { /* MWAIT C3 */ > .name = "C6-IVB", > .desc = "MWAIT 0x20", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 80, > .target_residency = 300, > .enter = &intel_idle }, > { /* MWAIT C4 */ > .name = "C7-IVB", > .desc = "MWAIT 0x30", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 87, > .target_residency = 300, > .enter = &intel_idle }, > @@ -207,14 +217,14 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C1 */ > .name = "C1-ATM", > .desc = "MWAIT 0x00", > - .flags = CPUIDLE_FLAG_TIME_VALID, > + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID, > .exit_latency = 1, > .target_residency = 4, > .enter = &intel_idle }, > { /* MWAIT C2 */ > .name = "C2-ATM", > .desc = "MWAIT 0x10", > - .flags = CPUIDLE_FLAG_TIME_VALID, > + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID, > .exit_latency = 20, > .target_residency = 80, > .enter = &intel_idle }, > @@ -222,7 +232,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C4 */ > .name = "C4-ATM", > .desc = "MWAIT 0x30", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 100, > .target_residency = 400, > .enter = &intel_idle }, > @@ -230,41 +240,12 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = { > { /* MWAIT C6 */ > .name = "C6-ATM", > .desc = "MWAIT 0x52", > - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > + .flags = MWAIT_EAX_2_flags(0x52) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 140, > .target_residency = 560, > .enter = &intel_idle }, > }; > > -static long get_driver_data(int cstate) > -{ > - int driver_data; > - switch (cstate) { > - > - case 1: /* MWAIT C1 */ > - driver_data = 0x00; > - break; > - case 2: /* MWAIT C2 */ > - driver_data = 0x10; > - break; > - case 3: /* MWAIT C3 */ > - driver_data = 0x20; > - break; > - case 4: /* MWAIT C4 */ > - driver_data = 0x30; > - break; > - case 5: /* MWAIT C5 */ > - driver_data = 0x40; > - break; > - case 6: /* MWAIT C6 */ > - driver_data = 0x52; > - break; > - default: > - driver_data = 0x00; > - } > - return driver_data; > -} > - > /** > * intel_idle > * @dev: cpuidle_device > @@ -278,8 +259,7 @@ static int intel_idle(struct cpuidle_device *dev, > { > unsigned long ecx = 1; /* break on interrupt flag */ > struct cpuidle_state *state = &drv->states[index]; > - struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; > - unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage); > + unsigned long eax = flags_2_MWAIT_EAX(state->flags); > unsigned int cstate; > int cpu = smp_processor_id(); > > @@ -558,9 +538,6 @@ static int intel_idle_cpu_init(int cpu) > if (cpuidle_state_table[cstate].enter == NULL) > continue; > > - dev->states_usage[dev->state_count].driver_data = > - (void *)get_driver_data(cstate); > - > dev->state_count += 1; > } > > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-01 8:44 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Daniel Lezcano @ 2013-02-01 18:40 ` Len Brown 2013-02-01 22:16 ` Daniel Lezcano 0 siblings, 1 reply; 11+ messages in thread From: Len Brown @ 2013-02-01 18:40 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-pm, linux-kernel On 02/01/2013 03:44 AM, Daniel Lezcano wrote: > On 02/01/2013 05:11 AM, Len Brown wrote: >> From: Len Brown <len.brown@intel.com> >> >> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2 >> (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields) >> observed that the MWAIT flags for Cn on every processor to date were the >> same, and created get_driver_data() to supply them. >> >> Unfortunately, that assumption is false, going forward. >> So here we restore the MWAIT flags to the cpuidle_state table. >> However, instead restoring the old "driver_data" field, >> we put the flags into the existing "flags" field, >> where they probalby should have lived all along. > > Removing the driver_data is a good thing but I am not sure it is the > case by moving the MWAIT flags to the cpuidle_state's flags field. We > are mixing arch specific flags with a generic code. > > This is prone to errors because new flags could appear for the cpuidle > core code and could collide with the arch specific flags. > > Wouldn't make sense to add a private field in the struct cpuidle_state > structure to let the driver/arch specific to store whatever is needed ? > > struct cpuidle_state { > > ... > unsigned long private; > ... > > } The top half of the flags are reserved for the driver, as noted by the definition of CPUIDLE_DRIVER_FLAGS_MASK with the generic flag definitions: #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */ #define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */ #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) intel_idle already uses a driver-specific flag: #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 This patch just uses 4 more bits along with that one. Sure, if we run out of space, we can add an additional field. But I don't see an immediate need for it. thanks, Len Brown Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-01 18:40 ` Len Brown @ 2013-02-01 22:16 ` Daniel Lezcano 2013-02-02 2:16 ` Len Brown 0 siblings, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2013-02-01 22:16 UTC (permalink / raw) To: Len Brown; +Cc: linux-pm, linux-kernel On 02/01/2013 07:40 PM, Len Brown wrote: > On 02/01/2013 03:44 AM, Daniel Lezcano wrote: >> On 02/01/2013 05:11 AM, Len Brown wrote: >>> From: Len Brown <len.brown@intel.com> >>> >>> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2 >>> (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields) >>> observed that the MWAIT flags for Cn on every processor to date were the >>> same, and created get_driver_data() to supply them. >>> >>> Unfortunately, that assumption is false, going forward. >>> So here we restore the MWAIT flags to the cpuidle_state table. >>> However, instead restoring the old "driver_data" field, >>> we put the flags into the existing "flags" field, >>> where they probalby should have lived all along. >> >> Removing the driver_data is a good thing but I am not sure it is the >> case by moving the MWAIT flags to the cpuidle_state's flags field. We >> are mixing arch specific flags with a generic code. >> >> This is prone to errors because new flags could appear for the cpuidle >> core code and could collide with the arch specific flags. >> >> Wouldn't make sense to add a private field in the struct cpuidle_state >> structure to let the driver/arch specific to store whatever is needed ? >> >> struct cpuidle_state { >> >> ... >> unsigned long private; >> ... >> >> } > > The top half of the flags are reserved for the driver, > as noted by the definition of CPUIDLE_DRIVER_FLAGS_MASK > with the generic flag definitions: > > #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */ > #define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */ > > #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) > > intel_idle already uses a driver-specific flag: > > #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 > > This patch just uses 4 more bits along with that one. Ok. In this case it would make sense to move this flag out of the generic core code to the intel_idle.c file ? and move the [dec/en]coding macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate name for a generic code) to the cpuidle.h file ? -- Daniel > Sure, if we run out of space, we can add an additional field. > But I don't see an immediate need for it. > > thanks, > Len Brown > Intel Open Source Technology Center > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-01 22:16 ` Daniel Lezcano @ 2013-02-02 2:16 ` Len Brown [not found] ` <510C7708.4080707-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Len Brown @ 2013-02-02 2:16 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-pm, linux-kernel >> intel_idle already uses a driver-specific flag: >> >> #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 >> >> This patch just uses 4 more bits along with that one. > > Ok. In this case it would make sense to move this flag out of the > generic core code to the intel_idle.c file ? This flag is already local to intel_idle.c. If another architecture finds it useful, then yes, it would make sense to move it to the shared header. > and move the [dec/en]coding > macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate > name for a generic code) to the cpuidle.h file ? I think that a driver's private flag definitions should remain local to the driver. It makes no sense to pollute the name space of other drivers for stuff that doesn't mean anything to them. MWAIT is pretty specific to x86 -- and re-naming it to something 'generic' isn't going to make the code easier to read. thanks, Len Brown, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <510C7708.4080707-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags [not found] ` <510C7708.4080707-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> @ 2013-02-02 9:44 ` Daniel Lezcano 2013-02-02 20:18 ` Len Brown 0 siblings, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2013-02-02 9:44 UTC (permalink / raw) To: Len Brown Cc: Lists Linaro-dev, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA On 02/02/2013 03:16 AM, Len Brown wrote: > >>> intel_idle already uses a driver-specific flag: >>> >>> #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 >>> >>> This patch just uses 4 more bits along with that one. >> >> Ok. In this case it would make sense to move this flag out of the >> generic core code to the intel_idle.c file ? > > This flag is already local to intel_idle.c. > If another architecture finds it useful, > then yes, it would make sense to move it to the shared header. Oh, right. Sorry I puzzled myself with the name. I was convinced it was in the shared header. >> and move the [dec/en]coding >> macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate >> name for a generic code) to the cpuidle.h file ? > > I think that a driver's private flag definitions > should remain local to the driver. It makes no sense > to pollute the name space of other drivers for stuff > that doesn't mean anything to them. MWAIT is pretty > specific to x86 -- and re-naming it to something 'generic' > isn't going to make the code easier to read. Ok, let me rephrase it because I think how it was presented was not clear. As we want to use the half of the state flags for private purpose, I suggested to add the encoding/decoding function in the shared header file. The mwait eax flags are not encoded and the CPUIDLE_FLAG_TLB_FLUSHED is encoded. I suggested to unify both and to use an encoding function from the shared header file. #define CPUIDLE_PRIVATE_FLAGS(_flags_) \ ((_flags_) << 16) & CPUIDLE_DRIVER_FLAGS_MASK For example: #define FLAG_TLB_FLUSHED CPUIDLE_PRIVATE_FLAGS(0x1) #define FLAG_MWAIT_C1 CPUIDLE_PRIVATE_FLAGS(0x0) #define FLAG_MWAIT_C2 CPUIDLE_PRIVATE_FLAGS(0x10) #define FLAG_MWAIT_C3 CPUIDLE_PRIVATE_FLAGS(0x20) #define FLAG_MWAIT_C4 CPUIDLE_PRIVATE_FLAGS(0x30) #define FLAG_MWAIT_C5 CPUIDLE_PRIVATE_FLAGS(0x40) #define FLAG_MWAIT_C6 CPUIDLE_PRIVATE_FLAGS(0x52) And then: ... .flags = FLAG_TLB_FLUSHED | FLAG_MWAIT_C2 | CPUIDLE_FLAG_TIME_VALID ... But in the idle function, you need to retrieve the 'value' of the EAX not a flag, so there is the need for an extra macro conversion and mask the TLB flag. Well, this is a detail, so feel free to ignore this suggestion :) Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags 2013-02-02 9:44 ` Daniel Lezcano @ 2013-02-02 20:18 ` Len Brown 0 siblings, 0 replies; 11+ messages in thread From: Len Brown @ 2013-02-02 20:18 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-pm, linux-kernel, Lists Linaro-dev >> I think that a driver's private flag definitions >> should remain local to the driver. It makes no sense >> to pollute the name space of other drivers for stuff >> that doesn't mean anything to them. MWAIT is pretty >> specific to x86 -- and re-naming it to something 'generic' >> isn't going to make the code easier to read. > > Ok, let me rephrase it because I think how it was presented was not clear. > > As we want to use the half of the state flags for private purpose, I > suggested to add the encoding/decoding function in the shared header file. > > The mwait eax flags are not encoded and the CPUIDLE_FLAG_TLB_FLUSHED is > encoded. > > I suggested to unify both and to use an encoding function from the > shared header file. > > #define CPUIDLE_PRIVATE_FLAGS(_flags_) \ > ((_flags_) << 16) & CPUIDLE_DRIVER_FLAGS_MASK > > For example: > > #define FLAG_TLB_FLUSHED CPUIDLE_PRIVATE_FLAGS(0x1) > #define FLAG_MWAIT_C1 CPUIDLE_PRIVATE_FLAGS(0x0) > #define FLAG_MWAIT_C2 CPUIDLE_PRIVATE_FLAGS(0x10) > #define FLAG_MWAIT_C3 CPUIDLE_PRIVATE_FLAGS(0x20) > #define FLAG_MWAIT_C4 CPUIDLE_PRIVATE_FLAGS(0x30) > #define FLAG_MWAIT_C5 CPUIDLE_PRIVATE_FLAGS(0x40) > #define FLAG_MWAIT_C6 CPUIDLE_PRIVATE_FLAGS(0x52) > > And then: > > ... > > .flags = FLAG_TLB_FLUSHED | FLAG_MWAIT_C2 | CPUIDLE_FLAG_TIME_VALID I like your syntax better than what I used -- thanks for suggesting it. I can't use it as-is, however, because I really do need to be able to support unique parameters on each entry (eg. different flags for C3 on different processors). When some of the logical and functional changes in flux settle out I'll come back to re-visit the syntax. thanks, -Len Brown, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-02-02 20:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-01 4:11 intel_idle and turbostat patches supporting Haswell Len Brown
2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown
2013-02-01 4:11 ` [PATCH 2/4] tools/power turbostat: support HSW Len Brown
2013-02-01 4:11 ` [PATCH 3/4] tools/power turbostat: decode MSR_IA32_POWER_CTL Len Brown
2013-02-01 4:11 ` [PATCH 4/4] intel_idle: initial HSW support Len Brown
2013-02-01 8:44 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Daniel Lezcano
2013-02-01 18:40 ` Len Brown
2013-02-01 22:16 ` Daniel Lezcano
2013-02-02 2:16 ` Len Brown
[not found] ` <510C7708.4080707-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2013-02-02 9:44 ` Daniel Lezcano
2013-02-02 20:18 ` Len Brown
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).