Re: [PATCHv2 1/2] x86, mrst: share APB timer code with other platforms

[Jamie Iles <jamie@jamieiles.com>]

Hi Jacob,

On Fri, Apr 08, 2011 at 03:03:03PM -0700, jacob pan wrote:
> On Fri,  8 Apr 2011 15:33:37 +0100
> Jamie Iles <jamie@jamieiles.com> wrote:
> 
> > The APB timers are an IP block from Synopsys (DesignWare APB timers)
> > and are also found in other systems including ARM SoC's.  This patch
> > adds functions for creating clock_event_devices and clocksources from
> > APB timers but does not do the resource allocation.  This is handled
> > in a higher layer to allow the timers to be created from multiple
> > methods such as platform_devices.
> > 
> > Changes since v1:
> > 	- Use the correct timer for clocksource on x86
> > 	- Select the correct timer rating for x86
> > 	- Restore freerunning timer behaviour for oneshot event
> > devices
> > 	- Reenable event irq correctly for hotplug
> > 
> > Cc: John Stultz <johnstul@us.ibm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> > ---
> >  arch/x86/Kconfig                   |    1 +
> >  arch/x86/include/asm/apb_timer.h   |   22 +--
> >  arch/x86/kernel/apb_timer.c        |  421
> > ++++++------------------------------
> > drivers/Kconfig                    |    2 +
> > drivers/clocksource/Kconfig        |    6 +
> > drivers/clocksource/Makefile       |    1 +
> > drivers/clocksource/dw_apb_timer.c |  322 +++++++++++++++++++++++++++
> > include/linux/dw_apb_timer.h       |  129 +++++++++++ 8 files
> > changed, 535 insertions(+), 369 deletions(-) create mode 100644
> > drivers/clocksource/Kconfig create mode 100644
> > drivers/clocksource/dw_apb_timer.c create mode 100644
> > include/linux/dw_apb_timer.h
> > 
> 
> > +static void apbt_eoi(struct dw_apb_timer *timer)
> > +{
> > +	apbt_readl(timer, APBTMR_N_EOI);
> > +}
> > +
> > +static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
> > +{
> > +	struct clock_event_device *evt = data;
> > +	struct dw_apb_clock_event_device *dw_ced =
> > ced_to_dw_apb_ced(evt); +
> > +	if (!evt->event_handler) {
> > +		pr_info("Spurious APBT timer interrupt %d", irq);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	dw_ced->eoi(&dw_ced->timer);
> I know I proposed this to deal with the fact that X86_MRST does not
> need eoi. I was hoping gcc to generate nops but it doesn't, especially
> with FRAME_POINTER turned on in our build.
>
> Since this is in the performance critical path and across ARM and X86
> architectures, can we use #define to skip eoi() for CONFIG_X86_MRST?

How about testing if dw_apb_timer::eoi is defined first before calling 
and not defining it for x86?  It's not zero overhead but it's less than 
what there is now.  The call would still be there for ARM but I don't 
have a good feel for how much this overhead is in relation to the rest 
of the IRQ and clock events handling.  I would guess that it isn't too 
significant.

Jamie