diff mbox

[v3,22/25] irq_domain/x86: Convert x86 (embedded) to use common irq_domain

Message ID 1327700179-17454-23-git-send-email-grant.likely@secretlab.ca (mailing list archive)
State Not Applicable
Headers show

Commit Message

Grant Likely Jan. 27, 2012, 9:36 p.m. UTC
This patch removes the x86-specific definition of irq_domain and replaces
it with the common implementation.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/Kconfig                      |    2 +
 arch/x86/include/asm/irq_controller.h |   12 ----
 arch/x86/include/asm/prom.h           |   10 ---
 arch/x86/kernel/devicetree.c          |  101 ++++++++++-----------------------
 drivers/net/phy/mdio-gpio.c           |    4 +-
 5 files changed, 34 insertions(+), 95 deletions(-)
 delete mode 100644 arch/x86/include/asm/irq_controller.h

Comments

Sebastian Andrzej Siewior Jan. 28, 2012, 4:44 p.m. UTC | #1
* Grant Likely | 2012-01-27 14:36:16 [-0700]:

>This patch removes the x86-specific definition of irq_domain and replaces
>it with the common implementation.

I pulled your devicetree/next tree. After this patch I get:

|Hierarchical RCU implementation.
|NR_IRQS:2304 nr_irqs:256 16
|------------[ cut here ]------------
|WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
|Modules linked in:
|Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
|Call Trace:
| [<c15044e0>] ? printk+0x18/0x1a
| [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
| [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
| [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
| [<c102ce0d>] warn_slowpath_null+0x1d/0x20
| [<c1095575>] irq_domain_add_legacy+0x75/0x150
| [<c1714824>] x86_add_irq_domains+0x96/0xd6
| [<c1708df2>] init_IRQ+0x8/0x33
| [<c170557f>] start_kernel+0x191/0x2e1
| [<c170517f>] ? loglevel+0x2b/0x2b
| [<c1705081>] i386_start_kernel+0x81/0x86
|---[ end trace 4eaa2a86a8e2da22 ]---
|------------[ cut here ]------------
|kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!

The warning is comming from this piece in irq_domain_add_legacy()
|for (i = 0; i < size; i++) {
|                 int irq = first_irq + i;
|                 struct irq_data *irq_data = irq_get_irq_data(irq);
| 
|                 if (WARN_ON(!irq_data || irq_data->domain)) {

irq_data is NULL here.

|                         mutex_unlock(&irq_domain_mutex);
|                         of_node_put(domain->of_node);
|                         kfree(domain);
|                         return NULL;
|                 }
|         }
| 

This is not always the case. arch_early_irq_init() in [0] sets up the
first 16 entries. The reminaing few (there is a toal of 24 irqs for
first ioapic and a second ioapic) are not initialized. This happens
later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.

[0] arch/x86/kernel/apic/io_apic.c

Sebastian
Grant Likely Jan. 29, 2012, 12:35 a.m. UTC | #2
On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
> 
> >This patch removes the x86-specific definition of irq_domain and replaces
> >it with the common implementation.
> 
> I pulled your devicetree/next tree. After this patch I get:
> 
> |Hierarchical RCU implementation.
> |NR_IRQS:2304 nr_irqs:256 16
> |------------[ cut here ]------------
> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
> |Modules linked in:
> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
> |Call Trace:
> | [<c15044e0>] ? printk+0x18/0x1a
> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
> | [<c1708df2>] init_IRQ+0x8/0x33
> | [<c170557f>] start_kernel+0x191/0x2e1
> | [<c170517f>] ? loglevel+0x2b/0x2b
> | [<c1705081>] i386_start_kernel+0x81/0x86
> |---[ end trace 4eaa2a86a8e2da22 ]---
> |------------[ cut here ]------------
> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
> 
> The warning is comming from this piece in irq_domain_add_legacy()
> |for (i = 0; i < size; i++) {
> |                 int irq = first_irq + i;
> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
> | 
> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
> 
> irq_data is NULL here.
> 
> |                         mutex_unlock(&irq_domain_mutex);
> |                         of_node_put(domain->of_node);
> |                         kfree(domain);
> |                         return NULL;
> |                 }
> |         }
> | 
> 
> This is not always the case. arch_early_irq_init() in [0] sets up the
> first 16 entries. The reminaing few (there is a toal of 24 irqs for
> first ioapic and a second ioapic) are not initialized. This happens
> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
> 
> [0] arch/x86/kernel/apic/io_apic.c

That's not going to work then.  The irqs must not be set up in ->xlate.
They need to be set up either before creating the irq_domain, or in the
.map hook.  Inside the map hook is preferred, but having some
configured and some not at initialization time does make it difficult

g.
Grant Likely Jan. 29, 2012, 12:39 a.m. UTC | #3
On Sat, Jan 28, 2012 at 5:35 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
>> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
>>
>> >This patch removes the x86-specific definition of irq_domain and replaces
>> >it with the common implementation.
>>
>> I pulled your devicetree/next tree. After this patch I get:
>>
>> |Hierarchical RCU implementation.
>> |NR_IRQS:2304 nr_irqs:256 16
>> |------------[ cut here ]------------
>> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
>> |Modules linked in:
>> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
>> |Call Trace:
>> | [<c15044e0>] ? printk+0x18/0x1a
>> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
>> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
>> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
>> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
>> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
>> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
>> | [<c1708df2>] init_IRQ+0x8/0x33
>> | [<c170557f>] start_kernel+0x191/0x2e1
>> | [<c170517f>] ? loglevel+0x2b/0x2b
>> | [<c1705081>] i386_start_kernel+0x81/0x86
>> |---[ end trace 4eaa2a86a8e2da22 ]---
>> |------------[ cut here ]------------
>> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
>>
>> The warning is comming from this piece in irq_domain_add_legacy()
>> |for (i = 0; i < size; i++) {
>> |                 int irq = first_irq + i;
>> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
>> |
>> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
>>
>> irq_data is NULL here.
>>
>> |                         mutex_unlock(&irq_domain_mutex);
>> |                         of_node_put(domain->of_node);
>> |                         kfree(domain);
>> |                         return NULL;
>> |                 }
>> |         }
>> |
>>
>> This is not always the case. arch_early_irq_init() in [0] sets up the
>> first 16 entries. The reminaing few (there is a toal of 24 irqs for
>> first ioapic and a second ioapic) are not initialized. This happens
>> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
>> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
>>
>> [0] arch/x86/kernel/apic/io_apic.c
>
> That's not going to work then.  The irqs must not be set up in ->xlate.
> They need to be set up either before creating the irq_domain, or in the
> .map hook.  Inside the map hook is preferred, but having some
> configured and some not at initialization time does make it difficult

Actually, that's still not right.  irq_set_chip_data() (sets
desc->irq_data.chip_data) has nothing to do with irq_get_irq_data()
(which returns desc->irq_data).  It's the   allocation of the irq_desc
that is at issue here.  Where does the allocation occur?  (I haven't
dug in to find it yet)

g.
Grant Likely Jan. 30, 2012, 7:58 p.m. UTC | #4
On Sat, Jan 28, 2012 at 05:44:05PM +0100, Sebastian Andrzej Siewior wrote:
> * Grant Likely | 2012-01-27 14:36:16 [-0700]:
> 
> >This patch removes the x86-specific definition of irq_domain and replaces
> >it with the common implementation.
> 
> I pulled your devicetree/next tree. After this patch I get:
> 
> |Hierarchical RCU implementation.
> |NR_IRQS:2304 nr_irqs:256 16
> |------------[ cut here ]------------
> |WARNING: at /home/bigeasy/work/shiva/git/linux-2.6-tip/kernel/irq/irqdomain.c:114 irq_domain_add_legacy+0x75/0x150()
> |Modules linked in:
> |Pid: 0, comm: swapper/0 Not tainted 3.3.0-rc1+ #65
> |Call Trace:
> | [<c15044e0>] ? printk+0x18/0x1a
> | [<c102cdbd>] warn_slowpath_common+0x6d/0xa0
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c1095575>] ? irq_domain_add_legacy+0x75/0x150
> | [<c102ce0d>] warn_slowpath_null+0x1d/0x20
> | [<c1095575>] irq_domain_add_legacy+0x75/0x150
> | [<c1714824>] x86_add_irq_domains+0x96/0xd6
> | [<c1708df2>] init_IRQ+0x8/0x33
> | [<c170557f>] start_kernel+0x191/0x2e1
> | [<c170517f>] ? loglevel+0x2b/0x2b
> | [<c1705081>] i386_start_kernel+0x81/0x86
> |---[ end trace 4eaa2a86a8e2da22 ]---
> |------------[ cut here ]------------
> |kernel BUG at /home/bigeasy/work/shiva/git/linux-2.6-tip/arch/x86/kernel/devicetree.c:367!
> 
> The warning is comming from this piece in irq_domain_add_legacy()
> |for (i = 0; i < size; i++) {
> |                 int irq = first_irq + i;
> |                 struct irq_data *irq_data = irq_get_irq_data(irq);
> | 
> |                 if (WARN_ON(!irq_data || irq_data->domain)) {
> 
> irq_data is NULL here.
> 
> |                         mutex_unlock(&irq_domain_mutex);
> |                         of_node_put(domain->of_node);
> |                         kfree(domain);
> |                         return NULL;
> |                 }
> |         }
> | 
> 
> This is not always the case. arch_early_irq_init() in [0] sets up the
> first 16 entries. The reminaing few (there is a toal of 24 irqs for
> first ioapic and a second ioapic) are not initialized. This happens
> later via ->xlate, ioapic_xlate() => io_apic_setup_irq_pin() =>
> alloc_irq_and_cfg_at() calls irq_set_chip_data() on demand.
> 
> [0] arch/x86/kernel/apic/io_apic.c

Ugh.  This isn't easy.  The legacy mapping really needs all the
irq_desc structures to be allocated.  You could call irq_alloc_descs()
before calling irq_domain_add_legacy(), but that causes all the
irq_descs to be allocated (regardless of whether they are used), and
it will break io_apic_setup_irq_pin() which also wants to call
irq_alloc_desc().

Ideally irq_domain support would be rolled directly into ioapic.
That's more work though, and a greater change of breaking x86 on
non-embedded.  Looking at the ioapic code, it seems to me that it
could be simplified quite a bit by switching to irq_domain instead of
using the custom irq_cfg linked list.  Faster too since it could use
the irq_data->hwirq to go from linux irq to the hw irq number.  It
doesn't look like it would be even that hard, but of course the devil
is in the details and I don't have sufficient time right now to dig
into the guts of the ioapic.  Maybe after connect, but it would help
if you can find time to look at it.

If integrated into the ioapic code, then the irq_domain linear map is
probably the type of irq_domain to use.

g.
Sebastian Andrzej Siewior Feb. 1, 2012, 2:17 p.m. UTC | #5
* Grant Likely | 2012-01-30 12:58:42 [-0700]:

>Ugh.  This isn't easy.  The legacy mapping really needs all the

Feel free to merge this patch. I don't have the time to look at this now
so I take a look at the ioapic later.

>g.

Sebastian
Grant Likely Feb. 1, 2012, 6:06 p.m. UTC | #6
On Wed, Feb 1, 2012 at 7:17 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> * Grant Likely | 2012-01-30 12:58:42 [-0700]:
>
>>Ugh.  This isn't easy.  The legacy mapping really needs all the
>
> Feel free to merge this patch. I don't have the time to look at this now
> so I take a look at the ioapic later.

There's no rush here.  I can leave it as-is with IRQ_DOMAIN turned off
for x86 for now.

g.
diff mbox

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 864cc6e..5c44039 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -399,6 +399,7 @@  config X86_INTEL_CE
 	select X86_REBOOTFIXUPS
 	select OF
 	select OF_EARLY_FLATTREE
+	select IRQ_DOMAIN
 	---help---
 	  Select for the Intel CE media processor (CE4100) SOC.
 	  This option compiles in support for the CE4100 SOC for settop
@@ -2077,6 +2078,7 @@  config OLPC
 	select GPIOLIB
 	select OF
 	select OF_PROMTREE
+	select IRQ_DOMAIN
 	---help---
 	  Add support for detecting the unique features of the OLPC
 	  XO hardware.
diff --git a/arch/x86/include/asm/irq_controller.h b/arch/x86/include/asm/irq_controller.h
deleted file mode 100644
index 423bbbd..0000000
--- a/arch/x86/include/asm/irq_controller.h
+++ /dev/null
@@ -1,12 +0,0 @@ 
-#ifndef __IRQ_CONTROLLER__
-#define __IRQ_CONTROLLER__
-
-struct irq_domain {
-	int (*xlate)(struct irq_domain *h, const u32 *intspec, u32 intsize,
-			u32 *out_hwirq, u32 *out_type);
-	void *priv;
-	struct device_node *controller;
-	struct list_head l;
-};
-
-#endif
diff --git a/arch/x86/include/asm/prom.h b/arch/x86/include/asm/prom.h
index 644dd885..60bef66 100644
--- a/arch/x86/include/asm/prom.h
+++ b/arch/x86/include/asm/prom.h
@@ -21,7 +21,6 @@ 
 #include <asm/irq.h>
 #include <linux/atomic.h>
 #include <asm/setup.h>
-#include <asm/irq_controller.h>
 
 #ifdef CONFIG_OF
 extern int of_ioapic;
@@ -43,15 +42,6 @@  extern char cmd_line[COMMAND_LINE_SIZE];
 #define pci_address_to_pio pci_address_to_pio
 unsigned long pci_address_to_pio(phys_addr_t addr);
 
-/**
- * irq_dispose_mapping - Unmap an interrupt
- * @virq: linux virq number of the interrupt to unmap
- *
- * FIXME: We really should implement proper virq handling like power,
- * but that's going to be major surgery.
- */
-static inline void irq_dispose_mapping(unsigned int virq) { }
-
 #define HAVE_ARCH_DEVTREE_FIXUPS
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5282179..3ae2ced 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -4,6 +4,7 @@ 
 #include <linux/bootmem.h>
 #include <linux/export.h>
 #include <linux/io.h>
+#include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
 #include <linux/of.h>
@@ -17,64 +18,14 @@ 
 #include <linux/initrd.h>
 
 #include <asm/hpet.h>
-#include <asm/irq_controller.h>
 #include <asm/apic.h>
 #include <asm/pci_x86.h>
 
 __initdata u64 initial_dtb;
 char __initdata cmd_line[COMMAND_LINE_SIZE];
-static LIST_HEAD(irq_domains);
-static DEFINE_RAW_SPINLOCK(big_irq_lock);
 
 int __initdata of_ioapic;
 
-#ifdef CONFIG_X86_IO_APIC
-static void add_interrupt_host(struct irq_domain *ih)
-{
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&big_irq_lock, flags);
-	list_add(&ih->l, &irq_domains);
-	raw_spin_unlock_irqrestore(&big_irq_lock, flags);
-}
-#endif
-
-static struct irq_domain *get_ih_from_node(struct device_node *controller)
-{
-	struct irq_domain *ih, *found = NULL;
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&big_irq_lock, flags);
-	list_for_each_entry(ih, &irq_domains, l) {
-		if (ih->controller ==  controller) {
-			found = ih;
-			break;
-		}
-	}
-	raw_spin_unlock_irqrestore(&big_irq_lock, flags);
-	return found;
-}
-
-unsigned int irq_create_of_mapping(struct device_node *controller,
-				   const u32 *intspec, unsigned int intsize)
-{
-	struct irq_domain *ih;
-	u32 virq, type;
-	int ret;
-
-	ih = get_ih_from_node(controller);
-	if (!ih)
-		return 0;
-	ret = ih->xlate(ih, intspec, intsize, &virq, &type);
-	if (ret)
-		return 0;
-	if (type == IRQ_TYPE_NONE)
-		return virq;
-	irq_set_irq_type(virq, type);
-	return virq;
-}
-EXPORT_SYMBOL_GPL(irq_create_of_mapping);
-
 unsigned long pci_address_to_pio(phys_addr_t address)
 {
 	/*
@@ -354,36 +305,43 @@  static struct of_ioapic_type of_ioapic_type[] =
 	},
 };
 
-static int ioapic_xlate(struct irq_domain *id, const u32 *intspec, u32 intsize,
-			u32 *out_hwirq, u32 *out_type)
+static int ioapic_xlate(struct irq_domain *domain,
+			struct device_node *controller,
+			const u32 *intspec, u32 intsize,
+			irq_hw_number_t *out_hwirq, u32 *out_type)
 {
-	struct mp_ioapic_gsi *gsi_cfg;
 	struct io_apic_irq_attr attr;
 	struct of_ioapic_type *it;
-	u32 line, idx, type;
+	u32 line, idx;
+	int rc;
 
-	if (intsize < 2)
+	if (WARN_ON(intsize < 2))
 		return -EINVAL;
 
-	line = *intspec;
-	idx = (u32) id->priv;
-	gsi_cfg = mp_ioapic_gsi_routing(idx);
-	*out_hwirq = line + gsi_cfg->gsi_base;
-
-	intspec++;
-	type = *intspec;
+	line = intspec[0];
 
-	if (type >= ARRAY_SIZE(of_ioapic_type))
+	if (intspec[1] >= ARRAY_SIZE(of_ioapic_type))
 		return -EINVAL;
 
-	it = of_ioapic_type + type;
-	*out_type = it->out_type;
+	it = &of_ioapic_type[intspec[1]];
 
+	idx = (u32) domain->host_data;
 	set_io_apic_irq_attr(&attr, idx, line, it->trigger, it->polarity);
 
-	return io_apic_setup_irq_pin_once(*out_hwirq, cpu_to_node(0), &attr);
+	rc = io_apic_setup_irq_pin_once(irq_find_mapping(domain, line),
+					cpu_to_node(0), &attr);
+	if (rc)
+		return rc;
+
+	*out_hwirq = line;
+	*out_type = it->out_type;
+	return 0;
 }
 
+const struct irq_domain_ops ioapic_irq_domain_ops = {
+	.xlate = ioapic_xlate,
+};
+
 static void __init ioapic_add_ofnode(struct device_node *np)
 {
 	struct resource r;
@@ -399,13 +357,14 @@  static void __init ioapic_add_ofnode(struct device_node *np)
 	for (i = 0; i < nr_ioapics; i++) {
 		if (r.start == mpc_ioapic_addr(i)) {
 			struct irq_domain *id;
+			struct mp_ioapic_gsi *gsi_cfg;
+
+			gsi_cfg = mp_ioapic_gsi_routing(i);
 
-			id = kzalloc(sizeof(*id), GFP_KERNEL);
+			id = irq_domain_add_legacy(np, 32, gsi_cfg->gsi_base, 0,
+						   &ioapic_irq_domain_ops,
+						   (void*)i);
 			BUG_ON(!id);
-			id->controller = np;
-			id->xlate = ioapic_xlate;
-			id->priv = (void *)i;
-			add_interrupt_host(id);
 			return;
 		}
 	}
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 50e8e5e..7189adf 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -255,13 +255,13 @@  static inline int __init mdio_ofgpio_init(void)
 	return platform_driver_register(&mdio_ofgpio_driver);
 }
 
-static inline void __exit mdio_ofgpio_exit(void)
+static inline void mdio_ofgpio_exit(void)
 {
 	platform_driver_unregister(&mdio_ofgpio_driver);
 }
 #else
 static inline int __init mdio_ofgpio_init(void) { return 0; }
-static inline void __exit mdio_ofgpio_exit(void) { }
+static inline void mdio_ofgpio_exit(void) { }
 #endif /* CONFIG_OF_GPIO */
 
 static struct platform_driver mdio_gpio_driver = {