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

[Thomas Gleixner <tglx@linutronix.de>]

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