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

[jacob pan <jacob.jun.pan@linux.intel.com>]

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?


Thanks,

Jacob