[PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[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 <opendmb@gmail.com>
---
 include/linux/irq.h       |   6 +++
 kernel/irq/generic-chip.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 00db35b61e9e..23b9617bb682 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -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);
 
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index f7086b78ad6e..168887a81a29 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -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
  */
-- 
2.13.0

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Thomas Gleixner]

On Fri, 7 Jul 2017, Doug Berger wrote:

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

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

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Doug Berger]

On 07/08/2017 05:08 AM, Thomas Gleixner wrote:
> On Fri, 7 Jul 2017, Doug Berger wrote:
> 
>> 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.
> 
> 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.

> 
> Thanks,
> 
> 	tglx
> 

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

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Doug Berger]

On 07/10/2017 10:39 AM, Doug Berger wrote:
> On 07/08/2017 05:08 AM, Thomas Gleixner wrote:
>> On Fri, 7 Jul 2017, Doug Berger wrote:
>>
>>> 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.
>>
>> 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.
> 
>>
>> 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?

Thanks,
    Doug

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Thomas Gleixner]

On Mon, 17 Jul 2017, Doug Berger wrote:

> 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?

Please resubmit with the implementation which is actually used. We can add
the others when the need arises. You might mention that in the changelog
and the function comment, so people looking for a suitable alternative will
get the proper hint.

Thanks,

	tglx

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Doug Berger]

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

-Doug

On 07/07/2017 12:20 PM, Doug Berger wrote:
> 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 <opendmb@gmail.com>
> ---
>  include/linux/irq.h       |   6 +++
>  kernel/irq/generic-chip.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 00db35b61e9e..23b9617bb682 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -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);
>  
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index f7086b78ad6e..168887a81a29 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -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
>   */
> 

Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Måns Rullgård]

Doug Berger <opendmb@gmail.com> writes:

> Mans, as the author of the only existing upstream user of this code,
> should have received this as well.
>
> -Doug
>
> On 07/07/2017 12:20 PM, Doug Berger wrote:
>> 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 <opendmb@gmail.com>

Hmm, something is wrong here.  The irq_gc_mask_disable_reg_and_ack()
function writes to regs.mask, but the irq-tango driver doesn't set this
field (there is no corresponding hardware register).  Either it is never
called, or the write ends up being harmless.  I don't remember why I set
irq_mask_ack that way.

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

This function looks like it should probably be used instead.  I'll try
to remember to test it when I have time to fire up that hardware.

-- 
Måns Rullgård

[PATCH 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Doug Berger]

From: Florian Fainelli <f.fainelli@gmail.com>

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 <f.fainelli@gmail.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/irqchip/irq-tango.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
index bdbb5c0ff7fe..0c085303a583 100644
--- a/drivers/irqchip/irq-tango.c
+++ b/drivers/irqchip/irq-tango.c
@@ -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;
-- 
2.13.0

Re: [PATCH 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Marc Gonzalez]

Hello,

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

Regards.

On 07/07/2017 21:20, Doug Berger wrote:

> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> 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 <f.fainelli@gmail.com>
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  drivers/irqchip/irq-tango.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..0c085303a583 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -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;
> 

Re: [PATCH 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> Hello,
>
> I've added Mans (the code's author) to the recipient list.
>
> Regards.
>
> On 07/07/2017 21:20, Doug Berger wrote:
>
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> 
>> 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 <f.fainelli@gmail.com>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>

As I said elsewhere, this looks more correct although I haven't tested it.

Acked-by: Mans Rullgard <mans@mansr.com>

>> ---
>>  drivers/irqchip/irq-tango.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>> index bdbb5c0ff7fe..0c085303a583 100644
>> --- a/drivers/irqchip/irq-tango.c
>> +++ b/drivers/irqchip/irq-tango.c
>> @@ -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;
>> 
>

-- 
Måns Rullgård

[PATCH 3/6] genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()

[Doug Berger]

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.

The incorrect and ambiguously named function is removed here to
prevent accidental misuse.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 include/linux/irq.h       |  1 -
 kernel/irq/generic-chip.c | 16 ----------------
 2 files changed, 17 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 23b9617bb682..d2a9150aace7 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1002,7 +1002,6 @@ void irq_gc_mask_clr_bit(struct irq_data *d);
 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);
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 168887a81a29..5853d9b74924 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -135,22 +135,6 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 }
 
 /**
- * irq_gc_mask_disable_reg_and_ack - Mask and ack pending interrupt
- * @d: irq_data
- */
-void irq_gc_mask_disable_reg_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.mask);
-	irq_reg_writel(gc, mask, ct->regs.ack);
-	irq_gc_unlock(gc);
-}
-
-/**
  * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
  * @d: irq_data
  *
-- 
2.13.0

[PATCH 4/6] irqchip: brcmstb-l2: Remove some processing from the handler

[Doug Berger]

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 <opendmb@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 46 +++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index bddf169c4b37..977ae55d47d4 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,9 +41,8 @@
 
 /* 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 */
 };
@@ -51,15 +50,14 @@ struct brcmstb_l2_intc_data {
 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 +68,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 +112,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 +167,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 = irq_gc_mask_disable_and_ack_set;
 	ct->regs.disable = CPU_MASK_SET;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
@@ -194,19 +192,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.13.0

Re: [PATCH 4/6] irqchip: brcmstb-l2: Remove some processing from the handler

[Florian Fainelli]

On 07/07/2017 12:20 PM, Doug Berger wrote:
> 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 <opendmb@gmail.com>

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

[PATCH 5/6] irqchip: brcmstb-l2: Abstract register accesses

[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.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index 977ae55d47d4..ce3850530e2b 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -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 */
 };
@@ -56,8 +58,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);
@@ -78,16 +80,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);
 }
@@ -95,15 +98,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);
 }
 
@@ -173,6 +180,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;
@@ -181,6 +191,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 = irq_gc_mask_disable_and_ack_set;
 	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;
-- 
2.13.0

Re: [PATCH 5/6] irqchip: brcmstb-l2: Abstract register accesses

[Florian Fainelli]

On 07/07/2017 12:20 PM, Doug Berger wrote:
> 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 <opendmb@gmail.com>

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

[PATCH 6/6] irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

[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.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 .../bindings/interrupt-controller/brcm,l2-intc.txt |  3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 86 ++++++++++++++++------
 2 files changed, 66 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
index 448273a30a11..36df06c5c567 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
@@ -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
diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index ce3850530e2b..f77e6c9530dc 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -31,13 +31,34 @@
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
 
-/* Register offsets in the L2 interrupt controller */
-#define CPU_STATUS	0x00
-#define CPU_SET		0x04
-#define CPU_CLEAR	0x08
-#define CPU_MASK_STATUS	0x0c
-#define CPU_MASK_SET	0x10
-#define CPU_MASK_CLEAR	0x14
+struct brcmstb_intc_init_params {
+	irq_flow_handler_t handler;
+	int cpu_status;
+	int cpu_clear;
+	int cpu_mask_status;
+	int cpu_mask_set;
+	int cpu_mask_clear;
+};
+
+/* Register offsets in the L2 latched interrupt controller */
+static const struct brcmstb_intc_init_params l2_edge_intc_init = {
+	.handler		= handle_edge_irq,
+	.cpu_status		= 0x00,
+	.cpu_clear		= 0x08,
+	.cpu_mask_status	= 0x0c,
+	.cpu_mask_set		= 0x10,
+	.cpu_mask_clear		= 0x14
+};
+
+/* Register offsets in the L2 level interrupt controller */
+static const struct brcmstb_intc_init_params l2_lvl_intc_init = {
+	.handler		= handle_level_irq,
+	.cpu_status		= 0x00,
+	.cpu_clear		= -1, /* Register not present */
+	.cpu_mask_status	= 0x04,
+	.cpu_mask_set		= 0x08,
+	.cpu_mask_clear		= 0x0C
+};
 
 /* L2 intc private data structure */
 struct brcmstb_l2_intc_data {
@@ -102,7 +123,7 @@ static void brcmstb_l2_intc_resume(struct irq_data *d)
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
-	if (ct->chip.irq_ack != irq_gc_noop) {
+	if (ct->chip.irq_ack) {
 		/* Clear unmasked non-wakeup interrupts */
 		irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active,
 				ct->regs.ack);
@@ -115,7 +136,9 @@ static void brcmstb_l2_intc_resume(struct irq_data *d)
 }
 
 static int __init brcmstb_l2_intc_of_init(struct device_node *np,
-					  struct device_node *parent)
+					  struct device_node *parent,
+					  const struct brcmstb_intc_init_params
+					  *init_params)
 {
 	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
 	struct brcmstb_l2_intc_data *data;
@@ -137,12 +160,12 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	}
 
 	/* Disable all interrupts by default */
-	writel(0xffffffff, base + CPU_MASK_SET);
+	writel(0xffffffff, base + init_params->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, base + CPU_CLEAR);
+	if (!data->can_wake && (init_params->cpu_clear >= 0))
+		writel(0xffffffff, base + init_params->cpu_clear);
 
 	parent_irq = irq_of_parse_and_map(np, 0);
 	if (!parent_irq) {
@@ -167,7 +190,7 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 
 	/* Allocate a single Generic IRQ chip for this node */
 	ret = irq_alloc_domain_generic_chips(data->domain, 32, 1,
-				np->full_name, handle_edge_irq, clr, 0, flags);
+			np->full_name, init_params->handler, clr, 0, flags);
 	if (ret) {
 		pr_err("failed to allocate generic irq chip\n");
 		goto out_free_domain;
@@ -180,21 +203,26 @@ 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;
+	data->status_offset = init_params->cpu_status;
+	data->mask_offset = init_params->cpu_mask_status;
 
 	ct = data->gc->chip_types;
 
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->regs.ack = CPU_CLEAR;
+	if (init_params->cpu_clear >= 0) {
+		ct->regs.ack = init_params->cpu_clear;
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
+	} else {
+		/* No Ack - but still slightly more efficient to define this */
+		ct->chip.irq_mask_ack = irq_gc_mask_disable_reg;
+	}
 
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
-	ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
-	ct->regs.disable = CPU_MASK_SET;
-	ct->regs.mask = CPU_MASK_STATUS;
+	ct->regs.disable = init_params->cpu_mask_set;
+	ct->regs.mask = init_params->cpu_mask_status;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
-	ct->regs.enable = CPU_MASK_CLEAR;
+	ct->regs.enable = init_params->cpu_mask_clear;
 
 	ct->chip.irq_suspend = brcmstb_l2_intc_suspend;
 	ct->chip.irq_resume = brcmstb_l2_intc_resume;
@@ -220,4 +248,18 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	kfree(data);
 	return ret;
 }
-IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,l2-intc", brcmstb_l2_intc_of_init);
+
+int __init brcmstb_l2_edge_intc_of_init(struct device_node *np,
+	struct device_node *parent)
+{
+	return brcmstb_l2_intc_of_init(np, parent, &l2_edge_intc_init);
+}
+IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,l2-intc", brcmstb_l2_edge_intc_of_init);
+
+int __init brcmstb_l2_lvl_intc_of_init(struct device_node *np,
+	struct device_node *parent)
+{
+	return brcmstb_l2_intc_of_init(np, parent, &l2_lvl_intc_init);
+}
+IRQCHIP_DECLARE(bcm7271_l2_intc, "brcm,bcm7271-l2-intc",
+	brcmstb_l2_lvl_intc_of_init);
-- 
2.13.0

Re: [PATCH 6/6] irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

[Rob Herring]

On Fri, Jul 07, 2017 at 12:20:16PM -0700, Doug Berger wrote:
> 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 <opendmb@gmail.com>
> ---
>  .../bindings/interrupt-controller/brcm,l2-intc.txt |  3 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/irqchip/irq-brcmstb-l2.c                   | 86 ++++++++++++++++------
>  2 files changed, 66 insertions(+), 23 deletions(-)

Re: [PATCH 6/6] irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

[Florian Fainelli]

On 07/07/2017 12:20 PM, Doug Berger wrote:
> 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 <opendmb@gmail.com>

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

Re: [PATCH 0/6] Add support for BCM7271 style interrupt controller

[Doug Berger]

Sorry, messed up the CC list.

On 07/07/2017 12:20 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.  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(-)
> 

Re: [PATCH 0/6] Add support for BCM7271 style interrupt controller

[Florian Fainelli]

On 07/07/2017 12:34 PM, Doug Berger wrote:
> Sorry, messed up the CC list.
> 
> On 07/07/2017 12:20 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.  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.

The irqchip maintainers (Thomas, Jason, Marc Z.) will probably want to
get irqchip drivers changes through their tree:

IRQCHIP DRIVERS
M:      Thomas Gleixner <tglx@linutronix.de>
M:      Jason Cooper <jason@lakedaemon.net>
M:      Marc Zyngier <marc.zyngier@arm.com>
L:      linux-kernel@vger.kernel.org
S:      Maintained
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
irq/core
T:      git git://git.infradead.org/users/jcooper/linux.git irqchip/core
F:      Documentation/devicetree/bindings/interrupt-controller/
F:      drivers/irqchip/

Will reply to the individual patches, thanks for getting this out.

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


-- 
Florian