[RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Michal Ostrowski]

The following set of patches is aimed at killing the
ppc64_interrupt_controller global variable, and similarly that various
global variables used to tweak the parameters of virtual IRQ re-mapping.
Instead each platform can use a single function to configure IRQ
re-mapping in "init_early()".

Platforms can choose the default configuration (which is to
say identity mappings of IRQ vectors 0..511) or specify their own
configuration:  an offset (for ISA vectors), the range that is to be
identity mapped and the range of vectors that may be arbitrarily
re-mapper.

This allows us to remove the special-casing based on
ppc64_interrupt_controller in virt_irq_create_mapping(). 

Subsequently most uses of ppc64_interrupt_controller are on
configuration paths where we can typically obtain comparable information
by alternate means.

In the end, only pSeries-specific code must be aware of the choice of
PIC and it will check to see if an MPIC has been initialized (and if
not, assume XICS). (Thus no platforms beyond pSeries are affected.)

This code has been tested on pSeries LPAR, G5, and Maple.  I'll be
re-testing it on a Power3 with MPIC shortly once a system for testing
becomes available again.

-- 
Michal Ostrowski <mostrows@watson.ibm.com>

[PATCH 1/8] Formalize virtual-IRQ remapping configuration.

[mostrows]

Instead of setting various global various to configure the behavior of
virtual-IRQ remapping, each platform has the option of calling
virt_irq_config() in "init_early" to specify:

- the offset to be used (i.e. NUM_ISA_INTERRUPTS)
- the number of interrupt vectors which are to be mapped 1-1
- the number of interrupt vectors that may be arbitrarily remapped

If virt_irq_config() is not called, the default configuration will be
used (512 identity-mapped interrupts with no offset).

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/kernel/irq.c              |   62 ++++++++++++++++----------------
 arch/powerpc/kernel/prom.c             |    4 --
 arch/powerpc/kernel/setup_64.c         |    6 +++
 arch/powerpc/platforms/iseries/setup.c |    3 +-
 arch/powerpc/platforms/pseries/setup.c |   10 +++++
 include/asm-powerpc/irq.h              |   30 +++++++++++----
 6 files changed, 70 insertions(+), 45 deletions(-)

2f99449cbcacf67694e38698ec9860f86ba052e2
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 57d560c..efcb859 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -268,32 +268,35 @@ void __init init_IRQ(void)
 #define UNDEFINED_IRQ 0xffffffff
 unsigned int virt_irq_to_real_map[NR_IRQS];
 
-/*
- * Don't use virtual irqs 0, 1, 2 for devices.
- * The pcnet32 driver considers interrupt numbers < 2 to be invalid,
- * and 2 is the XICS IPI interrupt.
- * We limit virtual irqs to __irq_offet_value less than virt_irq_max so
- * that when we offset them we don't end up with an interrupt
- * number >= virt_irq_max.
- */
-#define MIN_VIRT_IRQ	3
-
-unsigned int virt_irq_max;
+static unsigned int min_virt_irq;
 static unsigned int max_virt_irq;
 static unsigned int nr_virt_irqs;
 
 void
-virt_irq_init(void)
+virt_irq_config(unsigned int offset, 
+		unsigned int num_shifted, unsigned int num_remapped)
 {
-	int i;
+	__irq_offset_value = offset;
+	min_virt_irq = num_shifted;
+	max_virt_irq = num_shifted + num_remapped - 1;
+	nr_virt_irqs = num_remapped;
 
-	if ((virt_irq_max == 0) || (virt_irq_max > (NR_IRQS - 1)))
-		virt_irq_max = NR_IRQS - 1;
-	max_virt_irq = virt_irq_max - __irq_offset_value;
-	nr_virt_irqs = max_virt_irq - MIN_VIRT_IRQ + 1;
+	BUG_ON(max_virt_irq + __irq_offset_value > (NR_IRQS-1));
+}
 
+void
+virt_irq_init(void)
+{
+	int i;
 	for (i = 0; i < NR_IRQS; i++)
 		virt_irq_to_real_map[i] = UNDEFINED_IRQ;
+
+	/* Configure the default mappings; don't remap interrupts, 
+	 * use 1-1 virt <-> real mappings. Subsequently platforms may
+	 * choose to call virt_irq_config with their own custom settings.
+	 */
+	virt_irq_config(0, NR_IRQS, 0);
+	
 }
 
 /* Create a mapping for a real_irq if it doesn't already exist.
@@ -304,30 +307,25 @@ int virt_irq_create_mapping(unsigned int
 	unsigned int virq, first_virq;
 	static int warned;
 
-	if (ppc64_interrupt_controller == IC_OPEN_PIC)
-		return real_irq;	/* no mapping for openpic (for now) */
-
-	if (ppc64_interrupt_controller == IC_CELL_PIC)
-		return real_irq;	/* no mapping for iic either */
-
-	/* don't map interrupts < MIN_VIRT_IRQ */
-	if (real_irq < MIN_VIRT_IRQ) {
+	/* don't map interrupts < min_virt_irq */
+	if (real_irq < min_virt_irq) {
 		virt_irq_to_real_map[real_irq] = real_irq;
 		return real_irq;
 	}
 
-	/* map to a number between MIN_VIRT_IRQ and max_virt_irq */
+	/* map to a number between min_virt_irq and max_virt_irq */
 	virq = real_irq;
 	if (virq > max_virt_irq)
-		virq = (virq % nr_virt_irqs) + MIN_VIRT_IRQ;
+		virq = (virq % nr_virt_irqs) + min_virt_irq;
 
 	/* search for this number or a free slot */
 	first_virq = virq;
 	while (virt_irq_to_real_map[virq] != UNDEFINED_IRQ) {
 		if (virt_irq_to_real_map[virq] == real_irq)
 			return virq;
+		
 		if (++virq > max_virt_irq)
-			virq = MIN_VIRT_IRQ;
+			virq = min_virt_irq;
 		if (virq == first_virq)
 			goto nospace;	/* oops, no free slots */
 	}
@@ -338,8 +336,10 @@ int virt_irq_create_mapping(unsigned int
  nospace:
 	if (!warned) {
 		printk(KERN_CRIT "Interrupt table is full\n");
-		printk(KERN_CRIT "Increase virt_irq_max (currently %d) "
-		       "in your kernel sources and rebuild.\n", virt_irq_max);
+		printk(KERN_CRIT "Modify kernel sources to call "
+		       "virt_irq_config() with a larger value "
+		       "of \"num_remapped\" (currently %d)  and rebuild.\n",
+		       nr_virt_irqs);
 		warned = 1;
 	}
 	return NO_IRQ;
@@ -358,7 +358,7 @@ unsigned int real_irq_to_virt_slowpath(u
 	virq = real_irq;
 
 	if (virq > max_virt_irq)
-		virq = (virq % nr_virt_irqs) + MIN_VIRT_IRQ;
+		virq = (virq % nr_virt_irqs) + min_virt_irq;
 
 	first_virq = virq;
 
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 9a07f97..bfbe6a7 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -472,10 +472,6 @@ void __init finish_device_tree(void)
 
 	DBG(" -> finish_device_tree\n");
 
-#ifdef CONFIG_PPC64
-	/* Initialize virtual IRQ map */
-	virt_irq_init();
-#endif
 	scan_interrupt_controllers();
 
 	/*
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 4467c49..4669179 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -389,6 +389,12 @@ void __init setup_system(void)
 	 */
 	check_for_initrd();
 
+
+	/* Configure default setting for virtual IRQ remapping.
+	 * ppc_md.init_early() may reconfigure this with virt_irq_config()
+	 */
+	virt_irq_init();
+
 	/*
 	 * Do some platform specific early initializations, that includes
 	 * setting up the hash table pointers. It also sets up some interrupt-mapping
diff --git a/arch/powerpc/platforms/iseries/setup.c b/arch/powerpc/platforms/iseries/setup.c
index a6fd9be..6a7d04b 100644
--- a/arch/powerpc/platforms/iseries/setup.c
+++ b/arch/powerpc/platforms/iseries/setup.c
@@ -688,8 +688,9 @@ static int __init iseries_probe(void)
 	/*
 	 * The Hypervisor only allows us up to 256 interrupt
 	 * sources (the irq number is passed in a u8).
+	 * This call provides us with vectors 0 .. 255 without remapping.
 	 */
-	virt_irq_max = 255;
+	virt_irq_config(0, 256, 0);
 
 	return 1;
 }
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 5f79f01..9d22265 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -270,7 +270,6 @@ static  void __init pSeries_discover_pic
 	 * Setup interrupt mapping options that are needed for finish_device_tree
 	 * to properly parse the OF interrupt tree & do the virtual irq mapping
 	 */
-	__irq_offset_value = NUM_ISA_INTERRUPTS;
 	ppc64_interrupt_controller = IC_INVALID;
 	for (np = NULL; (np = of_find_node_by_name(np, "interrupt-controller"));) {
 		typep = (char *)get_property(np, "compatible", NULL);
@@ -286,6 +285,15 @@ static  void __init pSeries_discover_pic
 		printk("pSeries_discover_pic: failed to recognize"
 			" interrupt-controller\n");
 
+	/*
+	 * Don't use virtual irqs 0, 1, 2 for devices.
+	 * The pcnet32 driver considers interrupt numbers < 2 to be invalid,
+	 * and 2 is the XICS IPI interrupt.
+	 */
+	
+	virt_irq_config(NUM_ISA_INTERRUPTS, 3,  
+			NR_IRQS - (3 + NUM_ISA_INTERRUPTS));
+
 }
 
 static void pSeries_mach_cpu_die(void)
diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h
index 7bc6d73..f2adbc0 100644
--- a/include/asm-powerpc/irq.h
+++ b/include/asm-powerpc/irq.h
@@ -54,18 +54,32 @@
  */
 extern unsigned int virt_irq_to_real_map[NR_IRQS];
 
-/* The maximum virtual IRQ number that we support.  This
- * can be set by the platform and will be reduced by the
- * value of __irq_offset_value.  It defaults to and is
- * capped by (NR_IRQS - 1).
- */
-extern unsigned int virt_irq_max;
-
 /* Create a mapping for a real_irq if it doesn't already exist.
  * Return the virtual irq as a convenience.
  */
 int virt_irq_create_mapping(unsigned int real_irq);
-void virt_irq_init(void);
+
+/* Initializes the virtual IRQ re-mapping, before ppc_md.init_early() */
+extern void virt_irq_init(void);
+
+/* virt_irq_config - Configure the division of the virtual IRQ space.
+ *		     Each platform may call this to set it's own specific 
+ *		     IRQ remapping parameters.
+ * @offset:	  offset of remapped space (i.e. NUM_ISA_INTERRUPTS).
+ * @num_shifted:  number of vectors to be simply shifted to account for
+ *		  the offset with no other remapping.
+ * @num_remapper: number of vectors that may be arbitrarily remapped.
+ *
+ * Virtual IRQ vectors will have the following mappings to the IRQ vectors
+ * as seen by the approrpiate interrupt controller:
+ * 0 .. offset-1		 -> 0 .. offset-1   (1-1 mapping)
+ * offset .. offset+num_shifted-1 -> 0 .. num_shifted-1 (shift by offset)
+ * offset+num_shifted .. offset+num_shifted+num_remapped-1 ->
+ *		arbitrary mappings to real irq as required.
+ */
+extern void virt_irq_config(unsigned int offset,
+			    unsigned int num_linear,
+			    unsigned int num_remapped);
 
 static inline unsigned int virt_irq_to_real(unsigned int virt_irq)
 {
-- 
1.1.4.g0b63-dirty

[PATCH 2/8] PIC discovery re-organization.

[mostrows]

Change pSeries_discover_pic() to simply return the PIC type, rather
than setting the global "ppc64_interrupt_controller" value.  They type of
controller present will be passed down as an argument to functions
that need it.

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/platforms/pseries/setup.c |   36 +++++++++++++++++---------------
 1 files changed, 19 insertions(+), 17 deletions(-)

3675f032e1b03d6f000ffb08b2487ee9aa44fa2d
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 9d22265..33ae521 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -83,6 +83,7 @@ int fwnmi_active;  /* TRUE if an FWNMI h
 
 static void pseries_shared_idle_sleep(void);
 static void pseries_dedicated_idle_sleep(void);
+static  int pSeries_discover_pic(void);
 
 struct mpic *pSeries_mpic;
 
@@ -195,8 +196,11 @@ static void pseries_lpar_enable_pmcs(voi
 
 static void __init pSeries_setup_arch(void)
 {
+	int int_ctrl = pSeries_discover_pic();
+	ppc64_interrupt_controller = int_ctrl;
+
 	/* Fixup ppc_md depending on the type of interrupt controller */
-	if (ppc64_interrupt_controller == IC_OPEN_PIC) {
+	if (int_ctrl == IC_OPEN_PIC) {
 		ppc_md.init_IRQ       = pSeries_init_mpic;
 		ppc_md.get_irq        = mpic_get_irq;
 		/* Allocate the mpic now, so that find_and_init_phbs() can
@@ -261,39 +265,30 @@ static int __init pSeries_init_panel(voi
 }
 arch_initcall(pSeries_init_panel);
 
-static  void __init pSeries_discover_pic(void)
+static  int __init pSeries_discover_pic(void)
 {
 	struct device_node *np;
 	char *typep;
-
 	/*
 	 * Setup interrupt mapping options that are needed for finish_device_tree
 	 * to properly parse the OF interrupt tree & do the virtual irq mapping
 	 */
-	ppc64_interrupt_controller = IC_INVALID;
+	int int_ctrl = IC_INVALID;
 	for (np = NULL; (np = of_find_node_by_name(np, "interrupt-controller"));) {
 		typep = (char *)get_property(np, "compatible", NULL);
 		if (strstr(typep, "open-pic")) {
-			ppc64_interrupt_controller = IC_OPEN_PIC;
+			int_ctrl = IC_OPEN_PIC;
 			break;
 		} else if (strstr(typep, "ppc-xicp")) {
-			ppc64_interrupt_controller = IC_PPC_XIC;
+			int_ctrl = IC_PPC_XIC;
 			break;
 		}
 	}
-	if (ppc64_interrupt_controller == IC_INVALID)
+	if (int_ctrl == IC_INVALID)
 		printk("pSeries_discover_pic: failed to recognize"
 			" interrupt-controller\n");
 
-	/*
-	 * Don't use virtual irqs 0, 1, 2 for devices.
-	 * The pcnet32 driver considers interrupt numbers < 2 to be invalid,
-	 * and 2 is the XICS IPI interrupt.
-	 */
-	
-	virt_irq_config(NUM_ISA_INTERRUPTS, 3,  
-			NR_IRQS - (3 + NUM_ISA_INTERRUPTS));
-
+	return int_ctrl;
 }
 
 static void pSeries_mach_cpu_die(void)
@@ -346,7 +341,14 @@ static void __init pSeries_init_early(vo
 
 	iommu_init_early_pSeries();
 
-	pSeries_discover_pic();
+	/*
+	 * Don't use virtual irqs 0, 1, 2 for devices.
+	 * The pcnet32 driver considers interrupt numbers < 2 to be invalid,
+	 * and 2 is the XICS IPI interrupt.
+	 */
+	
+	virt_irq_config(NUM_ISA_INTERRUPTS, 3,  
+			NR_IRQS - (3 + NUM_ISA_INTERRUPTS));
 
 	DBG(" <- pSeries_init_early()\n");
 }
-- 
1.1.4.g0b63-dirty

[PATCH 3/8] Avoid use of ppc64_interrupt_controller.

[mostrows]

xics_irq_8259_cascade being a valid (non -1) value is sufficient criteria
for registering the cascade interrupt vector.

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/platforms/pseries/xics.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

35c3db8657f7716ae93d7423ce49f32a5f008359
diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index 2d60ea3..0fae097 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -82,8 +82,8 @@ struct xics_ipl {
 
 static struct xics_ipl __iomem *xics_per_cpu[NR_CPUS];
 
-static int xics_irq_8259_cascade = 0;
-static int xics_irq_8259_cascade_real = 0;
+static int xics_irq_8259_cascade = -1; /* default to invalid value*/
+static int xics_irq_8259_cascade_real = -1;
 static unsigned int default_server = 0xFF;
 static unsigned int default_distrib_server = 0;
 static unsigned int interrupt_server_size = 8;
@@ -571,8 +571,7 @@ nextnode:
  */
 static int __init xics_setup_i8259(void)
 {
-	if (ppc64_interrupt_controller == IC_PPC_XIC &&
-	    xics_irq_8259_cascade != -1) {
+	if (xics_irq_8259_cascade != -1) {
 		if (request_irq(irq_offset_up(xics_irq_8259_cascade),
 				no_action, 0, "8259 cascade", NULL))
 			printk(KERN_ERR "xics_setup_i8259: couldn't get 8259 "
-- 
1.1.4.g0b63-dirty

[PATCH 4/8] Avoid use of ppc64_interrupt_controller.

[mostrows]

Existence of "/platform-open-pic" is asserted by pSeries_setup_mpic if
an mpic interrupt controller exists.  Thus pSeries_mpic is non-NULL
only if the opprop value obtained here is good. Hence, no need to
check ppc64_intrerrupt_controller.

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/kernel/rtas_pci.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

3a5eaf5436e0c413e154a933867ff5a09c019e3c
diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index 57b539a..e1dbd53 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c
@@ -301,10 +301,8 @@ unsigned long __init find_and_init_phbs(
 	unsigned int *opprop = NULL;
 	struct device_node *root = of_find_node_by_path("/");
 
-	if (ppc64_interrupt_controller == IC_OPEN_PIC) {
-		opprop = (unsigned int *)get_property(root,
-				"platform-open-pic", NULL);
-	}
+	/* If pSeries_mpic is non-NULL, it's because opprop is non-0. */
+	opprop = (unsigned int *)get_property(root, "platform-open-pic", NULL);
 
 	root_size_cells = prom_n_size_cells(root);
 
@@ -324,7 +322,7 @@ unsigned long __init find_and_init_phbs(
 		pci_setup_phb_io(phb, index == 0);
 #ifdef CONFIG_PPC_PSERIES
 		/* XXX This code need serious fixing ... --BenH */
-		if (ppc64_interrupt_controller == IC_OPEN_PIC && pSeries_mpic) {
+		if (pSeries_mpic) {
 			int addr = root_size_cells * (index + 2) - 1;
 			mpic_assign_isu(pSeries_mpic, index, opprop[addr]);
 		}
-- 
1.1.4.g0b63-dirty

[PATCH 6/8] Avoid use of ppc64_interrupt_controller.

[mostrows]

Checking the pSeries_mpic pointer will tell us if we have an MPIC
(and if not we assume XICS).

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/platforms/pseries/setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

b8d30cee0f0257489bb79c3d36f0afc39b8e63e6
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 38bf976..4ff127b 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -536,7 +536,7 @@ static void pseries_kexec_cpu_down(int c
 		}
 	}
 
-	if (ppc64_interrupt_controller == IC_OPEN_PIC)
+	if (pSeries_mpic)
 		mpic_teardown_this_cpu(secondary);
 	else
 		xics_teardown_cpu(secondary);
-- 
1.1.4.g0b63-dirty

[PATCH 5/8] Avoid use of ppc64_interrupt_controller.

[mostrows]

smp_init_pSeries() will use an XICS, unless it is told to explicitly
use an MPIC.

Checking for built-in support of the detect PIC is now consolidated in
pSeries_setup_arch.  (Eventually it should be possible to avoid building
code for a PIC if support for it has not been enabled.)

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/platforms/pseries/setup.c |   28 +++++++++++++++++++++-------
 arch/powerpc/platforms/pseries/smp.c   |   15 +++------------
 include/asm-powerpc/smp.h              |    6 +++++-
 3 files changed, 29 insertions(+), 20 deletions(-)

c294c06d77fbe3be12a705595ecd95f93054943e
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 33ae521..38bf976 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -153,6 +153,9 @@ static void __init pSeries_setup_mpic(vo
         struct device_node *root;
 	int irq_count;
 
+	ppc_md.init_IRQ       = pSeries_init_mpic;
+	ppc_md.get_irq        = mpic_get_irq;
+
 	/* Find the Open PIC if present */
 	root = of_find_node_by_path("/");
 	opprop = (unsigned int *) get_property(root, "platform-open-pic", NULL);
@@ -199,21 +202,32 @@ static void __init pSeries_setup_arch(vo
 	int int_ctrl = pSeries_discover_pic();
 	ppc64_interrupt_controller = int_ctrl;
 
+	
 	/* Fixup ppc_md depending on the type of interrupt controller */
-	if (int_ctrl == IC_OPEN_PIC) {
-		ppc_md.init_IRQ       = pSeries_init_mpic;
-		ppc_md.get_irq        = mpic_get_irq;
+	switch (int_ctrl) {
+	case IC_OPEN_PIC:
+#ifndef CONFIG_MPIC
+		panic("Kernel not configured for MPIC interrupt controller.");
+#else
 		/* Allocate the mpic now, so that find_and_init_phbs() can
 		 * fill the ISUs */
 		pSeries_setup_mpic();
-	} else {
+		smp_init_pSeries(1);
+#endif
+		break;
+	case IC_PPC_XIC:
+#ifndef CONFIG_XICS
+		panic("Kernel not configured for XICS interrupt controller.");
+#else
 		ppc_md.init_IRQ       = xics_init_IRQ;
 		ppc_md.get_irq        = xics_get_irq;
+		smp_init_pSeries(0);
+#endif
+		break;
+	default:
+		panic("Invalid interrupt controller");
 	}
 
-#ifdef CONFIG_SMP
-	smp_init_pSeries();
-#endif
 	/* openpic global configuration register (64-bit format). */
 	/* openpic Interrupt Source Unit pointer (64-bit format). */
 	/* python0 facility area (mmio) (64-bit format) REAL address. */
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index 3cf78a6..ef8676f 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -417,25 +417,16 @@ static struct smp_ops_t pSeries_xics_smp
 #endif
 
 /* This is called very early */
-void __init smp_init_pSeries(void)
+void __init smp_init_pSeries(int uses_mpic)
 {
 	int i;
 
 	DBG(" -> smp_init_pSeries()\n");
 
-	switch (ppc64_interrupt_controller) {
-#ifdef CONFIG_MPIC
-	case IC_OPEN_PIC:
+	if (uses_mpic) {
 		smp_ops = &pSeries_mpic_smp_ops;
-		break;
-#endif
-#ifdef CONFIG_XICS
-	case IC_PPC_XIC:
+	} else {
 		smp_ops = &pSeries_xics_smp_ops;
-		break;
-#endif
-	default:
-		panic("Invalid interrupt controller");
 	}
 
 #ifdef CONFIG_HOTPLUG_CPU
diff --git a/include/asm-powerpc/smp.h b/include/asm-powerpc/smp.h
index 4a716f7..11f1830 100644
--- a/include/asm-powerpc/smp.h
+++ b/include/asm-powerpc/smp.h
@@ -75,7 +75,7 @@ extern cpumask_t cpu_sibling_map[NR_CPUS
 #define PPC_MSG_DEBUGGER_BREAK  3
 
 void smp_init_iSeries(void);
-void smp_init_pSeries(void);
+void smp_init_pSeries(int uses_mpic);
 void smp_init_cell(void);
 void smp_setup_cpu_maps(void);
 
@@ -86,6 +86,10 @@ extern void __cpu_die(unsigned int cpu);
 /* for UP */
 #define smp_setup_cpu_maps()
 
+#define  smp_init_pSeries(uses_mpic) do {} while(0)
+#define  smp_init_iSeries() do {} while(0)
+#define  smp_init_cell() do {} while(0)
+
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_PPC64
-- 
1.1.4.g0b63-dirty

[PATCH 7/8] Cleaner checks for MPIC on pSeries.

[mostrows]

Instead of checking the pSeries_mpic pointer, use a wrapper
(pSeries_uses_mpic()) that tells us what we want to know.

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/kernel/rtas_pci.c         |    2 +-
 arch/powerpc/platforms/pseries/setup.c |    2 +-
 include/asm-powerpc/mpic.h             |    6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

082a93566e954ce0cc945b89aced0b28feb7ee60
diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index e1dbd53..2a6b729 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c
@@ -322,7 +322,7 @@ unsigned long __init find_and_init_phbs(
 		pci_setup_phb_io(phb, index == 0);
 #ifdef CONFIG_PPC_PSERIES
 		/* XXX This code need serious fixing ... --BenH */
-		if (pSeries_mpic) {
+		if (pSeries_uses_mpic()) {
 			int addr = root_size_cells * (index + 2) - 1;
 			mpic_assign_isu(pSeries_mpic, index, opprop[addr]);
 		}
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 4ff127b..551da6a 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -536,7 +536,7 @@ static void pseries_kexec_cpu_down(int c
 		}
 	}
 
-	if (pSeries_mpic)
+	if (pSeries_uses_mpic())
 		mpic_teardown_this_cpu(secondary);
 	else
 		xics_teardown_cpu(secondary);
diff --git a/include/asm-powerpc/mpic.h b/include/asm-powerpc/mpic.h
index 6b9e781..1f1b2eb 100644
--- a/include/asm-powerpc/mpic.h
+++ b/include/asm-powerpc/mpic.h
@@ -287,5 +287,11 @@ extern int mpic_get_irq(struct pt_regs *
 /* global mpic for pSeries */
 extern struct mpic *pSeries_mpic;
 
+#ifdef CONFIG_MPIC
+#define pSeries_uses_mpic() (pSeries_mpic != NULL)
+#else
+#define pSeries_uses_mpic() 0
+#endif
+
 #endif /* __KERNEL__ */
 #endif	/* _ASM_POWERPC_MPIC_H */
-- 
1.1.4.g0b63-dirty

[PATCH 8/8] Kill the ppc64_interrupt_controller global symbol.

[mostrows]

--
Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>

---

 arch/powerpc/kernel/irq.c               |    1 -
 arch/powerpc/kernel/setup_64.c          |    2 --
 arch/powerpc/platforms/cell/setup.c     |    2 --
 arch/powerpc/platforms/iseries/setup.c  |    2 --
 arch/powerpc/platforms/maple/setup.c    |    3 ---
 arch/powerpc/platforms/powermac/setup.c |    3 ---
 arch/powerpc/platforms/pseries/setup.c  |   15 +++++++++++----
 include/asm-powerpc/irq.h               |   11 -----------
 8 files changed, 11 insertions(+), 28 deletions(-)

5a0403834813838657ce9a3b762203f6479f36fa
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index efcb859..bee60d5 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -89,7 +89,6 @@ extern atomic_t ipi_sent;
 EXPORT_SYMBOL(irq_desc);
 
 int distribute_irqs = 1;
-u64 ppc64_interrupt_controller;
 #endif /* CONFIG_PPC64 */
 
 int show_interrupts(struct seq_file *p, void *v)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 4669179..f791bcc 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -445,8 +445,6 @@ void __init setup_system(void)
 
 	printk("-----------------------------------------------------\n");
 	printk("ppc64_pft_size                = 0x%lx\n", ppc64_pft_size);
-	printk("ppc64_interrupt_controller    = 0x%ld\n",
-	       ppc64_interrupt_controller);
 	printk("physicalMemorySize            = 0x%lx\n", lmb_phys_mem_size());
 	printk("ppc64_caches.dcache_line_size = 0x%x\n",
 	       ppc64_caches.dline_size);
diff --git a/arch/powerpc/platforms/cell/setup.c b/arch/powerpc/platforms/cell/setup.c
index 6574b22..e1cf54e 100644
--- a/arch/powerpc/platforms/cell/setup.c
+++ b/arch/powerpc/platforms/cell/setup.c
@@ -117,8 +117,6 @@ static void __init cell_init_early(void)
 
 	cell_init_iommu();
 
-	ppc64_interrupt_controller = IC_CELL_PIC;
-
 	DBG(" <- cell_init_early()\n");
 }
 
diff --git a/arch/powerpc/platforms/iseries/setup.c b/arch/powerpc/platforms/iseries/setup.c
index 6a7d04b..ca623c5 100644
--- a/arch/powerpc/platforms/iseries/setup.c
+++ b/arch/powerpc/platforms/iseries/setup.c
@@ -303,8 +303,6 @@ static void __init iSeries_init_early(vo
 {
 	DBG(" -> iSeries_init_early()\n");
 
-	ppc64_interrupt_controller = IC_ISERIES;
-
 #if defined(CONFIG_BLK_DEV_INITRD)
 	/*
 	 * If the init RAM disk has been configured and there is
diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index 24c0aef..592972c 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -204,9 +204,6 @@ static void __init maple_init_early(void
 	 */
 	hpte_init_native();
 
-	/* Setup interrupt mapping options */
-	ppc64_interrupt_controller = IC_OPEN_PIC;
-
 	iommu_init_early_dart();
 
 	DBG(" <- maple_init_early\n");
diff --git a/arch/powerpc/platforms/powermac/setup.c b/arch/powerpc/platforms/powermac/setup.c
index 4d15e39..2b2dc75 100644
--- a/arch/powerpc/platforms/powermac/setup.c
+++ b/arch/powerpc/platforms/powermac/setup.c
@@ -609,9 +609,6 @@ static void __init pmac_init_early(void)
 	udbg_adb_init(!!strstr(cmd_line, "btextdbg"));
 
 #ifdef CONFIG_PPC64
-	/* Setup interrupt mapping options */
-	ppc64_interrupt_controller = IC_OPEN_PIC;
-
 	iommu_init_early_dart();
 #endif
 }
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 551da6a..a85b810 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -83,7 +83,15 @@ int fwnmi_active;  /* TRUE if an FWNMI h
 
 static void pseries_shared_idle_sleep(void);
 static void pseries_dedicated_idle_sleep(void);
-static  int pSeries_discover_pic(void);
+
+/*
+ * List of interrupt controller identifiers; possible return values from
+ * pSeries_discover_pic().
+ */
+#define IC_INVALID    0
+#define IC_OPEN_PIC   1
+#define IC_PPC_XIC    2
+static int pSeries_discover_pic(void);
 
 struct mpic *pSeries_mpic;
 
@@ -200,9 +208,7 @@ static void pseries_lpar_enable_pmcs(voi
 static void __init pSeries_setup_arch(void)
 {
 	int int_ctrl = pSeries_discover_pic();
-	ppc64_interrupt_controller = int_ctrl;
 
-	
 	/* Fixup ppc_md depending on the type of interrupt controller */
 	switch (int_ctrl) {
 	case IC_OPEN_PIC:
@@ -279,7 +285,8 @@ static int __init pSeries_init_panel(voi
 }
 arch_initcall(pSeries_init_panel);
 
-static  int __init pSeries_discover_pic(void)
+
+static int __init pSeries_discover_pic(void)
 {
 	struct device_node *np;
 	char *typep;
diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h
index f2adbc0..66cfeab 100644
--- a/include/asm-powerpc/irq.h
+++ b/include/asm-powerpc/irq.h
@@ -88,17 +88,6 @@ static inline unsigned int virt_irq_to_r
 
 extern unsigned int real_irq_to_virt_slowpath(unsigned int real_irq);
 
-/*
- * List of interrupt controllers.
- */
-#define IC_INVALID    0
-#define IC_OPEN_PIC   1
-#define IC_PPC_XIC    2
-#define IC_CELL_PIC   3
-#define IC_ISERIES    4
-
-extern u64 ppc64_interrupt_controller;
-
 #else /* 32-bit */
 
 #if defined(CONFIG_40x)
-- 
1.1.4.g0b63-dirty

Re: [PATCH 7/8] Cleaner checks for MPIC on pSeries.

[Olof Johansson]

On Mon, May 29, 2006 at 04:42:06PM -0400, mostrows@watson.ibm.com wrote:
> Instead of checking the pSeries_mpic pointer, use a wrapper
> (pSeries_uses_mpic()) that tells us what we want to know.

You might as well kill the SillyCaps while you're at it.

-Olof


> --
> Signed-off-by: Michal Ostrowski <mostrows@watson.ibm.com>
> 
> ---
> 
>  arch/powerpc/kernel/rtas_pci.c         |    2 +-
>  arch/powerpc/platforms/pseries/setup.c |    2 +-
>  include/asm-powerpc/mpic.h             |    6 ++++++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> 082a93566e954ce0cc945b89aced0b28feb7ee60
> diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
> index e1dbd53..2a6b729 100644
> --- a/arch/powerpc/kernel/rtas_pci.c
> +++ b/arch/powerpc/kernel/rtas_pci.c
> @@ -322,7 +322,7 @@ unsigned long __init find_and_init_phbs(
>  		pci_setup_phb_io(phb, index == 0);
>  #ifdef CONFIG_PPC_PSERIES
>  		/* XXX This code need serious fixing ... --BenH */
> -		if (pSeries_mpic) {
> +		if (pSeries_uses_mpic()) {
>  			int addr = root_size_cells * (index + 2) - 1;
>  			mpic_assign_isu(pSeries_mpic, index, opprop[addr]);
>  		}
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 4ff127b..551da6a 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -536,7 +536,7 @@ static void pseries_kexec_cpu_down(int c
>  		}
>  	}
>  
> -	if (pSeries_mpic)
> +	if (pSeries_uses_mpic())
>  		mpic_teardown_this_cpu(secondary);
>  	else
>  		xics_teardown_cpu(secondary);
> diff --git a/include/asm-powerpc/mpic.h b/include/asm-powerpc/mpic.h
> index 6b9e781..1f1b2eb 100644
> --- a/include/asm-powerpc/mpic.h
> +++ b/include/asm-powerpc/mpic.h
> @@ -287,5 +287,11 @@ extern int mpic_get_irq(struct pt_regs *
>  /* global mpic for pSeries */
>  extern struct mpic *pSeries_mpic;
>  
> +#ifdef CONFIG_MPIC
> +#define pSeries_uses_mpic() (pSeries_mpic != NULL)
> +#else
> +#define pSeries_uses_mpic() 0
> +#endif
> +
>  #endif /* __KERNEL__ */
>  #endif	/* _ASM_POWERPC_MPIC_H */
> -- 
> 1.1.4.g0b63-dirty
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Benjamin Herrenschmidt]

On Mon, 2006-05-29 at 16:41 -0400, Michal Ostrowski wrote:
> The following set of patches is aimed at killing the
> ppc64_interrupt_controller global variable, and similarly that various
> global variables used to tweak the parameters of virtual IRQ re-mapping.
> Instead each platform can use a single function to configure IRQ
> re-mapping in "init_early()".

 .../...

Hi ! Interesting.. I've start looking at reworking that with a slightly
different approach though...

We need irq controller instances (which we get from the genirq patch
from Ingo & Thomas Gleixner), and we decided that interrupt controller
nodes are mandatory in the device-tree thus we can do something like:

- Each interrupt controller instance, when allocated, requests a certain
number of interrupts and gets that allocated (gets an offset allocated).
It can locally use the virtual irq remapping too, I'm still toying with
the best way to deal with that. This interrupt controller structure is
associated to the device-node of the controller (or the root node of the
tree if no controller node is found)

 - Interrupt 0..15 are reserved. 0 is always invalid. Only ISA PICs that
carry legacy devices can request those (by passing a special flag to the
allocation routine). Any other gets remapped (including powermac MPICs).
That will avoid endless problems that we never properly solved with
legacy drivers and the fact that Linus decided that 0 should be the
invalid interrupt number on all platforms

 - Provide in prom_parse.c functions for obtaining the PIC node and
local interrupt number of a given device based on a passed-in array
matching the semantics of an "interrupts" property and a parent node.
Along with a helper that may just take a child node. The former is
needed for PCI devices that have no device node. Provide a
pci_ppc_map_interrupt() that takes a pci_dev and does the interrupt
mapping, either by using the standard OF approach if a device-node is
present, or walking up the PCI tree while doing standard swizzling until
it reaches a device node

 - Mapping from a virtual number to a local number can be done by a
helper that does the offset and possible virq mapping if requested by
the controller. Same goes with inverse mapping. Mapping is done locally
by the controller code using that helper.

Ben.

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Paul Mackerras]

Michal Ostrowski writes:

> Platforms can choose the default configuration (which is to
> say identity mappings of IRQ vectors 0..511) or specify their own
> configuration:  an offset (for ISA vectors), the range that is to be

Given that the decree has come down from on high that irq 0 is always
to mean "none" or "no IRQ", I have been thinking that we should use
the ISA offset everywhere, even on machines without any ISA hardware.

Paul.

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Michal Ostrowski]

On Tue, 2006-05-30 at 07:28 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2006-05-29 at 16:41 -0400, Michal Ostrowski wrote:
> > The following set of patches is aimed at killing the
> > ppc64_interrupt_controller global variable, and similarly that various
> > global variables used to tweak the parameters of virtual IRQ re-mapping.
> > Instead each platform can use a single function to configure IRQ
> > re-mapping in "init_early()".
> 
>  .../...
> 
> Hi ! Interesting.. I've start looking at reworking that with a slightly
> different approach though...
> 

I'm in total agreement with your assessment of where things should go.

My main motivation here was to kill ppc64_interrupt_controller and to
consolidate our knowledge of the requirements of the different platforms
into a single interface (i.e. virt_irq_config). 

As such I see this patch as either a first step, or clean-up along-side,
the changes you're considering.

I did consider at first making more changes than published here; in
particular to address the offsetting issue you and Paul have commented
on.  

Current code such as:

        virq = virt_irq_create_mapping(irq[0]);

        ...

        np->intrs[intrcount].line = irq_offset_up(virq);


Should be more like:

        np->intrs[intrcount].line = virt_irq_create_mapping(irq[0]);


(Where the knowledge regarding all remapping/offsetting is embedded
behind the remapping interfaces.  We should kill irq_offset_{up,down}
and replace them with things such as real_irq_to_virt() and
virt_irq_to_real().)


The patches I've sent out were not intended to deal with this, as I
wanted to keep the change set focused and small. However, things similar
to what you've outlined I've been keeping in mind as potential follow-on
work; and I had intended these patches to facilitate that.


-- 
Michal Ostrowski <mostrows@watson.ibm.com>

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Michal Ostrowski]

On Tue, 2006-05-30 at 07:28 +1000, Benjamin Herrenschmidt wrote:
> Hi ! Interesting.. I've start looking at reworking that with a slightly
> different approach though...
> 
> We need irq controller instances (which we get from the genirq patch
> from Ingo & Thomas Gleixner), and we decided that interrupt controller
> nodes are mandatory in the device-tree thus we can do something like:
> 

The genirq patches were not available until this morning, but now that
I've got them here are some more thought as to what can be done.


> - Each interrupt controller instance, when allocated, requests a certain
> number of interrupts and gets that allocated (gets an offset allocated).
> It can locally use the virtual irq remapping too, I'm still toying with
> the best way to deal with that. This interrupt controller structure is
> associated to the device-node of the controller (or the root node of the
> tree if no controller node is found)

A PIC would not need to reserve anything is when it is allocated.  Only
when interrupt numbers need to be presented to generic kernel code is a
virq number required.

One can use irq_desc->chip_data to easily go from virq -> (PIC, line).
The PIC then maintains an array to map each of it's lines to virq.  
This allows for all re-mappings to always be arbitrary in nature and
still allows for O(1) look-up in either direction.

> 
>  - Interrupt 0..15 are reserved. 0 is always invalid. Only ISA PICs that
> carry legacy devices can request those (by passing a special flag to the
> allocation routine). 

Always create an ISA PIC that immediately requests lines 0..15.
Should one actually exists, we can associate the ISA PIC with the
appropriate device node.  Should ISA devices exist, once they request
interrupts (using (PIC,line) as arguments) we'll short-circuit virq
allocation since all ISA PIC mappings already exists.

Then there's no need to special case anything (and all other interrupts
are forced to be remapped out of the 0..15 range, without an explicit
"offset" concept).


> Any other gets remapped (including powermac MPICs).
> That will avoid endless problems that we never properly solved with
> legacy drivers and the fact that Linus decided that 0 should be the
> invalid interrupt number on all platforms
> 
>  - Provide in prom_parse.c functions for obtaining the PIC node and
> local interrupt number of a given device based on a passed-in array
> matching the semantics of an "interrupts" property and a parent node.
> Along with a helper that may just take a child node. The former is
> needed for PCI devices that have no device node. Provide a
> pci_ppc_map_interrupt() that takes a pci_dev and does the interrupt
> mapping, either by using the standard OF approach if a device-node is
> present, or walking up the PCI tree while doing standard swizzling until
> it reaches a device node

How is this different from the current use of map_interrupt() in
finish_node_interrupts()?  

It seems to me that it would be better to have the struct device_node
store the raw interrupt vector data as presented in the dev tree
(without remapping) along with a pointer to the struct device_node for
the appropriate PIC.

Later on, when we need to provide interrupt lines to the PCI device
structures (e.g. pci_read_irq_line()) we have the PIC and the raw
interrupt vectors identified and we ask the PIC to provide us with a
kernel/virt IRQ.

Deferring the remapping of the interrupt vectors to this later time
should allow for some simplification opportunities. All code that
handles device nodes would not need to deal with offsets or remapping,
only when IRQ information crosses the boundary between powerpc and
generic code would one need to be aware of the need to re-map (i.e.
dev->irq = ??? and ppc_md.get_irq(regs);/ __do_IRQ() interactions ).  
Since arbitrary re-mappings need to be supported, the reservation of
vectors 0..15 can be hidden as a re-mapping implementation detail.
Consequently one could eliminate irq_offset_up() and irq_offset_down()
altogether.


-- 
Michal Ostrowski <mostrows@watson.ibm.com>

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Benjamin Herrenschmidt]

> A PIC would not need to reserve anything is when it is allocated.  Only
> when interrupt numbers need to be presented to generic kernel code is a
> virq number required.

We could have completely sparse allocation indeed and any virt number
matching any PIC but I don't like that too much. Complicates things
unnecessarily don't you think ? I like irq numbers to be somewhat stable
on a given platform :) Helps debugging & diagnosing problems...

> One can use irq_desc->chip_data to easily go from virq -> (PIC, line).
> The PIC then maintains an array to map each of it's lines to virq.  
> This allows for all re-mappings to always be arbitrary in nature and
> still allows for O(1) look-up in either direction.

I'll think about it, but I'm really tempted to keep simple ranges for
now... we'll see.

> >  - Interrupt 0..15 are reserved. 0 is always invalid. Only ISA PICs that
> > carry legacy devices can request those (by passing a special flag to the
> > allocation routine). 
> 
> Always create an ISA PIC that immediately requests lines 0..15.

Well, if we use your suggested "sparse" allocation method, then yes, we
need to request them right away before anything else. But I'd like to
start with the range allocation in which case simply reserving that
range for use by whatever PIC says "I'm the legacy ISA PIC" is enough.

> Should one actually exists, we can associate the ISA PIC with the
> appropriate device node.  Should ISA devices exist, once they request
> interrupts (using (PIC,line) as arguments) we'll short-circuit virq
> allocation since all ISA PIC mappings already exists.

I'm not sure I understand there. ISA devices don't "request interrupts
using (PCI,line)" or whatever... we don't necessarily know in advance.
Part of the reasoning here is to also make sure that if no ISA PIC
allocated 0...15 then a stupid legacy driver loaded by the user will
fail it's call to request_irq()

> Then there's no need to special case anything (and all other interrupts
> are forced to be remapped out of the 0..15 range, without an explicit
> "offset" concept).

/me is a bit dubious...

> > Any other gets remapped (including powermac MPICs).
> > That will avoid endless problems that we never properly solved with
> > legacy drivers and the fact that Linus decided that 0 should be the
> > invalid interrupt number on all platforms
> > 
> >  - Provide in prom_parse.c functions for obtaining the PIC node and
> > local interrupt number of a given device based on a passed-in array
> > matching the semantics of an "interrupts" property and a parent node.
> > Along with a helper that may just take a child node. The former is
> > needed for PCI devices that have no device node. Provide a
> > pci_ppc_map_interrupt() that takes a pci_dev and does the interrupt
> > mapping, either by using the standard OF approach if a device-node is
> > present, or walking up the PCI tree while doing standard swizzling until
> > it reaches a device node
> 
> How is this different from the current use of map_interrupt() in
> finish_node_interrupts()?  

Slightly... basically cleaned up version of it.

> It seems to me that it would be better to have the struct device_node
> store the raw interrupt vector data as presented in the dev tree
> (without remapping) along with a pointer to the struct device_node for
> the appropriate PIC.

I don't understand what you have in mind. Remember we are working with
cases where devices may not have a node. There is no such thing as "an
interrupt == a node" anyway. Beside, I want to _remove_ anything in
struct device_node that isn't specifically the node linkage and property
list. All the pre-parsed junk has to go.

> Later on, when we need to provide interrupt lines to the PCI device
> structures (e.g. pci_read_irq_line()) we have the PIC and the raw
> interrupt vectors identified and we ask the PIC to provide us with a
> kernel/virt IRQ.

Yah, well, in order to have the PIC and the raw IRQ identified, we have
to do the algorithm I described :) Not sure how your scheme differs
except maybe by putting things in the device node itself... We do have a
void * there though we can use for non-PCI devices, thus if the PIC node
is always guaranteed _not_ to be a PCI device, we can use that to get
the PIC quickly but old MPICs are PCI devices afaik, and beside, that is
not a performance critical path.

> Deferring the remapping of the interrupt vectors to this later time
> should allow for some simplification opportunities. All code that
> handles device nodes would not need to deal with offsets or remapping,

I still don't see what you mean here.... the only things that has to
deal with offset and/or remapping are the PIC code itself when it gets
called with virtual numbers and the binding of a device to an irq when
going from the raw number to the virtual number.

> only when IRQ information crosses the boundary between powerpc and
> generic code would one need to be aware of the need to re-map (i.e.
> dev->irq = ??? and ppc_md.get_irq(regs);/ __do_IRQ() interactions ).  
> Since arbitrary re-mappings need to be supported, the reservation of
> vectors 0..15 can be hidden as a re-mapping implementation detail.
> Consequently one could eliminate irq_offset_up() and irq_offset_down()
> altogether.

I'm not sure how your scheme differs from what I have in mind at this
point except from the fact that you'll shuffle interrupt numbers way
more than I intend to. I suppose it _might_ be simpler to go through the
virt irq remapper once rather than having both a set of ranges +
eventually the remapper, and I've thought about using the remapper for
everything too, but I'd still like to keep the concept of ranges, thus
I'm tempted to still allocate all irqs for a given controller
continuously in the remapper when instanciating the PIC rather than when
actually looking for IRQs....

Ben.

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Michal Ostrowski]

On Wed, 2006-05-31 at 08:13 +1000, Benjamin Herrenschmidt wrote:
> > A PIC would not need to reserve anything is when it is allocated.  Only
> > when interrupt numbers need to be presented to generic kernel code is a
> > virq number required.
> 
> We could have completely sparse allocation indeed and any virt number
> matching any PIC but I don't like that too much. Complicates things
> unnecessarily don't you think ? I like irq numbers to be somewhat stable
> on a given platform :) Helps debugging & diagnosing problems...
> 

The fact that the allocation may be sparse does not mean it actually
will be.  As long as code expects arbitrary remappings it will have a
degree of robustness; but that does not mean that the mechanism by which
remappings are done cannot instantiate simple mappings.  In other words,
I'd like to see the concept of "irq offsets" being banished, but having
re-mapping code be smart enough so that, for example, an MPIC that must
deal with interrupt vectors 0..127 has these vectors made visible to the
system at 16..143.

I think that the current virt irq remapping algorithm would provide you
with exactly this behavior.


> > One can use irq_desc->chip_data to easily go from virq -> (PIC, line).
> > The PIC then maintains an array to map each of it's lines to virq.  
> > This allows for all re-mappings to always be arbitrary in nature and
> > still allows for O(1) look-up in either direction.
> 
> I'll think about it, but I'm really tempted to keep simple ranges for
> now... we'll see.
> 
> > >  - Interrupt 0..15 are reserved. 0 is always invalid. Only ISA PICs that
> > > carry legacy devices can request those (by passing a special flag to the
> > > allocation routine). 
> > 


> > > Any other gets remapped (including powermac MPICs).
> > > That will avoid endless problems that we never properly solved with
> > > legacy drivers and the fact that Linus decided that 0 should be the
> > > invalid interrupt number on all platforms
> > > 
> > >  - Provide in prom_parse.c functions for obtaining the PIC node and
> > > local interrupt number of a given device based on a passed-in array
> > > matching the semantics of an "interrupts" property and a parent node.
> > > Along with a helper that may just take a child node. The former is
> > > needed for PCI devices that have no device node. Provide a
> > > pci_ppc_map_interrupt() that takes a pci_dev and does the interrupt
> > > mapping, either by using the standard OF approach if a device-node is
> > > present, or walking up the PCI tree while doing standard swizzling until
> > > it reaches a device node
> > 
> > How is this different from the current use of map_interrupt() in
> > finish_node_interrupts()?  
> 
> Slightly... basically cleaned up version of it.
> 
> > It seems to me that it would be better to have the struct device_node
> > store the raw interrupt vector data as presented in the dev tree
> > (without remapping) along with a pointer to the struct device_node for
> > the appropriate PIC.
> 
> I don't understand what you have in mind. Remember we are working with
> cases where devices may not have a node. There is no such thing as "an
> interrupt == a node" anyway. Beside, I want to _remove_ anything in
> struct device_node that isn't specifically the node linkage and property
> list. All the pre-parsed junk has to go.
> 

In cases where we have a struct device_node, the struct device_node
should have a pointer to the PIC (or the PIC's device_node), and the
raw, unmodified interrupt line values.   When one wants to present an
irq to generic code, we use the (PIC,line) information from the
device_node to instantiate a virq.

If there is no struct device_node, you still have to come up with a
(PIC,line) pair in order to get a virq.

virt_irq_create_mapping() (or whatever replaces it) should take the
physical line and a PIC identifier/pointer as arguments so that it can
inform the PIC about a mapping it has instantiated (in case we really
have to make an arbitrary re-mapping) and so that it can try to
instantiate an identity or offset-only translation if possible.  In the
latter case, the PIC object will tell the remapper what offset range to
use.


> I'm not sure how your scheme differs from what I have in mind at this
> point except from the fact that you'll shuffle interrupt numbers way
> more than I intend to. I suppose it _might_ be simpler to go through the
> virt irq remapper once rather than having both a set of ranges +
> eventually the remapper, and I've thought about using the remapper for
> everything too, but I'd still like to keep the concept of ranges, thus
> I'm tempted to still allocate all irqs for a given controller
> continuously in the remapper when instanciating the PIC rather than when
> actually looking for IRQs....

I think our thoughts on this differ only on the aspect of whether the
remapper is considered to be an arbitrary remapper, a smart
implementation of which results in IRQ being remapped via offsets,
versus multi-level remapping, with offsets first followed by arbitrary
remappings as necessary (and the distinction between offsets and
arbitrary remapping being visible beyond the remapper function).


-- 
Michal Ostrowski <mostrows@watson.ibm.com>

Re: [RFC/PATCH 0/8] Overhaul of virt IRQ configuration. / Kill ppc64_interrupt_controller.

[Benjamin Herrenschmidt]

> The fact that the allocation may be sparse does not mean it actually
> will be.  As long as code expects arbitrary remappings it will have a
> degree of robustness; but that does not mean that the mechanism by which
> remappings are done cannot instantiate simple mappings.  In other words,
> I'd like to see the concept of "irq offsets" being banished, but having
> re-mapping code be smart enough so that, for example, an MPIC that must
> deal with interrupt vectors 0..127 has these vectors made visible to the
> system at 16..143.
> 
> I think that the current virt irq remapping algorithm would provide you
> with exactly this behavior.

The current virt irq remapper is not very efficient. It's especially
slow in the critical path of real -> virt translation which is probably
why xics locally has a radix tree version. I suppose we could generalize
that in a "better" remapper using both the array and the radix tree
though.

Offsets sounded like a simpler thing, specially for platforms that don't
need arbitrary remappings, and less code/memory hungry (embedded
platformns would like that for example).

> In cases where we have a struct device_node, the struct device_node
> should have a pointer to the PIC (or the PIC's device_node), and the
> raw, unmodified interrupt line values.   When one wants to present an
> irq to generic code, we use the (PIC,line) information from the
> device_node to instantiate a virq.
> 
> If there is no struct device_node, you still have to come up with a
> (PIC,line) pair in order to get a virq.

I won't add anything to struct device node. Only remove from it. The
device node is only used initially when binding a given device to an
irq, no need to "cache" that in the struct device node or whatever. The
generic code will provide means for walking the interrupt tree along the
lines I described to obtain a line,PIC pair, which will then be looked
up in a table of PICs so an offset  can be obtained or a remapping of
the line into a global virq.

> virt_irq_create_mapping() (or whatever replaces it) should take the
> physical line and a PIC identifier/pointer as arguments so that it can
> inform the PIC about a mapping it has instantiated (in case we really
> have to make an arbitrary re-mapping) and so that it can try to
> instantiate an identity or offset-only translation if possible.  In the
> latter case, the PIC object will tell the remapper what offset range to
> use.

Sounds over complicated to me.

> I think our thoughts on this differ only on the aspect of whether the
> remapper is considered to be an arbitrary remapper, a smart
> implementation of which results in IRQ being remapped via offsets,
> versus multi-level remapping, with offsets first followed by arbitrary
> remappings as necessary (and the distinction between offsets and
> arbitrary remapping being visible beyond the remapper function).

Beware of over engineering :)

Ben.