All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] powernow-k6: fixes for the frequency driver
@ 2013-12-12  0:37 Mikulas Patocka
  2013-12-12  0:38 ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Mikulas Patocka
  2014-01-06 20:51 ` [PATCH 0/3] powernow-k6: fixes for the frequency driver Rafael J. Wysocki
  0 siblings, 2 replies; 13+ messages in thread
From: Mikulas Patocka @ 2013-12-12  0:37 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar; +Cc: cpufreq, linux-kernel

Hi

Here I'm sending three patches for k6 frequency governor.

1. the processor doesn't respond to cache snoops when changing the 
frequency, so we need to disable cache while doing the change.

2. it is impossible to read the default multiplier. After reset, the 
register always reports zero regardless of the current multiplier. So we 
set the multiplier based on current frequency and allow the user to 
override it with module parameter.

3. reorder frequencies from the highest to the lowest.

Mikulas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] powernow-k6: disable cache when changing frequency
  2013-12-12  0:37 [PATCH 0/3] powernow-k6: fixes for the frequency driver Mikulas Patocka
@ 2013-12-12  0:38 ` Mikulas Patocka
  2013-12-12  0:38   ` [PATCH 2/3] powernow-k6: correctly initialize default parameters Mikulas Patocka
  2013-12-17  8:45   ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Viresh Kumar
  2014-01-06 20:51 ` [PATCH 0/3] powernow-k6: fixes for the frequency driver Rafael J. Wysocki
  1 sibling, 2 replies; 13+ messages in thread
From: Mikulas Patocka @ 2013-12-12  0:38 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar; +Cc: cpufreq, linux-kernel

I found out that a system with k6-3+ processor is unstable during network
server load. The system locks up or the network card stops receiving. The
reason for the instability is the CPU frequency scaling.

During frequency transition the processor is in "EPM Stop Grant" state.
The documentation says that the processor doesn't respond to inquiry
requests in this state. Consequently, coherency of processor caches and
bus master devices is not maintained, causing the system instability.

This patch flushes the cache during frequency transition. It fixes the
instability.

Other minor changes:
* u64 invalue changed to unsigned long because the variable is 32-bit
* move the logic to set the multiplier to a separate function
  powernow_k6_set_cpu_multiplier
* preserve lower 5 bits of the powernow port instead of 4 (the voltage
  field has 5 bits)
* mask interrupts when reading the multiplier, so that the port is not
  open during other activity (running other kernel code with the port open
  shouldn't cause any misbehavior, but we should better be safe and keep
  the port closed)

This patch should be backported to all stable kernels. If it doesn't
apply cleanly, change it, or ask me to change it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@kernel.org

---
 drivers/cpufreq/powernow-k6.c |   56 +++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 17 deletions(-)

Index: linux-3.13-rc3/drivers/cpufreq/powernow-k6.c
===================================================================
--- linux-3.13-rc3.orig/drivers/cpufreq/powernow-k6.c	2013-11-30 00:19:59.000000000 +0100
+++ linux-3.13-rc3/drivers/cpufreq/powernow-k6.c	2013-12-06 22:25:42.000000000 +0100
@@ -44,23 +44,58 @@ static struct cpufreq_frequency_table cl
 /**
  * powernow_k6_get_cpu_multiplier - returns the current FSB multiplier
  *
- *   Returns the current setting of the frequency multiplier. Core clock
+ * Returns the current setting of the frequency multiplier. Core clock
  * speed is frequency of the Front-Side Bus multiplied with this value.
  */
 static int powernow_k6_get_cpu_multiplier(void)
 {
-	u64 invalue = 0;
+	unsigned long invalue = 0;
 	u32 msrval;
 
+	local_irq_disable();
+
 	msrval = POWERNOW_IOPORT + 0x1;
 	wrmsr(MSR_K6_EPMR, msrval, 0); /* enable the PowerNow port */
 	invalue = inl(POWERNOW_IOPORT + 0x8);
 	msrval = POWERNOW_IOPORT + 0x0;
 	wrmsr(MSR_K6_EPMR, msrval, 0); /* disable it again */
 
+	local_irq_enable();
+
 	return clock_ratio[(invalue >> 5)&7].driver_data;
 }
 
+static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
+{
+	unsigned long outvalue, invalue;
+	unsigned long msrval;
+	unsigned long cr0;
+
+	/* we now need to transform best_i to the BVC format, see AMD#23446 */
+
+	/*
+	 * The processor doesn't respond to inquiry cycles while changing the
+	 * frequency, so we must disable cache.
+	 */
+	local_irq_disable();
+	cr0 = read_cr0();
+	write_cr0(cr0 | X86_CR0_CD);
+	wbinvd();
+
+	outvalue = (1<<12) | (1<<10) | (1<<9) | (best_i<<5);
+
+	msrval = POWERNOW_IOPORT + 0x1;
+	wrmsr(MSR_K6_EPMR, msrval, 0); /* enable the PowerNow port */
+	invalue = inl(POWERNOW_IOPORT + 0x8);
+	invalue = invalue & 0x1f;
+	outvalue = outvalue | invalue;
+	outl(outvalue, (POWERNOW_IOPORT + 0x8));
+	msrval = POWERNOW_IOPORT + 0x0;
+	wrmsr(MSR_K6_EPMR, msrval, 0); /* disable it again */
+
+	write_cr0(cr0);
+	local_irq_enable();
+}
 
 /**
  * powernow_k6_target - set the PowerNow! multiplier
@@ -71,8 +106,6 @@ static int powernow_k6_get_cpu_multiplie
 static int powernow_k6_target(struct cpufreq_policy *policy,
 		unsigned int best_i)
 {
-	unsigned long outvalue = 0, invalue = 0;
-	unsigned long msrval;
 	struct cpufreq_freqs freqs;
 
 	if (clock_ratio[best_i].driver_data > max_multiplier) {
@@ -85,18 +118,7 @@ static int powernow_k6_target(struct cpu
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 
-	/* we now need to transform best_i to the BVC format, see AMD#23446 */
-
-	outvalue = (1<<12) | (1<<10) | (1<<9) | (best_i<<5);
-
-	msrval = POWERNOW_IOPORT + 0x1;
-	wrmsr(MSR_K6_EPMR, msrval, 0); /* enable the PowerNow port */
-	invalue = inl(POWERNOW_IOPORT + 0x8);
-	invalue = invalue & 0xf;
-	outvalue = outvalue | invalue;
-	outl(outvalue , (POWERNOW_IOPORT + 0x8));
-	msrval = POWERNOW_IOPORT + 0x0;
-	wrmsr(MSR_K6_EPMR, msrval, 0); /* disable it again */
+	powernow_k6_set_cpu_multiplier(best_i);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
@@ -125,7 +147,7 @@ static int powernow_k6_cpu_init(struct c
 	}
 
 	/* cpuinfo and default policy values */
-	policy->cpuinfo.transition_latency = 200000;
+	policy->cpuinfo.transition_latency = 500000;
 
 	return cpufreq_table_validate_and_show(policy, clock_ratio);
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] powernow-k6: correctly initialize default parameters
  2013-12-12  0:38 ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Mikulas Patocka
@ 2013-12-12  0:38   ` Mikulas Patocka
  2013-12-12  0:39     ` [PATCH 3/3] powernow-k6: reorder frequencies Mikulas Patocka
  2013-12-17  8:45   ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Viresh Kumar
  1 sibling, 1 reply; 13+ messages in thread
From: Mikulas Patocka @ 2013-12-12  0:38 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar; +Cc: cpufreq, linux-kernel

The powernow-k6 driver used to read the initial multiplier from the
powernow register. However, there is a problem with this:

* If there was a frequency transition before, the multiplier read from the
  register corresponds to the current multiplier.
* If there was no frequency transition since reset, the field in the
  register always reads as zero, regardless of the current multiplier that
  is set using switches on the mainboard and that the CPU is running at.

The zero value corresponds to multiplier 4.5, so as a consequence, the
powernow-k6 driver always assumes multiplier 4.5.

For example, if we have 550MHz CPU with bus frequency 100MHz and
multiplier 5.5, the powernow-k6 driver thinks that the multiplier is 4.5
and bus frequency is 122MHz. The powernow-k6 driver then sets the
multiplier to 4.5, underclocking the CPU to 450MHz, but reports the
current frequency as 550MHz.

There is no reliable way how to read the initial multiplier. I modified
the driver so that it contains a table of known frequencies (based on
parameters of existing CPUs and some common overclocking schemes) and sets
the multiplier according to the frequency. If the frequency is unknown
(because of unusual overclocking or underclocking), the user must supply
the bus speed and maximum multiplier as module parameters.

This patch should be backported to all stable kernels. If it doesn't
apply cleanly, change it, or ask me to change it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@kernel.org

---
 drivers/cpufreq/powernow-k6.c |   76 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 4 deletions(-)

Index: linux-3.13-rc3/drivers/cpufreq/powernow-k6.c
===================================================================
--- linux-3.13-rc3.orig/drivers/cpufreq/powernow-k6.c	2013-12-06 22:28:32.000000000 +0100
+++ linux-3.13-rc3/drivers/cpufreq/powernow-k6.c	2013-12-06 22:29:51.000000000 +0100
@@ -26,6 +26,14 @@
 static unsigned int                     busfreq;   /* FSB, in 10 kHz */
 static unsigned int                     max_multiplier;
 
+static unsigned int			param_busfreq = 0;
+static unsigned int			param_max_multiplier = 0;
+
+module_param_named(max_multiplier, param_max_multiplier, uint, S_IRUGO);
+MODULE_PARM_DESC(max_multiplier, "Maximum multiplier (allowed values: 20 30 35 40 45 50 55 60)");
+
+module_param_named(bus_frequency, param_busfreq, uint, S_IRUGO);
+MODULE_PARM_DESC(bus_frequency, "Bus frequency in kHz");
 
 /* Clock ratio multiplied by 10 - see table 27 in AMD#23446 */
 static struct cpufreq_frequency_table clock_ratio[] = {
@@ -40,6 +48,27 @@ static struct cpufreq_frequency_table cl
 	{0, CPUFREQ_TABLE_END}
 };
 
+static const struct {
+	unsigned freq;
+	unsigned mult;
+} usual_frequency_table[] = {
+	{ 400000, 40 },	// 100   * 4
+	{ 450000, 45 }, // 100   * 4.5
+	{ 475000, 50 }, //  95   * 5
+	{ 500000, 50 }, // 100   * 5
+	{ 506250, 45 }, // 112.5 * 4.5
+	{ 533500, 55 }, //  97   * 5.5
+	{ 550000, 55 }, // 100   * 5.5
+	{ 562500, 50 }, // 112.5 * 5
+	{ 570000, 60 }, //  95   * 6
+	{ 600000, 60 }, // 100   * 6
+	{ 618750, 55 }, // 112.5 * 5.5
+	{ 660000, 55 }, // 120   * 5.5
+	{ 675000, 60 }, // 112.5 * 6
+	{ 720000, 60 }, // 120   * 6
+};
+
+#define FREQ_RANGE		3000
 
 /**
  * powernow_k6_get_cpu_multiplier - returns the current FSB multiplier
@@ -125,17 +154,56 @@ static int powernow_k6_target(struct cpu
 	return 0;
 }
 
-
 static int powernow_k6_cpu_init(struct cpufreq_policy *policy)
 {
 	unsigned int i, f;
+	unsigned khz;
 
 	if (policy->cpu != 0)
 		return -ENODEV;
 
-	/* get frequencies */
-	max_multiplier = powernow_k6_get_cpu_multiplier();
-	busfreq = cpu_khz / max_multiplier;
+	max_multiplier = 0;
+	khz = cpu_khz;
+	for (i = 0; i < ARRAY_SIZE(usual_frequency_table); i++) {
+		if (khz >= usual_frequency_table[i].freq - FREQ_RANGE &&
+		    khz <= usual_frequency_table[i].freq + FREQ_RANGE) {
+			khz = usual_frequency_table[i].freq;
+			max_multiplier = usual_frequency_table[i].mult;
+			break;
+		}
+	}
+	if (param_max_multiplier) {
+		for (i = 0; (clock_ratio[i].frequency != CPUFREQ_TABLE_END); i++) {
+			if (clock_ratio[i].driver_data == param_max_multiplier) {
+				max_multiplier = param_max_multiplier;
+				goto have_max_multiplier;
+			}
+		}
+		printk(KERN_ERR "powernow-k6: invalid max_multiplier parameter, valid parameters 20, 30, 35, 40, 45, 50, 55, 60\n");
+		return -EINVAL;
+	}
+
+	if (!max_multiplier) {
+		printk(KERN_WARNING "powernow-k6: unknown frequency %u, cannot determine current multiplier\n", khz);
+		printk(KERN_WARNING "powernow-k6: use module parameters max_multiplier and bus_frequency\n");
+		return -EOPNOTSUPP;
+	}
+
+have_max_multiplier:
+	param_max_multiplier = max_multiplier;
+
+	if (param_busfreq) {
+		if (param_busfreq >= 50000 && param_busfreq <= 150000) {
+			busfreq = param_busfreq / 10;
+			goto have_busfreq;
+		}
+		printk(KERN_ERR "powernow-k6: invalid bus_frequency parameter, allowed range 50000 - 150000 kHz\n");
+		return -EINVAL;
+	}
+
+	busfreq = khz / max_multiplier;
+have_busfreq:
+	param_busfreq = busfreq * 10;
 
 	/* table init */
 	for (i = 0; (clock_ratio[i].frequency != CPUFREQ_TABLE_END); i++) {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] powernow-k6: reorder frequencies
  2013-12-12  0:38   ` [PATCH 2/3] powernow-k6: correctly initialize default parameters Mikulas Patocka
@ 2013-12-12  0:39     ` Mikulas Patocka
  2013-12-12  0:55       ` [PATCH] speedstep-smi: enable interrupts when waiting Mikulas Patocka
                         ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Mikulas Patocka @ 2013-12-12  0:39 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar; +Cc: cpufreq, linux-kernel

This patch reorders reported frequencies from the highest to the lowest,
just like in other frequency drivers.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@kernel.org

---
 drivers/cpufreq/powernow-k6.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Index: linux-3.12.3/drivers/cpufreq/powernow-k6.c
===================================================================
--- linux-3.12.3.orig/drivers/cpufreq/powernow-k6.c	2013-12-06 22:08:27.000000000 +0100
+++ linux-3.12.3/drivers/cpufreq/powernow-k6.c	2013-12-06 22:17:44.000000000 +0100
@@ -37,17 +37,20 @@ MODULE_PARM_DESC(bus_frequency, "Bus fre
 
 /* Clock ratio multiplied by 10 - see table 27 in AMD#23446 */
 static struct cpufreq_frequency_table clock_ratio[] = {
-	{45,  /* 000 -> 4.5x */ 0},
+	{60,  /* 110 -> 6.0x */ 0},
+	{55,  /* 011 -> 5.5x */ 0},
 	{50,  /* 001 -> 5.0x */ 0},
+	{45,  /* 000 -> 4.5x */ 0},
 	{40,  /* 010 -> 4.0x */ 0},
-	{55,  /* 011 -> 5.5x */ 0},
-	{20,  /* 100 -> 2.0x */ 0},
-	{30,  /* 101 -> 3.0x */ 0},
-	{60,  /* 110 -> 6.0x */ 0},
 	{35,  /* 111 -> 3.5x */ 0},
+	{30,  /* 101 -> 3.0x */ 0},
+	{20,  /* 100 -> 2.0x */ 0},
 	{0, CPUFREQ_TABLE_END}
 };
 
+static const u8 index_to_register[8] = { 6, 3, 1, 0, 2, 7, 5, 4 };
+static const u8 register_to_index[8] = { 3, 2, 4, 1, 7, 6, 0, 5 };
+
 static const struct {
 	unsigned freq;
 	unsigned mult;
@@ -91,7 +94,7 @@ static int powernow_k6_get_cpu_multiplie
 
 	local_irq_enable();
 
-	return clock_ratio[(invalue >> 5)&7].driver_data;
+	return clock_ratio[register_to_index[(invalue >> 5)&7]].driver_data;
 }
 
 static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
@@ -111,7 +114,7 @@ static void powernow_k6_set_cpu_multipli
 	write_cr0(cr0 | X86_CR0_CD);
 	wbinvd();
 
-	outvalue = (1<<12) | (1<<10) | (1<<9) | (best_i<<5);
+	outvalue = (1<<12) | (1<<10) | (1<<9) | (index_to_register[best_i]<<5);
 
 	msrval = POWERNOW_IOPORT + 0x1;
 	wrmsr(MSR_K6_EPMR, msrval, 0); /* enable the PowerNow port */


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] speedstep-smi: enable interrupts when waiting
  2013-12-12  0:39     ` [PATCH 3/3] powernow-k6: reorder frequencies Mikulas Patocka
@ 2013-12-12  0:55       ` Mikulas Patocka
  2013-12-17  8:47       ` [PATCH 3/3] powernow-k6: reorder frequencies Viresh Kumar
  2014-01-03  5:38       ` Viresh Kumar
  2 siblings, 0 replies; 13+ messages in thread
From: Mikulas Patocka @ 2013-12-12  0:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar; +Cc: cpufreq, linux-kernel

On Dell Latitude C600 laptop with Pentium 3 850MHz processor, the
speedstep-smi driver sometimes loads and sometimes doesn't load with
"change to state X failed" message.

I found out that we need to enable interrupts while waiting. When we
enable interrupts, the blockage that prevents frequency transition
resolves and the transition is possible. With disabled interrupts, the
blockage doesn't resolve (no matter how long do we wait).

This patch enables interrupts in the function speedstep_set_state that can
be called with disabled interrupts. However, this function is called with
disabled interrupts only from speedstep_get_freqs, so it shouldn't cause
any problem.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com
Cc: stable@kernel.org

---
 drivers/cpufreq/speedstep-lib.c |    1 +
 drivers/cpufreq/speedstep-smi.c |   13 +++++++++++++
 2 files changed, 14 insertions(+)

Index: linux-3.13-rc3/drivers/cpufreq/speedstep-smi.c
===================================================================
--- linux-3.13-rc3.orig/drivers/cpufreq/speedstep-smi.c	2013-12-12 01:32:37.769060946 +0100
+++ linux-3.13-rc3/drivers/cpufreq/speedstep-smi.c	2013-12-12 01:42:29.240216311 +0100
@@ -200,7 +200,19 @@ static void speedstep_set_state(unsigned
 		if (retry) {
 			pr_debug("retry %u, previous result %u, waiting...\n",
 					retry, result);
+			/*
+			 * We need to enable interrupts, otherwise the blockage
+			 * won't resolve.
+			 *
+			 * We disable preemption so that other processes don't
+			 * run. If other processes were running, they could
+			 * submit more DMA requests, making the blockage worse.
+			 */
+			preempt_disable();
+			local_irq_enable();
 			mdelay(retry * 50);
+			local_irq_disable();
+			preempt_enable_no_resched();
 		}
 		retry++;
 		__asm__ __volatile__(
@@ -217,6 +229,7 @@ static void speedstep_set_state(unsigned
 
 	/* enable IRQs */
 	local_irq_restore(flags);
+	preempt_check_resched();
 
 	if (new_state == state)
 		pr_debug("change to %u MHz succeeded after %u tries "
Index: linux-3.13-rc3/drivers/cpufreq/speedstep-lib.c
===================================================================
--- linux-3.13-rc3.orig/drivers/cpufreq/speedstep-lib.c	2013-10-18 20:24:16.000000000 +0200
+++ linux-3.13-rc3/drivers/cpufreq/speedstep-lib.c	2013-12-12 01:42:41.435120321 +0100
@@ -464,6 +464,7 @@ unsigned int speedstep_get_freqs(enum sp
 
 out:
 	local_irq_restore(flags);
+	preempt_check_resched();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(speedstep_get_freqs);

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] powernow-k6: disable cache when changing frequency
  2013-12-12  0:38 ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Mikulas Patocka
  2013-12-12  0:38   ` [PATCH 2/3] powernow-k6: correctly initialize default parameters Mikulas Patocka
@ 2013-12-17  8:45   ` Viresh Kumar
  2014-01-02 17:38     ` Mikulas Patocka
  1 sibling, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2013-12-17  8:45 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List

On 12 December 2013 06:08, Mikulas Patocka <mpatocka@redhat.com> wrote:
> During frequency transition the processor is in "EPM Stop Grant" state.
> The documentation says that the processor doesn't respond to inquiry
> requests in this state. Consequently, coherency of processor caches and
> bus master devices is not maintained, causing the system instability.
>
> This patch flushes the cache during frequency transition. It fixes the
> instability.

I don't have much idea of these systems but wouldn't this affect performance
badly?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] powernow-k6: reorder frequencies
  2013-12-12  0:39     ` [PATCH 3/3] powernow-k6: reorder frequencies Mikulas Patocka
  2013-12-12  0:55       ` [PATCH] speedstep-smi: enable interrupts when waiting Mikulas Patocka
@ 2013-12-17  8:47       ` Viresh Kumar
  2014-01-02 17:40         ` Mikulas Patocka
  2014-01-03  5:38       ` Viresh Kumar
  2 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2013-12-17  8:47 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List

On 12 December 2013 06:09, Mikulas Patocka <mpatocka@redhat.com> wrote:
> This patch reorders reported frequencies from the highest to the lowest,
> just like in other frequency drivers.

This isn't required I believe.. cpufreq core doesn't enforce any restrictions
on order of freqs present in table.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] powernow-k6: disable cache when changing frequency
  2013-12-17  8:45   ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Viresh Kumar
@ 2014-01-02 17:38     ` Mikulas Patocka
  2014-01-03  5:40       ` Viresh Kumar
  0 siblings, 1 reply; 13+ messages in thread
From: Mikulas Patocka @ 2014-01-02 17:38 UTC (permalink / raw)
  To: Viresh Kumar; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List



On Tue, 17 Dec 2013, Viresh Kumar wrote:

> On 12 December 2013 06:08, Mikulas Patocka <mpatocka@redhat.com> wrote:
> > During frequency transition the processor is in "EPM Stop Grant" state.
> > The documentation says that the processor doesn't respond to inquiry
> > requests in this state. Consequently, coherency of processor caches and
> > bus master devices is not maintained, causing the system instability.
> >
> > This patch flushes the cache during frequency transition. It fixes the
> > instability.
> 
> I don't have much idea of these systems but wouldn't this affect performance
> badly?

Flushing the cache and changing frequency takes approximatelly 500us. The 
patch increases policy->cpuinfo.transition_latency to that value.

I suppose that the governors should be smart enough to not do frequency 
transitions so often so that majority of time is spent doing the 
transition.

Mikulas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] powernow-k6: reorder frequencies
  2013-12-17  8:47       ` [PATCH 3/3] powernow-k6: reorder frequencies Viresh Kumar
@ 2014-01-02 17:40         ` Mikulas Patocka
  0 siblings, 0 replies; 13+ messages in thread
From: Mikulas Patocka @ 2014-01-02 17:40 UTC (permalink / raw)
  To: Viresh Kumar; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List



On Tue, 17 Dec 2013, Viresh Kumar wrote:

> On 12 December 2013 06:09, Mikulas Patocka <mpatocka@redhat.com> wrote:
> > This patch reorders reported frequencies from the highest to the lowest,
> > just like in other frequency drivers.
> 
> This isn't required I believe.. cpufreq core doesn't enforce any restrictions
> on order of freqs present in table.

But the cpufreq-info tool lists the frequencies misordered.

This is not a serious bug. The patch just makes the output of cpufreq-info 
look better.

Mikulas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] powernow-k6: reorder frequencies
  2013-12-12  0:39     ` [PATCH 3/3] powernow-k6: reorder frequencies Mikulas Patocka
  2013-12-12  0:55       ` [PATCH] speedstep-smi: enable interrupts when waiting Mikulas Patocka
  2013-12-17  8:47       ` [PATCH 3/3] powernow-k6: reorder frequencies Viresh Kumar
@ 2014-01-03  5:38       ` Viresh Kumar
  2 siblings, 0 replies; 13+ messages in thread
From: Viresh Kumar @ 2014-01-03  5:38 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List

On 12 December 2013 06:09, Mikulas Patocka <mpatocka@redhat.com> wrote:
> This patch reorders reported frequencies from the highest to the lowest,
> just like in other frequency drivers.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@kernel.org
>
> ---
>  drivers/cpufreq/powernow-k6.c |   17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> Index: linux-3.12.3/drivers/cpufreq/powernow-k6.c
> ===================================================================
> --- linux-3.12.3.orig/drivers/cpufreq/powernow-k6.c     2013-12-06 22:08:27.000000000 +0100
> +++ linux-3.12.3/drivers/cpufreq/powernow-k6.c  2013-12-06 22:17:44.000000000 +0100
> @@ -37,17 +37,20 @@ MODULE_PARM_DESC(bus_frequency, "Bus fre
>
>  /* Clock ratio multiplied by 10 - see table 27 in AMD#23446 */
>  static struct cpufreq_frequency_table clock_ratio[] = {
> -       {45,  /* 000 -> 4.5x */ 0},
> +       {60,  /* 110 -> 6.0x */ 0},
> +       {55,  /* 011 -> 5.5x */ 0},
>         {50,  /* 001 -> 5.0x */ 0},
> +       {45,  /* 000 -> 4.5x */ 0},
>         {40,  /* 010 -> 4.0x */ 0},
> -       {55,  /* 011 -> 5.5x */ 0},
> -       {20,  /* 100 -> 2.0x */ 0},
> -       {30,  /* 101 -> 3.0x */ 0},
> -       {60,  /* 110 -> 6.0x */ 0},
>         {35,  /* 111 -> 3.5x */ 0},
> +       {30,  /* 101 -> 3.0x */ 0},
> +       {20,  /* 100 -> 2.0x */ 0},
>         {0, CPUFREQ_TABLE_END}
>  };
>
> +static const u8 index_to_register[8] = { 6, 3, 1, 0, 2, 7, 5, 4 };
> +static const u8 register_to_index[8] = { 3, 2, 4, 1, 7, 6, 0, 5 };
> +
>  static const struct {
>         unsigned freq;
>         unsigned mult;
> @@ -91,7 +94,7 @@ static int powernow_k6_get_cpu_multiplie
>
>         local_irq_enable();
>
> -       return clock_ratio[(invalue >> 5)&7].driver_data;
> +       return clock_ratio[register_to_index[(invalue >> 5)&7]].driver_data;
>  }
>
>  static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
> @@ -111,7 +114,7 @@ static void powernow_k6_set_cpu_multipli
>         write_cr0(cr0 | X86_CR0_CD);
>         wbinvd();
>
> -       outvalue = (1<<12) | (1<<10) | (1<<9) | (best_i<<5);
> +       outvalue = (1<<12) | (1<<10) | (1<<9) | (index_to_register[best_i]<<5);
>
>         msrval = POWERNOW_IOPORT + 0x1;
>         wrmsr(MSR_K6_EPMR, msrval, 0); /* enable the PowerNow port */

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] powernow-k6: disable cache when changing frequency
  2014-01-02 17:38     ` Mikulas Patocka
@ 2014-01-03  5:40       ` Viresh Kumar
  2014-01-06 20:17         ` Mikulas Patocka
  0 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2014-01-03  5:40 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List

On 2 January 2014 23:08, Mikulas Patocka <mpatocka@redhat.com> wrote:
> Flushing the cache and changing frequency takes approximatelly 500us. The
> patch increases policy->cpuinfo.transition_latency to that value.

Its not about how fast caches get cleaned but how much time would
be wasted to get them filled again as same data could be required again
which is just flushed out. That would impact performance more than
flushing caches.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] powernow-k6: disable cache when changing frequency
  2014-01-03  5:40       ` Viresh Kumar
@ 2014-01-06 20:17         ` Mikulas Patocka
  0 siblings, 0 replies; 13+ messages in thread
From: Mikulas Patocka @ 2014-01-06 20:17 UTC (permalink / raw)
  To: Viresh Kumar; +Cc: Rafael J. Wysocki, cpufreq, Linux Kernel Mailing List



On Fri, 3 Jan 2014, Viresh Kumar wrote:

> On 2 January 2014 23:08, Mikulas Patocka <mpatocka@redhat.com> wrote:
> > Flushing the cache and changing frequency takes approximatelly 500us. The
> > patch increases policy->cpuinfo.transition_latency to that value.
> 
> Its not about how fast caches get cleaned but how much time would
> be wasted to get them filled again as same data could be required again
> which is just flushed out. That would impact performance more than
> flushing caches.

I didn't see any performance degradation when I tried changing the 
frequency manually with or without the cache flush patch - the overhead of 
running cpufreq (8ms) is far worse than the frequency transition itself.

Mikulas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] powernow-k6: fixes for the frequency driver
  2013-12-12  0:37 [PATCH 0/3] powernow-k6: fixes for the frequency driver Mikulas Patocka
  2013-12-12  0:38 ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Mikulas Patocka
@ 2014-01-06 20:51 ` Rafael J. Wysocki
  1 sibling, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2014-01-06 20:51 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Viresh Kumar, cpufreq, linux-kernel

On Wednesday, December 11, 2013 07:37:48 PM Mikulas Patocka wrote:
> Hi
> 
> Here I'm sending three patches for k6 frequency governor.
> 
> 1. the processor doesn't respond to cache snoops when changing the 
> frequency, so we need to disable cache while doing the change.
> 
> 2. it is impossible to read the default multiplier. After reset, the 
> register always reports zero regardless of the current multiplier. So we 
> set the multiplier based on current frequency and allow the user to 
> override it with module parameter.
> 
> 3. reorder frequencies from the highest to the lowest.

I've queued up this series (and the speedstep-smi patch) for 3.14, but I don't
want it to go to stable upfront, so I removed the "Cc: stable" tags.  Please
send a stable inclusion request to Greg & Co after it spends a few weeks in
the Linus' tree.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-01-06 20:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-12  0:37 [PATCH 0/3] powernow-k6: fixes for the frequency driver Mikulas Patocka
2013-12-12  0:38 ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Mikulas Patocka
2013-12-12  0:38   ` [PATCH 2/3] powernow-k6: correctly initialize default parameters Mikulas Patocka
2013-12-12  0:39     ` [PATCH 3/3] powernow-k6: reorder frequencies Mikulas Patocka
2013-12-12  0:55       ` [PATCH] speedstep-smi: enable interrupts when waiting Mikulas Patocka
2013-12-17  8:47       ` [PATCH 3/3] powernow-k6: reorder frequencies Viresh Kumar
2014-01-02 17:40         ` Mikulas Patocka
2014-01-03  5:38       ` Viresh Kumar
2013-12-17  8:45   ` [PATCH 1/3] powernow-k6: disable cache when changing frequency Viresh Kumar
2014-01-02 17:38     ` Mikulas Patocka
2014-01-03  5:40       ` Viresh Kumar
2014-01-06 20:17         ` Mikulas Patocka
2014-01-06 20:51 ` [PATCH 0/3] powernow-k6: fixes for the frequency driver Rafael J. Wysocki

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.