Raspberry Pi 2 support, part 1: Interrupt controller

Raspberry Pi 2 support, part 1: Interrupt controller

[Eric Anholt]

This is a little over half of the diff for the Raspberry Pi 2
platform.  A full series you can build and test can be found at:

https://github.com/anholt/linux/tree/bcm2836-irqchip

As you can see looking at that tree, it depends on the rpi-dt-clocks
series, which is still stalled waiting on the rpi-firmware series to
be merged (Lee said he pulled 2/3 of them back on June 5th, but they
haven't appeared in the tree).

[PATCH 1/4] irqchip: bcm2835: Refactor handle_IRQ() calls out of MAKE_HWIRQ.

[Eric Anholt]

For BCM2836, we want to chain into this IRQ chip from the root
controller, and for chaining we need to do something else instead of
handle_IRQ() once we have decoded the IRQ.

Note that this changes the behavior a little bit: Previously for a
non-shortcut IRQ, we'd loop reading and handling the second level IRQ
status until it was cleared before returning to the loop reading the
top level IRQ status (Note that the top level bit is just an OR of the
low level bits).  For the expected case of just one interrupt to be
handled, this was an extra register read, so we're down from 4 to 3
reads.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/irqchip/irq-bcm2835.c | 57 ++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
index e68c3b6..382450a 100644
--- a/drivers/irqchip/irq-bcm2835.c
+++ b/drivers/irqchip/irq-bcm2835.c
@@ -179,44 +179,45 @@ static int __init armctrl_of_init(struct device_node *node,
  * handle_IRQ may briefly re-enable interrupts for soft IRQ handling.
  */
 
-static void armctrl_handle_bank(int bank, struct pt_regs *regs)
+static u32 armctrl_translate_bank(int bank)
 {
-	u32 stat, irq;
+	u32 stat = readl_relaxed(intc.pending[bank]);
 
-	while ((stat = readl_relaxed(intc.pending[bank]))) {
-		irq = MAKE_HWIRQ(bank, ffs(stat) - 1);
-		handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
-	}
+	return MAKE_HWIRQ(bank, ffs(stat) - 1);
+}
+
+static u32 armctrl_translate_shortcut(int bank, u32 stat)
+{
+	return MAKE_HWIRQ(bank, shortcuts[ffs(stat >> SHORTCUT_SHIFT) - 1]);
 }
 
-static void armctrl_handle_shortcut(int bank, struct pt_regs *regs,
-	u32 stat)
+static u32 get_next_armctrl_hwirq(void)
 {
-	u32 irq = MAKE_HWIRQ(bank, shortcuts[ffs(stat >> SHORTCUT_SHIFT) - 1]);
-	handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
+	u32 stat = readl_relaxed(intc.pending[0]) & BANK0_VALID_MASK;
+
+	if (stat == 0)
+		return ~0;
+	else if (stat & BANK0_HWIRQ_MASK)
+		return MAKE_HWIRQ(0, ffs(stat & BANK0_HWIRQ_MASK) - 1);
+	else if (stat & SHORTCUT1_MASK)
+		return armctrl_translate_shortcut(1, stat & SHORTCUT1_MASK);
+	else if (stat & SHORTCUT2_MASK)
+		return armctrl_translate_shortcut(2, stat & SHORTCUT2_MASK);
+	else if (stat & BANK1_HWIRQ)
+		return armctrl_translate_bank(1);
+	else if (stat & BANK2_HWIRQ)
+		return armctrl_translate_bank(2);
+	else
+		BUG();
 }
 
 static void __exception_irq_entry bcm2835_handle_irq(
 	struct pt_regs *regs)
 {
-	u32 stat, irq;
-
-	while ((stat = readl_relaxed(intc.pending[0]) & BANK0_VALID_MASK)) {
-		if (stat & BANK0_HWIRQ_MASK) {
-			irq = MAKE_HWIRQ(0, ffs(stat & BANK0_HWIRQ_MASK) - 1);
-			handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
-		} else if (stat & SHORTCUT1_MASK) {
-			armctrl_handle_shortcut(1, regs, stat & SHORTCUT1_MASK);
-		} else if (stat & SHORTCUT2_MASK) {
-			armctrl_handle_shortcut(2, regs, stat & SHORTCUT2_MASK);
-		} else if (stat & BANK1_HWIRQ) {
-			armctrl_handle_bank(1, regs);
-		} else if (stat & BANK2_HWIRQ) {
-			armctrl_handle_bank(2, regs);
-		} else {
-			BUG();
-		}
-	}
+	u32 hwirq;
+
+	while ((hwirq = get_next_armctrl_hwirq()) != ~0)
+		handle_IRQ(irq_linear_revmap(intc.domain, hwirq), regs);
 }
 
 IRQCHIP_DECLARE(bcm2835_armctrl_ic, "brcm,bcm2835-armctrl-ic", armctrl_of_init);
-- 
2.1.4

[PATCH 2/4] irqchip: bcm2835: If a parent interrupt is registered, chain from it.

[Eric Anholt]

The BCM2836 (Raspberry Pi 2) uses two levels of interrupt handling
with the CPU-local interrupts being the root, so we need to register
ours as chained off of the CPU's local interrupt.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../brcm,bcm2835-armctrl-ic.txt                    | 22 ++++++++++++++++++++++
 drivers/irqchip/irq-bcm2835.c                      | 20 ++++++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
index 7da578d..8363bc4 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
@@ -5,6 +5,10 @@ The BCM2835 contains a custom top-level interrupt controller, which supports
 controller, or the HW block containing it, is referred to occasionally
 as "armctrl" in the SoC documentation, hence naming of this binding.
 
+The BCM2836 contains the same interrupt controller with the same
+interrupts, but the per-CPU interrupt controller is the root, and an
+interrupt there indicates that the ARMCTRL has an interrupt to handle.
+
 Required properties:
 
 - compatible : should be "brcm,bcm2835-armctrl-ic"
@@ -20,6 +24,12 @@ Required properties:
   The 2nd cell contains the interrupt number within the bank. Valid values
   are 0..7 for bank 0, and 0..31 for bank 1.
 
+Optional properties:
+- interrupt-parent : Specifies the parent interrupt controller when this
+  controller is the second level.
+- interrupts : Specifies the interrupt on the parent for this interrupt
+  controller to handle.
+
 The interrupt sources are as follows:
 
 Bank 0:
@@ -102,9 +112,21 @@ Bank 2:
 
 Example:
 
+/* BCM2835, first level */
+intc: interrupt-controller {
+	compatible = "brcm,bcm2835-armctrl-ic";
+	reg = <0x7e00b200 0x200>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+};
+
+/* BCM2836, second level */
 intc: interrupt-controller {
 	compatible = "brcm,bcm2835-armctrl-ic";
 	reg = <0x7e00b200 0x200>;
 	interrupt-controller;
 	#interrupt-cells = <2>;
+
+	interrupt-parent = <&local_intc>;
+	interrupts = <8>;
 };
diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
index 382450a..dc6b159 100644
--- a/drivers/irqchip/irq-bcm2835.c
+++ b/drivers/irqchip/irq-bcm2835.c
@@ -97,6 +97,7 @@ struct armctrl_ic {
 static struct armctrl_ic intc __read_mostly;
 static void __exception_irq_entry bcm2835_handle_irq(
 	struct pt_regs *regs);
+static void bcm2835_chained_handle_irq(unsigned int irq, struct irq_desc *desc);
 
 static void armctrl_mask_irq(struct irq_data *d)
 {
@@ -143,7 +144,7 @@ static int __init armctrl_of_init(struct device_node *node,
 	struct device_node *parent)
 {
 	void __iomem *base;
-	int irq, b, i;
+	int irq, parent_irq, b, i;
 
 	base = of_iomap(node, 0);
 	if (!base)
@@ -169,7 +170,14 @@ static int __init armctrl_of_init(struct device_node *node,
 		}
 	}
 
-	set_handle_irq(bcm2835_handle_irq);
+	parent_irq = irq_of_parse_and_map(node, 0);
+	if (!parent_irq) {
+		/* No parent IRQ, so we're the root interrupt controller */
+		set_handle_irq(bcm2835_handle_irq);
+	} else {
+		irq_set_chained_handler(parent_irq, bcm2835_chained_handle_irq);
+	}
+
 	return 0;
 }
 
@@ -220,4 +228,12 @@ static void __exception_irq_entry bcm2835_handle_irq(
 		handle_IRQ(irq_linear_revmap(intc.domain, hwirq), regs);
 }
 
+static void bcm2835_chained_handle_irq(unsigned int irq, struct irq_desc *desc)
+{
+	u32 hwirq;
+
+	while ((hwirq = get_next_armctrl_hwirq()) != ~0)
+		generic_handle_irq(irq_linear_revmap(intc.domain, hwirq));
+}
+
 IRQCHIP_DECLARE(bcm2835_armctrl_ic, "brcm,bcm2835-armctrl-ic", armctrl_of_init);
-- 
2.1.4

[PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.

[Eric Anholt]

This is a new per-cpu root interrupt controller on the Raspberry Pi 2,
which will chain to the bcm2835 interrupt controller for peripheral
interrupts.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../interrupt-controller/brcm,bcm2836-l1-intc.txt  | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
new file mode 100644
index 0000000..f320dcd
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
@@ -0,0 +1,37 @@
+BCM2836 per-CPU interrupt controller
+
+The BCM2836 has a per-cpu interrupt controller for the timer, PMU
+events, and SMP IPIs.  One of the CPUs may receive interrupts for the
+peripheral (GPU) events, which chain to the BCM2835-style interrupt
+controller.
+
+Required properties:
+
+- compatible:	 	Should be "brcm,bcm2836-l1-intc"
+- reg:			Specifies base physical address and size of the
+			  registers
+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			  interrupt source. The value shall be 1
+
+Please refer to interrupts.txt in this directory for details of the common
+Interrupt Controllers bindings used by client devices.
+
+The interrupt sources are as follows:
+
+0: CNTPSIRQ
+1: CNTPNSIRQ
+2: CNTHPIRQ
+3: CNTVIRQ
+8: GPU_FAST
+9: PMU_FAST
+
+Example:
+
+local_intc: local_intc {
+	compatible = "brcm,bcm2836-l1-intc";
+	reg = <0x40000000 0x100>;
+	interrupt-controller;
+	#interrupt-cells = <1>;
+	interrupt-parent = <&local_intc>;
+};
-- 
2.1.4

[PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.

[Stephen Warren]

On 07/07/2015 03:13 PM, Eric Anholt wrote:
> This is a new per-cpu root interrupt controller on the Raspberry Pi 2,
> which will chain to the bcm2835 interrupt controller for peripheral
> interrupts.

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt

> +local_intc: local_intc {

> +	interrupt-parent = <&local_intc>;

I think that property shouldn't be there?

[PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>> This is a new per-cpu root interrupt controller on the Raspberry Pi 2,
>> which will chain to the bcm2835 interrupt controller for peripheral
>> interrupts.
>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>
>> +local_intc: local_intc {
>
>> +	interrupt-parent = <&local_intc>;
>
> I think that property shouldn't be there?

If you don't have it there, the core finds the interrupt-parent in the
parent node, and waits for that one before initializing (which is in
turn waiting for us).  Note that for original 2835, you're finding the
parent node as well.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150710/89a22ce9/attachment.sig>

[PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.

[Stephen Warren]

On 07/11/2015 12:01 AM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>>> This is a new per-cpu root interrupt controller on the
>>> Raspberry Pi 2, which will chain to the bcm2835 interrupt
>>> controller for peripheral interrupts.
>> 
>>> diff --git
>>> a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>>> b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>>
>>>
>>> 
+local_intc: local_intc {
>> 
>>> +	interrupt-parent = <&local_intc>;
>> 
>> I think that property shouldn't be there?
> 
> If you don't have it there, the core finds the interrupt-parent in
> the parent node, and waits for that one before initializing (which
> is in turn waiting for us).  Note that for original 2835, you're
> finding the parent node as well.

Ah yes. It does indeed look like it's typical that the root IRQ
controller points at itself.

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Eric Anholt]

This interrupt controller is the new root interrupt controller with
the timer, PMU events, and IPIs, and the bcm2835's interrupt
controller is chained off of it to handle the peripherals.

SMP IPI support was mostly written by Andrea Merello, while I wrote
most of the rest of the IRQ handling.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/irqchip/Makefile      |   1 +
 drivers/irqchip/irq-bcm2836.c | 200 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+)
 create mode 100644 drivers/irqchip/irq-bcm2836.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b8d4e96..9727681 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_IRQCHIP)			+= irqchip.o
 
 obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2835.o
+obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
 obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
 obj-$(CONFIG_ARCH_HIP04)		+= irq-hip04.o
 obj-$(CONFIG_ARCH_MMP)			+= irq-mmp.o
diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
new file mode 100644
index 0000000..cdb545f
--- /dev/null
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -0,0 +1,200 @@
+/*
+ * Root interrupt controller for the BCM2836 (Raspberry Pi 2).
+ *
+ * Copyright 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <asm/exception.h>
+
+#define LOCAL_PM_ROUTING_SET		0x010
+#define LOCAL_PM_ROUTING_CLR		0x014
+#define LOCAL_TIMER_INT_CONTROL0	0x040
+#define LOCAL_MAILBOX_INT_CONTROL0	0x050
+#define LOCAL_IRQ_PENDING0		0x060
+#define LOCAL_MAILBOX0_SET0		0x080
+#define LOCAL_MAILBOX0_CLR0		0x0c0
+
+#define LOCAL_IRQ_CNTPSIRQ	0
+#define LOCAL_IRQ_CNTPNSIRQ	1
+#define LOCAL_IRQ_CNTHPIRQ	2
+#define LOCAL_IRQ_CNTVIRQ	3
+#define LOCAL_IRQ_MAILBOX0	4
+#define LOCAL_IRQ_MAILBOX1	5
+#define LOCAL_IRQ_MAILBOX2	6
+#define LOCAL_IRQ_MAILBOX3	7
+#define LOCAL_IRQ_GPU_FAST	8
+#define LOCAL_IRQ_PMU_FAST	9
+#define LAST_IRQ		LOCAL_IRQ_PMU_FAST
+
+struct arm_local_intc {
+	struct irq_domain *domain;
+	void __iomem *base;
+};
+
+static struct arm_local_intc intc  __read_mostly;
+
+static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
+{
+	void __iomem *reg_base = intc.base + reg;
+	unsigned int i;
+
+	for (i = 0; i < 4; i++)
+		writel(readl(reg_base + 4 * i) & ~BIT(bit), reg_base + 4 * i);
+}
+
+static void bcm2836_unmask_per_cpu_irq(unsigned int reg, unsigned int bit)
+{
+	void __iomem *reg_base = intc.base + reg;
+	unsigned int i;
+
+	for (i = 0; i < 4; i++)
+		writel(readl(reg_base + 4 * i) | BIT(bit), reg_base + 4 * i);
+}
+
+static void bcm2836_mask_timer_irq(struct irq_data *d)
+{
+	bcm2836_mask_per_cpu_irq(LOCAL_TIMER_INT_CONTROL0,
+				 d->hwirq - LOCAL_IRQ_CNTPSIRQ);
+}
+
+static void bcm2836_unmask_timer_irq(struct irq_data *d)
+{
+	bcm2836_unmask_per_cpu_irq(LOCAL_TIMER_INT_CONTROL0,
+				   d->hwirq - LOCAL_IRQ_CNTPSIRQ);
+}
+
+static struct irq_chip bcm2836_timer_irqchip = {
+	.name = "bcm2836-timer",
+	.irq_mask = bcm2836_mask_timer_irq,
+	.irq_unmask = bcm2836_unmask_timer_irq
+};
+
+static void bcm2836_mask_pmu_irq(struct irq_data *d)
+{
+	writel(0xf, intc.base + LOCAL_PM_ROUTING_CLR);
+}
+
+static void bcm2836_unmask_pmu_irq(struct irq_data *d)
+{
+	writel(0xf, intc.base + LOCAL_PM_ROUTING_SET);
+}
+
+static struct irq_chip bcm2836_pmu_irqchip = {
+	.name = "bcm2836-pmu",
+	.irq_mask = bcm2836_mask_pmu_irq,
+	.irq_unmask = bcm2836_unmask_pmu_irq
+};
+
+static void bcm2836_mask_gpu_irq(struct irq_data *d)
+{
+}
+
+static void bcm2836_unmask_gpu_irq(struct irq_data *d)
+{
+}
+
+static struct irq_chip bcm2836_gpu_irqchip = {
+	.name = "bcm2836-gpu",
+	.irq_mask = bcm2836_mask_gpu_irq,
+	.irq_unmask = bcm2836_unmask_gpu_irq
+};
+
+static void bcm2836_register_irq(int hwirq, struct irq_chip *chip)
+{
+	int irq = irq_create_mapping(intc.domain, hwirq);
+
+	irq_set_percpu_devid(irq);
+	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+}
+
+static void __exception_irq_entry bcm2836_handle_irq(struct pt_regs *regs)
+{
+	int cpu = smp_processor_id();
+	u32 stat;
+
+	stat = readl_relaxed(intc.base + LOCAL_IRQ_PENDING0 + 4 * cpu);
+	if (stat & 0x10) {
+		void __iomem *mailbox0 = (intc.base +
+					  LOCAL_MAILBOX0_CLR0 + 16 * cpu);
+		u32 mbox_val = readl(mailbox0);
+		u32 ipi = ffs(mbox_val) - 1;
+
+		writel(1 << ipi, mailbox0);
+		handle_IPI(ipi, regs);
+	} else {
+		u32 hwirq = ffs(stat) - 1;
+
+		handle_IRQ(irq_linear_revmap(intc.domain, hwirq), regs);
+	}
+}
+
+#ifdef CONFIG_SMP
+static void bcm2836_send_ipi(const struct cpumask *mask, unsigned int ipi)
+{
+	int cpu;
+	void __iomem *mailbox0_base = intc.base + LOCAL_MAILBOX0_SET0;
+
+	/*
+	 * Ensure that stores to normal memory are visible to the
+	 * other CPUs before issuing the IPI.
+	 */
+	dsb();
+
+	for_each_cpu(cpu, mask)	{
+		writel(1 << ipi, mailbox0_base + 16 * cpu);
+	}
+}
+#endif
+
+static const struct irq_domain_ops bcm2836_intc_ops = {
+	.xlate = irq_domain_xlate_onecell
+};
+
+static int __init bcm2836_l1_intc_of_init(struct device_node *node,
+					  struct device_node *parent)
+{
+	intc.base = of_iomap(node, 0);
+	if (!intc.base) {
+		panic("%s: unable to map local interrupt registers\n",
+			node->full_name);
+	}
+
+	intc.domain = irq_domain_add_linear(node, LAST_IRQ + 1,
+					    &bcm2836_intc_ops, NULL);
+	if (!intc.domain)
+		panic("%s: unable to create IRQ domain\n", node->full_name);
+
+	bcm2836_register_irq(LOCAL_IRQ_CNTPSIRQ, &bcm2836_timer_irqchip);
+	bcm2836_register_irq(LOCAL_IRQ_CNTPNSIRQ, &bcm2836_timer_irqchip);
+	bcm2836_register_irq(LOCAL_IRQ_CNTHPIRQ, &bcm2836_timer_irqchip);
+	bcm2836_register_irq(LOCAL_IRQ_CNTVIRQ, &bcm2836_timer_irqchip);
+	bcm2836_register_irq(LOCAL_IRQ_GPU_FAST, &bcm2836_gpu_irqchip);
+	bcm2836_register_irq(LOCAL_IRQ_PMU_FAST, &bcm2836_pmu_irqchip);
+
+#ifdef CONFIG_SMP
+	/* unmask IPIs */
+	bcm2836_unmask_per_cpu_irq(LOCAL_MAILBOX_INT_CONTROL0, 0);
+	set_smp_cross_call(bcm2836_send_ipi);
+#endif
+
+	set_handle_irq(bcm2836_handle_irq);
+	return 0;
+}
+
+IRQCHIP_DECLARE(bcm2836_l1_intc, "brcm,bcm2836-l1-intc",
+		bcm2836_l1_intc_of_init);
-- 
2.1.4

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Stephen Warren]

On 07/07/2015 03:13 PM, Eric Anholt wrote:
> This interrupt controller is the new root interrupt controller with
> the timer, PMU events, and IPIs, and the bcm2835's interrupt
> controller is chained off of it to handle the peripherals.
> 
> SMP IPI support was mostly written by Andrea Merello, while I wrote
> most of the rest of the IRQ handling.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>

I'd expect the git patch author to be Andrea if he wrote the original
patch and you enhanced it.

> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c

> +struct arm_local_intc {
> +	struct irq_domain *domain;
> +	void __iomem *base;
> +};
> +
> +static struct arm_local_intc intc  __read_mostly;

It'd be nice to give everything (types, functions, variables) a
consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
bikeshed to me, but perhaps just propagating the above arm_local_ to the
functions too would be good, although that seems to risk symbol name
collisions with other ARM SoCs.

> +static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
> +{
> +	void __iomem *reg_base = intc.base + reg;
> +	unsigned int i;
> +
> +	for (i = 0; i < 4; i++)

Is "4" there the CPU count? Perhaps this should use one of the Linux
APIs to query the CPU count rather than hard-coding it?

Should per-CPU IRQs automatically be masked on all CPUs at once, or only
on the current CPU? A very quick look@the ARM GIC driver implies it
doesn't iterate over all CPUs when masking per-CPU IRQs.

> +static void bcm2836_mask_gpu_irq(struct irq_data *d)
> +{
> +}
> +
> +static void bcm2836_unmask_gpu_irq(struct irq_data *d)
> +{
> +}

If the IRQs can't be masked, should these functions actually be implemented?

> +static void __exception_irq_entry bcm2836_handle_irq(struct pt_regs *regs)
> +{
> +	int cpu = smp_processor_id();
> +	u32 stat;
> +
> +	stat = readl_relaxed(intc.base + LOCAL_IRQ_PENDING0 + 4 * cpu);
> +	if (stat & 0x10) {
> +		void __iomem *mailbox0 = (intc.base +
> +					  LOCAL_MAILBOX0_CLR0 + 16 * cpu);
> +		u32 mbox_val = readl(mailbox0);
> +		u32 ipi = ffs(mbox_val) - 1;
> +
> +		writel(1 << ipi, mailbox0);
> +		handle_IPI(ipi, regs);

Given that bcm2836_send_ipi() is #ifdef CONFIG_SMP, should this code be too?

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Thomas Gleixner]

On Fri, 10 Jul 2015, Stephen Warren wrote:
> On 07/07/2015 03:13 PM, Eric Anholt wrote:
> > +static struct arm_local_intc intc  __read_mostly;
> 
> It'd be nice to give everything (types, functions, variables) a
> consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
> bikeshed to me, but perhaps just propagating the above arm_local_ to the
> functions too would be good, although that seems to risk symbol name
> collisions with other ARM SoCs.

Which is irrelevant because its static.

> > +static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
> > +{
> > +	void __iomem *reg_base = intc.base + reg;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < 4; i++)
> 
> Is "4" there the CPU count? Perhaps this should use one of the Linux
> APIs to query the CPU count rather than hard-coding it?
> 
> Should per-CPU IRQs automatically be masked on all CPUs at once, or only
> on the current CPU? A very quick look at the ARM GIC driver implies it
> doesn't iterate over all CPUs when masking per-CPU IRQs.

Usually per cpu interrupts are only masked on the cpu which is calling
the function. The whole reason why per cpu interrupts exist is that
you can share the same interrupt number for all cores.

So masking all interrupts is not a good idea. In this case if a cpu is
hot unplugged, then all other cpus would not longer get timer
interrupts. Not what you really want, right?
 
> > +static void bcm2836_mask_gpu_irq(struct irq_data *d)
> > +{
> > +}
> > +
> > +static void bcm2836_unmask_gpu_irq(struct irq_data *d)
> > +{
> > +}
> 
> If the IRQs can't be masked, should these functions actually be implemented?

We have a few places in the core which expect at least mask/unmask to
be implemented.
 
Thanks,

	tglx

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Eric Anholt]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Fri, 10 Jul 2015, Stephen Warren wrote:
>> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>> > +static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
>> > +{
>> > +	void __iomem *reg_base = intc.base + reg;
>> > +	unsigned int i;
>> > +
>> > +	for (i = 0; i < 4; i++)
>> 
>> Is "4" there the CPU count? Perhaps this should use one of the Linux
>> APIs to query the CPU count rather than hard-coding it?
>> 
>> Should per-CPU IRQs automatically be masked on all CPUs at once, or only
>> on the current CPU? A very quick look at the ARM GIC driver implies it
>> doesn't iterate over all CPUs when masking per-CPU IRQs.
>
> Usually per cpu interrupts are only masked on the cpu which is calling
> the function. The whole reason why per cpu interrupts exist is that
> you can share the same interrupt number for all cores.
>
> So masking all interrupts is not a good idea. In this case if a cpu is
> hot unplugged, then all other cpus would not longer get timer
> interrupts. Not what you really want, right?

I was replicating the behavior of the downstream driver, but it seemed
suspicious.  Converting to using smp_processor_id() to just mask/unmask
this CPU's interrupts seems to have gone fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150713/4eca8e6e/attachment.sig>

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Stephen Warren]

On 07/11/2015 01:51 AM, Thomas Gleixner wrote:
> On Fri, 10 Jul 2015, Stephen Warren wrote:
>> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>>> +static struct arm_local_intc intc  __read_mostly;
>>
>> It'd be nice to give everything (types, functions, variables) a
>> consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
>> bikeshed to me, but perhaps just propagating the above arm_local_ to the
>> functions too would be good, although that seems to risk symbol name
>> collisions with other ARM SoCs.
> 
> Which is irrelevant because its static.

Except if you want to look up a symbol name without having to qualify it
by filename.

Or, does e.g. gdb require statics always be qualified even if they're
globally unique?

[PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>> This interrupt controller is the new root interrupt controller with
>> the timer, PMU events, and IPIs, and the bcm2835's interrupt
>> controller is chained off of it to handle the peripherals.
>> 
>> SMP IPI support was mostly written by Andrea Merello, while I wrote
>> most of the rest of the IRQ handling.
>> 
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> I'd expect the git patch author to be Andrea if he wrote the original
> patch and you enhanced it.

I wrote the IRQs patch, and Andrea added the IPI bits to it.

>> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
>
>> +struct arm_local_intc {
>> +	struct irq_domain *domain;
>> +	void __iomem *base;
>> +};
>> +
>> +static struct arm_local_intc intc  __read_mostly;
>
> It'd be nice to give everything (types, functions, variables) a
> consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
> bikeshed to me, but perhaps just propagating the above arm_local_ to the
> functions too would be good, although that seems to risk symbol name
> collisions with other ARM SoCs.

Done.

>> +static void bcm2836_mask_gpu_irq(struct irq_data *d)
>> +{
>> +}
>> +
>> +static void bcm2836_unmask_gpu_irq(struct irq_data *d)
>> +{
>> +}
>
> If the IRQs can't be masked, should these functions actually be implemented?

They are called unconditionally at IRQ enable time.  I don't see a way
to mask them.

>> +static void __exception_irq_entry bcm2836_handle_irq(struct pt_regs *regs)
>> +{
>> +	int cpu = smp_processor_id();
>> +	u32 stat;
>> +
>> +	stat = readl_relaxed(intc.base + LOCAL_IRQ_PENDING0 + 4 * cpu);
>> +	if (stat & 0x10) {
>> +		void __iomem *mailbox0 = (intc.base +
>> +					  LOCAL_MAILBOX0_CLR0 + 16 * cpu);
>> +		u32 mbox_val = readl(mailbox0);
>> +		u32 ipi = ffs(mbox_val) - 1;
>> +
>> +		writel(1 << ipi, mailbox0);
>> +		handle_IPI(ipi, regs);
>
> Given that bcm2836_send_ipi() is #ifdef CONFIG_SMP, should this code be too?

Sure, done.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150713/4d75b89e/attachment.sig>