[PATCH 0/2] v3 Moorestown updated patches 2,3,4 of v2

[PATCH 0/2] v3 Moorestown updated patches 2,3,4 of v2

[Jacob Pan]

These two patches are the updates of the v2 series. Changes include
1. comblined two patches below per tglx suggestion
	0003-x86-mrst-add-more-timer-options-to-include-Medfield.patch
	0004-x86-apbt-support-more-timer-configurations-on-mrst.patch

	Fixes based on review comments:
	a) removed duplicated '(' in mrst_setup_secondary_clock()
	b) changed clock selection logic, trying to make it more readable
		mrst_setup_secondary_clock()

2. export mrst_identify_cpu(), removed cache copy.


*** BLURB HERE ***

Jacob Pan (2):
  x86/mrst: add cpu type detection
  x86/mrst: add more timer config options

 arch/x86/include/asm/apb_timer.h |    2 +-
 arch/x86/include/asm/mrst.h      |   20 +++++++++
 arch/x86/kernel/apb_timer.c      |   18 ++++----
 arch/x86/kernel/mrst.c           |   86 ++++++++++++++++++++++++++++++++++----
 4 files changed, 108 insertions(+), 18 deletions(-)

[PATCH 1/2] x86/mrst: add cpu type detection

[Jacob Pan]

Medfield is the follow-up of Moorestown, it is treated under the same
HW sub-architecture. However, we do need to know the CPU type in order
for some of the driver to act accordingly.
We also have different optimal clock configuration for each CPU type.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 arch/x86/include/asm/mrst.h |   19 +++++++++++++++++++
 arch/x86/kernel/mrst.c      |   22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mrst.h b/arch/x86/include/asm/mrst.h
index 451d30e..a25eff3 100644
--- a/arch/x86/include/asm/mrst.h
+++ b/arch/x86/include/asm/mrst.h
@@ -11,8 +11,27 @@
 #ifndef _ASM_X86_MRST_H
 #define _ASM_X86_MRST_H
 extern int pci_mrst_init(void);
+extern int mrst_identify_cpu(void);
 int __init sfi_parse_mrtc(struct sfi_table_header *table);
 
+/**
+ * Medfield is the follow-up of Moorestown, it combines two chip solution into
+ * one. Other than that it also added always-on and constant tsc and lapic
+ * timers. Medfield is the platform name, and the chip name is called Penwell
+ * we treat Medfield/Penwell as a variant of Moorestown. Penwell can be
+ * identified via MSRs.
+ */
+enum mrst_cpu_type {
+	MRST_CPU_CHIP_LINCROFT = 1,
+	MRST_CPU_CHIP_PENWELL,
+};
+
+enum mrst_timer_options {
+	MRST_TIMER_DEFAULT,
+	MRST_TIMER_APBT_ONLY,
+	MRST_TIMER_LAPIC_APBT,
+};
+
 #define SFI_MTMR_MAX_NUM 8
 #define SFI_MRTC_MAX	8
 
diff --git a/arch/x86/kernel/mrst.c b/arch/x86/kernel/mrst.c
index 0aad867..f1ccabd 100644
--- a/arch/x86/kernel/mrst.c
+++ b/arch/x86/kernel/mrst.c
@@ -216,6 +216,28 @@ static void __init mrst_setup_boot_clock(void)
 		setup_boot_APIC_clock();
 };
 
+int mrst_identify_cpu(void)
+{
+	u32 mrst_cpu_chip;
+
+	if (boot_cpu_data.x86 == 6 &&
+		boot_cpu_data.x86_model == 0x27 &&
+		boot_cpu_data.x86_mask == 1)
+		mrst_cpu_chip = MRST_CPU_CHIP_PENWELL;
+	else if (boot_cpu_data.x86 == 6 &&
+		boot_cpu_data.x86_model == 0x26)
+		mrst_cpu_chip = MRST_CPU_CHIP_LINCROFT;
+	else {
+		pr_err("Unknown Moorestown CPU type, default to Lincroft\n");
+		mrst_cpu_chip = MRST_CPU_CHIP_LINCROFT;
+	}
+	pr_debug("Moorestown CPU %s identified\n",
+		(mrst_cpu_chip == MRST_CPU_CHIP_LINCROFT) ?
+		"Lincroft" : "Penwell");
+	return mrst_cpu_chip;
+}
+EXPORT_SYMBOL_GPL(mrst_identify_cpu);
+
 /*
  * Moorestown specific x86_init function overrides and early setup
  * calls.
-- 
1.6.3.3

[PATCH 2/2] x86/mrst: add more timer config options

[Jacob Pan]

Always-on local APIC timer (ARAT) has been introduced to Medfield, along
with the platform APB timers we have more timer configuration options
between Moorestown and Medfield.

This patch adds run-time detection of avaiable timer features so that
we can treat Medfield as a variant of Moorestown and set up the optimal
timer options for each platform. i.e.

Medfield: per cpu always-on local APIC timer
Moorestown: per cpu APB timer

Manual override is possible via cmdline option x86_mrst_timer.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 arch/x86/include/asm/apb_timer.h |    2 +-
 arch/x86/include/asm/mrst.h      |    1 +
 arch/x86/kernel/apb_timer.c      |   18 ++++++-----
 arch/x86/kernel/mrst.c           |   64 ++++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/apb_timer.h b/arch/x86/include/asm/apb_timer.h
index c74a2ee..4127fd1 100644
--- a/arch/x86/include/asm/apb_timer.h
+++ b/arch/x86/include/asm/apb_timer.h
@@ -55,7 +55,7 @@ extern unsigned long apbt_quick_calibrate(void);
 extern int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu);
 extern void apbt_setup_secondary_clock(void);
 extern unsigned int boot_cpu_id;
-extern int disable_apbt_percpu;
+extern int mrst_timer_options;
 
 extern struct sfi_timer_table_entry *sfi_get_mtmr(int hint);
 extern void sfi_free_mtmr(struct sfi_timer_table_entry *mtmr);
diff --git a/arch/x86/include/asm/mrst.h b/arch/x86/include/asm/mrst.h
index a25eff3..f683603 100644
--- a/arch/x86/include/asm/mrst.h
+++ b/arch/x86/include/asm/mrst.h
@@ -12,6 +12,7 @@
 #define _ASM_X86_MRST_H
 extern int pci_mrst_init(void);
 extern int mrst_identify_cpu(void);
+extern int mrst_timer_options __cpuinitdata;
 int __init sfi_parse_mrtc(struct sfi_table_header *table);
 
 /**
diff --git a/arch/x86/kernel/apb_timer.c b/arch/x86/kernel/apb_timer.c
index a353475..08dfbf8 100644
--- a/arch/x86/kernel/apb_timer.c
+++ b/arch/x86/kernel/apb_timer.c
@@ -43,10 +43,11 @@
 
 #include <asm/fixmap.h>
 #include <asm/apb_timer.h>
+#include <asm/mrst.h>
 
 #define APBT_MASK			CLOCKSOURCE_MASK(32)
 #define APBT_SHIFT			22
-#define APBT_CLOCKEVENT_RATING		150
+#define APBT_CLOCKEVENT_RATING		110
 #define APBT_CLOCKSOURCE_RATING		250
 #define APBT_MIN_DELTA_USEC		200
 
@@ -83,8 +84,6 @@ struct apbt_dev {
 	char name[10];
 };
 
-int disable_apbt_percpu __cpuinitdata;
-
 static DEFINE_PER_CPU(struct apbt_dev, cpu_apbt_dev);
 
 #ifdef CONFIG_SMP
@@ -204,9 +203,9 @@ static inline int __init setup_x86_mrst_timer(char *arg)
 		return -EINVAL;
 
 	if (strcmp("apbt_only", arg) == 0)
-		disable_apbt_percpu = 0;
+		mrst_timer_options = MRST_TIMER_APBT_ONLY;
 	else if (strcmp("lapic_and_apbt", arg) == 0)
-		disable_apbt_percpu = 1;
+		mrst_timer_options = MRST_TIMER_LAPIC_APBT;
 	else {
 		pr_warning("X86 MRST timer option %s not recognised"
 			   " use x86_mrst_timer=apbt_only or lapic_and_apbt\n",
@@ -335,7 +334,7 @@ static int __init apbt_clockevent_register(void)
 	adev->num = smp_processor_id();
 	memcpy(&adev->evt, &apbt_clockevent, sizeof(struct clock_event_device));
 
-	if (disable_apbt_percpu) {
+	if (mrst_timer_options == MRST_TIMER_LAPIC_APBT) {
 		apbt_clockevent.rating = APBT_CLOCKEVENT_RATING - 100;
 		global_clock_event = &adev->evt;
 		printk(KERN_DEBUG "%s clockevent registered as global\n",
@@ -429,7 +428,8 @@ static int apbt_cpuhp_notify(struct notifier_block *n,
 
 static __init int apbt_late_init(void)
 {
-	if (disable_apbt_percpu || !apb_timer_block_enabled)
+	if (mrst_timer_options == MRST_TIMER_LAPIC_APBT ||
+		!apb_timer_block_enabled)
 		return 0;
 	/* This notifier should be called after workqueue is ready */
 	hotcpu_notifier(apbt_cpuhp_notify, -20);
@@ -450,6 +450,8 @@ static void apbt_set_mode(enum clock_event_mode mode,
 	int timer_num;
 	struct apbt_dev *adev = EVT_TO_APBT_DEV(evt);
 
+	BUG_ON(!apbt_virt_address);
+
 	timer_num = adev->num;
 	pr_debug("%s CPU %d timer %d mode=%d\n",
 		 __func__, first_cpu(*evt->cpumask), timer_num, mode);
@@ -676,7 +678,7 @@ void __init apbt_time_init(void)
 	}
 #ifdef CONFIG_SMP
 	/* kernel cmdline disable apb timer, so we will use lapic timers */
-	if (disable_apbt_percpu) {
+	if (mrst_timer_options == MRST_TIMER_LAPIC_APBT) {
 		printk(KERN_INFO "apbt: disabled per cpu timer\n");
 		return;
 	}
diff --git a/arch/x86/kernel/mrst.c b/arch/x86/kernel/mrst.c
index f1ccabd..87c5c7d 100644
--- a/arch/x86/kernel/mrst.c
+++ b/arch/x86/kernel/mrst.c
@@ -25,6 +25,29 @@
 #include <asm/i8259.h>
 #include <asm/apb_timer.h>
 
+/**
+ * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,
+ * cmdline option x86_mrst_timer can be used to override the configuration
+ * to prefer one or the other.
+ * at runtime, there are basically three timer configurations:
+ * 1. per cpu apbt clock only
+ * 2. per cpu always-on lapic clocks only, this is Penwell/Medfield only
+ * 3. per cpu lapic clock (C3STOP) and one apbt clock, with broadcast.
+ *
+ * by default (without cmdline option), platform code first detects cpu type
+ * to see if we are on lincroft or penwell, then set up both lapic or apbt
+ * clocks accordingly.
+ * i.e. by default, medfield uses configuration #2, moorestown uses #1.
+ * config #3 is supported but not recommended on medfield.
+ *
+ * rating and feature summary:
+ * lapic (with C3STOP) --------- 100
+ * apbt (always-on) ------------ 110
+ * lapic (always-on,ARAT) ------ 150
+ */
+
+int mrst_timer_options __cpuinitdata;
+
 static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
 static struct sfi_timer_table_entry sfi_mtimer_array[SFI_MTMR_MAX_NUM];
 int sfi_mtimer_num;
@@ -167,16 +190,25 @@ int __init sfi_parse_mrtc(struct sfi_table_header *table)
 	return 0;
 }
 
-/*
- * the secondary clock in Moorestown can be APBT or LAPIC clock, default to
- * APBT but cmdline option can also override it.
- */
 static void __cpuinit mrst_setup_secondary_clock(void)
 {
-	/* restore default lapic clock if disabled by cmdline */
-	if (disable_apbt_percpu)
+	switch (mrst_timer_options) {
+	case MRST_TIMER_APBT_ONLY:
+		return apbt_setup_secondary_clock();
+	case MRST_TIMER_LAPIC_APBT:
+		pr_info("using lapic timers for secondary clock\n");
 		return setup_secondary_APIC_clock();
-	apbt_setup_secondary_clock();
+	default:
+		/**
+		 * Default behavior is to use always-on lapic timer if available
+		 * (Medfield). Or, choose apbt timer (Moorestown).
+		 */
+		if (cpu_has(&current_cpu_data, X86_FEATURE_ARAT))
+			setup_secondary_APIC_clock();
+		else
+			apbt_setup_secondary_clock();
+		return;
+	}
 }
 
 static unsigned long __init mrst_calibrate_tsc(void)
@@ -195,6 +227,11 @@ static unsigned long __init mrst_calibrate_tsc(void)
 
 void __init mrst_time_init(void)
 {
+	/* if cpu is penwell, lapic timer will be used by default */
+	if ((mrst_identify_cpu() == MRST_CPU_CHIP_PENWELL) &&
+		(mrst_timer_options == MRST_TIMER_DEFAULT))
+		return;
+
 	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
 	pre_init_apic_IRQ0();
 	apbt_time_init();
@@ -211,9 +248,18 @@ void __init mrst_rtc_init(void)
  */
 static void __init mrst_setup_boot_clock(void)
 {
-	pr_info("%s: per cpu apbt flag %d \n", __func__, disable_apbt_percpu);
-	if (disable_apbt_percpu)
+	switch (mrst_timer_options) {
+	case MRST_TIMER_APBT_ONLY:
+		break;
+	case MRST_TIMER_LAPIC_APBT:
 		setup_boot_APIC_clock();
+		break;
+	default:
+		/* check if this is Penwell */
+		if (cpu_has(&boot_cpu_data, X86_FEATURE_ARAT))
+			setup_boot_APIC_clock();
+		break;
+	}
 };
 
 int mrst_identify_cpu(void)
-- 
1.6.3.3

Re: [PATCH 2/2] x86/mrst: add more timer config options

[Randy Dunlap]

On Mon, 17 May 2010 12:23:03 -0700 Jacob Pan wrote:

> Always-on local APIC timer (ARAT) has been introduced to Medfield, along
> with the platform APB timers we have more timer configuration options
> between Moorestown and Medfield.
> 
> This patch adds run-time detection of avaiable timer features so that
> we can treat Medfield as a variant of Moorestown and set up the optimal
> timer options for each platform. i.e.
> 
> Medfield: per cpu always-on local APIC timer
> Moorestown: per cpu APB timer
> 
> Manual override is possible via cmdline option x86_mrst_timer.

which is documented where??

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  arch/x86/include/asm/apb_timer.h |    2 +-
>  arch/x86/include/asm/mrst.h      |    1 +
>  arch/x86/kernel/apb_timer.c      |   18 ++++++-----
>  arch/x86/kernel/mrst.c           |   64 ++++++++++++++++++++++++++++++++-----
>  4 files changed, 67 insertions(+), 18 deletions(-)

> diff --git a/arch/x86/kernel/mrst.c b/arch/x86/kernel/mrst.c
> index f1ccabd..87c5c7d 100644
> --- a/arch/x86/kernel/mrst.c
> +++ b/arch/x86/kernel/mrst.c
> @@ -25,6 +25,29 @@
>  #include <asm/i8259.h>
>  #include <asm/apb_timer.h>
>  
> +/**

Don't use /** since this is not a kernel-doc comment block.

> + * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock,
> + * cmdline option x86_mrst_timer can be used to override the configuration
> + * to prefer one or the other.
> + * at runtime, there are basically three timer configurations:
> + * 1. per cpu apbt clock only
> + * 2. per cpu always-on lapic clocks only, this is Penwell/Medfield only
> + * 3. per cpu lapic clock (C3STOP) and one apbt clock, with broadcast.
> + *
> + * by default (without cmdline option), platform code first detects cpu type
> + * to see if we are on lincroft or penwell, then set up both lapic or apbt
> + * clocks accordingly.
> + * i.e. by default, medfield uses configuration #2, moorestown uses #1.
> + * config #3 is supported but not recommended on medfield.
> + *
> + * rating and feature summary:
> + * lapic (with C3STOP) --------- 100
> + * apbt (always-on) ------------ 110
> + * lapic (always-on,ARAT) ------ 150
> + */
> +
> +int mrst_timer_options __cpuinitdata;
> +
>  static u32 sfi_mtimer_usage[SFI_MTMR_MAX_NUM];
>  static struct sfi_timer_table_entry sfi_mtimer_array[SFI_MTMR_MAX_NUM];
>  int sfi_mtimer_num;


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 2/2] x86/mrst: add more timer config options

[jacob pan]

Randy Dunlap Mon, 17 May 2010 12:56:19 -0700

>> Manual override is possible via cmdline option x86_mrst_timer.
>
>which is documented where??
It is documented in Documentation/kernel-parameters.txt, the option has not changed but the impact has been extended to the new Medfield platforms.

        x86_mrst_timer= [X86-32,APBT]
                        Choose timer option for x86 Moorestown MID platform.
                        Two valid options are apbt timer only and lapic timer
                        plus one apbt timer for broadcast timer.
                        x86_mrst_timer=apbt_only | lapic_and_apbt


Thanks,

Jacob

Re: [PATCH 2/2] x86/mrst: add more timer config options

[Thomas Gleixner]

On Mon, 17 May 2010, Jacob Pan wrote:

> diff --git a/arch/x86/include/asm/apb_timer.h b/arch/x86/include/asm/apb_timer.h
> index c74a2ee..4127fd1 100644
> --- a/arch/x86/include/asm/apb_timer.h
> +++ b/arch/x86/include/asm/apb_timer.h
> @@ -55,7 +55,7 @@ extern unsigned long apbt_quick_calibrate(void);
>  extern int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu);
>  extern void apbt_setup_secondary_clock(void);
>  extern unsigned int boot_cpu_id;
> -extern int disable_apbt_percpu;
> +extern int mrst_timer_options;

  Please remove this, it belongs to and is in mrst.h

>  #ifdef CONFIG_SMP
> @@ -204,9 +203,9 @@ static inline int __init setup_x86_mrst_timer(char *arg)
>  		return -EINVAL;
>  
>  	if (strcmp("apbt_only", arg) == 0)
> -		disable_apbt_percpu = 0;
> +		mrst_timer_options = MRST_TIMER_APBT_ONLY;
>  	else if (strcmp("lapic_and_apbt", arg) == 0)
> -		disable_apbt_percpu = 1;
> +		mrst_timer_options = MRST_TIMER_LAPIC_APBT;
>  	else {
>  		pr_warning("X86 MRST timer option %s not recognised"
>  			   " use x86_mrst_timer=apbt_only or lapic_and_apbt\n",

  Please move that to msrt.c It's not apbt specific. We parse the
  options where we have the variable.

>  static void __init mrst_setup_boot_clock(void)
>  {
> -	pr_info("%s: per cpu apbt flag %d \n", __func__, disable_apbt_percpu);
> -	if (disable_apbt_percpu)
> +	switch (mrst_timer_options) {
> +	case MRST_TIMER_APBT_ONLY:
> +		break;
> +	case MRST_TIMER_LAPIC_APBT:
>  		setup_boot_APIC_clock();
> +		break;
> +	default:
> +		/* check if this is Penwell */
> +		if (cpu_has(&boot_cpu_data, X86_FEATURE_ARAT))
> +			setup_boot_APIC_clock();
> +		break;
> +	}
>  };

Did you notice that you are copying these checks all over the place ?

The first call into that code is mrst_time_init(), right ? So why not
check there and setup the function pointers once ?

void __init mrst_time_init(void)
{
	switch (mrst_timer_options) {
	case MRST_TIMER_APBT_ONLY:
		break;

	default:
		if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARAT))
		   	break;

		x86_init.timers.timer_init = x86_init_noop;

	case MRST_TIMER_LAPIC_APBT:
		x86_init.timers.setup_percpu_clockev = setup_boot_APIC_clock;
		x86_cpuinit.setup_percpu_clockev = setup_secondary_APIC_clock;
		return;
	}

        sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
	....

and change the code in x86_mrst_early_setup()

-   	 x86_init.timers.setup_percpu_clockev = mrst_setup_boot_clock;
+   	 x86_init.timers.setup_percpu_clockev = x86_init_noop;

-        x86_cpuinit.setup_percpu_clockev = mrst_setup_secondary_clock;
+        x86_cpuinit.setup_percpu_clockev = apbt_setup_secondary_clock;

So this makes everything default to apbt and in case of lapic selected
by command line or cpu type you switch everything over in one place.

That removes mrst_setup_boot_clock and mrst_setup_secondary_clock
along with all the duplicated code.

Thanks,

	tglx

Re: [PATCH 2/2] x86/mrst: add more timer config options

[Randy Dunlap]

On 05/17/10 13:26, jacob pan wrote:
> Randy Dunlap Mon, 17 May 2010 12:56:19 -0700
> 
>>> Manual override is possible via cmdline option x86_mrst_timer.
>>
>> which is documented where??
> It is documented in Documentation/kernel-parameters.txt, the option has not changed but the impact has been extended to the new Medfield platforms.
> 
>         x86_mrst_timer= [X86-32,APBT]
>                         Choose timer option for x86 Moorestown MID platform.
>                         Two valid options are apbt timer only and lapic timer
>                         plus one apbt timer for broadcast timer.
>                         x86_mrst_timer=apbt_only | lapic_and_apbt


Thanks.  I missed seeing that somehow.  Sorry about that.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***