[PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Pan, Jacob jun]

>From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@intel.com>
Date: Fri, 12 Jun 2009 02:16:05 -0700
Subject: [PATCH] x86/apic: support moorestown interrupt subsystem

This patch uses platform flags to selectively enable apic related setup
code.

Since moorestown does not have legacy timer or PIC, the only system
timer irqs are routed via ioapic. Early timer ioapic enabling is also
added to allow boot time timing services.

Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
---
 arch/x86/include/asm/apic.h    |    1 +
 arch/x86/include/asm/io_apic.h |    4 +-
 arch/x86/kernel/apic/apic.c    |    3 +
 arch/x86/kernel/apic/io_apic.c |  107 ++++++++++++++++++++++++++++++++++++---
 4 files changed, 104 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index bb7d479..59e888a 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -87,6 +87,7 @@ extern void xapic_wait_icr_idle(void);
 extern u32 safe_xapic_wait_icr_idle(void);
 extern void xapic_icr_write(u32, u32);
 extern int setup_profiling_timer(unsigned int);
+extern void __init pre_init_apic_IRQ(void);
 
 static inline void native_apic_mem_write(u32 reg, u32 v)
 {
diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index daf866e..9b373db 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -150,11 +150,11 @@ extern int timer_through_8259;
 #define io_apic_assign_pci_irqs \
 	(mp_irq_entries && !skip_ioapic_setup && io_apic_irqs)
 
-#ifdef CONFIG_ACPI
+#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
 extern int io_apic_get_unique_id(int ioapic, int apic_id);
 extern int io_apic_get_version(int ioapic);
 extern int io_apic_get_redir_entries(int ioapic);
-#endif /* CONFIG_ACPI */
+#endif /* CONFIG_ACPI || CONFIG_SFI */
 
 struct io_apic_irq_attr;
 extern int io_apic_set_pci_routing(struct device *dev, int irq,
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 8c7c042..c2b67d4 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -49,6 +49,7 @@
 #include <asm/mtrr.h>
 #include <asm/smp.h>
 #include <asm/mce.h>
+#include <asm/platform_feature.h>
 
 unsigned int num_processors;
 
@@ -1115,6 +1116,8 @@ void __init init_bsp_APIC(void)
 	value |= SPURIOUS_APIC_VECTOR;
 	apic_write(APIC_SPIV, value);
 
+	if (!platform_has(X86_PLATFORM_FEATURE_8259))
+		return;
 	/*
 	 * Set up the virtual wire mode.
 	 */
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c84dc3d..b50e33f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -36,6 +36,7 @@
 #include <linux/freezer.h>
 #include <linux/kthread.h>
 #include <linux/jiffies.h>	/* time_after() */
+#include <linux/sfi.h>
 #ifdef CONFIG_ACPI
 #include <acpi/acpi_bus.h>
 #endif
@@ -62,7 +63,8 @@
 #include <asm/hw_irq.h>
 #include <asm/uv/uv_hub.h>
 #include <asm/uv/uv_irq.h>
-
+#include <asm/platform_feature.h>
+#include <asm/apb_timer.h>
 #include <asm/apic.h>
 
 #define __apicdebuginit(type) static type __init
@@ -130,6 +132,14 @@ struct irq_pin_list {
 	struct irq_pin_list *next;
 };
 
+/* Use static early entry before kzalloc is ready, for platforms need system
+ * timer IRQ0 setup in IOAPIC early.
+ */
+static struct irq_pin_list early_entry;
+static inline struct irq_pin_list *get_one_free_irq_2_pin_early(int node)
+{
+	return &early_entry;
+}
 static struct irq_pin_list *get_one_free_irq_2_pin(int node)
 {
 	struct irq_pin_list *pin;
@@ -504,7 +514,11 @@ static void add_pin_to_irq_node(struct irq_cfg *cfg, int node, int apic, int pin
 
 	entry = cfg->irq_2_pin;
 	if (!entry) {
-		entry = get_one_free_irq_2_pin(node);
+		/* Setup APB timer 0 is prior to kzalloc() becomes ready */
+		if (platform_has(X86_PLATFORM_FEATURE_APBT) && (!pin)) {
+			entry = get_one_free_irq_2_pin_early(node);
+		} else
+			entry = get_one_free_irq_2_pin(node);
 		if (!entry) {
 			printk(KERN_ERR "can not alloc irq_2_pin to add %d - %d\n",
 					apic, pin);
@@ -1450,6 +1464,12 @@ int setup_ioapic_entry(int apic_id, int irq,
 	return 0;
 }
 
+static inline int is_8259_legacy_irq(int irq)
+{
+	return (((irq < NR_IRQS_LEGACY)
+		&& platform_has(X86_PLATFORM_FEATURE_8259)));
+}
+
 static void setup_IO_APIC_irq(int apic_id, int pin, unsigned int irq, struct irq_desc *desc,
 			      int trigger, int polarity)
 {
@@ -1483,7 +1503,7 @@ static void setup_IO_APIC_irq(int apic_id, int pin, unsigned int irq, struct irq
 	}
 
 	ioapic_register_intr(irq, desc, trigger);
-	if (irq < NR_IRQS_LEGACY)
+	if (is_8259_legacy_irq(irq))
 		disable_8259A_irq(irq);
 
 	ioapic_write_entry(apic_id, pin, entry);
@@ -1892,7 +1912,8 @@ __apicdebuginit(void) print_PIC(void)
 
 __apicdebuginit(int) print_all_ICs(void)
 {
-	print_PIC();
+	if (platform_has(X86_PLATFORM_FEATURE_8259))
+		print_PIC();
 
 	/* don't print out if apic is not there */
 	if (!cpu_has_apic || disable_apic)
@@ -1926,6 +1947,8 @@ void __init enable_IO_APIC(void)
 		spin_unlock_irqrestore(&ioapic_lock, flags);
 		nr_ioapic_registers[apic] = reg_01.bits.entries+1;
 	}
+	if (!platform_has(X86_PLATFORM_FEATURE_8259))
+		return;
 	for(apic = 0; apic < nr_ioapics; apic++) {
 		int pin;
 		/* See if any of the pins is in ExtINT mode */
@@ -1980,6 +2003,8 @@ void disable_IO_APIC(void)
 	 */
 	clear_IO_APIC();
 
+	if (!platform_has(X86_PLATFORM_FEATURE_8259))
+		return;
 	/*
 	 * If the i8259 is routed through an IOAPIC
 	 * Put that IOAPIC in virtual wire mode
@@ -2211,7 +2236,7 @@ static unsigned int startup_ioapic_irq(unsigned int irq)
 	struct irq_cfg *cfg;
 
 	spin_lock_irqsave(&ioapic_lock, flags);
-	if (irq < NR_IRQS_LEGACY) {
+	if (is_8259_legacy_irq(irq)) {
 		disable_8259A_irq(irq);
 		if (i8259A_irq_pending(irq))
 			was_pending = 1;
@@ -2723,7 +2748,7 @@ static inline void init_IO_APIC_traps(void)
 			 * so default to an old-fashioned 8259
 			 * interrupt if we can..
 			 */
-			if (irq < NR_IRQS_LEGACY)
+			if (is_8259_legacy_irq(irq))
 				make_8259A_irq(irq);
 			else
 				/* Strange. Oh, well.. */
@@ -2878,6 +2903,13 @@ static inline void __init check_timer(void)
 
 	local_irq_save(flags);
 
+	if (platform_has(X86_PLATFORM_FEATURE_APBT)) {
+		if (timer_irq_works()) {
+			printk(KERN_INFO "APB timer works\n");
+			return;
+		} else
+			panic("Check APB timer failed\n");
+	}
 	/*
 	 * get/set the timer IRQ vector:
 	 */
@@ -3067,8 +3099,10 @@ void __init setup_IO_APIC(void)
 	/*
 	 * calling enable_IO_APIC() is moved to setup_local_APIC for BP
 	 */
-
-	io_apic_irqs = ~PIC_IRQS;
+	if (!platform_has(X86_PLATFORM_FEATURE_8259))
+		io_apic_irqs = ~0;
+	else
+		io_apic_irqs = ~PIC_IRQS;
 
 	apic_printk(APIC_VERBOSE, "ENABLING IO-APIC IRQs\n");
 	/*
@@ -3959,7 +3993,7 @@ int io_apic_set_pci_routing(struct device *dev, int irq,
                           ACPI-based IOAPIC Configuration
    -------------------------------------------------------------------------- */
 
-#ifdef CONFIG_ACPI
+#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
 
 #ifdef CONFIG_X86_32
 int __init io_apic_get_unique_id(int ioapic, int apic_id)
@@ -4223,3 +4257,58 @@ static int __init ioapic_insert_resources(void)
 /* Insert the IO APIC resources after PCI initialization has occured to handle
  * IO APICS that are mapped in on a BAR in PCI space. */
 late_initcall(ioapic_insert_resources);
+/* Enable IOAPIC early just for system timer */
+void __init pre_init_apic_IRQ(void)
+{
+	struct irq_cfg *cfg;
+	printk(KERN_INFO "Early APIC setup for system timer\n");
+#ifndef CONFIG_SMP
+	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
+#endif
+	setup_local_APIC();
+	cfg = irq_cfg(0);
+	add_pin_to_irq_node(cfg, 0, 0, 0);
+	setup_timer_IRQ0_pin(0, 0, cfg->vector);
+}
+#ifdef CONFIG_APB_TIMER
+int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu)
+{
+	struct IO_APIC_route_entry entry;
+	struct irq_cfg *cfg;
+	int apic_id;
+	memset(&entry, 0, sizeof(entry));
+	cfg = irq_cfg(irq);
+	apic_id = mp_sfi_find_ioapic(irq);
+	if (apic_id == -1) {
+		printk(KERN_ERR "Failed to setup APB timer IRQ %d\n", irq);
+		return apic_id;
+	}
+	/*
+	 * We use logical delivery to get the timer IRQ
+	 * to the first CPU. RH must work or cleared in MSI address
+	 */
+	entry.dest_mode = apic->irq_dest_mode;
+	entry.mask = mask;
+	if (!apic->irq_dest_mode)
+		entry.dest = cpu;		/* physical apic id */
+	else
+		entry.dest = apic->cpu_to_logical_apicid(cpu);
+	entry.delivery_mode = apic->irq_delivery_mode;
+	entry.polarity = 0;
+	entry.trigger = trigger;
+	entry.vector = cfg->vector;
+
+	if (trigger == 0)
+		set_irq_chip_and_handler_name(irq, &ioapic_chip,
+			handle_edge_irq, "edge");
+	else
+		set_irq_chip_and_handler_name(irq, &ioapic_chip,
+			handle_fasteoi_irq, "fasteoi");
+	/*
+	 * Add it to the IO-APIC irq-routing table:
+	 * pin and irq are 1:1 mapped
+	 */
+	ioapic_write_entry(apic_id, irq, entry);
+	return 0;
+}
+#endif
-- 
1.5.6.5

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Ingo Molnar]

* Pan, Jacob jun <jacob.jun.pan@intel.com> wrote:

> >From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Fri, 12 Jun 2009 02:16:05 -0700
> Subject: [PATCH] x86/apic: support moorestown interrupt subsystem
> 
> This patch uses platform flags to selectively enable apic related setup
> code.
> 
> Since moorestown does not have legacy timer or PIC, the only system
> timer irqs are routed via ioapic. Early timer ioapic enabling is also
> added to allow boot time timing services.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> ---
>  arch/x86/include/asm/apic.h    |    1 +
>  arch/x86/include/asm/io_apic.h |    4 +-
>  arch/x86/kernel/apic/apic.c    |    3 +
>  arch/x86/kernel/apic/io_apic.c |  107 ++++++++++++++++++++++++++++++++++++---
>  4 files changed, 104 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index bb7d479..59e888a 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -87,6 +87,7 @@ extern void xapic_wait_icr_idle(void);
>  extern u32 safe_xapic_wait_icr_idle(void);
>  extern void xapic_icr_write(u32, u32);
>  extern int setup_profiling_timer(unsigned int);
> +extern void __init pre_init_apic_IRQ(void);

externs dont need __init annotations.

>  static inline void native_apic_mem_write(u32 reg, u32 v)
>  {
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index daf866e..9b373db 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -150,11 +150,11 @@ extern int timer_through_8259;
>  #define io_apic_assign_pci_irqs \
>  	(mp_irq_entries && !skip_ioapic_setup && io_apic_irqs)
>  
> -#ifdef CONFIG_ACPI
> +#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)

Please add a new helper non-interactive Kconfig symbol instead of 
increasing the #ifdef jungle.

>  extern int io_apic_get_unique_id(int ioapic, int apic_id);
>  extern int io_apic_get_version(int ioapic);
>  extern int io_apic_get_redir_entries(int ioapic);
> -#endif /* CONFIG_ACPI */
> +#endif /* CONFIG_ACPI || CONFIG_SFI */
>  
>  struct io_apic_irq_attr;
>  extern int io_apic_set_pci_routing(struct device *dev, int irq,
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 8c7c042..c2b67d4 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -49,6 +49,7 @@
>  #include <asm/mtrr.h>
>  #include <asm/smp.h>
>  #include <asm/mce.h>
> +#include <asm/platform_feature.h>

hm, this include file name is pretty meaningless.

>  
>  unsigned int num_processors;
>  
> @@ -1115,6 +1116,8 @@ void __init init_bsp_APIC(void)
>  	value |= SPURIOUS_APIC_VECTOR;
>  	apic_write(APIC_SPIV, value);
>  
> +	if (!platform_has(X86_PLATFORM_FEATURE_8259))
> +		return;
>  	/*
>  	 * Set up the virtual wire mode.
>  	 */
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index c84dc3d..b50e33f 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -36,6 +36,7 @@
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
>  #include <linux/jiffies.h>	/* time_after() */
> +#include <linux/sfi.h>
>  #ifdef CONFIG_ACPI
>  #include <acpi/acpi_bus.h>
>  #endif
> @@ -62,7 +63,8 @@
>  #include <asm/hw_irq.h>
>  #include <asm/uv/uv_hub.h>
>  #include <asm/uv/uv_irq.h>
> -
> +#include <asm/platform_feature.h>
> +#include <asm/apb_timer.h>
>  #include <asm/apic.h>
>  
>  #define __apicdebuginit(type) static type __init
> @@ -130,6 +132,14 @@ struct irq_pin_list {
>  	struct irq_pin_list *next;
>  };
>  
> +/* Use static early entry before kzalloc is ready, for platforms need system
> + * timer IRQ0 setup in IOAPIC early.
> + */

please use the customary comment style:

  /*
   * Comment .....
   * ...... goes here:
   */

specified in Documentation/CodingStyle.

> +static struct irq_pin_list early_entry;
> +static inline struct irq_pin_list *get_one_free_irq_2_pin_early(int node)
> +{
> +	return &early_entry;
> +}

please put a newline between variable and function.

>  static struct irq_pin_list *get_one_free_irq_2_pin(int node)
>  {
>  	struct irq_pin_list *pin;
> @@ -504,7 +514,11 @@ static void add_pin_to_irq_node(struct irq_cfg *cfg, int node, int apic, int pin
>  
>  	entry = cfg->irq_2_pin;
>  	if (!entry) {
> -		entry = get_one_free_irq_2_pin(node);
> +		/* Setup APB timer 0 is prior to kzalloc() becomes ready */
> +		if (platform_has(X86_PLATFORM_FEATURE_APBT) && (!pin)) {
> +			entry = get_one_free_irq_2_pin_early(node);
> +		} else
> +			entry = get_one_free_irq_2_pin(node);

Unbalanced curly braces.

>  		if (!entry) {
>  			printk(KERN_ERR "can not alloc irq_2_pin to add %d - %d\n",
>  					apic, pin);
> @@ -1450,6 +1464,12 @@ int setup_ioapic_entry(int apic_id, int irq,
>  	return 0;
>  }
>  
> +static inline int is_8259_legacy_irq(int irq)
> +{
> +	return (((irq < NR_IRQS_LEGACY)
> +		&& platform_has(X86_PLATFORM_FEATURE_8259)));
> +}

excessive parathesis.

> +
>  static void setup_IO_APIC_irq(int apic_id, int pin, unsigned int irq, struct irq_desc *desc,
>  			      int trigger, int polarity)
>  {
> @@ -1483,7 +1503,7 @@ static void setup_IO_APIC_irq(int apic_id, int pin, unsigned int irq, struct irq
>  	}
>  
>  	ioapic_register_intr(irq, desc, trigger);
> -	if (irq < NR_IRQS_LEGACY)
> +	if (is_8259_legacy_irq(irq))
>  		disable_8259A_irq(irq);

all i8259 related functions use the 'i8259' token. This one uses 
'8259'.

>  
>  	ioapic_write_entry(apic_id, pin, entry);
> @@ -1892,7 +1912,8 @@ __apicdebuginit(void) print_PIC(void)
>  
>  __apicdebuginit(int) print_all_ICs(void)
>  {
> -	print_PIC();
> +	if (platform_has(X86_PLATFORM_FEATURE_8259))
> +		print_PIC();

the check should be within print_PIC(), to not contaminate this 
function with that detail.

>  
>  	/* don't print out if apic is not there */
>  	if (!cpu_has_apic || disable_apic)
> @@ -1926,6 +1947,8 @@ void __init enable_IO_APIC(void)
>  		spin_unlock_irqrestore(&ioapic_lock, flags);
>  		nr_ioapic_registers[apic] = reg_01.bits.entries+1;
>  	}
> +	if (!platform_has(X86_PLATFORM_FEATURE_8259))
> +		return;
>  	for(apic = 0; apic < nr_ioapics; apic++) {
>  		int pin;
>  		/* See if any of the pins is in ExtINT mode */
> @@ -1980,6 +2003,8 @@ void disable_IO_APIC(void)
>  	 */
>  	clear_IO_APIC();
>  
> +	if (!platform_has(X86_PLATFORM_FEATURE_8259))
> +		return;
>  	/*
>  	 * If the i8259 is routed through an IOAPIC
>  	 * Put that IOAPIC in virtual wire mode

this should be abstracted out cleaner, instead of splashing if 
(feature) branches all across the code.

> @@ -2211,7 +2236,7 @@ static unsigned int startup_ioapic_irq(unsigned int irq)
>  	struct irq_cfg *cfg;
>  
>  	spin_lock_irqsave(&ioapic_lock, flags);
> -	if (irq < NR_IRQS_LEGACY) {
> +	if (is_8259_legacy_irq(irq)) {
>  		disable_8259A_irq(irq);
>  		if (i8259A_irq_pending(irq))
>  			was_pending = 1;
> @@ -2723,7 +2748,7 @@ static inline void init_IO_APIC_traps(void)
>  			 * so default to an old-fashioned 8259
>  			 * interrupt if we can..
>  			 */
> -			if (irq < NR_IRQS_LEGACY)
> +			if (is_8259_legacy_irq(irq))
>  				make_8259A_irq(irq);
>  			else
>  				/* Strange. Oh, well.. */
> @@ -2878,6 +2903,13 @@ static inline void __init check_timer(void)
>  
>  	local_irq_save(flags);
>  
> +	if (platform_has(X86_PLATFORM_FEATURE_APBT)) {
> +		if (timer_irq_works()) {
> +			printk(KERN_INFO "APB timer works\n");
> +			return;
> +		} else
> +			panic("Check APB timer failed\n");
> +	}

ditto.

>  	/*
>  	 * get/set the timer IRQ vector:
>  	 */
> @@ -3067,8 +3099,10 @@ void __init setup_IO_APIC(void)
>  	/*
>  	 * calling enable_IO_APIC() is moved to setup_local_APIC for BP
>  	 */
> -
> -	io_apic_irqs = ~PIC_IRQS;
> +	if (!platform_has(X86_PLATFORM_FEATURE_8259))
> +		io_apic_irqs = ~0;
> +	else
> +		io_apic_irqs = ~PIC_IRQS;

-ENOCOMMENT.

>  
>  	apic_printk(APIC_VERBOSE, "ENABLING IO-APIC IRQs\n");
>  	/*
> @@ -3959,7 +3993,7 @@ int io_apic_set_pci_routing(struct device *dev, int irq,
>                            ACPI-based IOAPIC Configuration
>     -------------------------------------------------------------------------- */
>  
> -#ifdef CONFIG_ACPI
> +#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
>  
>  #ifdef CONFIG_X86_32
>  int __init io_apic_get_unique_id(int ioapic, int apic_id)
> @@ -4223,3 +4257,58 @@ static int __init ioapic_insert_resources(void)
>  /* Insert the IO APIC resources after PCI initialization has occured to handle
>   * IO APICS that are mapped in on a BAR in PCI space. */
>  late_initcall(ioapic_insert_resources);
> +/* Enable IOAPIC early just for system timer */
> +void __init pre_init_apic_IRQ(void)

missing newline.

> +{
> +	struct irq_cfg *cfg;
> +	printk(KERN_INFO "Early APIC setup for system timer\n");
> +#ifndef CONFIG_SMP
> +	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
> +#endif
> +	setup_local_APIC();

missing newline.

> +	cfg = irq_cfg(0);
> +	add_pin_to_irq_node(cfg, 0, 0, 0);
> +	setup_timer_IRQ0_pin(0, 0, cfg->vector);
> +}
> +#ifdef CONFIG_APB_TIMER
> +int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu)

missing newline.

> +{
> +	struct IO_APIC_route_entry entry;
> +	struct irq_cfg *cfg;
> +	int apic_id;
> +	memset(&entry, 0, sizeof(entry));
> +	cfg = irq_cfg(irq);

missing newline.

> +	apic_id = mp_sfi_find_ioapic(irq);
> +	if (apic_id == -1) {
> +		printk(KERN_ERR "Failed to setup APB timer IRQ %d\n", irq);
> +		return apic_id;
> +	}
> +	/*
> +	 * We use logical delivery to get the timer IRQ
> +	 * to the first CPU. RH must work or cleared in MSI address
> +	 */
> +	entry.dest_mode = apic->irq_dest_mode;
> +	entry.mask = mask;
> +	if (!apic->irq_dest_mode)
> +		entry.dest = cpu;		/* physical apic id */
> +	else
> +		entry.dest = apic->cpu_to_logical_apicid(cpu);
> +	entry.delivery_mode = apic->irq_delivery_mode;
> +	entry.polarity = 0;
> +	entry.trigger = trigger;
> +	entry.vector = cfg->vector;
> +
> +	if (trigger == 0)
> +		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> +			handle_edge_irq, "edge");
> +	else
> +		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> +			handle_fasteoi_irq, "fasteoi");
> +	/*
> +	 * Add it to the IO-APIC irq-routing table:
> +	 * pin and irq are 1:1 mapped
> +	 */
> +	ioapic_write_entry(apic_id, irq, entry);
> +	return 0;
> +}
> +#endif

This code is stylistically and structurally challenged and will need 
a lot more work to be acceptable. Please clean it up and abstract 
out the timer driver bits:

The proper solution here is to first introduce a proper abstraction 
without adding the ABP bits - then can come a separate ABP patch 
that adds the new hardware support - without touching any other code 
but adding its own small driver module.

	Ingo

RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Pan, Jacob jun]

>
>This code is stylistically and structurally challenged and will need
>a lot more work to be acceptable. Please clean it up and abstract
>out the timer driver bits:
>
>The proper solution here is to first introduce a proper abstraction
>without adding the ABP bits - then can come a separate ABP patch
>that adds the new hardware support - without touching any other code
>but adding its own small driver module.
>
>	Ingo
[[JPAN]] thanks for the detailed comments, I will fix the style problem in the
 next version. I can separate APB timer code into another patch. But I need
a little more clarification for the abstraction you want. The reason why I did
it this way was it is similar to the other arch_setup_xx, e.g. HPET. In
 io_apic.c 
So do you want to abstract all the arch_setup_xxx in the new abstraction layer?

RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Pan, Jacob jun]

>> -#ifdef CONFIG_ACPI
>> +#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
>
>Please add a new helper non-interactive Kconfig symbol instead of
>increasing the #ifdef jungle.
>
[[JPAN]] I agreed, maybe this should be part of the SFI patch or already have similar plans?

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Eric W. Biederman]

"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:

>>From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Fri, 12 Jun 2009 02:16:05 -0700
> Subject: [PATCH] x86/apic: support moorestown interrupt subsystem
>
> This patch uses platform flags to selectively enable apic related setup
> code.
>
> Since moorestown does not have legacy timer or PIC, the only system
> timer irqs are routed via ioapic. Early timer ioapic enabling is also
> added to allow boot time timing services.

This patch is horribly wrong.   We should not have a moorestown specific
hack  we should not do early timer ioapic on everything that supports
it which is most x86 machines since apics became common.

At which point moorestown support should just be a little work somewhere
in the table parsers.

If you can't compile out the 8259 support code this has been factored
wrong. 

There are a handful of legacy systems with mptables that run in ioapic
mode yet use the timer and sometimes a couple of other devices on
the 8259 PIC.  Handling that case will complicate things a bit.

Hopefully it will be easier now to properly rework the code.  When
I tried it.  Linus's laptop died somewhere half way through bootup.
So we had to revert the support.

In summary if moorestown does not have an 8259 PIC it is time to remove
this long standing deficiency of the x86 ioapic code, not hack around
it.

Eric

RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Pan, Jacob jun]

>-----Original Message-----
>From: Eric W. Biederman [mailto:ebiederm@xmission.com]
>Sent: Friday, June 26, 2009 12:42 PM
>To: Pan, Jacob jun
>Cc: linux-kernel@vger.kernel.org; H. Peter Anvin
>Subject: Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem
>
>"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:
>
>>>From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
>> From: Jacob Pan <jacob.jun.pan@intel.com>
>> Date: Fri, 12 Jun 2009 02:16:05 -0700
>> Subject: [PATCH] x86/apic: support moorestown interrupt subsystem
>>
>> This patch uses platform flags to selectively enable apic related setup
>> code.
>>
>> Since moorestown does not have legacy timer or PIC, the only system
>> timer irqs are routed via ioapic. Early timer ioapic enabling is also
>> added to allow boot time timing services.
>
>This patch is horribly wrong.   We should not have a moorestown specific
>hack  we should not do early timer ioapic on everything that supports
>it which is most x86 machines since apics became common.
>
[[JPAN]] maybe I misunderstood. But I am doing the special early timer ioapic
setup in x86_quirks, so it is not for every platform.

>At which point moorestown support should just be a little work somewhere
>in the table parsers.
>
[[JPAN]] can you elaborate a little? I was hoping to move APIC initialization earlier so that I don't have to have the special treatment for system timer.
But it seems require early memory allocator.

>If you can't compile out the 8259 support code this has been factored
>wrong.
>
>There are a handful of legacy systems with mptables that run in ioapic
>mode yet use the timer and sometimes a couple of other devices on
>the 8259 PIC.  Handling that case will complicate things a bit.
>
>Hopefully it will be easier now to properly rework the code.  When
>I tried it.  Linus's laptop died somewhere half way through bootup.
>So we had to revert the support.
>
>In summary if moorestown does not have an 8259 PIC it is time to remove
>this long standing deficiency of the x86 ioapic code, not hack around
>it.
>
[[JPAN]] Moorestown does not have 8259, even IOAPIC is emulated by FW. Are you
saying we should try to completely separate 8259 instead of use if(8259) here
and there in the IOAPIC code? T
>Eric
[[JPAN]] thanks for the feedback.

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Eric W. Biederman]

"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:

>>-----Original Message-----
>>From: Eric W. Biederman [mailto:ebiederm@xmission.com]
>>Sent: Friday, June 26, 2009 12:42 PM
>>To: Pan, Jacob jun
>>Cc: linux-kernel@vger.kernel.org; H. Peter Anvin
>>Subject: Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem
>>
>>"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:
>>
>>>>From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
>>> From: Jacob Pan <jacob.jun.pan@intel.com>
>>> Date: Fri, 12 Jun 2009 02:16:05 -0700
>>> Subject: [PATCH] x86/apic: support moorestown interrupt subsystem
>>>
>>> This patch uses platform flags to selectively enable apic related setup
>>> code.
>>>
>>> Since moorestown does not have legacy timer or PIC, the only system
>>> timer irqs are routed via ioapic. Early timer ioapic enabling is also
>>> added to allow boot time timing services.
>>
>>This patch is horribly wrong.   We should not have a moorestown specific
>>hack  we should not do early timer ioapic on everything that supports
>>it which is most x86 machines since apics became common.
>>
> [[JPAN]] maybe I misunderstood. But I am doing the special early timer ioapic
> setup in x86_quirks, so it is not for every platform.
>
>>At which point moorestown support should just be a little work somewhere
>>in the table parsers.
>>
> [[JPAN]] can you elaborate a little? I was hoping to move APIC initialization earlier so that I don't have to have the special treatment for system timer.
> But it seems require early memory allocator.

We have early memory allocators.  And we are not talking super
early.  Just at the point init_IRQ is called.

> [[JPAN]] Moorestown does not have 8259, even IOAPIC is emulated by FW. Are you
> saying we should try to completely separate 8259 instead of use if(8259) here
> and there in the IOAPIC code? T

Yes, completely separate out the 8259.  I believe that complete
separation will come for free if you move the ioapic earlier.

What kind of PIC is native?

Eric

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Feng Tang]

On Sat, 27 Jun 2009 01:18:50 +0800
"Pan, Jacob jun" <jacob.jun.pan@intel.com> wrote:

> >> -#ifdef CONFIG_ACPI
> >> +#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
> >
> >Please add a new helper non-interactive Kconfig symbol instead of
> >increasing the #ifdef jungle.
> >
> [[JPAN]] I agreed, maybe this should be part of the SFI patch or
> already have similar plans?

Yes, Ingo has given similar comments for SFI code. I thought about 2
methods for this:
1. these "#ifdef...#endif" covers three functions:
 extern int io_apic_get_unique_id(int ioapic, int apic_id);
 extern int io_apic_get_version(int ioapic);
 extern int io_apic_get_redir_entries(int ioapic);	
how about just completely removing the "#ifdef...#endif", as ACPI/SFI 
codes are heavily used on x86 platforms, and it may bring one hundred
additional bytes for None-ACPI/SFI platforms 
2. create a new "CONFIG_ACPI_SFI" option, using this new option to cover
this case, and let "ACPI"/"SFI" option select it in Kconfig files

any comments? thanks

- Feng

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Len Brown]

On Mon, 29 Jun 2009, Feng Tang wrote:

> On Sat, 27 Jun 2009 01:18:50 +0800
> "Pan, Jacob jun" <jacob.jun.pan@intel.com> wrote:
> 
> > >> -#ifdef CONFIG_ACPI
> > >> +#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
> > >
> > >Please add a new helper non-interactive Kconfig symbol instead of
> > >increasing the #ifdef jungle.
> > >
> > [[JPAN]] I agreed, maybe this should be part of the SFI patch or
> > already have similar plans?
> 
> Yes, Ingo has given similar comments for SFI code. I thought about 2
> methods for this:
> 1. these "#ifdef...#endif" covers three functions:
>  extern int io_apic_get_unique_id(int ioapic, int apic_id);
>  extern int io_apic_get_version(int ioapic);
>  extern int io_apic_get_redir_entries(int ioapic);	
> how about just completely removing the "#ifdef...#endif", as ACPI/SFI 
> codes are heavily used on x86 platforms, and it may bring one hundred
> additional bytes for None-ACPI/SFI platforms 
> 2. create a new "CONFIG_ACPI_SFI" option, using this new option to cover
> this case, and let "ACPI"/"SFI" option select it in Kconfig files
> 
> any comments? thanks

I vote #2 -- remove the #ifdefs.
All of these routines are both small and __init,
and the only build that would notice the extra bytes in .text
is the x86 IOAPIC ACPI=n build, which is uncommon today
and becoming more uncommon over time.

The SFI patch series doesn't actually depend on these routines,
the IO-APIC patch depends on them.  So this change
should be in the IO-APIC series.

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Len Brown]

> I vote #2 -- remove the #ifdefs.

er, that was option #1 -- sorry if that was confusing.

in any case, i've updated the SFI series to not touch io_apic.h at all,
and that can be done by the x86/ioapic series.

thanks,
-Len

RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem

[Pan, Jacob jun]

>Yes, completely separate out the 8259.  I believe that complete
>separation will come for free if you move the ioapic earlier.
>
>What kind of PIC is native?
>

[[JPAN]] the emulated IOAPIC is for the south complex, and north complex 
has true PCI MSI capability so it does not need an ioapic. There is no native
hw interrupt controller, all interrupts are forwarded by the FW and delivered
 to local APIC via FSB writes (kind of).

The programming interface for the emulated ioapic is compatiable but there are 
are some special "features" such as no special/legacy entries such as cascade 
i8259, hpet legacy replacement routes (2,8), etc.

I will add a more detail introduction in the next version of this patch set.