Add support for BCM7271 style interrupt controller

PATCH In review Doug Berger 2 participants 5 messages Sep 19, 2017

Subsystems (5)

Device tree bindings MIPS irqchip drivers Broadcom BCM7XXX Arm architecture +1 more subsystems

lore

Share & tools

lore

Doug Berger

author

2017-09-19 00:59 UTC #0 lore raw

v4 cover Add support for BCM7271 style interrupt controller

This patch set extends the functionality of the irq-brcmstb-l2 interrupt controller driver to cover a hardware variant first introduced in the BCM7271 SoC. The main difference between this variant and the block found in earlier brcmstb SoCs is that this variant only supports level sensitive interrupts and therefore does not latch the interrupt state based on edges. Since there is no longer a need to ack interrupts with a register write to clear the latch the register map has been changed.

Therefore the change to add support for the new hardware block is to abstract the register accesses to accommodate different maps and to identify the block with a new device-tree compatible string.

I also took the opportunity to make some small efficiency enhancements to the driver. One of these was to make use of the slightly more efficient irq_mask_ack method. However, I discovered that the defined irq_gc_mask_disable_reg_and_ack() generic irq function was insufficient for my needs. Previous submissions offered candidate solutions to address my needs within the generic irqchip library, but since those submissions appear to have stalled I am submitting this version that includes the function in the driver to prevent controversy and allow the new functionality to be included.

Changes in v4:

- The first three commits were removed from the patch set to remove any dependencies on changing the generic irqchip or irqchip-tango imple- mentations. If there is a will to make those changes in the future they can be applied at that time, but they needn't hold up the accept- ance of this patch set.

Changes in v3:

- I did not submit a v3 patch set, but Marc Gonzalez included a PATCH v3 in a response to the v2 patch so I am skipping ahead to v4 to avoid confusion.

Changes in v2:

- removed unused permutations of irq_mask_ack methods
- added Reviewed-by and Acked-by responses from first submission

Doug Berger (3):
irqchip: brcmstb-l2: Remove some processing from the handler
irqchip: brcmstb-l2: Abstract register accesses
irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

 .../bindings/interrupt-controller/brcm,l2-intc.txt |   3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 171 +++++++++++++++------
 2 files changed, 126 insertions(+), 48 deletions(-)

Signature · 1 line

2.14.1

Doug Berger

author

2017-09-19 00:59 UTC #1 lore raw

PATCH v4 1/3 irqchip: brcmstb-l2: Remove some processing from the handler

Saving the generic chip pointer in the brcmstb_l2_intc_data prevents the need to call irq_get_domain_generic_chip(). Also don't need to save parent_irq and base there since local variables in the brcmstb_l2_intc_of_init() function are just as good.

The handle_edge_irq flow or chained_irq_enter takes care of the acknowledgment of the interrupt so it is redundant to clear it in brcmstb_l2_intc_irq_handle().

irq_linear_revmap() is a fast path equivalent of irq_find_mapping() that is appropriate to use for domain controllers of this type.

Defining irq_mask_ack is slightly more efficient than just implementing irq_mask and irq_ack separately.

Signed-off-by Doug Berger <[email protected]>

---
 drivers/irqchip/irq-brcmstb-l2.c | 72 ++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 24 deletions(-)

drivers/irqchip/irq-brcmstb-l2.c +48 ~~-24~~

index b009b916a292..48bd1a36c7d4 100644
@@ -1,7 +1,7 @@
 /*
  * Generic Broadcom Set Top Box Level 2 Interrupt controller driver
  *
- * Copyright (C) 2014 Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -41,25 +41,49 @@
 
 /* L2 intc private data structure */
 struct brcmstb_l2_intc_data {
-	int parent_irq;
-	void __iomem *base;
 	struct irq_domain *domain;
+	struct irq_chip_generic *gc;
 	bool can_wake;
 	u32 saved_mask; /* for suspend/resume */
 };
 
+/**
+ * brcmstb_l2_mask_and_ack - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has separate enable/disable registers instead of a single mask
+ * register and pending interrupt is acknowledged by setting a bit.
+ *
+ * Note: This function is generic and could easily be added to the
+ * generic irqchip implementation if there ever becomes a will to do so.
+ * Perhaps with a name like irq_gc_mask_disable_and_ack_set().
+ *
+ * e.g.: https://patchwork.kernel.org/patch/9831047/
+ */
+static void brcmstb_l2_mask_and_ack(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	irq_reg_writel(gc, mask, ct->regs.disable);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
 static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 {
 	struct brcmstb_l2_intc_data *b = irq_desc_get_handler_data(desc);
-	struct irq_chip_generic *gc = irq_get_domain_generic_chip(b->domain, 0);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned int irq;
 	u32 status;
 
 	chained_irq_enter(chip, desc);
 
-	status = irq_reg_readl(gc, CPU_STATUS) &
-		~(irq_reg_readl(gc, CPU_MASK_STATUS));
+	status = irq_reg_readl(b->gc, CPU_STATUS) &
+		~(irq_reg_readl(b->gc, CPU_MASK_STATUS));
 
 	if (status == 0) {
 		raw_spin_lock(&desc->lock);

@@ -70,10 +94,8 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 
 	do {
 		irq = ffs(status) - 1;
-		/* ack at our level */
-		irq_reg_writel(gc, 1 << irq, CPU_CLEAR);
 		status &= ~(1 << irq);
-		generic_handle_irq(irq_find_mapping(b->domain, irq));
+		generic_handle_irq(irq_linear_revmap(b->domain, irq));
 	} while (status);
 out:
 	chained_irq_exit(chip, desc);

@@ -116,32 +138,33 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 {
 	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
 	struct brcmstb_l2_intc_data *data;
-	struct irq_chip_generic *gc;
 	struct irq_chip_type *ct;
 	int ret;
 	unsigned int flags;
+	int parent_irq;
+	void __iomem *base;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->base = of_iomap(np, 0);
-	if (!data->base) {
+	base = of_iomap(np, 0);
+	if (!base) {
 		pr_err("failed to remap intc L2 registers\n");
 		ret = -ENOMEM;
 		goto out_free;
 	}
 
 	/* Disable all interrupts by default */
-	writel(0xffffffff, data->base + CPU_MASK_SET);
+	writel(0xffffffff, base + CPU_MASK_SET);
 
 	/* Wakeup interrupts may be retained from S5 (cold boot) */
 	data->can_wake = of_property_read_bool(np, "brcm,irq-can-wake");
 	if (!data->can_wake)
-		writel(0xffffffff, data->base + CPU_CLEAR);
+		writel(0xffffffff, base + CPU_CLEAR);
 
-	data->parent_irq = irq_of_parse_and_map(np, 0);
-	if (!data->parent_irq) {
+	parent_irq = irq_of_parse_and_map(np, 0);
+	if (!parent_irq) {
 		pr_err("failed to find parent interrupt\n");
 		ret = -EINVAL;
 		goto out_unmap;

@@ -170,18 +193,19 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	}
 
 	/* Set the IRQ chaining logic */
-	irq_set_chained_handler_and_data(data->parent_irq,
+	irq_set_chained_handler_and_data(parent_irq,
 					 brcmstb_l2_intc_irq_handle, data);
 
-	gc = irq_get_domain_generic_chip(data->domain, 0);
-	gc->reg_base = data->base;
-	gc->private = data;
-	ct = gc->chip_types;
+	data->gc = irq_get_domain_generic_chip(data->domain, 0);
+	data->gc->reg_base = base;
+	data->gc->private = data;
+	ct = data->gc->chip_types;
 
 	ct->chip.irq_ack = irq_gc_ack_set_bit;
 	ct->regs.ack = CPU_CLEAR;
 
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
+	ct->chip.irq_mask_ack = brcmstb_l2_mask_and_ack;
 	ct->regs.disable = CPU_MASK_SET;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;

@@ -195,19 +219,19 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 		/* This IRQ chip can wake the system, set all child interrupts
 		 * in wake_enabled mask
 		 */
-		gc->wake_enabled = 0xffffffff;
+		data->gc->wake_enabled = 0xffffffff;
 		ct->chip.irq_set_wake = irq_gc_set_wake;
 	}
 
 	pr_info("registered L2 intc (mem: 0x%p, parent irq: %d)\n",
-			data->base, data->parent_irq);
+			base, parent_irq);
 
 	return 0;
 
 out_free_domain:
 	irq_domain_remove(data->domain);
 out_unmap:
-	iounmap(data->base);
+	iounmap(base);
 out_free:
 	kfree(data);
 	return ret;

Signature · 1 line

2.14.1

Doug Berger

author

2017-09-19 00:59 UTC #2 lore raw

PATCH v4 2/3 irqchip: brcmstb-l2: Abstract register accesses

In reply to Doug Berger

Added register block offsets to the brcmstb_l2_intc_data structure for the status and mask registers to support reading the active interupts in an abstracted way. It seems like an irq_chip method should have been provided for this, but it's not there yet.

Abstracted the implementation of the handler, suspend, and resume functions to not use any hard coded register offsets.

Reviewed-by Florian Fainelli <[email protected]>

Signed-off-by Doug Berger <[email protected]>

---
 drivers/irqchip/irq-brcmstb-l2.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

drivers/irqchip/irq-brcmstb-l2.c +20 -9

index 48bd1a36c7d4..8d54cd7a090d 100644
@@ -43,6 +43,8 @@
 struct brcmstb_l2_intc_data {
 	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
+	int status_offset;
+	int mask_offset;
 	bool can_wake;
 	u32 saved_mask; /* for suspend/resume */
 };

@@ -82,8 +84,8 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 
-	status = irq_reg_readl(b->gc, CPU_STATUS) &
-		~(irq_reg_readl(b->gc, CPU_MASK_STATUS));
+	status = irq_reg_readl(b->gc, b->status_offset) &
+		~(irq_reg_readl(b->gc, b->mask_offset));
 
 	if (status == 0) {
 		raw_spin_lock(&desc->lock);

@@ -104,16 +106,17 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 static void brcmstb_l2_intc_suspend(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
 	/* Save the current mask */
-	b->saved_mask = irq_reg_readl(gc, CPU_MASK_STATUS);
+	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
 
 	if (b->can_wake) {
 		/* Program the wakeup mask */
-		irq_reg_writel(gc, ~gc->wake_active, CPU_MASK_SET);
-		irq_reg_writel(gc, gc->wake_active, CPU_MASK_CLEAR);
+		irq_reg_writel(gc, ~gc->wake_active, ct->regs.disable);
+		irq_reg_writel(gc, gc->wake_active, ct->regs.enable);
 	}
 	irq_gc_unlock(gc);
 }

@@ -121,15 +124,19 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
 static void brcmstb_l2_intc_resume(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
-	/* Clear unmasked non-wakeup interrupts */
-	irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active, CPU_CLEAR);
+	if (ct->chip.irq_ack != irq_gc_noop) {
+		/* Clear unmasked non-wakeup interrupts */
+		irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active,
+				ct->regs.ack);
+	}
 
 	/* Restore the saved mask */
-	irq_reg_writel(gc, b->saved_mask, CPU_MASK_SET);
-	irq_reg_writel(gc, ~b->saved_mask, CPU_MASK_CLEAR);
+	irq_reg_writel(gc, b->saved_mask, ct->regs.disable);
+	irq_reg_writel(gc, ~b->saved_mask, ct->regs.enable);
 	irq_gc_unlock(gc);
 }

@@ -199,6 +206,9 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	data->gc = irq_get_domain_generic_chip(data->domain, 0);
 	data->gc->reg_base = base;
 	data->gc->private = data;
+	data->status_offset = CPU_STATUS;
+	data->mask_offset = CPU_MASK_STATUS;
+
 	ct = data->gc->chip_types;
 
 	ct->chip.irq_ack = irq_gc_ack_set_bit;

@@ -207,6 +217,7 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
 	ct->chip.irq_mask_ack = brcmstb_l2_mask_and_ack;
 	ct->regs.disable = CPU_MASK_SET;
+	ct->regs.mask = CPU_MASK_STATUS;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
 	ct->regs.enable = CPU_MASK_CLEAR;

Signature · 1 line

2.14.1

Doug Berger

author

2017-09-19 01:00 UTC #3 lore raw

PATCH v4 3/3 irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

In reply to Doug Berger

Add the initialization of the generic irq chip for the BCM7271 L2 interrupt controller. This controller only supports level interrupts and uses the "brcm,bcm7271-l2-intc" compatibility string.

Acked-by Rob Herring <[email protected]>

Signed-off-by Doug Berger <[email protected]>

---
 .../bindings/interrupt-controller/brcm,l2-intc.txt |  3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 86 ++++++++++++++++------
 2 files changed, 66 insertions(+), 23 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt +2 -1

index 448273a30a11..36df06c5c567 100644
@@ -2,7 +2,8 @@ Broadcom Generic Level 2 Interrupt Controller
 
 Required properties:
 
-- compatible: should be "brcm,l2-intc"
+- compatible: should be "brcm,l2-intc" for latched interrupt controllers
+              should be "brcm,bcm7271-l2-intc" for level interrupt controllers
 - reg: specifies the 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

drivers/irqchip/irq-brcmstb-l2.c +64 ~~-22~~

135 lines not on this page · raw message

Signature · 1 line

2.14.1

Florian Fainelli

Reviewed-by on the v4 series

2017-09-19 18:05 UTC #4 lore raw

In reply to Doug Berger

Quoted 12 lines · Doug Berger

This patch set extends the functionality of the irq-brcmstb-l2 interrupt controller driver to cover a hardware variant first introduced in the BCM7271 SoC. The main difference between this variant and the block found in earlier brcmstb SoCs is that this variant only supports level sensitive interrupts and therefore does not latch the interrupt state based on edges. Since there is no longer a need to ack interrupts with a register write to clear the latch the register map has been changed.
Therefore the change to add support for the new hardware block is to abstract the register accesses to accommodate different maps and to identify the block with a new device-tree compatible string.

Quoted 9 lines

I also took the opportunity to make some small efficiency enhancements to the driver. One of these was to make use of the slightly more efficient irq_mask_ack method. However, I discovered that the defined irq_gc_mask_disable_reg_and_ack() generic irq function was insufficient for my needs. Previous submissions offered candidate solutions to address my needs within the generic irqchip library, but since those submissions appear to have stalled I am submitting this version that includes the function in the driver to prevent controversy and allow the new functionality to be included.

For this entire series:

Reviewed-by Florian Fainelli <[email protected]>

Thanks Doug.

Quoted 29 lines

Changes in v4:
- The first three commits were removed from the patch set to remove any dependencies on changing the generic irqchip or irqchip-tango imple- mentations. If there is a will to make those changes in the future they can be applied at that time, but they needn't hold up the accept- ance of this patch set.
Changes in v3:
- I did not submit a v3 patch set, but Marc Gonzalez included a PATCH v3 in a response to the v2 patch so I am skipping ahead to v4 to avoid confusion.
Changes in v2:
- removed unused permutations of irq_mask_ack methods
- added Reviewed-by and Acked-by responses from first submission
Doug Berger (3):
irqchip: brcmstb-l2: Remove some processing from the handler
irqchip: brcmstb-l2: Abstract register accesses
irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller
 .../bindings/interrupt-controller/brcm,l2-intc.txt |   3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 171 +++++++++++++++------
 2 files changed, 126 insertions(+), 48 deletions(-)

Signature · 1 line

Florian