lkml.org 
[lkml]   [2010]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] x86/mrst: add more timer config options
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


\
 
 \ /
  Last update: 2010-05-17 22:31    [W:0.035 / U:1.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site