Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734587 > unrolled thread

[PATCH v4 0/3] Add support for BCM7271 style interrupt controller

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] Add support for BCM7271 style interrupt controller Doug Berger <opendmb@gmail.com> - 2017-09-19 03:10 +0200
    [PATCH v4 1/3] irqchip: brcmstb-l2: Remove some processing from the handler Doug Berger <opendmb@gmail.com> - 2017-09-19 03:10 +0200
    Re: [PATCH v4 0/3] Add support for BCM7271 style interrupt controller Florian Fainelli <f.fainelli@gmail.com> - 2017-09-19 20:10 +0200

#1734587 — [PATCH v4 0/3] 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(-)

-- 
2.14.1

[toc] | [next] | [standalone]

#1734588 — [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 <opendmb@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 72 ++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index b009b916a292..48bd1a36c7d4 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -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;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1735141

On 09/18/2017 05:59 PM, Doug Berger wrote:
> 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. 

For this entire series:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks Doug.

> 
> 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(-)
> 


-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web