irqchip/tango: Use irq_gc_mask_disable_and_ack_set

The irq_gc_mask_disable_reg_and_ack() function name implies that it provides the combined functions of irq_gc_mask_disable_reg() and irq_gc_ack(). However, the implementation does not actually do that since it writes the mask instead of the disable register. It also does not maintain the mask cache which makes it inappropriate to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to irq_gc_set_bit() so this function probably should have also been renamed at that time.

Since this generic chip code provides three mask functions and two ack functions, this commit provides generic implementations for all six combinations of the mask and ack functions suitable for use with the irq_mask_ack member of the struct irq_chip.

Quoted 12 lines · Doug Berger

The irq_gc_mask_disable_reg_and_ack() function name implies that it provides the combined functions of irq_gc_mask_disable_reg() and irq_gc_ack(). However, the implementation does not actually do that since it writes the mask instead of the disable register. It also does not maintain the mask cache which makes it inappropriate to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to irq_gc_set_bit() so this function probably should have also been renamed at that time.

Quoted 4 lines

Since this generic chip code provides three mask functions and two ack functions, this commit provides generic implementations for all six combinations of the mask and ack functions suitable for use with the irq_mask_ack member of the struct irq_chip.

We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that needs exactly on function as replacement. Why do we need 6 variants of that right now?

Thanks,

tglx

Quoted 14 lines · Thomas Gleixner

Quoted 12 lines · Doug Berger

The irq_gc_mask_disable_reg_and_ack() function name implies that it provides the combined functions of irq_gc_mask_disable_reg() and irq_gc_ack(). However, the implementation does not actually do that since it writes the mask instead of the disable register. It also does not maintain the mask cache which makes it inappropriate to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to irq_gc_set_bit() so this function probably should have also been renamed at that time.

Quoted 8 lines

Quoted 4 lines

Since this generic chip code provides three mask functions and two ack functions, this commit provides generic implementations for all six combinations of the mask and ack functions suitable for use with the irq_mask_ack member of the struct irq_chip.

We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that needs exactly on function as replacement. Why do we need 6 variants of that right now?

This is merely a suggestion.

When I was originally adding support for the BCM7271 variant of the irq-brcmstb-l2 interrupt controller I noticed that providing an irq_mask_ack implementation would be slightly more efficient than only providing irq_mask and irq_ack. I assumed that it was philosophically better to use a generic chip implementation than creating yet another driver specific version of the method.

This task was originally done on a downstream development kernel derived from the v4.1 kernel and I'm finally taking the opportunity to attempt to upstream the change. At that time, I was drawn to the irq_gc_mask_disable_reg_and_ack() function based on the name, but I discovered that it was actually using the mask register rather than the disable register contrary to its name and hadn't been included in the changes when mask caching was added and when some similar functions were renamed. I considered submitting a patch to correct what I perceived as a bug, but after discovering there were no users of the function at that time I decided that it should probably be removed and replaced with the irq_gc_mask_disable_and_ack_set() function that I needed.

While preparing the upstream submission I discovered that the tango interrupt controller driver had apparently started using the potentially problematic function. I'm not comfortable making changes to drivers for devices that I'm not able to test (I'm still making mistakes with git send-email --cc-cmd ;) so Florian accepted authorship of that change.

I had perhaps incorrectly assumed that the irq_gc_mask_disable_reg_and_ack() function was originally included in the generic chip implementation nearly 5 years before its first use was to encourage driver developers to adopt generic chip implementations rather than implementing unique versions in every driver. To that end I'm suggesting offering all currently possible generic chip implementations of the irq_mask_ack method to encourage drivers to adopt use of generic chip methods even though I only need one of them.

Perhaps someone bolder than I may be inspired to undertake converting more irqchip drivers to use these methods.

If I am mistaken and this is an undesired change I am happy to implement an irq-brcmstb-l2 driver specific implementation of the irq_mask_ack method or just changing the single function.

Quoted 1 line

Quoted 4 lines

Thanks,

tglx

Thanks for your consideration and please let me know how you would like me to proceed with the submission.
Doug

Seeing as Mans has acked the change to his driver should I submit a V2 with just the function he and I are using and remove the other five permutations, or are you willing to move forward with the patch as is?

The irq_gc_mask_disable_reg_and_ack() function name implies that it provides the combined functions of irq_gc_mask_disable_reg() and irq_gc_ack(). However, the implementation does not actually do that since it writes the mask instead of the disable register. It also does not maintain the mask cache which makes it inappropriate to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to irq_gc_set_bit() so this function probably should have also been renamed at that time.

Since this generic chip code provides three mask functions and two ack functions, this commit provides generic implementations for all six combinations of the mask and ack functions suitable for use with the irq_mask_ack member of the struct irq_chip.

The '_reg' and '_bit' portions of the base function names were left out of the new combined function names in an attempt to keep the function name lengths manageable with the 80 character source code line length while still capturing the distinct aspects of each combination of functions.

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

---
 include/linux/irq.h       |   6 +++
 kernel/irq/generic-chip.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

include/linux/irq.h +6 -0

index 00db35b61e9e..23b9617bb682 100644
@@ -1003,6 +1003,12 @@ void irq_gc_unmask_enable_reg(struct irq_data *d);
 void irq_gc_ack_set_bit(struct irq_data *d);
 void irq_gc_ack_clr_bit(struct irq_data *d);
 void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
+void irq_gc_mask_disable_and_ack_set(struct irq_data *d);
+void irq_gc_mask_disable_and_ack_clr(struct irq_data *d);
+void irq_gc_mask_set_and_ack_set(struct irq_data *d);
+void irq_gc_mask_set_and_ack_clr(struct irq_data *d);
+void irq_gc_mask_clr_and_ack_set(struct irq_data *d);
+void irq_gc_mask_clr_and_ack_clr(struct irq_data *d);
 void irq_gc_eoi(struct irq_data *d);
 int irq_gc_set_wake(struct irq_data *d, unsigned int on);
 

kernel/irq/generic-chip.c +120 -0

index f7086b78ad6e..168887a81a29 100644
@@ -151,6 +151,126 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 }
 
 /**
+ * irq_gc_mask_disable_and_ack_set - 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.
+ */
+void irq_gc_mask_disable_and_ack_set(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);
+}
+
+/**
+ * irq_gc_mask_disable_and_ack_clr - 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 clearing a bit.
+ */
+void irq_gc_mask_disable_and_ack_clr(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);
+}
+
+/**
+ * irq_gc_mask_set_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where setting bits masks the interrupt
+ * and the pending interrupt is acknowledged by setting a bit.
+ */
+void irq_gc_mask_set_and_ack_set(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);
+	*ct->mask_cache |= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_set_and_ack_clr - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where setting bits masks the interrupt
+ * and the pending interrupt is acknowledged by clearing a bit.
+ */
+void irq_gc_mask_set_and_ack_clr(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);
+	*ct->mask_cache |= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, ~mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_clr_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where clearing bits masks the interrupt
+ * and the pending interrupt is acknowledged by setting a bit.
+ */
+void irq_gc_mask_clr_and_ack_set(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);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
+ * irq_gc_mask_clr_and_ack_clr - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * Chip has a single mask register where clearing bits masks the interrupt
+ * and the pending interrupt is acknowledged by clearing a bit.
+ */
+void irq_gc_mask_clr_and_ack_clr(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);
+	*ct->mask_cache &= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
  * irq_gc_eoi - EOI interrupt
  * @d: irq_data
  */

Mans, as the author of the only existing upstream user of this code, should have received this as well.

-Doug

Quoted 24 lines · Doug Berger

The irq_gc_mask_disable_reg_and_ack() function name implies that it provides the combined functions of irq_gc_mask_disable_reg() and irq_gc_ack(). However, the implementation does not actually do that since it writes the mask instead of the disable register. It also does not maintain the mask cache which makes it inappropriate to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to irq_gc_set_bit() so this function probably should have also been renamed at that time.

Since this generic chip code provides three mask functions and two ack functions, this commit provides generic implementations for all six combinations of the mask and ack functions suitable for use with the irq_mask_ack member of the struct irq_chip.

The '_reg' and '_bit' portions of the base function names were left out of the new combined function names in an attempt to keep the function name lengths manageable with the 80 character source code line length while still capturing the distinct aspects of each combination of functions.

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

Quoted 11 lines

/**
+ * irq_gc_mask_disable_and_ack_set - 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.
+ */

kernel/irq/generic-chip.c +4 -0

+void irq_gc_mask_disable_and_ack_set(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);

kernel/irq/generic-chip.c void irq_gc_mask_disable_reg_and_ack(struct irq_data *d) · line 171

Quoted 8 lines

kernel/irq/generic-chip.c +8 -0

+	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);
+}

From: Florian Fainelli <[email protected]>

Any usage of the irq_gc_mask_disable_reg_and_ack() function should be replaced with either the irq_gc_mask_disable_and_ack_set() or the irq_gc_mask_set_and_ack_set() function depending on which corrects the bugs in irq_gc_mask_disable_reg_and_ack() for a given usage.

For the Tango irqchip driver, irq_gc_mask_disable_and_ack_set() seems to be what is intended, so use it.

Signed-off-by Florian Fainelli <[email protected]>

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

---
 drivers/irqchip/irq-tango.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-tango.c +1 -1

index bdbb5c0ff7fe..0c085303a583 100644
@@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
 	for (i = 0; i < 2; i++) {
 		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
 		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
-		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
+		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
 		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
 		ct[i].chip.irq_set_type = tangox_irq_set_type;
 		ct[i].chip.name = gc->domain->name;

Hello,

I've added Mans (the code's author) to the recipient list.

Regards.

Quoted 12 lines · Doug Berger

From: Florian Fainelli <[email protected]>

Any usage of the irq_gc_mask_disable_reg_and_ack() function should be replaced with either the irq_gc_mask_disable_and_ack_set() or the irq_gc_mask_set_and_ack_set() function depending on which corrects the bugs in irq_gc_mask_disable_reg_and_ack() for a given usage.

For the Tango irqchip driver, irq_gc_mask_disable_and_ack_set() seems to be what is intended, so use it.

Signed-off-by Florian Fainelli <[email protected]>

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

Quoted 18 lines

---
 drivers/irqchip/irq-tango.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-tango.c +1 -1

index bdbb5c0ff7fe..0c085303a583 100644
@@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
 	for (i = 0; i < 2; i++) {
 		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
 		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
-		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
+		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
 		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
 		ct[i].chip.irq_set_type = tangox_irq_set_type;
 		ct[i].chip.name = gc->domain->name;

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 acknowledgement 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 seperately.

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

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.

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

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.

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

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.

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

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. The first three commits of this set are intended to be a correction and extension of the existing generic irq implementation to provide a set of functions that can be used by interrupt controller drivers for their irq_mask_ack method.

I believe these first three commits should be added to the irq/core repository and the remaining commits should be added to the Broadcom github repository but have included the complete set here for improved context. This entire set is therefore based on the irq/core master branch. Please let me know if you would like a different packaging.

If the changes to genirq are not acceptable I can implement the irq_mask_ask method locally in the irq-brcmstb-l2 driver and submit that on its own.

Doug Berger (5):
genirq: generic chip: add generic irq_mask_ack functions
genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()
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

Florian Fainelli (1):
irqchip/tango: Use irq_gc_mask_disable_and_ack_set

 .../bindings/interrupt-controller/brcm,l2-intc.txt |   3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 145 ++++++++++++++-------
 drivers/irqchip/irq-tango.c                        |   2 +-
 include/linux/irq.h                                |   7 +-
 kernel/irq/generic-chip.c                          | 110 +++++++++++++++-
 5 files changed, 214 insertions(+), 53 deletions(-)

Sorry, messed up the CC list.

Quoted 26 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.

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. The first three commits of this set are intended to be a correction and extension of the existing generic irq implementation to provide a set of functions that can be used by interrupt controller drivers for their irq_mask_ack method.

I believe these first three commits should be added to the irq/core repository and the remaining commits should be added to the Broadcom github repository but have included the complete set here for improved context. This entire set is therefore based on the irq/core master branch. Please let me know if you would like a different packaging.

Quoted 22 lines

If the changes to genirq are not acceptable I can implement the irq_mask_ask method locally in the irq-brcmstb-l2 driver and submit that on its own.

Doug Berger (5):
genirq: generic chip: add generic irq_mask_ack functions
genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()
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

Florian Fainelli (1):
irqchip/tango: Use irq_gc_mask_disable_and_ack_set

 .../bindings/interrupt-controller/brcm,l2-intc.txt |   3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 145 ++++++++++++++-------
 drivers/irqchip/irq-tango.c                        |   2 +-
 include/linux/irq.h                                |   7 +-
 kernel/irq/generic-chip.c                          | 110 +++++++++++++++-
 5 files changed, 214 insertions(+), 53 deletions(-)