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

From: Ingo Molnar
Date: Fri Jun 26 2009 - 03:07:47 EST

Next message: Pekka Enberg: "Re: kmemleak suggestion (long message)"
Previous message: Peter Zijlstra: "Re: register_timer_hook use in arch/sh/oprofile"
In reply to: Pan, Jacob jun: "[PATCH 9/9] x86/apic: support moorestown interrupt subsystem"
Next in thread: Pan, Jacob jun: "RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

> >From 82d64ca4f963d2e205326534aff0c77d9bfa5858 Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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
--
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/

Next message: Pekka Enberg: "Re: kmemleak suggestion (long message)"
Previous message: Peter Zijlstra: "Re: register_timer_hook use in arch/sh/oprofile"
In reply to: Pan, Jacob jun: "[PATCH 9/9] x86/apic: support moorestown interrupt subsystem"
Next in thread: Pan, Jacob jun: "RE: [PATCH 9/9] x86/apic: support moorestown interrupt subsystem"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]