Prev: [PATCH] signals: check_kill_permission: don't check creds if same_thread_group()
Next: addjust discard request to be aligned with hwsect size to support SSDs with larger sector size
From: Randy Dunlap on 17 May 2010 16:00
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(a)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 ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Randy Dunlap on 17 May 2010 16:30
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 ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: jacob pan on 17 May 2010 16:30
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 17 May 2010 16:30
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH] signals: check_kill_permission: don't check creds if same_thread_group()
Next: addjust discard request to be aligned with hwsect size to support SSDs with larger sector size