All of lore.kernel.org
 help / color / mirror / Atom feed
* 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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
  2013-02-02  9:44             ` Daniel Lezcano
  0 siblings, 1 reply; 12+ 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] 12+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags
@ 2013-02-02  9:44             ` Daniel Lezcano
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2013-02-02  9:44 UTC (permalink / raw)
  To: Len Brown; +Cc: linux-pm, linux-kernel, Lists Linaro-dev

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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags
@ 2013-02-02  9:44             ` Daniel Lezcano
  0 siblings, 0 replies; 12+ 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] 12+ 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
  -1 siblings, 0 replies; 12+ 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] 12+ messages in thread
end of thread, other threads:[~2013-02-02 20:18 UTC | newest]

Thread overview: 12+ 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
2013-02-02  9:44           ` Daniel Lezcano
2013-02-02  9:44             ` Daniel Lezcano
2013-02-02 20:18             ` Len Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.