RE: [PATCH 3/8] x86/apic: allow use of lapic timer earlycalibration result

[Pan, Jacob jun]

Thanks for the review.

> >
> > 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.
> 
I am trying to avoid the risks of completely remove the current lapic 
calibration code since there are so many platforms with different timer
options. And I don't understand things like why pm timer is preferred.
Why not use the rating in clocksource?

> > Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxx>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> 
> 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.
> 
perhaps, the calibration data can directly be assigned to lapic timer
clock_event_device.mult? There is no need for the device specific result
scale (e.g. bus clocks per tick)

> >  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
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/