Re: [PATCH 3/8] x86/apic: allow use of lapic timer early calibration result

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, 7 May 2010, Jacob Pan wrote:

> From: Jacob Pan <jacob.jun.pan@intel.com>
> 
> lapic timer calibration can be combined with tsc in platform specific
> calibration functions. if such calibration result is obtained early,
> we can skip the redundent calibration loops.

I'd rather move lapic calibration into TSC calibration in general as
we do the same thing twice for no good reason.

That needs some code restructuring, but that's worth it.

> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Hehe. So you handed the patch to yourself :)

> ---
>  arch/x86/kernel/apic/apic.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index e5a4a1e..8ef56ac 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -175,7 +175,7 @@ static struct resource lapic_resource = {
>  	.flags = IORESOURCE_MEM | IORESOURCE_BUSY,
>  };
>  
> -static unsigned int calibration_result;
> +unsigned int calibration_result;

Aside of my general objection it'd be not a good idea to make this
global w/o renaming it to something sensible like
lapic_timer_frequency.
  
>  static int lapic_next_event(unsigned long delta,
>  			    struct clock_event_device *evt);
> @@ -597,6 +597,25 @@ static int __init calibrate_APIC_clock(void)
>  	long delta, deltatsc;
>  	int pm_referenced = 0;
>  
> +	/**
> +	 * check if lapic timer has already been calibrated by platform
> +	 * specific routine, such as tsc calibration code. if so, we just fill
> +	 * in the clockevent structure and return.
> +	 */
> +	if (calibration_result) {
> +		apic_printk(APIC_VERBOSE, "lapic timer already calibrated %d\n",
> +				calibration_result);
> +		lapic_clockevent.mult = div_sc(calibration_result/APIC_DIVISOR,
> +					TICK_NSEC, lapic_clockevent.shift);
> +		lapic_clockevent.max_delta_ns =
> +			clockevent_delta2ns(0x7FFFFF, &lapic_clockevent);
> +		lapic_clockevent.min_delta_ns =
> +			clockevent_delta2ns(0xF, &lapic_clockevent);
> +		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_DUMMY;
> +		return 0;

  Grr. I hate duplicated code.

> +	}
> +
>  	local_irq_disable();
>  
>  	/* Replace the global interrupt handler */
> -- 
> 1.6.3.3
>