DT support for ARM GIC and TWD

Add support for populating early platform devices from the device tree, by walking the tree and adding nodes whose 'compatible' property matches the 'class' string passed as a parameter.

This allows devices to be probed long before the whole device infrastructure is available.

Signed-off-by Marc Zyngier <[email protected]>

---
?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++ ?include/linux/of_platform.h | ? ?1 +
?2 files changed, 27 insertions(+), 0 deletions(-)

drivers/of/platform.c +6 -0

index e75af39..2a323ee 100644
@@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
 ? ? ? ?of_node_put(root);
 ? ? ? ?return rc;
 ?}
+
+/**
+ * of_early_platform_populate() - Populate early platform devices from DT
+ * @class: string to compare to the 'compatible' attributes
+ *
+ * This function walks the device tree and register devices whose

[cc'ing linux kernel since I discuss driver core issues] [cc'ing greg and kay, 'cause I've included a patch I'd like you to look at (see end of email)]

Quoted 27 lines · Marc Zyngier

Add support for populating early platform devices from the device tree, by walking the tree and adding nodes whose 'compatible' property matches the 'class' string passed as a parameter.

This allows devices to be probed long before the whole device infrastructure is available.

Signed-off-by Marc Zyngier <[email protected]>

---
?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++ ?include/linux/of_platform.h | ? ?1 +
?2 files changed, 27 insertions(+), 0 deletions(-)

drivers/of/platform.c +6 -0

index e75af39..2a323ee 100644
@@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
 ? ? ? ?of_node_put(root);
 ? ? ? ?return rc;
 ?}
+
+/**
+ * of_early_platform_populate() - Populate early platform devices from DT
+ * @class: string to compare to the 'compatible' attributes
+ *
+ * This function walks the device tree and register devices whose

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 476

Quoted 10 lines

drivers/of/platform.c +10 -0

+ * 'compatible' property matches the 'class' parameter.
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_early_platform_populate(const char *class)
+{
+ ? ? ? struct platform_device *pdev;
+ ? ? ? struct device_node *np = NULL;
+
+ ? ? ? while ((np = of_find_compatible_node(np, NULL, class))) {

for_each_compatible_node()

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 482

Quoted 6 lines

drivers/of/platform.c +6 -0

+ ? ? ? ? ? ? ? pdev = of_device_alloc(np, NULL, NULL);
+ ? ? ? ? ? ? ? if (!pdev)
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? list_del(&pdev->dev.devres_head);
+ ? ? ? ? ? ? ? memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
+ ? ? ? ? ? ? ? early_platform_add_devices(&pdev, 1);

I'm not sure this will work reliably. The of_platform semantics are still slightly different from 'regular' platform devices (which I do intend to fix though), so it may cause problems with how the device name gets assigned. I'd need to dig into it though.

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 486

Quoted 4 lines

drivers/of/platform.c +4 -0

+ ? ? ? }
+
+ ? ? ? return 0;
+}

Hmmm, I'd really rather not go down the path of having different 'classes' of devices that need to be registered at different times. There just isn't a really good generic way to know which devices should be the 'early' ones, it gets hairy with regard to making sure devices don't get registered twice, and it doesn't actually solve the problem that there is no way to handle dependencies between devices in the Linux device model.

What I /want/ to do is allow drivers to defer initialization at .probe time if some of the resources it needs aren't yet available. Unfortunately, that doesn't work so well for interrupt controllers since irq numbers are expected to be correctly populated in the platform device resource table. What complicates things further is that most gpio controllers are doubling as irq controllers nowdays, and those are typically modelled with platform devices themselves.

Quoted 70 lines · On Fri, 24 Jun 2011 22:49:56 -0600 Grant Likely <[email protected]>…

[cc'ing linux kernel since I discuss driver core issues] [cc'ing greg and kay, 'cause I've included a patch I'd like you to look at (see end of email)]

Quoted 27 lines · Marc Zyngier

Add support for populating early platform devices from the device tree, by walking the tree and adding nodes whose 'compatible' property matches the 'class' string passed as a parameter.

This allows devices to be probed long before the whole device infrastructure is available.

Signed-off-by Marc Zyngier <[email protected]>

---
?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++ ?include/linux/of_platform.h | ? ?1 +
?2 files changed, 27 insertions(+), 0 deletions(-)

drivers/of/platform.c +6 -0

index e75af39..2a323ee 100644
@@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
 ? ? ? ?of_node_put(root);
 ? ? ? ?return rc;
 ?}
+
+/**
+ * of_early_platform_populate() - Populate early platform devices from DT
+ * @class: string to compare to the 'compatible' attributes
+ *
+ * This function walks the device tree and register devices whose

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 476

Quoted 10 lines

drivers/of/platform.c +10 -0

+ * 'compatible' property matches the 'class' parameter.
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_early_platform_populate(const char *class)
+{
+ ? ? ? struct platform_device *pdev;
+ ? ? ? struct device_node *np = NULL;
+
+ ? ? ? while ((np = of_find_compatible_node(np, NULL, class))) {

for_each_compatible_node()

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 482

Quoted 6 lines

drivers/of/platform.c +6 -0

+ ? ? ? ? ? ? ? pdev = of_device_alloc(np, NULL, NULL);
+ ? ? ? ? ? ? ? if (!pdev)
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? list_del(&pdev->dev.devres_head);
+ ? ? ? ? ? ? ? memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
+ ? ? ? ? ? ? ? early_platform_add_devices(&pdev, 1);

I'm not sure this will work reliably. The of_platform semantics are still slightly different from 'regular' platform devices (which I do intend to fix though), so it may cause problems with how the device name gets assigned. I'd need to dig into it though.

drivers/of/platform.c int of_platform_populate(struct device_node *root, · line 486

Quoted 4 lines

drivers/of/platform.c +4 -0

+ ? ? ? }
+
+ ? ? ? return 0;
+}

Hmmm, I'd really rather not go down the path of having different 'classes' of devices that need to be registered at different times. There just isn't a really good generic way to know which devices should be the 'early' ones, it gets hairy with regard to making sure devices don't get registered twice, and it doesn't actually solve the problem that there is no way to handle dependencies between devices in the Linux device model.

Quoted 7 lines

What I /want/ to do is allow drivers to defer initialization at .probe time if some of the resources it needs aren't yet available. Unfortunately, that doesn't work so well for interrupt controllers since irq numbers are expected to be correctly populated in the platform device resource table. What complicates things further is that most gpio controllers are doubling as irq controllers nowdays, and those are typically modelled with platform devices themselves.

While I totally agree with all the above, this patch tries to address a slightly different problem. On top of device/device dependencies, there is also a number of implicit dependencies.

In the case I'm currently trying to solve, the kernel expects a timer to be up and running when hitting the delay loop calibration. At that stage, it is impossible to register a platform device, hence the use (abuse?) of early platform devices.

The current ARM code relies on the timers *not* being a standard device, and being directly setup by an board specific method. The SMP timers are even worse, as they are directly called by the core code, meaning that we can only have *one* implementation at a time in the kernel.

So the early platform stuff is a potential solution for that, though there is no way it would handle any kind of dependency. What I dream of is to have the full device/driver infrastructure available much earlier, before the rest of the kernel starts relying on some hardware being up and running...

Quoted 7 lines · Marc Zyngier

On Fri, 24 Jun 2011 22:49:56 -0600
The current ARM code relies on the timers *not* being a standard device, and being directly setup by an board specific method. The SMP timers are even worse, as they are directly called by the core code, meaning that we can only have *one* implementation at a time in the kernel.

Quoted 5 lines

So the early platform stuff is a potential solution for that, though there is no way it would handle any kind of dependency. What I dream of is to have the full device/driver infrastructure available much earlier, before the rest of the kernel starts relying on some hardware being up and running...

My suggestion: don't use platform_device for this. I think it is entirely the wrong model. Keep the timers *not* standard devices.

Instead, use a timer setup function that accepts a list of compatible properties and an initialization hook for each timer type. Since timers have exist, have to be called by core code, and cannot ever be built as modules, the Linux device model really doesn't offer much advantage.

It is completely valid and often done to access device tree data from early setup code without any context of a struct device.

Use of_early_platform_populate() to collect nodes with the "localtimer" compatible property and register them with the early platform "bus".

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/kernel/time.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/arm/kernel/time.c +1 -0

index 32d0df8..08a28ef 100644
@@ -25,6 +25,7 @@
 #include <linux/timer.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 #include <linux/mc146818rtc.h>
 

Quoted 22 lines · Marc Zyngier

Use of_early_platform_populate() to collect nodes with the "localtimer" compatible property and register them with the early platform "bus".

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/kernel/time.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/arm/kernel/time.c +1 -0

index 32d0df8..08a28ef 100644
@@ -25,6 +25,7 @@
 #include <linux/timer.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 #include <linux/mc146818rtc.h>
 

arch/arm/kernel/time.c static void __init __arm_late_time_init(void) · line 162

Quoted 6 lines

arch/arm/kernel/time.c +3 -0

@@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
 		arm_late_time_init();
 
 #ifdef CONFIG_LOCAL_TIMER_DEVICES
+#ifdef CONFIG_OF_FLATTREE
+	of_early_platform_populate("localtimer");
+#endif

Rather than #ifdeffing around the function call, it is often cleaner to have an #else in the header file that defines an empty static inline.

arch/arm/kernel/time.c static void __init __arm_late_time_init(void) · line 164

Quoted 2 lines

arch/arm/kernel/time.c +0 -0

 	early_platform_driver_register_all("localtimer");
 	early_platform_driver_probe("localtimer", 1, 0);

I suggested in my other reply that early_platform_driver should not be used. It looks like it is already being used, so I'll back off a bit from that position. However, the structure of the code really shouldn't be any different between clock devices being statically declared vs. clock data being obtained from the DT.

Grant,

Quoted 34 lines · Grant Likely

Quoted 25 lines · Marc Zyngier

Use of_early_platform_populate() to collect nodes with the "localtimer" compatible property and register them with the early platform "bus".

Signed-off-by Marc Zyngier <[email protected]>

---
?arch/arm/kernel/time.c | ? ?4 ++++
?1 files changed, 4 insertions(+), 0 deletions(-)

arch/arm/kernel/time.c +0 -0

index 32d0df8..08a28ef 100644
@@ -25,6 +25,7 @@

?#include <linux/timer.h>
?#include <linux/irq.h>
?#include <linux/platform_device.h>
+#include <linux/of_platform.h>

?#include <linux/mc146818rtc.h>

arch/arm/kernel/time.c +0 -0

@@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
 ? ? ? ? ? ? ?arm_late_time_init();
 

?#ifdef CONFIG_LOCAL_TIMER_DEVICES

arch/arm/kernel/time.c static void __init __arm_late_time_init(void) · line 162

Quoted 3 lines

arch/arm/kernel/time.c +3 -0

+#ifdef CONFIG_OF_FLATTREE
+ ? ?of_early_platform_populate("localtimer");
+#endif

Rather than #ifdeffing around the function call, it is often cleaner to have an #else in the header file that defines an empty static inline.

Quoted 9 lines

Quoted 2 lines

? ? ?early_platform_driver_register_all("localtimer"); ? ? ?early_platform_driver_probe("localtimer", 1, 0);

I suggested in my other reply that early_platform_driver should not be used. ?It looks like it is already being used, so I'll back off a bit from that position. ?However, the structure of the code really shouldn't be any different between clock devices being statically declared vs. clock data being obtained from the DT.

It's not really already being used. It is added in Marc's previous patch series to move timers to drivers/clocksource.

Deferring driver probe doesn't really help for timers as they have to be up early. If early platform drivers shouldn't be used, then why was it accepted into the kernel in the first place? It doesn't make sense that it is okay for one arch (sh), but not another (arm), ...

... or that it is okay
for non-DT, but not for DT.

From [email protected] Wed Jun 1 12:03:18 2011

From [email protected] Wed Jun 1 12:03:18 2011

Some solutions to the early device problem

a) Most complex: Use new properties to create a dependency graph. This
is probably the most general solution, but also probably the hardest for
people to think about and maintain.

b) Define a set of init phases 0,1,2,... and mark each early device node with a phase number property. Scan the tree for each phase and handle only the devices with that phase number. Unmarked nodes are handled last.

c) Similar to b, but instead of properties in nodes, have properties like "linux-phase0", "linux-phase1", etc in /chosen, whose values are lists of phandles.

Convert the GIC driver to use the DT infrastructure. That involves introducing IRQ domains for the various types of interrupts (PPI and SPI).

The DT bindings for the GIC are defined as such:
- one interrupt-controller for SPIs
- one interrupt-controller for PPIs per CPU
- interrupt signalling for secondary GICs.

Tested on RealView PB11MPCore and VExpress.

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 3 files changed, 352 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

Documentation/devicetree/bindings/arm/gic.txt new file +2 -0

new file mode 100644
index 0000000..9975345
@@ -0,0 +1,92 @@
+* ARM Generic Interrupt Controller
+

Documentation/devicetree/bindings/arm/gic.txt new file +25 -0

+
+Primary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1f001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	reg = <0x1f001000 0x1000>, /* Distributor interface */
+	      <0x1f000100 0x100>;  /* CPU interface */
+
+	/* One PPI controller per CPU, only on primary GIC */
+	gic0ppi0: gic-ppi at 0 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0>;
+	};
+
+	gic0ppi1: gic-ppi at 1 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <1>;
+	};
+
+	gic0ppi2: gic-ppi at 2 {

Documentation/devicetree/bindings/arm/gic.txt new file +27 -0

+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic0spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>; /* IRQ, level/edge */
+	};
+};
+
+Secondary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1e001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupt-parent = <&gic0spi>;
+
+	reg = <0x1e001000 0x1000>, /* Distributor interface */
+	      <0x1e000000 0x100>;  /* CPU interface */
+
+	interrupts = <42 0>; /* Cascade */
+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic1spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+};

arch/arm/common/gic.c +3 -0

index c5acc76..34907ea 100644
@@ -28,6 +28,9 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>

arch/arm/common/gic.c +13 -24

 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+	struct irq_domain spi_dom;
+};
+
 #ifdef CONFIG_ARM_GIC_VPPI
-	/* These fields must be 0 on secondary GICs */
-	int	     ppi_base;
-	int	     vppi_base;
-	u16	     nrppis;
+static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
 #endif
-};
 
 /*
  * Supported arch specific GIC irq extension.

@@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
-	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
-	unsigned int vppi_irq;
-	unsigned int ppi;
+	struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
 
-	WARN_ON(!chip_data->vppi_base);
+	return irq_domain_to_irq(dom, irq);
+}
 
-	ppi = irq - chip_data->ppi_base;
-	vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
-	vppi_irq += chip_data->vppi_base;
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
+{
+	struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
 
-	return vppi_irq;
+	return irq_domain_to_irq(dom, irq);
 }
 
 static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
 {
-	unsigned int vppi_irq;
-
-	vppi_irq = gic_ppi_to_vppi(irq);
-	generic_handle_irq(vppi_irq);
+	generic_handle_irq(gic_ppi_to_vppi(irq));
 }
 
 static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
 {
-	struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
-	unsigned int ppi_irq;
-
-	ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
-	ppi_irq += chip_data->ppi_base;
-
-	return irq_get_irq_data(ppi_irq);
+	return irq_get_irq_data(d->irq - d->domain->irq_base);
 }
 
 static void gic_ppi_eoi_irq(struct irq_data *d)

@@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
 	.irq_set_type		= gic_ppi_set_type,
 	.irq_set_wake		= gic_ppi_set_wake,

arch/arm/common/gic.c +14 -0

+
+static struct irq_domain_ops gic_ppi_domain_ops = {
+	.dt_translate = git_dt_translate,
+};
+
+static struct of_device_id gic_ids[] = {
+	{ .compatible = "arm,gic", },
+	{ },
+};
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return of_find_matching_node(np, gic_ids);
+}

arch/arm/common/gic.c +8 -0

+		reg = of_get_property(np, "reg", NULL);
+		if (!reg)	/* Duh? */
+			continue;
+		if (be32_to_cpu(*reg) == cpu_nr)
+			break;
+	}
+
+	return np;

arch/arm/common/gic.c +32 -12

+static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+					    int cpu_nr)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+	return NULL;
+}
+#endif
 #endif
 
+static const char *get_of_name(struct device_node *np)
+{
+	return np ? np->full_name : "";
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
-	unsigned int irq_start)
+				 unsigned int irq_start,
+				 struct device_node *gic_node)
 {
-	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
+	unsigned int gic_irqs, irq_limit, i, c;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
-	u32 dist_ctr, nrcpus;
+	u32 dist_ctr, nrcpus, reg;
+	u32 ppi_base = 0;
+	u16 nrppis = 0;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;

@@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/* Find out how many CPUs are supported (8 max). */
 	nrcpus = ((dist_ctr >> 5) & 7) + 1;
 
-#ifdef CONFIG_ARM_GIC_VPPI
 	/*
 	 * Nobody would be insane enough to use PPIs on a secondary
 	 * GIC, right?
 	 */
 	if (gic == &gic_data[0]) {
-		gic->nrppis = 16 - (irq_start % 16);
-		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
-		nrvppis = gic->nrppis * nrcpus;
-	} else {
-		gic->ppi_base = 0;
-		gic->vppi_base = 0;
+		ppi_base = irq_start & 31;
+		nrppis = 16 - (ppi_base & 15);
 	}
-#endif
 
 	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
-		gic_irqs, nrvppis);
+		gic_irqs, nrppis * nrcpus);
 
 	/*

arch/arm/common/gic.c +116 -17

 	/*
 	 * Set all global interrupts to this CPU only.

@@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
-	for (i = irq_start; i < irq_limit; i++) {
+
+	gic->nr_irqs = gic_irqs;
+	gic->spi_dom.irq_base = irq_start & ~31;
+	gic->spi_dom.ops = &gic_ppi_domain_ops;
+	gic->spi_dom.of_node = gic_get_spi_node(gic_node);
+
+	pr_info("GIC SPI domain %s [%d:%d]\n",
+		get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
+
+	irq_domain_add(&gic->spi_dom);
+	
+	for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
+		unsigned int irq = irq_domain_map(&gic->spi_dom, i);
+		pr_debug("GIC mapping %s:%d to IRQ%d\n",
+			 get_of_name(gic->spi_dom.of_node), i, irq);
 #ifdef CONFIG_ARM_GIC_VPPI
-		if (nrvppis && gic_irq_is_ppi(gic, i))
-			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		if (nrppis && gic_irq_is_ppi(gic, irq))
+			irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
 		else
 #endif
 		{
-			irq_set_chip_and_handler(i, &gic_chip,
+			irq_set_chip_and_handler(irq, &gic_chip,
 						 handle_fasteoi_irq);
-			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
 		}
-		irq_set_chip_data(i, gic);
+		irq_set_chip_data(irq, gic);
 	}
 
 #ifdef CONFIG_ARM_GIC_VPPI
-	if (!nrvppis)
+	if (!nrppis)
 		goto out;
-	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
-	if (WARN_ON(gic->vppi_base < 0))
-		goto out;
-	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
-		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
-		irq_set_chip_data(i, gic);
-		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+	for (c = 0; c < nrcpus; c++) {
+		struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
+		int base;
+
+		base = irq_alloc_descs(-1, 0, nrppis, 0);
+		if (WARN_ON(base < 0))
+			goto out;
+
+		/*
+		 * We cheat here, and fold ppi_base into irq_base, as
+		 * it saves us some work at runtime.
+		 */
+		dom->irq_base = base - ppi_base;
+		dom->ops = &gic_ppi_domain_ops;
+		dom->of_node = gic_get_ppi_node(gic_node, c);
+
+		pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
+			get_of_name(dom->of_node), base, base + nrppis - 1, c);
+
+		irq_domain_add(dom);
+
+		for (i = 0; i < nrppis; i++) {
+			unsigned int irq = irq_domain_map(dom, i + irq_start);
+
+			pr_debug("GIC mapping %s:%d to IRQ%d\n",
+				 get_of_name(dom->of_node), i, irq);
+
+			irq_set_chip_and_handler(irq, &gic_ppi_chip,
+						 handle_percpu_irq);
+			irq_set_chip_data(irq, gic);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+		}
 	}
 out:
 #endif

@@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 	writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
-void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
-	void __iomem *dist_base, void __iomem *cpu_base)
+static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
+				void __iomem *dist_base, void __iomem *cpu_base,
+				struct device_node *gic_node)
 {
 	struct gic_chip_data *gic;
 

@@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	if (gic_nr == 0)
 		gic_cpu_base_addr = cpu_base;
 
-	gic_dist_init(gic, irq_start);
+	gic_dist_init(gic, irq_start, gic_node);
 	gic_cpu_init(gic);
 }
 
+void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
+		     void __iomem *dist_base, void __iomem *cpu_base)
+{
+	gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
+}
+
+#ifdef CONFIG_OF
+static void __init gic_of_init_one(int idx, unsigned int irq_start,
+				   struct device_node *np)
+{
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+
+	dist_base = of_iomap(np, 0);
+	cpu_base  = of_iomap(np, 1);
+	if (!dist_base || ! cpu_base) {
+		pr_err("Cannot map %s\n", np->full_name);
+		return;
+	}
+
+	/* Assume we use the full 16 PPIs */
+	gic_init_one(idx, irq_start, dist_base, cpu_base, np);
+}
+
+void __init gic_of_init(void)
+{
+	struct device_node *np = NULL;
+	unsigned int irq_start = 0;
+	int i = 0;
+
+	/* Look for the GIC providing PPIs first */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (!ppi_np)
+			continue;
+			
+		/* Assume we use the full 16 PPIs */
+		gic_of_init_one(i, 16, np);
+		irq_start += gic_data[i].nr_irqs;
+		i++;
+		break;
+	}
+	of_node_put(np);
+	np = NULL;
+
+	/* Now all the others */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		int irq;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (ppi_np)
+			continue;
+			
+		gic_of_init_one(i, irq_start, np);
+		irq = irq_of_parse_and_map(np, 0);
+		if (irq != IRQ_NONE) {

arch/arm/common/gic.c +0 -0

 void __cpuinit gic_secondary_init(unsigned int gic_nr)
 {
 	BUG_ON(gic_nr >= MAX_GIC_NR);

arch/arm/include/asm/hardware/gic.h +4 -0

index 069d4c0..04cfe33 100644
@@ -39,16 +39,20 @@ extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+void gic_of_init(void);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq);
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu);
 #else
 static inline unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
 	return irq;
 }
+
+#define gic_ppi_to_cpu_vppi(i,c) gic_ppi_to_vppi(i)
 #endif
 
 #endif

Signature · 1 line

1.7.0.4

Quoted 4 lines · Marc Zyngier

Convert the GIC driver to use the DT infrastructure. That involves introducing IRQ domains for the various types of interrupts (PPI and SPI).

Quoted 8 lines

The DT bindings for the GIC are defined as such:
- one interrupt-controller for SPIs
- one interrupt-controller for PPIs per CPU
- interrupt signalling for secondary GICs.

Tested on RealView PB11MPCore and VExpress.

Signed-off-by Marc Zyngier <[email protected]>

Hi Mark,

Thanks for doing this work. Comments below.

Quoted 23 lines

---
 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 3 files changed, 352 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

Documentation/devicetree/bindings/arm/gic.txt new file +10 -0

new file mode 100644
index 0000000..9975345
@@ -0,0 +1,92 @@
+* ARM Generic Interrupt Controller
+
+ARM SMP cores are often associated with a GIC, providing per processor
+interrupts (PPI), shared processor interrupts (SPI) and software
+generated interrupts (SGI).
+
+Primary GIC is attached directly to the CPU. Secondary GICs are
+cascaded into the upward interrupt controller.
+
+Main node properties:

Documentation/devicetree/bindings/arm/gic.txt line 12

Quoted 2 lines

Documentation/devicetree/bindings/arm/gic.txt new file +2 -0

+
+- compatible : "arm,gic"

I'm nervous about simply "arm,gic" with no specific silicon or core version number attached to it. Specifications for standard cores such as this could very well change with newer versions of the core. I'd like to see either the core version specified, or this value anchored on a specific chip implementation.

Documentation/devicetree/bindings/arm/gic.txt line 17

Quoted 5 lines

Documentation/devicetree/bindings/arm/gic.txt new file +5 -0

+- reg                : one register mapping for the distributor interface
+               another one for the CPU interface
+- #address-cells: <1>
+- #size-cells        : <0>
+- interrupts : optionnal, used on secondary GICs only

sp. optional

Documentation/devicetree/bindings/arm/gic.txt line 30

Quoted 13 lines

Documentation/devicetree/bindings/arm/gic.txt new file +13 -0

+
+PPI sub nodes: one node per CPU (primary GIC only)
+
+- interrupt-controller       : required
+- #interrupt-cells   : <1>
+- reg                        : index of the CPU this PPI controller is
+                       connected to.
+
+SPI sub node:
+
+- interrupt-controller       : required
+- #interrupt-cells   : <2> First parameter is the interrupt number,
+                       second is 0 for level-high, 1 for edge-rising.

Is there any possibility of level-low or edge-falling? Since irq flags are essentially arbitrary values chosen by the writer of the binding, I generally recommend that the best arbitrary values are the ones currently used by the kernel for IRQF_TRIGGER_*:
edge-rising: 1
edge-falling: 2
level-high: 4
level-low: 8

Quoted 25 lines

Documentation/devicetree/bindings/arm/gic.txt new file +25 -0

+
+Primary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1f001000 {
+     compatible = "arm,gic";
+     #address-cells = <1>;
+     #size-cells = <0>;
+
+     reg = <0x1f001000 0x1000>, /* Distributor interface */
+           <0x1f000100 0x100>;  /* CPU interface */
+
+     /* One PPI controller per CPU, only on primary GIC */
+     gic0ppi0: gic-ppi at 0 {
+             interrupt-controller;
+             #interrupt-cells = <1>;
+             reg = <0>;
+     };
+
+     gic0ppi1: gic-ppi at 1 {
+             interrupt-controller;
+             #interrupt-cells = <1>;
+             reg = <1>;
+     };
+
+     gic0ppi2: gic-ppi at 2 {

Documentation/devicetree/bindings/arm/gic.txt line 65

Quoted 10 lines

Documentation/devicetree/bindings/arm/gic.txt new file +10 -0

+             interrupt-controller;
+             #interrupt-cells = <1>;
+             reg = <2>;
+     };
+
+     gic0ppi3: gic-ppi at 3 {
+             interrupt-controller;
+             #interrupt-cells = <1>;
+             reg = <3>;
+     };

I don't know much about the GIC PPIs, so it is hard to provide good feedback on this binding. Are the PPI interrupts configurable in any way, or are certain PPI inputs hard wired to specific ppi

controllers? In other words, does this binding reflect how the device is hard wired, or is irq affinity a software decision? If it is hardwired, then the above binding probably makes sense. If it is a software configuration, then I would suggest dropping the per-ppi nodes and having a flat irq number space that only reflects the hardware attachments, and use a flags field in the irq specifier if the ppi affinity isn't something that the kernel can dynamically assign.

Quoted 43 lines

Documentation/devicetree/bindings/arm/gic.txt new file +27 -0

+
+     /* One global SPI controller per GIC, defined on all GICs */
+     gic0spi: gic-spi {
+             interrupt-controller;
+             #interrupt-cells = <2>; /* IRQ, level/edge */
+     };
+};
+
+Secondary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1e001000 {
+     compatible = "arm,gic";
+     #address-cells = <1>;
+     #size-cells = <0>;
+     interrupt-parent = <&gic0spi>;
+
+     reg = <0x1e001000 0x1000>, /* Distributor interface */
+           <0x1e000000 0x100>;  /* CPU interface */
+
+     interrupts = <42 0>; /* Cascade */
+
+     /* One global SPI controller per GIC, defined on all GICs */
+     gic1spi: gic-spi {
+             interrupt-controller;
+             #interrupt-cells = <2>;
+     };
+};

arch/arm/common/gic.c +3 -0

index c5acc76..34907ea 100644
@@ -28,6 +28,9 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>

@@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
 

arch/arm/common/gic.c void __iomem *gic_cpu_base_addr __read_mostly; · line 46

Quoted 3 lines

arch/arm/common/gic.c +1 -0

 struct gic_chip_data {
      unsigned int irq_offset;
+     unsigned int nr_irqs;

nr_irq probably needs to be part of struct irq_domain. I should change that.

Quoted 61 lines

arch/arm/common/gic.c +13 -24

      void __iomem *dist_base;
      void __iomem *cpu_base;
+     struct irq_domain spi_dom;
+};
+
 #ifdef CONFIG_ARM_GIC_VPPI
-     /* These fields must be 0 on secondary GICs */
-     int          ppi_base;
-     int          vppi_base;
-     u16          nrppis;
+static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
 #endif
-};
 
 /*
  * Supported arch specific GIC irq extension.

@@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
-     struct gic_chip_data *chip_data = irq_get_chip_data(irq);
-     unsigned int vppi_irq;
-     unsigned int ppi;
+     struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
 
-     WARN_ON(!chip_data->vppi_base);
+     return irq_domain_to_irq(dom, irq);
+}
 
-     ppi = irq - chip_data->ppi_base;
-     vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
-     vppi_irq += chip_data->vppi_base;
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
+{
+     struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
 
-     return vppi_irq;
+     return irq_domain_to_irq(dom, irq);
 }
 
 static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
 {
-     unsigned int vppi_irq;
-
-     vppi_irq = gic_ppi_to_vppi(irq);
-     generic_handle_irq(vppi_irq);
+     generic_handle_irq(gic_ppi_to_vppi(irq));
 }
 
 static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
 {
-     struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
-     unsigned int ppi_irq;
-
-     ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
-     ppi_irq += chip_data->ppi_base;
-
-     return irq_get_irq_data(ppi_irq);
+     return irq_get_irq_data(d->irq - d->domain->irq_base);
 }
 

Quoted 4 lines

arch/arm/common/gic.c +0 -0

 static void gic_ppi_eoi_irq(struct irq_data *d)

@@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
      .irq_set_type           = gic_ppi_set_type,
      .irq_set_wake           = gic_ppi_set_wake,

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 349

Quoted 10 lines

arch/arm/common/gic.c +9 -0

 };
+
+#ifdef CONFIG_OF
+static int git_dt_translate(struct irq_domain *dom,
+                         struct device_node *controler,
+                         const u32 *intspec, unsigned int intsize,
+                         irq_hw_number_t *out_hwirq, unsigned int *out_type)
+{
+     if (dom->of_node != controler)
+             return -1;

-EINVAL

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 353

Quoted 4 lines

arch/arm/common/gic.c +4 -0

+
+     if (intsize > 1) {
+             if (intspec[1])
+                     *out_type = IRQ_TYPE_EDGE_RISING;

nit: inconsistent whitespace.

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 357

Quoted 4 lines

arch/arm/common/gic.c +4 -0

+             else
+                     *out_type = IRQ_TYPE_LEVEL_HIGH;
+     } else
+             *out_type = IRQ_TYPE_NONE;

I would do something like (assuming you take my suggestion above about arbitrary values):

        if (intsize < 2 || intspec[0] > (max_gic_hwirq_number))

                return -EINVAL;
        /* Binding uses save values as IRQF_TRIGGER_* macros */
        *out_type = intspec[1] & IRQ_TRIGGER_MASK;
        *out_hwirq = intspec[0];
        return 0;

Quoted 18 lines

arch/arm/common/gic.c +18 -0

+
+static struct irq_domain_ops gic_ppi_domain_ops = {
+     .dt_translate = git_dt_translate,
+};
+
+static struct of_device_id gic_ids[] = {
+     { .compatible = "arm,gic", },
+     { },
+};
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+     return of_find_matching_node(np, gic_ids);
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+                                         int cpu_nr)
+{

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 385

Quoted 6 lines

arch/arm/common/gic.c +6 -0

+     struct device_node *np = NULL;
+
+     if (!gic_np)
+             return NULL;
+
+     while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 389

Quoted 4 lines

arch/arm/common/gic.c +4 -0

+             const __be32 *reg;
+
+             if (strcmp(np->name, "gic-ppi"))
+                     continue;

Generally, its a good idea to differentiate what a node represents by compatible property, not name. This would be a slight change to your binding, but I think it would be more consistent overall.

Quoted 8 lines

arch/arm/common/gic.c +8 -0

+             reg = of_get_property(np, "reg", NULL);
+             if (!reg)       /* Duh? */
+                     continue;
+             if (be32_to_cpu(*reg) == cpu_nr)
+                     break;
+     }
+
+     return np;

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 407

Quoted 10 lines

arch/arm/common/gic.c +10 -0

+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+     struct device_node *np = NULL;
+
+     if (!gic_np)
+             return NULL;
+
+     while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

arch/arm/common/gic.c static struct irq_chip gic_ppi_chip = { · line 414

Quoted 7 lines

arch/arm/common/gic.c +7 -0

+             if (!strcmp(np->name, "gic-spi"))
+                     break;
+     }
+
+     return np;
+}
+#else

/* CONFIG_BLAH */ comments are generally appreciated on #else/#endif statements.

Quoted 66 lines

arch/arm/common/gic.c +32 -12

+static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+     return NULL;
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+                                         int cpu_nr)
+{
+     return NULL;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+     return NULL;
+}
+#endif
 #endif
 
+static const char *get_of_name(struct device_node *np)
+{
+     return np ? np->full_name : "";
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
-     unsigned int irq_start)
+                              unsigned int irq_start,
+                              struct device_node *gic_node)
 {
-     unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
+     unsigned int gic_irqs, irq_limit, i, c;
      void __iomem *base = gic->dist_base;
      u32 cpumask = 1 << smp_processor_id();
-     u32 dist_ctr, nrcpus;
+     u32 dist_ctr, nrcpus, reg;
+     u32 ppi_base = 0;
+     u16 nrppis = 0;
 
      cpumask |= cpumask << 8;
      cpumask |= cpumask << 16;

@@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
      /* Find out how many CPUs are supported (8 max). */
      nrcpus = ((dist_ctr >> 5) & 7) + 1;
 
-#ifdef CONFIG_ARM_GIC_VPPI
      /*
       * Nobody would be insane enough to use PPIs on a secondary
       * GIC, right?
       */
      if (gic == &gic_data[0]) {
-             gic->nrppis = 16 - (irq_start % 16);
-             gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
-             nrvppis = gic->nrppis * nrcpus;
-     } else {
-             gic->ppi_base = 0;
-             gic->vppi_base = 0;
+             ppi_base = irq_start & 31;
+             nrppis = 16 - (ppi_base & 15);
      }
-#endif
 
      pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
-             gic_irqs, nrvppis);
+             gic_irqs, nrppis * nrcpus);
 

arch/arm/common/gic.c static void __init gic_dist_init(struct gic_chip_data *gic, · line 487

Quoted 11 lines

arch/arm/common/gic.c +6 -3

      /*
-      * Set all global interrupts to be level triggered, active low.
+      * Set all global interrupts to be level triggered, active high.
       */
-     for (i = 32; i < gic_irqs; i += 16)
-             writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+     for (i = 32; i < gic_irqs; i += 16) {
+             reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
+             reg &= 0x55555555;
+             writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
+     }

Have you got some unrelated fixups in this patch? It isn't obvious to me whether or not this hunk is related to setting up irq_domains.

Quoted 167 lines

arch/arm/common/gic.c +116 -17

      /*
       * Set all global interrupts to this CPU only.

@@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
      /*
       * Setup the Linux IRQ subsystem.
       */
-     for (i = irq_start; i < irq_limit; i++) {
+
+     gic->nr_irqs = gic_irqs;
+     gic->spi_dom.irq_base = irq_start & ~31;
+     gic->spi_dom.ops = &gic_ppi_domain_ops;
+     gic->spi_dom.of_node = gic_get_spi_node(gic_node);
+
+     pr_info("GIC SPI domain %s [%d:%d]\n",
+             get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
+
+     irq_domain_add(&gic->spi_dom);
+
+     for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
+             unsigned int irq = irq_domain_map(&gic->spi_dom, i);
+             pr_debug("GIC mapping %s:%d to IRQ%d\n",
+                      get_of_name(gic->spi_dom.of_node), i, irq);
 #ifdef CONFIG_ARM_GIC_VPPI
-             if (nrvppis && gic_irq_is_ppi(gic, i))
-                     irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+             if (nrppis && gic_irq_is_ppi(gic, irq))
+                     irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
              else
 #endif
              {
-                     irq_set_chip_and_handler(i, &gic_chip,
+                     irq_set_chip_and_handler(irq, &gic_chip,
                                               handle_fasteoi_irq);
-                     set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
              }
-             irq_set_chip_data(i, gic);
+             irq_set_chip_data(irq, gic);
      }
 
 #ifdef CONFIG_ARM_GIC_VPPI
-     if (!nrvppis)
+     if (!nrppis)
              goto out;
-     gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
-     if (WARN_ON(gic->vppi_base < 0))
-             goto out;
-     for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
-             irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
-             irq_set_chip_data(i, gic);
-             set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+     for (c = 0; c < nrcpus; c++) {
+             struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
+             int base;
+
+             base = irq_alloc_descs(-1, 0, nrppis, 0);
+             if (WARN_ON(base < 0))
+                     goto out;
+
+             /*
+              * We cheat here, and fold ppi_base into irq_base, as
+              * it saves us some work at runtime.
+              */
+             dom->irq_base = base - ppi_base;
+             dom->ops = &gic_ppi_domain_ops;
+             dom->of_node = gic_get_ppi_node(gic_node, c);
+
+             pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
+                     get_of_name(dom->of_node), base, base + nrppis - 1, c);
+
+             irq_domain_add(dom);
+
+             for (i = 0; i < nrppis; i++) {
+                     unsigned int irq = irq_domain_map(dom, i + irq_start);
+
+                     pr_debug("GIC mapping %s:%d to IRQ%d\n",
+                              get_of_name(dom->of_node), i, irq);
+
+                     irq_set_chip_and_handler(irq, &gic_ppi_chip,
+                                              handle_percpu_irq);
+                     irq_set_chip_data(irq, gic);
+                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+             }
      }
 out:
 #endif

@@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
      writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
-void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
-     void __iomem *dist_base, void __iomem *cpu_base)
+static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
+                             void __iomem *dist_base, void __iomem *cpu_base,
+                             struct device_node *gic_node)
 {
      struct gic_chip_data *gic;
 

@@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
      if (gic_nr == 0)
              gic_cpu_base_addr = cpu_base;
 
-     gic_dist_init(gic, irq_start);
+     gic_dist_init(gic, irq_start, gic_node);
      gic_cpu_init(gic);
 }
 
+void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
+                  void __iomem *dist_base, void __iomem *cpu_base)
+{
+     gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
+}
+
+#ifdef CONFIG_OF
+static void __init gic_of_init_one(int idx, unsigned int irq_start,
+                                struct device_node *np)
+{
+     void __iomem *dist_base;
+     void __iomem *cpu_base;
+
+     dist_base = of_iomap(np, 0);
+     cpu_base  = of_iomap(np, 1);
+     if (!dist_base || ! cpu_base) {
+             pr_err("Cannot map %s\n", np->full_name);
+             return;
+     }
+
+     /* Assume we use the full 16 PPIs */
+     gic_init_one(idx, irq_start, dist_base, cpu_base, np);
+}
+
+void __init gic_of_init(void)
+{
+     struct device_node *np = NULL;
+     unsigned int irq_start = 0;
+     int i = 0;
+
+     /* Look for the GIC providing PPIs first */
+     while((np = gic_get_node(np))) {
+             struct device_node *ppi_np;
+             ppi_np = gic_get_ppi_node(np, 0);
+             of_node_put(ppi_np);
+             if (!ppi_np)
+                     continue;
+
+             /* Assume we use the full 16 PPIs */
+             gic_of_init_one(i, 16, np);
+             irq_start += gic_data[i].nr_irqs;
+             i++;
+             break;
+     }
+     of_node_put(np);
+     np = NULL;
+
+     /* Now all the others */
+     while((np = gic_get_node(np))) {
+             struct device_node *ppi_np;
+             int irq;
+             ppi_np = gic_get_ppi_node(np, 0);
+             of_node_put(ppi_np);
+             if (ppi_np)
+                     continue;
+
+             gic_of_init_one(i, irq_start, np);
+             irq = irq_of_parse_and_map(np, 0);
+             if (irq != IRQ_NONE) {

arch/arm/common/gic.c void __init gic_init(unsigned int gic_nr, unsigned int irq_start, · line 699

Quoted 10 lines

arch/arm/common/gic.c +10 -0

+                     pr_info("GIC %s cascading to IRQ%d",
+                             np->full_name, irq);
+                     gic_cascade_irq(i, irq);
+             }
+             irq_start += gic_data[i].nr_irqs;
+             i++;
+     }
+     of_node_put(np);
+}
+#endif

I see what you're doing here, but I don't think it is the best approach in the long run. Rather than walking all the interrupt-controller nodes looking for primary and secondary gics while ignoring all others, I think there needs to be a function that finds all the interrupt controller nodes, sorts them based on dependencies, and calls their initialization hook in order. Other architectures will use such a thing. I've been intending to do it for powerpc for quite a while node.

Would you be willing to tackle such a beast? I've got a fairly good idea what it needs to look like, but I just haven't had the time to do it.

Overall the patch looks like it is in the right direction.

Quoted 9 lines · Grant Likely

I see what you're doing here, but I don't think it is the best approach in the long run. ?Rather than walking all the interrupt-controller nodes looking for primary and secondary gics while ignoring all others, I think there needs to be a function that finds all the interrupt controller nodes, sorts them based on dependencies, and calls their initialization hook in order. ?Other architectures will use such a thing. ?I've been intending to do it for powerpc for quite a while node.

Quoted 3 lines

Would you be willing to tackle such a beast? ?I've got a fairly good idea what it needs to look like, but I just haven't had the time to do it.

I can give it a go.

Add device tree support to the arm_smp_twd driver.

The DT bindings for the TWD are defined as such:
- one timer node per CPU, using corresponding PPI
interrupt controller
- provides both timer and watchdog interrupt

Tested on RealView PB11MPCore and VExpress.

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
 2 files changed, 134 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

Documentation/devicetree/bindings/arm/twd.txt new file +0 -0

new file mode 100644
index 0000000..3823a81

Documentation/devicetree/bindings/arm/twd.txt new file +46 -0

+- reg			: register mapping for the registers.
+- #address-cells	: <1>
+- #size-cells		: <0>
+
+Timer sub nodes (one per CPU)
+
+- interrupt-parent	: phandle to the corresponding gic-ppi.
+- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
+- reg			: index of the CPU this timer is connected to.
+
+Example (ARM RealView PB11-MPCore):
+
+localtimer at 1f000600 {
+	compatible = "arm,smp-twd", "localtimer";
+	reg = <0x1f000600 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/*
+	 * One timer per CPU, bound to the corresponding
+	 * PPI interface.
+	 */
+	timer at 0 {
+		interrupt-parent = <&gic0ppi0>;
+		interrupts = <29 30>;
+		reg = <0>;
+	};
+
+	timer at 1 {
+		interrupt-parent = <&gic0ppi1>;
+		interrupts = <29 30>;
+		reg = <1>;
+	};
+
+	timer at 2 {
+		interrupt-parent = <&gic0ppi2>;
+		interrupts = <29 30>;
+		reg = <2>;
+	};
+
+	timer at 3 {
+		interrupt-parent = <&gic0ppi3>;
+		interrupts = <29 30>;
+		reg = <3>;
+	};
+};

drivers/clocksource/arm_smp_twd.c +8 -3

index 65f4669..586acad 100644
@@ -19,6 +19,9 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/err.h>

@@ -37,11 +40,11 @@
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
 static void __iomem *twd_base;
-static int twd_ppi;
 
 static struct clk *twd_clk;
 static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, irq_reqd);
+static DEFINE_PER_CPU(int, twd_irq);
 static struct clock_event_device __percpu *twd_evt;
 
 static void twd_set_mode(enum clock_event_mode mode,

@@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
 	clk->rating = 450;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;
-	clk->irq = gic_ppi_to_vppi(twd_ppi);
+	clk->irq = __get_cpu_var(twd_irq);
 	clk->cpumask = cpumask_of(smp_processor_id());
 
 	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());

@@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
 	.notifier_call = twd_cpu_notify,
 };
 
-static int twd_probe(struct platform_device *pdev)
+#ifdef CONFIG_OF
+static struct device_node *twd_get_timer_node(struct device_node *twd_np,
+					      struct device_node *child,

Quoted 26 lines · Marc Zyngier

Add device tree support to the arm_smp_twd driver.

The DT bindings for the TWD are defined as such:
- one timer node per CPU, using corresponding PPI
interrupt controller
- provides both timer and watchdog interrupt

Tested on RealView PB11MPCore and VExpress.

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
 2 files changed, 134 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

Documentation/devicetree/bindings/arm/twd.txt new file +4 -0

new file mode 100644
index 0000000..3823a81
@@ -0,0 +1,54 @@
+* ARM Timer Watchdog
+
+ARM SMP cores are often associated with a TWD providing a per-cpu timer
+and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.

Documentation/devicetree/bindings/arm/twd.txt line 8

Quoted 4 lines

Documentation/devicetree/bindings/arm/twd.txt new file +4 -0

+
+Main node properties:
+
+- compatible		: "arm,smp-twd", "localtimer"

"localtimer" isn't a very good compatible value. Compatible is intended to identify the specific hardware, and "localtimer" is a very generic name.

Also, as commented on for the gic, I'd like to see some level of versioning on the arm,smp-twd string.

Quoted 96 lines

Documentation/devicetree/bindings/arm/twd.txt new file +46 -0

+- reg			: register mapping for the registers.
+- #address-cells	: <1>
+- #size-cells		: <0>
+
+Timer sub nodes (one per CPU)
+
+- interrupt-parent	: phandle to the corresponding gic-ppi.
+- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
+- reg			: index of the CPU this timer is connected to.
+
+Example (ARM RealView PB11-MPCore):
+
+localtimer at 1f000600 {
+	compatible = "arm,smp-twd", "localtimer";
+	reg = <0x1f000600 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/*
+	 * One timer per CPU, bound to the corresponding
+	 * PPI interface.
+	 */
+	timer at 0 {
+		interrupt-parent = <&gic0ppi0>;
+		interrupts = <29 30>;
+		reg = <0>;
+	};
+
+	timer at 1 {
+		interrupt-parent = <&gic0ppi1>;
+		interrupts = <29 30>;
+		reg = <1>;
+	};
+
+	timer at 2 {
+		interrupt-parent = <&gic0ppi2>;
+		interrupts = <29 30>;
+		reg = <2>;
+	};
+
+	timer at 3 {
+		interrupt-parent = <&gic0ppi3>;
+		interrupts = <29 30>;
+		reg = <3>;
+	};
+};

drivers/clocksource/arm_smp_twd.c +14 -3

index 65f4669..586acad 100644
@@ -19,6 +19,9 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/err.h>

@@ -37,11 +40,11 @@
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
 static void __iomem *twd_base;
-static int twd_ppi;
 
 static struct clk *twd_clk;
 static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, irq_reqd);
+static DEFINE_PER_CPU(int, twd_irq);
 static struct clock_event_device __percpu *twd_evt;
 
 static void twd_set_mode(enum clock_event_mode mode,

@@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
 	clk->rating = 450;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;
-	clk->irq = gic_ppi_to_vppi(twd_ppi);
+	clk->irq = __get_cpu_var(twd_irq);
 	clk->cpumask = cpumask_of(smp_processor_id());
 
 	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());

@@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
 	.notifier_call = twd_cpu_notify,
 };
 
-static int twd_probe(struct platform_device *pdev)
+#ifdef CONFIG_OF
+static struct device_node *twd_get_timer_node(struct device_node *twd_np,
+					      struct device_node *child,
+					      int *timer_nr)
+{
+	child = of_get_next_child(twd_np, child);
+	if (child) {
+		const __be32 *reg;
+		reg = of_get_property(child, "reg", NULL);

drivers/clocksource/arm_smp_twd.c static struct notifier_block __cpuinitdata twd_cpu_nb = { · line 308

Quoted 4 lines

drivers/clocksource/arm_smp_twd.c +4 -0

+		if (!reg)
+			*timer_nr = -1;
+		else
+			*timer_nr = be32_to_cpu(*reg);

There's a new function that should be done and merged in time for v3.1 that will help with reading properties. Keep an eye out for of_getprop_u32()